[PATCH v2 3/3] video: sunxi_display: Convert to DM_VIDEO
Andre Przywara
andre.przywara at arm.com
Thu Feb 4 23:30:36 CET 2021
On Fri, 19 Jun 2020 09:30:22 +0200
Maxime Ripard <maxime at cerno.tech> wrote:
Hi,
I have picked up this patch, rebased, updated and fixed some parts.
I will send a v3 ASAP, so we can continue the discussion on this.
I am replying here to some of the outstanding comments, since this has
the context:
> On Thu, Jun 18, 2020 at 09:37:07PM +0530, Jagan Teki wrote:
> > On Thu, Jun 18, 2020 at 12:54 PM Maxime Ripard <maxime at cerno.tech> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 01:54:37AM +0530, Jagan Teki wrote:
> > > > 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.
> > > > ====================================================
> > > >
> > > > So let's live these boards on the tree before the video maintainer
> > > > removes it by converting in to DM_VIDEO.
> > > >
> > > > Tested in Bananapi M1+ Plus 1920x1200 HDMI out.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - add BMP support
> > > >
> > > > arch/arm/mach-sunxi/Kconfig | 4 +-
> > > > drivers/video/sunxi/sunxi_display.c | 264 ++++++++++++++++------------
> > > > include/configs/sunxi-common.h | 17 --
> > > > scripts/config_whitelist.txt | 1 -
> > > > 4 files changed, 157 insertions(+), 129 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > > index be0822bfb7..7d2fb55ff0 100644
> > > > --- a/arch/arm/mach-sunxi/Kconfig
> > > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > > @@ -758,8 +758,10 @@ config VIDEO_SUNXI
> > > > depends on !MACH_SUN9I
> > > > depends on !MACH_SUN50I
> > > > depends on !MACH_SUN50I_H6
> > > > - select VIDEO
> > > > + select DM_VIDEO
> > > > + select DISPLAY
> > > > imply VIDEO_DT_SIMPLEFB
> > > > + imply CMD_BMP
> > > > default y
> > > > ---help---
> > > > Say Y here to add support for using a cfb console on the HDMI, LCD
> > > > diff --git a/drivers/video/sunxi/sunxi_display.c b/drivers/video/sunxi/sunxi_display.c
> > > > index f52aba4d21..fb51b0c5ba 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>
> > > > #include "../videomodes.h"
> > > > #include "../anx9804.h"
> > > > #include "../hitachi_tx18d42vm_lcd.h"
> > > > @@ -45,6 +49,13 @@
> > > >
> > > > DECLARE_GLOBAL_DATA_PTR;
> > > >
> > > > +enum {
> > > > + /* Maximum LCD size we support */
> > > > + LCD_MAX_WIDTH = 3840,
> > > > + LCD_MAX_HEIGHT = 2160,
> > > > + LCD_MAX_LOG2_BPP = VIDEO_BPP32,
> > > > +};
> > >
> > > Why is that an enum? Those three values don't have any relationship with
> > > each other.
> >
> > Not sure I understand your question. These are maximum resolution
> > followed by maximum bpp supporting. based on these the fbbuffer
> > allocation happens, please see sunxi_de_bind on this patch.
>
> An enum is used for variables that can hold multiple values. There's no
> relationship between those values, you cannot use the BPP in place of
> the width there for example, it doesn't make any sense. So you must not
> use an enum.
I agree that an enum does not make sense here. That seems to be
copied from sunxi_de2.c, which has the same code ;-)
My guess is this stems from some misunderstood "use enums instead of
#define!" recommendation. I replaced it with old-fashioned #defines.
>
> > > > -ulong board_get_usable_ram_top(ulong total_size)
> > > > -{
> > > > - return gd->ram_top - CONFIG_SUNXI_MAX_FB_SIZE;
> > > > -}
> > > > -
> > >
> > > I couldn't find how you're allocating and reserving the video buffer now
> > > for simplefb to use later on.
> >
> > Based on maximum supporting resolutions and bpp as explained above and
> > similar like what sunxi_de2 and other platforms like rockchip. via
> > plat->size.
>
> It really doesn't explain how you're allocating and reserving it for
> simplefb though, merely how much memory you're allocating.
>
> Let me rephrase this. simplefb will run in Linux based on an address
> that is passed from the bootloader to Linux through the device tree. How
> do you make sure that Linux will not reuse that buffer for something
> else once it's been booted?
>
> The previous code was doing that by removing enough space from the total
> memory available, so Linux could run only on the total amount of RAM
> minus the size of the framebuffer. You removed that, so how does it work
> now?
So the former code lowered U-Boot's perception of gd->ram_top, but
sunxi-de2 doesn't do this either. I don't think this on its own is a
real problem.
What both sunxi-de2 and this patch do is to patch the /memory node in
the DT to exclude the framebuffer region, at the same time when
patching in the framebuffer details into the chosen node (in the last
function of this patch, not in this email anymore).
There *is* a bug in this patch that creates a small overlap, which
makes Linux crash. That's easily fixed, though:
start = gd->bd->bi_dram[0].start;
- size = gd->bd->bi_dram[0].size - sunxi_display->fb_size;
+ size = sunxi_display->fb_addr - start;
ret = fdt_fixup_memory_banks(blob, &start, &size, 1);
Maxime, does that answer your concerns? I tested this (fixed) patch with
simplefb, and it works: the /memory node ends 32MB before the end of
DRAM, and the framebuffer works in Linux.
> And if you're changing that somehow, at the very least, it should be
> either in a separate patch or mentionned in the commit log.
>
> > >
> > > > static bool sunxi_has_hdmi(void)
> > > > {
> > > > #ifdef CONFIG_VIDEO_HDMI
> > > > @@ -1052,9 +1064,11 @@ static enum sunxi_monitor sunxi_get_default_mon(bool allow_hdmi)
> > > > return sunxi_monitor_none;
> > > > }
> > > >
> > > > -void *video_hw_init(void)
> > > > +static int sunxi_de_probe(struct udevice *dev)
> > > > {
> > > > - static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
> > > > + struct video_priv *uc_priv = dev_get_uclass_priv(dev);
> > > > + struct video_uc_platdata *plat = dev_get_uclass_platdata(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 +1081,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 +1090,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 +1126,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 +1166,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;
> > > > + memset((void *)sunxi_display->fb_addr, 0, sunxi_display->fb_size);
> > > > + flush_cache(sunxi_display->fb_addr, sunxi_display->fb_size);
> > > > }
> > > > - 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);
> > >
> > > This is new. You should explain in a separate patch why the flush_cache
> > > above isn't enough.
> >
> > This is new and but required when for DM_VIDEO conversion.
> > video-uclass has a common way to flush video via video_sync so the
> > driver has to trigger this flush and without this, the vidconsole
> > shows inconsistent characters.
>
> I still believe this is redundant with the flush_cache above.
So this video_set_flush_dcache() does not *do* an actual cache_clean, it
merely enables regular cache cleans in video-uclass.c:video_sync().
This replaces the automatic determination of
CONFIG_VIDEO's cfb_do_flush_cache in cfb_console.c, with an explicit
request.
The initial cache clean of the framebuffer above is independent of this.
sunxi_de2 does this video_set_flush_dcache() call as well.
Cheers,
Andre
More information about the U-Boot
mailing list