[PATCH v3] video: sunxi_display: Convert to DM_VIDEO
Simon Glass
sjg at chromium.org
Sun Feb 21 17:24:18 CET 2021
Hi Andre,
On Sat, 20 Feb 2021 at 17:08, Andre Przywara <andre.przywara at arm.com> wrote:
>
> 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().
Best avoided.
> 2) Use uclass_get_device_by_name() instead, but prevent the double
> probe by keeping an indication of the probe status.
Probing a probed device is a nop, so don't worry about that. We do it
all the time.
> 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.
Again, best to avoid a global.
> 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?
I think what you have is OK...at present those functions are not
called in device context. I don't really have a good solution. You
want to avoid probing it just to fill in the data.
BTW it is a bit faster to call
device_get_by_driver_info(DM_DRIVER_GET(sunxi_de), &dev)
>
> >
> > > #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?
I'm not sure about that, but I suppose if the maximum is too high you
can change it.
>
> > > +
> > > 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.
Ah OK. Well yes it needs some docs for the next person :-)
[..]
> > >
> > > 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?
In that case I feel that the fb should be allocated large enough for
the overscan area (how else could it be). But I'm not sure what is
happening here, so perhaps this is 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().
>
> 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()?
I suppose you can, but that is not what is expected. You don't save
memory for U-Boot (and it doesn't matter), but making it smaller so
that linux uses less would be worthwhile. We just need to make sure
this is documented in video.h and tested.
To be clear, you have my review thg, so these are things that can be done later.
Regards,
Simon
More information about the U-Boot
mailing list