On Sun, 7 Feb 2021 07:37:34 -0700
Simon Glass <s...@chromium.org> wrote:
Hi Simon,
> On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przyw...@arm.com> wrote:
> >
> > From: Jagan Teki <ja...@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 <ja...@amarulasolutions.com>
> > [Andre: rebase and smaller fixes]
> > Signed-off-by: Andre Przywara <andre.przyw...@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 <s...@chromium.org>
>
> Some thoughts below
Many thanks for having a thorough look, much appreciated.
I will have a closer look at your comments which I didn't reply to
below.
For the other points:
To be honest, I haven't checked every line in this patch, my goal was
merely to get it merged (this time), to prevent feature removal and
drop the nasty buildman warnings.
I see quite some cleanup potential here (some I already mentioned in
the commit message above), but wanted to get the main DM_VIDEO
conversion done first (as I think last time some discussion already
prevented a merge).
So my plan was to queue this for next ASAP, then send more cleanup patches
on top, before the next merge window. I understand that it's typically
done the other way around, but given this is dragging on for a while,
is long overdue and works for me (TM) as it is, I would prefer a
functional patch first, with cleanups on top.
>
> > 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 ?
So I was always wondering about the logic behind that.
My understanding would be: This driver is (now) implementing the
DM_VIDEO interface. Any user (at board or SoC level) would just be
selecting this driver, without caring about which driver interface it
uses.
So as this driver is (now) DM_VIDEO only, it would signal this via this
select line.
If that is not how it's meant to be, can you explain the the idea behind
this, please?
Also: with CONFIG_VIDEO now dying, CONFIG_DM_VIDEO somewhat loses its
purpose, doesn't it?
>
> > + 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.
>
> > #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
> > +
> > 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?
>
> > -} 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()?
Yes, in fact I have patches to replace all #ifdefs with IS_ENABLED(),
except the sun6i reset gate ones (which would break compilation on
non-sun6i). And those should be tackled by proper UCLASS_CLK usage.
But I wanted to get this basic conversion out first, see above.
The rest of your comments make sense on the first glance, I will try to
fix the easy things for a v4.
Cheers,
Andre
> > - 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.
>
> > {
> > - 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?
>
> > }
> > - 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().
>
>
> > +
> > + 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
