[PATCH v3] video: sunxi_display: Convert to DM_VIDEO
Andre Przywara
andre.przywara at arm.com
Sun Feb 21 01:07:35 CET 2021
On Sun, 7 Feb 2021 07:37:34 -0700
Simon Glass <sjg at chromium.org> wrote:
Hi Simon,
coming back to this patch, answering to the other comments I skipped
over the last time.
In general this patch is the shortest way to get to some kind of DM
driver, in many ways it still looks like a non-DM driver on the inside,
which shows.
> On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > From: Jagan Teki <jagan at amarulasolutions.com>
> >
> > DM_VIDEO migration deadline is already expired, but around
> > 80 Allwinner boards are still using video in a legacy way.
> >
> > ===================== WARNING ======================
> > This board does not use CONFIG_DM_VIDEO Please update
> > the board to use CONFIG_DM_VIDEO before the v2019.07 release.
> > Failure to update by the deadline may result in board removal.
> > See doc/driver-model/migration.rst for more info.
> > ====================================================
> >
> > Convert the legacy video driver over to the DM_VIDEO framework. This is
> > a minimal conversion: it doesn't use the DT for finding its resources,
> > nor does it use DM clocks or DM devices for the outputs (LCD, HDMI, CVBS).
> >
> > Tested in Bananapi M1+ Plus 1920x1200 HDMI out. (Jagan)
> >
> > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > [Andre: rebase and smaller fixes]
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > ---
> > Hi,
> >
> > I picked this one up to get rid of the warnings. I dropped the BMP
> > support for now (v2 1/3 and v2 2/3), I need to have a closer look, as
> > I am not convinced this was the right solution.
> >
> > Cheers,
> > Andre
> >
> > Changelog v2 .. v3:
> > - rebase against master, fixing up renamed variables and structs
> > - replace enum with #define
> > - remove BMP from Kconfig
> > - fix memory node size calculation in simplefb setup
> >
> > arch/arm/mach-sunxi/Kconfig | 9 +-
> > drivers/video/sunxi/sunxi_display.c | 262 ++++++++++++++++------------
> > include/configs/sunxi-common.h | 17 --
> > scripts/config_whitelist.txt | 1 -
> > 4 files changed, 157 insertions(+), 132 deletions(-)
> >
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Some thoughts below
>
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index 0135575ca1e..a29d11505aa 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -816,13 +816,14 @@ config VIDEO_SUNXI
> > depends on !MACH_SUN9I
> > depends on !MACH_SUN50I
> > depends on !SUN50I_GEN_H6
> > - select VIDEO
> > + select DM_VIDEO
>
> I wonder whether instead VIDEO_SUNXI should depend on DM_VIDEO ?
>
> > + select DISPLAY
> > imply VIDEO_DT_SIMPLEFB
> > default y
> > ---help---
> > - Say Y here to add support for using a cfb console on the HDMI, LCD
> > - or VGA output found on most sunxi devices. See doc/README.video for
> > - info on how to select the video output and mode.
> > + Say Y here to add support for using a graphical console on the HDMI,
> > + LCD or VGA output found on older sunxi devices. This will also provide
> > + a simple_framebuffer device for Linux.
> >
> > config VIDEO_HDMI
> > bool "HDMI output support"
> > diff --git a/drivers/video/sunxi/sunxi_display.c b/drivers/video/sunxi/sunxi_display.c
> > index f52aba4d21c..61498d1642f 100644
> > --- a/drivers/video/sunxi/sunxi_display.c
> > +++ b/drivers/video/sunxi/sunxi_display.c
> > @@ -7,6 +7,8 @@
> > */
> >
> > #include <common.h>
> > +#include <display.h>
> > +#include <dm.h>
> > #include <cpu_func.h>
> > #include <efi_loader.h>
> > #include <init.h>
> > @@ -28,7 +30,9 @@
> > #include <fdt_support.h>
> > #include <i2c.h>
> > #include <malloc.h>
> > +#include <video.h>
> > #include <video_fb.h>
> > +#include <dm/uclass-internal.h>
>
> Do you need that? Internal things should be avoided if posssible.
That's a good point. This is needed for uclass_find_device_by_name()
down in the simplefb setup function at the very end. This function will
be called from a different context (from ft_board_setup() in board.c),
and tries to find the (only) instance of this very driver to populate
the simplefb DT node accordingly. This is the approach the sunxi_de2.c
uses, and back at the time this was seemingly the best way to achieve
this. Alternatives I see:
1) Keep a global static variable, pointing to the struct udevice, set in
probe().
2) Use uclass_get_device_by_name() instead, but prevent the double
probe by keeping an indication of the probe status.
3) Gather all data needed for the simplefb setup already in probe(),
and store them in global variables, to be picked up by
sunxi_simplefb_setup() later.
4) as 3), but store the extra data (just the pipeline name to find the
right DT node to enable?) in generic uclass storage. This has the
potential of making this function platform and board agnostic (so meson
could benefit as well?). We could then setup the simplefb in common
code.
Any ideas on this? Maybe something else entirely?
>
> > #include "../videomodes.h"
> > #include "../anx9804.h"
> > #include "../hitachi_tx18d42vm_lcd.h"
> > @@ -45,6 +49,11 @@
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > +/* Maximum LCD size we support */
> > +#define LCD_MAX_WIDTH 3840
> > +#define LCD_MAX_HEIGHT 2160
> > +#define LCD_MAX_LOG2_BPP VIDEO_BPP32
So this resolution is not achievable with a maximum pixel clock
of 165 MHz (not even at 24 Hz). Given that the framebuffer reservation
is fixed, this wastes quite some memory. Realistically 165 MHz means
1920x1200 (at 60Hz), shall we use that one instead?
> > +
> > enum sunxi_monitor {
> > sunxi_monitor_none,
> > sunxi_monitor_dvi,
> > @@ -59,12 +68,11 @@ enum sunxi_monitor {
> > #define SUNXI_MONITOR_LAST sunxi_monitor_composite_pal_nc
> >
> > struct sunxi_display {
> > - GraphicDevice graphic_device;
> > enum sunxi_monitor monitor;
> > unsigned int depth;
> > unsigned int fb_addr;
> > unsigned int fb_size;
>
> These last three are repeated from the uclass info. But fb_addr seems
> to sometimes be different from the ucass frame buffer, in which case I
> wonder how output actually works.
>
> If you do actually need these, can you please document them here?
I think this might be mostly non-DM legacy, but for the composite
overscan we tweak the framebuffer address. I haven't fully wrapped
my mind around this, though, and ideally we can indeed lose those
extra members.
I will try to find some device with composite input to test it.
> > -} sunxi_display;
> > +};
> >
> > const struct ctfb_res_modes composite_video_modes[2] = {
> > /* x y hz pixclk ps/kHz le ri up lo hs vs s vmode */
>
> [..]
>
> > -static void sunxi_mode_set(const struct ctfb_res_modes *mode,
> > +static void sunxi_mode_set(struct sunxi_display *sunxi_display,
> > + const struct ctfb_res_modes *mode,
> > unsigned int address)
> > {
> > + enum sunxi_monitor monitor = sunxi_display->monitor;
> > int __maybe_unused clk_div, clk_double;
> > struct sunxi_lcdc_reg * const lcdc =
> > (struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
> > struct sunxi_tve_reg * __maybe_unused const tve =
> > (struct sunxi_tve_reg *)SUNXI_TVE0_BASE;
> >
> > - switch (sunxi_display.monitor) {
> > + switch (sunxi_display->monitor) {
> > case sunxi_monitor_none:
> > break;
> > case sunxi_monitor_dvi:
> > case sunxi_monitor_hdmi:
> > #ifdef CONFIG_VIDEO_HDMI
>
> I wonder if all of these can use IS_ENABLED()?
>
> > - sunxi_composer_mode_set(mode, address);
> > - sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0);
> > - sunxi_hdmi_mode_set(mode, clk_div, clk_double);
> > + sunxi_composer_mode_set(mode, address, monitor);
> > + sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0, monitor);
> > + sunxi_hdmi_mode_set(mode, clk_div, clk_double, monitor);
> > sunxi_composer_enable();
> > - lcdc_enable(lcdc, sunxi_display.depth);
> > + lcdc_enable(lcdc, sunxi_display->depth);
> > sunxi_hdmi_enable();
> > #endif
> > break;
>
> [..]
>
> > -void *video_hw_init(void)
> > +static int sunxi_de_probe(struct udevice *dev)
>
> This function is way too long and would benefit from splitting into
> several parts IMO.
Yeah, indeed. I started working on this, but it probably won't be part
of the next version already.
> > {
> > - static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> > + struct video_priv *uc_priv = dev_get_uclass_priv(dev);
> > + struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> > + struct sunxi_display *sunxi_display = dev_get_priv(dev);
> > const struct ctfb_res_modes *mode;
> > struct ctfb_res_modes custom;
> > const char *options;
> > @@ -1067,10 +1079,8 @@ void *video_hw_init(void)
> > char mon[16];
> > char *lcd_mode = CONFIG_VIDEO_LCD_MODE;
> >
> > - memset(&sunxi_display, 0, sizeof(struct sunxi_display));
> > -
> > video_get_ctfb_res_modes(RES_MODE_1024x768, 24, &mode,
> > - &sunxi_display.depth, &options);
> > + &sunxi_display->depth, &options);
> > #ifdef CONFIG_VIDEO_HDMI
> > hpd = video_get_option_int(options, "hpd", 1);
> > hpd_delay = video_get_option_int(options, "hpd_delay", 500);
> > @@ -1078,35 +1088,35 @@ void *video_hw_init(void)
> > #endif
> > overscan_x = video_get_option_int(options, "overscan_x", -1);
> > overscan_y = video_get_option_int(options, "overscan_y", -1);
> > - sunxi_display.monitor = sunxi_get_default_mon(true);
> > + sunxi_display->monitor = sunxi_get_default_mon(true);
> > video_get_option_string(options, "monitor", mon, sizeof(mon),
> > - sunxi_get_mon_desc(sunxi_display.monitor));
> > + sunxi_get_mon_desc(sunxi_display->monitor));
> > for (i = 0; i <= SUNXI_MONITOR_LAST; i++) {
> > if (strcmp(mon, sunxi_get_mon_desc(i)) == 0) {
> > - sunxi_display.monitor = i;
> > + sunxi_display->monitor = i;
> > break;
> > }
> > }
> > if (i > SUNXI_MONITOR_LAST)
> > printf("Unknown monitor: '%s', falling back to '%s'\n",
> > - mon, sunxi_get_mon_desc(sunxi_display.monitor));
> > + mon, sunxi_get_mon_desc(sunxi_display->monitor));
> >
> > #ifdef CONFIG_VIDEO_HDMI
> > /* If HDMI/DVI is selected do HPD & EDID, and handle fallback */
> > - if (sunxi_display.monitor == sunxi_monitor_dvi ||
> > - sunxi_display.monitor == sunxi_monitor_hdmi) {
> > + if (sunxi_display->monitor == sunxi_monitor_dvi ||
> > + sunxi_display->monitor == sunxi_monitor_hdmi) {
> > /* Always call hdp_detect, as it also enables clocks, etc. */
> > hdmi_present = (sunxi_hdmi_hpd_detect(hpd_delay) == 1);
> > if (hdmi_present && edid) {
> > printf("HDMI connected: ");
> > - if (sunxi_hdmi_edid_get_mode(&custom, true) == 0)
> > + if (sunxi_hdmi_edid_get_mode(sunxi_display, &custom, true) == 0)
> > mode = &custom;
> > else
> > hdmi_present = false;
> > }
> > /* Fall back to EDID in case HPD failed */
> > if (edid && !hdmi_present) {
> > - if (sunxi_hdmi_edid_get_mode(&custom, false) == 0) {
> > + if (sunxi_hdmi_edid_get_mode(sunxi_display, &custom, false) == 0) {
> > mode = &custom;
> > hdmi_present = true;
> > }
> > @@ -1114,38 +1124,39 @@ void *video_hw_init(void)
> > /* Shut down when display was not found */
> > if ((hpd || edid) && !hdmi_present) {
> > sunxi_hdmi_shutdown();
> > - sunxi_display.monitor = sunxi_get_default_mon(false);
> > + sunxi_display->monitor = sunxi_get_default_mon(false);
> > } /* else continue with hdmi/dvi without a cable connected */
> > }
> > #endif
> >
> > - switch (sunxi_display.monitor) {
> > + switch (sunxi_display->monitor) {
> > case sunxi_monitor_none:
> > - return NULL;
> > + printf("Unknown monitor\n");
> > + return -EINVAL;
> > case sunxi_monitor_dvi:
> > case sunxi_monitor_hdmi:
> > if (!sunxi_has_hdmi()) {
> > printf("HDMI/DVI not supported on this board\n");
> > - sunxi_display.monitor = sunxi_monitor_none;
> > - return NULL;
> > + sunxi_display->monitor = sunxi_monitor_none;
> > + return -EINVAL;
> > }
> > break;
> > case sunxi_monitor_lcd:
> > if (!sunxi_has_lcd()) {
> > printf("LCD not supported on this board\n");
> > - sunxi_display.monitor = sunxi_monitor_none;
> > - return NULL;
> > + sunxi_display->monitor = sunxi_monitor_none;
> > + return -EINVAL;
> > }
> > - sunxi_display.depth = video_get_params(&custom, lcd_mode);
> > + sunxi_display->depth = video_get_params(&custom, lcd_mode);
> > mode = &custom;
> > break;
> > case sunxi_monitor_vga:
> > if (!sunxi_has_vga()) {
> > printf("VGA not supported on this board\n");
> > - sunxi_display.monitor = sunxi_monitor_none;
> > - return NULL;
> > + sunxi_display->monitor = sunxi_monitor_none;
> > + return -EINVAL;
> > }
> > - sunxi_display.depth = 18;
> > + sunxi_display->depth = 18;
> > break;
> > case sunxi_monitor_composite_pal:
> > case sunxi_monitor_composite_ntsc:
> > @@ -1153,85 +1164,103 @@ void *video_hw_init(void)
> > case sunxi_monitor_composite_pal_nc:
> > if (!sunxi_has_composite()) {
> > printf("Composite video not supported on this board\n");
> > - sunxi_display.monitor = sunxi_monitor_none;
> > - return NULL;
> > + sunxi_display->monitor = sunxi_monitor_none;
> > + return -EINVAL;
> > }
> > - if (sunxi_display.monitor == sunxi_monitor_composite_pal ||
> > - sunxi_display.monitor == sunxi_monitor_composite_pal_nc)
> > + if (sunxi_display->monitor == sunxi_monitor_composite_pal ||
> > + sunxi_display->monitor == sunxi_monitor_composite_pal_nc)
> > mode = &composite_video_modes[0];
> > else
> > mode = &composite_video_modes[1];
> > - sunxi_display.depth = 24;
> > + sunxi_display->depth = 24;
> > break;
> > }
> >
> > /* Yes these defaults are quite high, overscan on composite sucks... */
> > if (overscan_x == -1)
> > - overscan_x = sunxi_is_composite() ? 32 : 0;
> > + overscan_x = sunxi_is_composite(sunxi_display->monitor) ? 32 : 0;
> > if (overscan_y == -1)
> > - overscan_y = sunxi_is_composite() ? 20 : 0;
> > + overscan_y = sunxi_is_composite(sunxi_display->monitor) ? 20 : 0;
> >
> > - sunxi_display.fb_size =
> > - (mode->xres * mode->yres * 4 + 0xfff) & ~0xfff;
> > + sunxi_display->fb_size = plat->size;
> > overscan_offset = (overscan_y * mode->xres + overscan_x) * 4;
> > /* We want to keep the fb_base for simplefb page aligned, where as
> > * the sunxi dma engines will happily accept an unaligned address. */
> > if (overscan_offset)
> > - sunxi_display.fb_size += 0x1000;
> > -
> > - if (sunxi_display.fb_size > CONFIG_SUNXI_MAX_FB_SIZE) {
> > - printf("Error need %dkB for fb, but only %dkB is reserved\n",
> > - sunxi_display.fb_size >> 10,
> > - CONFIG_SUNXI_MAX_FB_SIZE >> 10);
> > - return NULL;
> > - }
> > + sunxi_display->fb_size += 0x1000;
> >
> > printf("Setting up a %dx%d%s %s console (overscan %dx%d)\n",
> > mode->xres, mode->yres,
> > (mode->vmode == FB_VMODE_INTERLACED) ? "i" : "",
> > - sunxi_get_mon_desc(sunxi_display.monitor),
> > + sunxi_get_mon_desc(sunxi_display->monitor),
> > overscan_x, overscan_y);
> >
> > - gd->fb_base = gd->bd->bi_dram[0].start +
> > - gd->bd->bi_dram[0].size - sunxi_display.fb_size;
> > + sunxi_display->fb_addr = plat->base;
> > sunxi_engines_init();
> >
> > #ifdef CONFIG_EFI_LOADER
> > - efi_add_memory_map(gd->fb_base, sunxi_display.fb_size,
> > + efi_add_memory_map(sunxi_display->fb_addr, sunxi_display->fb_size,
> > EFI_RESERVED_MEMORY_TYPE);
> > #endif
> >
> > - fb_dma_addr = gd->fb_base - CONFIG_SYS_SDRAM_BASE;
> > - sunxi_display.fb_addr = gd->fb_base;
> > + fb_dma_addr = sunxi_display->fb_addr - CONFIG_SYS_SDRAM_BASE;
> > if (overscan_offset) {
> > fb_dma_addr += 0x1000 - (overscan_offset & 0xfff);
> > - sunxi_display.fb_addr += (overscan_offset + 0xfff) & ~0xfff;
> > - memset((void *)gd->fb_base, 0, sunxi_display.fb_size);
> > - flush_cache(gd->fb_base, sunxi_display.fb_size);
> > + sunxi_display->fb_addr += (overscan_offset + 0xfff) & ~0xfff;
>
> ALIGN(overscan_offset, 0x100) ?
>
> > + memset((void *)sunxi_display->fb_addr, 0, sunxi_display->fb_size);
> > + flush_cache(sunxi_display->fb_addr, sunxi_display->fb_size);
>
> Driver model clears the frame buffer. Is this needed?
This is just when overscan is enabled (for composite video). I *think*
it aims to clear the unused overscan area?
>
> > }
> > - sunxi_mode_set(mode, fb_dma_addr);
> > + sunxi_mode_set(sunxi_display, mode, fb_dma_addr);
> >
> > /*
> > * These are the only members of this structure that are used. All the
> > * others are driver specific. The pitch is stored in plnSizeX.
> > */
> > - graphic_device->frameAdrs = sunxi_display.fb_addr;
> > - graphic_device->gdfIndex = GDF_32BIT_X888RGB;
> > - graphic_device->gdfBytesPP = 4;
> > - graphic_device->winSizeX = mode->xres - 2 * overscan_x;
> > - graphic_device->winSizeY = mode->yres - 2 * overscan_y;
> > - graphic_device->plnSizeX = mode->xres * graphic_device->gdfBytesPP;
> > -
> > - return graphic_device;
> > + uc_priv->bpix = VIDEO_BPP32;
> > + uc_priv->xsize = mode->xres;
> > + uc_priv->ysize = mode->yres;
> > +
> > + video_set_flush_dcache(dev, 1);
>
> true
>
> > +
> > + return 0;
> > }
> >
> > +static int sunxi_de_bind(struct udevice *dev)
> > +{
> > + struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> > +
> > + plat->size = LCD_MAX_WIDTH * LCD_MAX_HEIGHT *
> > + (1 << LCD_MAX_LOG2_BPP) / 8;
>
> Should use enum video_log2_bpp here. Also see VNBYTES().
LCD_MAX_LOG2_BPP is defined as VIDEO_BPP32 at the very top. Sure will
use VNBYTES.
On a related topic: IIUC this is called several times, for a start once
before relocation, where it's supposed to give an upper bound?
Are subsequent calls then expected to be more precise? Our 32MB frame
buffer is *very* generous, for the usual FullHD display we just need
8MB. But we would only know this in probe(), when we have learned the
output device and the video modes it supports.
So is there a way to restrict (and possibly also move) the framebuffer
in probe()?
Cheers,
Andre
> > +
> > + return 0;
> > +}
> > +
> > +static const struct video_ops sunxi_de_ops = {
> > +};
> > +
> > +U_BOOT_DRIVER(sunxi_de) = {
> > + .name = "sunxi_de",
> > + .id = UCLASS_VIDEO,
> > + .ops = &sunxi_de_ops,
> > + .bind = sunxi_de_bind,
> > + .probe = sunxi_de_probe,
> > + .priv_auto = sizeof(struct sunxi_display),
>
> Should ideally have an _priv suffix
>
> > + .flags = DM_FLAG_PRE_RELOC,
> > +};
> > +
> > +U_BOOT_DRVINFO(sunxi_de) = {
> > + .name = "sunxi_de"
> > +};
> > +
>
> [..]
>
> Regards,
> Simon
More information about the U-Boot
mailing list