All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
@ 2021-02-05  1:07 Andre Przywara
  2021-02-05 16:04 ` Maxime Ripard
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Andre Przywara @ 2021-02-05  1:07 UTC (permalink / raw)
  To: u-boot

From: Jagan Teki <jagan@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@amarulasolutions.com>
[Andre: rebase and smaller fixes]
Signed-off-by: Andre Przywara <andre.przywara@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(-)

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
+	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>
 #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;
-} 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 */
@@ -214,7 +222,8 @@ static int sunxi_hdmi_edid_get_block(int block, u8 *buf)
 	return r;
 }
 
-static int sunxi_hdmi_edid_get_mode(struct ctfb_res_modes *mode,
+static int sunxi_hdmi_edid_get_mode(struct sunxi_display *sunxi_display,
+				    struct ctfb_res_modes *mode,
 				    bool verbose_mode)
 {
 	struct edid1_info edid1;
@@ -291,14 +300,14 @@ static int sunxi_hdmi_edid_get_mode(struct ctfb_res_modes *mode,
 	}
 
 	/* Check for basic audio support, if found enable hdmi output */
-	sunxi_display.monitor = sunxi_monitor_dvi;
+	sunxi_display->monitor = sunxi_monitor_dvi;
 	for (i = 0; i < ext_blocks; i++) {
 		if (cea681[i].extension_tag != EDID_CEA861_EXTENSION_TAG ||
 		    cea681[i].revision < 2)
 			continue;
 
 		if (EDID_CEA861_SUPPORTS_BASIC_AUDIO(cea681[i]))
-			sunxi_display.monitor = sunxi_monitor_hdmi;
+			sunxi_display->monitor = sunxi_monitor_hdmi;
 	}
 
 	return 0;
@@ -414,9 +423,9 @@ static void sunxi_frontend_mode_set(const struct ctfb_res_modes *mode,
 static void sunxi_frontend_enable(void) {}
 #endif
 
-static bool sunxi_is_composite(void)
+static bool sunxi_is_composite(enum sunxi_monitor monitor)
 {
-	switch (sunxi_display.monitor) {
+	switch (monitor) {
 	case sunxi_monitor_none:
 	case sunxi_monitor_dvi:
 	case sunxi_monitor_hdmi:
@@ -473,7 +482,8 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
 };
 
 static void sunxi_composer_mode_set(const struct ctfb_res_modes *mode,
-				    unsigned int address)
+				    unsigned int address,
+				    enum sunxi_monitor monitor)
 {
 	struct sunxi_de_be_reg * const de_be =
 		(struct sunxi_de_be_reg *)SUNXI_DE_BE0_BASE;
@@ -502,7 +512,7 @@ static void sunxi_composer_mode_set(const struct ctfb_res_modes *mode,
 #endif
 			     SUNXI_DE_BE_MODE_INTERLACE_ENABLE);
 
-	if (sunxi_is_composite()) {
+	if (sunxi_is_composite(monitor)) {
 		writel(SUNXI_DE_BE_OUTPUT_COLOR_CTRL_ENABLE,
 		       &de_be->output_color_ctrl);
 		for (i = 0; i < 12; i++)
@@ -616,7 +626,8 @@ static void sunxi_lcdc_backlight_enable(void)
 		gpio_direction_output(pin, PWM_ON);
 }
 
-static void sunxi_lcdc_tcon0_mode_set(const struct ctfb_res_modes *mode,
+static void sunxi_lcdc_tcon0_mode_set(struct sunxi_display *sunxi_display,
+				      const struct ctfb_res_modes *mode,
 				      bool for_ext_vga_dac)
 {
 	struct sunxi_lcdc_reg * const lcdc =
@@ -643,17 +654,18 @@ static void sunxi_lcdc_tcon0_mode_set(const struct ctfb_res_modes *mode,
 	}
 
 	lcdc_pll_set(ccm, 0, mode->pixclock_khz, &clk_div, &clk_double,
-		     sunxi_is_composite());
+		     sunxi_is_composite(sunxi_display->monitor));
 
 	video_ctfb_mode_to_display_timing(mode, &timing);
 	lcdc_tcon0_mode_set(lcdc, &timing, clk_div, for_ext_vga_dac,
-			    sunxi_display.depth, CONFIG_VIDEO_LCD_DCLK_PHASE);
+			    sunxi_display->depth, CONFIG_VIDEO_LCD_DCLK_PHASE);
 }
 
 #if defined CONFIG_VIDEO_HDMI || defined CONFIG_VIDEO_VGA || defined CONFIG_VIDEO_COMPOSITE
 static void sunxi_lcdc_tcon1_mode_set(const struct ctfb_res_modes *mode,
 				      int *clk_div, int *clk_double,
-				      bool use_portd_hvsync)
+				      bool use_portd_hvsync,
+				      enum sunxi_monitor monitor)
 {
 	struct sunxi_lcdc_reg * const lcdc =
 		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
@@ -663,7 +675,7 @@ static void sunxi_lcdc_tcon1_mode_set(const struct ctfb_res_modes *mode,
 
 	video_ctfb_mode_to_display_timing(mode, &timing);
 	lcdc_tcon1_mode_set(lcdc, &timing, use_portd_hvsync,
-			    sunxi_is_composite());
+			    sunxi_is_composite(monitor));
 
 	if (use_portd_hvsync) {
 		sunxi_gpio_set_cfgpin(SUNXI_GPD(26), SUNXI_GPD_LCD0);
@@ -671,7 +683,7 @@ static void sunxi_lcdc_tcon1_mode_set(const struct ctfb_res_modes *mode,
 	}
 
 	lcdc_pll_set(ccm, 1, mode->pixclock_khz, clk_div, clk_double,
-		     sunxi_is_composite());
+		     sunxi_is_composite(monitor));
 }
 #endif /* CONFIG_VIDEO_HDMI || defined CONFIG_VIDEO_VGA || CONFIG_VIDEO_COMPOSITE */
 
@@ -725,7 +737,8 @@ static void sunxi_hdmi_setup_info_frames(const struct ctfb_res_modes *mode)
 }
 
 static void sunxi_hdmi_mode_set(const struct ctfb_res_modes *mode,
-				int clk_div, int clk_double)
+				int clk_div, int clk_double,
+				enum sunxi_monitor monitor)
 {
 	struct sunxi_hdmi_reg * const hdmi =
 		(struct sunxi_hdmi_reg *)SUNXI_HDMI_BASE;
@@ -734,7 +747,7 @@ static void sunxi_hdmi_mode_set(const struct ctfb_res_modes *mode,
 	/* Write clear interrupt status bits */
 	writel(SUNXI_HDMI_IRQ_STATUS_BITS, &hdmi->irq);
 
-	if (sunxi_display.monitor == sunxi_monitor_hdmi)
+	if (monitor == sunxi_monitor_hdmi)
 		sunxi_hdmi_setup_info_frames(mode);
 
 	/* Set input sync enable */
@@ -789,7 +802,7 @@ static void sunxi_hdmi_enable(void)
 
 #if defined CONFIG_VIDEO_VGA || defined CONFIG_VIDEO_COMPOSITE
 
-static void sunxi_tvencoder_mode_set(void)
+static void sunxi_tvencoder_mode_set(enum sunxi_monitor monitor)
 {
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
@@ -801,7 +814,7 @@ static void sunxi_tvencoder_mode_set(void)
 	/* Clock on */
 	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_TVE0);
 
-	switch (sunxi_display.monitor) {
+	switch (monitor) {
 	case sunxi_monitor_vga:
 		tvencoder_mode_set(tve, tve_mode_vga);
 		break;
@@ -896,26 +909,28 @@ static void sunxi_engines_init(void)
 	sunxi_drc_init();
 }
 
-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
-		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;
@@ -930,7 +945,7 @@ static void sunxi_mode_set(const struct ctfb_res_modes *mode,
 			axp_set_eldo(3, 1800);
 			anx9804_init(CONFIG_VIDEO_LCD_I2C_BUS, 4,
 				     ANX9804_DATA_RATE_1620M,
-				     sunxi_display.depth);
+				     sunxi_display->depth);
 		}
 		if (IS_ENABLED(CONFIG_VIDEO_LCD_HITACHI_TX18D42VM)) {
 			mdelay(50); /* Wait for lcd controller power on */
@@ -942,10 +957,10 @@ static void sunxi_mode_set(const struct ctfb_res_modes *mode,
 			i2c_reg_write(0x5c, 0x04, 0x42); /* Turn on the LCD */
 			i2c_set_bus_num(orig_i2c_bus);
 		}
-		sunxi_composer_mode_set(mode, address);
-		sunxi_lcdc_tcon0_mode_set(mode, false);
+		sunxi_composer_mode_set(mode, address, monitor);
+		sunxi_lcdc_tcon0_mode_set(sunxi_display, mode, false);
 		sunxi_composer_enable();
-		lcdc_enable(lcdc, sunxi_display.depth);
+		lcdc_enable(lcdc, sunxi_display->depth);
 #ifdef CONFIG_VIDEO_LCD_SSD2828
 		sunxi_ssd2828_init(mode);
 #endif
@@ -953,17 +968,17 @@ static void sunxi_mode_set(const struct ctfb_res_modes *mode,
 		break;
 	case sunxi_monitor_vga:
 #ifdef CONFIG_VIDEO_VGA
-		sunxi_composer_mode_set(mode, address);
-		sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 1);
-		sunxi_tvencoder_mode_set();
+		sunxi_composer_mode_set(mode, address, monitor);
+		sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 1, monitor);
+		sunxi_tvencoder_mode_set(monitor);
 		sunxi_composer_enable();
-		lcdc_enable(lcdc, sunxi_display.depth);
+		lcdc_enable(lcdc, sunxi_display->depth);
 		tvencoder_enable(tve);
 #elif defined CONFIG_VIDEO_VGA_VIA_LCD
-		sunxi_composer_mode_set(mode, address);
-		sunxi_lcdc_tcon0_mode_set(mode, true);
+		sunxi_composer_mode_set(mode, address, monitor);
+		sunxi_lcdc_tcon0_mode_set(sunxi_display, mode, true);
 		sunxi_composer_enable();
-		lcdc_enable(lcdc, sunxi_display.depth);
+		lcdc_enable(lcdc, sunxi_display->depth);
 		sunxi_vga_external_dac_enable();
 #endif
 		break;
@@ -972,11 +987,11 @@ static void sunxi_mode_set(const struct ctfb_res_modes *mode,
 	case sunxi_monitor_composite_pal_m:
 	case sunxi_monitor_composite_pal_nc:
 #ifdef CONFIG_VIDEO_COMPOSITE
-		sunxi_composer_mode_set(mode, address);
-		sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0);
-		sunxi_tvencoder_mode_set();
+		sunxi_composer_mode_set(mode, address, monitor);
+		sunxi_lcdc_tcon1_mode_set(mode, &clk_div, &clk_double, 0, monitor);
+		sunxi_tvencoder_mode_set(monitor);
 		sunxi_composer_enable();
-		lcdc_enable(lcdc, sunxi_display.depth);
+		lcdc_enable(lcdc, sunxi_display->depth);
 		tvencoder_enable(tve);
 #endif
 		break;
@@ -999,11 +1014,6 @@ static const char *sunxi_get_mon_desc(enum sunxi_monitor monitor)
 	}
 }
 
-ulong board_get_usable_ram_top(ulong total_size)
-{
-	return gd->ram_top - CONFIG_SUNXI_MAX_FB_SIZE;
-}
-
 static bool sunxi_has_hdmi(void)
 {
 #ifdef CONFIG_VIDEO_HDMI
@@ -1052,9 +1062,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_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;
+		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);
+
+	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;
+
+	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),
+	.flags	= DM_FLAG_PRE_RELOC,
+};
+
+U_BOOT_DRVINFO(sunxi_de) = {
+	.name = "sunxi_de"
+};
+
 /*
  * Simplefb support.
  */
 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_VIDEO_DT_SIMPLEFB)
 int sunxi_simplefb_setup(void *blob)
 {
-	static GraphicDevice *graphic_device = &sunxi_display.graphic_device;
+	struct sunxi_display *sunxi_display;
+	struct video_priv *uc_priv;
+	struct udevice *de;
 	int offset, ret;
 	u64 start, size;
 	const char *pipeline = NULL;
@@ -1242,7 +1271,19 @@ int sunxi_simplefb_setup(void *blob)
 #define PIPELINE_PREFIX
 #endif
 
-	switch (sunxi_display.monitor) {
+	ret = uclass_find_device_by_name(UCLASS_VIDEO, "sunxi_de", &de);
+	if (ret) {
+		printf("DE not present\n");
+		return 0;
+	} else if (!device_active(de)) {
+		printf("DE is present but not probed\n");
+		return 0;
+	}
+
+	uc_priv = dev_get_uclass_priv(de);
+	sunxi_display = dev_get_priv(de);
+
+	switch (sunxi_display->monitor) {
 	case sunxi_monitor_none:
 		return 0;
 	case sunxi_monitor_dvi:
@@ -1280,16 +1321,17 @@ int sunxi_simplefb_setup(void *blob)
 	 * linux/arch/arm/mm/ioremap.c around line 301.
 	 */
 	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);
 	if (ret) {
 		eprintf("Cannot setup simplefb: Error reserving memory\n");
 		return ret;
 	}
 
-	ret = fdt_setup_simplefb_node(blob, offset, sunxi_display.fb_addr,
-			graphic_device->winSizeX, graphic_device->winSizeY,
-			graphic_device->plnSizeX, "x8r8g8b8");
+	ret = fdt_setup_simplefb_node(blob, offset, sunxi_display->fb_addr,
+				      uc_priv->xsize, uc_priv->ysize,
+				      VNBYTES(uc_priv->bpix) * uc_priv->xsize,
+				      "x8r8g8b8");
 	if (ret)
 		eprintf("Cannot setup simplefb: Error setting properties\n");
 
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 000f3864702..97661329ba4 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -256,23 +256,6 @@ extern int soft_i2c_gpio_scl;
 #endif
 #endif /* ifdef CONFIG_REQUIRE_SERIAL_CONSOLE */
 
-#ifdef CONFIG_VIDEO_SUNXI
-/*
- * The amount of RAM to keep free at the top of RAM when relocating u-boot,
- * to use as framebuffer. This must be a multiple of 4096.
- */
-#define CONFIG_SUNXI_MAX_FB_SIZE (16 << 20)
-
-#define CONFIG_VIDEO_LOGO
-#define CONFIG_VIDEO_STD_TIMINGS
-#define CONFIG_I2C_EDID
-#define VIDEO_LINE_LEN (pGD->plnSizeX)
-
-/* allow both serial and cfb console. */
-/* stop x86 thinking in cfbconsole from trying to init a pc keyboard */
-
-#endif /* CONFIG_VIDEO_SUNXI */
-
 /* Ethernet support */
 
 #ifdef CONFIG_USB_EHCI_HCD
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index c6a83124956..ed469f74034 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -1661,7 +1661,6 @@ CONFIG_STV0991
 CONFIG_STV0991_HZ
 CONFIG_STV0991_HZ_CLOCK
 CONFIG_ST_SMI
-CONFIG_SUNXI_MAX_FB_SIZE
 CONFIG_SXNI855T
 CONFIG_SYSFLAGS_ADDR
 CONFIG_SYSFS
-- 
2.17.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-05  1:07 [PATCH v3] video: sunxi_display: Convert to DM_VIDEO Andre Przywara
@ 2021-02-05 16:04 ` Maxime Ripard
  2021-02-05 16:27 ` Jernej Škrabec
  2021-02-07 14:37 ` Simon Glass
  2 siblings, 0 replies; 15+ messages in thread
From: Maxime Ripard @ 2021-02-05 16:04 UTC (permalink / raw)
  To: u-boot

On Fri, Feb 05, 2021 at 01:07:48AM +0000, Andre Przywara wrote:
> From: Jagan Teki <jagan@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@amarulasolutions.com>
> [Andre: rebase and smaller fixes]
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-05  1:07 [PATCH v3] video: sunxi_display: Convert to DM_VIDEO Andre Przywara
  2021-02-05 16:04 ` Maxime Ripard
@ 2021-02-05 16:27 ` Jernej Škrabec
  2021-02-05 19:31   ` Andre Przywara
  2021-02-07 14:37 ` Simon Glass
  2 siblings, 1 reply; 15+ messages in thread
From: Jernej Škrabec @ 2021-02-05 16:27 UTC (permalink / raw)
  To: u-boot

Dne petek, 05. februar 2021 ob 02:07:48 CET je Andre Przywara napisal(a):
> From: Jagan Teki <jagan@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@amarulasolutions.com>
> [Andre: rebase and smaller fixes]
> Signed-off-by: Andre Przywara <andre.przywara@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(-)
> 
> 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
> +	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"

<snip>

> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index 000f3864702..97661329ba4 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -256,23 +256,6 @@ extern int soft_i2c_gpio_scl;
>  #endif
>  #endif /* ifdef CONFIG_REQUIRE_SERIAL_CONSOLE */
>  
> -#ifdef CONFIG_VIDEO_SUNXI
> -/*
> - * The amount of RAM to keep free at the top of RAM when relocating u-boot,
> - * to use as framebuffer. This must be a multiple of 4096.
> - */
> -#define CONFIG_SUNXI_MAX_FB_SIZE (16 << 20)
> -
> -#define CONFIG_VIDEO_LOGO
> -#define CONFIG_VIDEO_STD_TIMINGS
> -#define CONFIG_I2C_EDID
> -#define VIDEO_LINE_LEN (pGD->plnSizeX)
> -
> -/* allow both serial and cfb console. */
> -/* stop x86 thinking in cfbconsole from trying to init a pc keyboard */
> -
> -#endif /* CONFIG_VIDEO_SUNXI */
> -
>  /* Ethernet support */
>  
>  #ifdef CONFIG_USB_EHCI_HCD

This file has another ifdef which can be removed (#ifdef CONFIG_VIDEO).

After that,
Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> index c6a83124956..ed469f74034 100644
> --- a/scripts/config_whitelist.txt
> +++ b/scripts/config_whitelist.txt
> @@ -1661,7 +1661,6 @@ CONFIG_STV0991
>  CONFIG_STV0991_HZ
>  CONFIG_STV0991_HZ_CLOCK
>  CONFIG_ST_SMI
> -CONFIG_SUNXI_MAX_FB_SIZE
>  CONFIG_SXNI855T
>  CONFIG_SYSFLAGS_ADDR
>  CONFIG_SYSFS
> -- 
> 2.17.5
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-05 16:27 ` Jernej Škrabec
@ 2021-02-05 19:31   ` Andre Przywara
  0 siblings, 0 replies; 15+ messages in thread
From: Andre Przywara @ 2021-02-05 19:31 UTC (permalink / raw)
  To: u-boot

On Fri, 05 Feb 2021 17:27:25 +0100
Jernej ?krabec <jernej.skrabec@siol.net> wrote:

> Dne petek, 05. februar 2021 ob 02:07:48 CET je Andre Przywara napisal(a):
> > From: Jagan Teki <jagan@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@amarulasolutions.com>
> > [Andre: rebase and smaller fixes]
> > Signed-off-by: Andre Przywara <andre.przywara@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(-)
> > 
> > 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
> > +	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"  
> 
> <snip>
> 
> > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> > index 000f3864702..97661329ba4 100644
> > --- a/include/configs/sunxi-common.h
> > +++ b/include/configs/sunxi-common.h
> > @@ -256,23 +256,6 @@ extern int soft_i2c_gpio_scl;
> >  #endif
> >  #endif /* ifdef CONFIG_REQUIRE_SERIAL_CONSOLE */
> >  
> > -#ifdef CONFIG_VIDEO_SUNXI
> > -/*
> > - * The amount of RAM to keep free at the top of RAM when relocating u-boot,
> > - * to use as framebuffer. This must be a multiple of 4096.
> > - */
> > -#define CONFIG_SUNXI_MAX_FB_SIZE (16 << 20)
> > -
> > -#define CONFIG_VIDEO_LOGO
> > -#define CONFIG_VIDEO_STD_TIMINGS
> > -#define CONFIG_I2C_EDID
> > -#define VIDEO_LINE_LEN (pGD->plnSizeX)
> > -
> > -/* allow both serial and cfb console. */
> > -/* stop x86 thinking in cfbconsole from trying to init a pc keyboard */
> > -
> > -#endif /* CONFIG_VIDEO_SUNXI */
> > -
> >  /* Ethernet support */
> >  
> >  #ifdef CONFIG_USB_EHCI_HCD  
> 
> This file has another ifdef which can be removed (#ifdef CONFIG_VIDEO).

Ah, indeed, thanks for pointing this out.

I think they are even more cleanups lurking in the shadows ;-)

> After that,
> Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Thanks!
Andre

> 
> Best regards,
> Jernej
> 
> > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> > index c6a83124956..ed469f74034 100644
> > --- a/scripts/config_whitelist.txt
> > +++ b/scripts/config_whitelist.txt
> > @@ -1661,7 +1661,6 @@ CONFIG_STV0991
> >  CONFIG_STV0991_HZ
> >  CONFIG_STV0991_HZ_CLOCK
> >  CONFIG_ST_SMI
> > -CONFIG_SUNXI_MAX_FB_SIZE
> >  CONFIG_SXNI855T
> >  CONFIG_SYSFLAGS_ADDR
> >  CONFIG_SYSFS
> > -- 
> > 2.17.5
> > 
> >   
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-05  1:07 [PATCH v3] video: sunxi_display: Convert to DM_VIDEO Andre Przywara
  2021-02-05 16:04 ` Maxime Ripard
  2021-02-05 16:27 ` Jernej Škrabec
@ 2021-02-07 14:37 ` Simon Glass
  2021-02-08  1:36   ` Andre Przywara
  2021-02-21  0:07   ` Andre Przywara
  2 siblings, 2 replies; 15+ messages in thread
From: Simon Glass @ 2021-02-07 14:37 UTC (permalink / raw)
  To: u-boot

Hi Andre,

On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przywara@arm.com> wrote:
>
> From: Jagan Teki <jagan@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@amarulasolutions.com>
> [Andre: rebase and smaller fixes]
> Signed-off-by: Andre Przywara <andre.przywara@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@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.

>  #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()?

> -               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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-07 14:37 ` Simon Glass
@ 2021-02-08  1:36   ` Andre Przywara
  2021-02-08  4:21     ` Simon Glass
  2021-02-21  0:07   ` Andre Przywara
  1 sibling, 1 reply; 15+ messages in thread
From: Andre Przywara @ 2021-02-08  1:36 UTC (permalink / raw)
  To: u-boot

On Sun, 7 Feb 2021 07:37:34 -0700
Simon Glass <sjg@chromium.org> wrote:

Hi Simon,
 
> On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > From: Jagan Teki <jagan@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@amarulasolutions.com>
> > [Andre: rebase and smaller fixes]
> > Signed-off-by: Andre Przywara <andre.przywara@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@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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-08  1:36   ` Andre Przywara
@ 2021-02-08  4:21     ` Simon Glass
  0 siblings, 0 replies; 15+ messages in thread
From: Simon Glass @ 2021-02-08  4:21 UTC (permalink / raw)
  To: u-boot

Hi Andre,

On Sun, 7 Feb 2021 at 18:37, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 7 Feb 2021 07:37:34 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> > On Thu, 4 Feb 2021 at 18:08, Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > From: Jagan Teki <jagan@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@amarulasolutions.com>
> > > [Andre: rebase and smaller fixes]
> > > Signed-off-by: Andre Przywara <andre.przywara@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@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.

That's fine...you have my review tag. My comments are just suggestions
for improvement and much of it relates to the existing code.

>
> >
> > > 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?

That sounds fine to me, but what happens when someone selects a DM and
non-DM driver? That has always been the intent - to ensure that people
are selecting DM for a subsystem on purpose.

Of course these days most stuff is DM, so perhaps that approach is
obsolete and we should do what you say. I'm OK with that.

>
> Also: with CONFIG_VIDEO now dying, CONFIG_DM_VIDEO somewhat loses its
> purpose, doesn't it?

All of the CONFIG_DM... things should be removed at some point, yet.
Just waiting for the mythical deadlines...

[...]

Regards,
Simon

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-07 14:37 ` Simon Glass
  2021-02-08  1:36   ` Andre Przywara
@ 2021-02-21  0:07   ` Andre Przywara
  2021-02-21  0:45     ` Tom Rini
                       ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Andre Przywara @ 2021-02-21  0:07 UTC (permalink / raw)
  To: u-boot

On Sun, 7 Feb 2021 07:37:34 -0700
Simon Glass <sjg@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@arm.com> wrote:
> >
> > From: Jagan Teki <jagan@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@amarulasolutions.com>
> > [Andre: rebase and smaller fixes]
> > Signed-off-by: Andre Przywara <andre.przywara@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@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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21  0:07   ` Andre Przywara
@ 2021-02-21  0:45     ` Tom Rini
  2021-02-21 16:47       ` Anatolij Gustschin
  2021-02-21 16:24     ` Simon Glass
  2021-02-22  8:59     ` Maxime Ripard
  2 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-02-21  0:45 UTC (permalink / raw)
  To: u-boot

On Sun, Feb 21, 2021 at 12:07:35AM +0000, Andre Przywara wrote:

> On Sun, 7 Feb 2021 07:37:34 -0700
> Simon Glass <sjg@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.

For the record, I'm OK with pulling this patch in as-is and improving
the situation rather than requiring further changes right now.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210220/fe0b532f/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21  0:07   ` Andre Przywara
  2021-02-21  0:45     ` Tom Rini
@ 2021-02-21 16:24     ` Simon Glass
  2021-02-22  9:02       ` Maxime Ripard
  2021-02-22  8:59     ` Maxime Ripard
  2 siblings, 1 reply; 15+ messages in thread
From: Simon Glass @ 2021-02-21 16:24 UTC (permalink / raw)
  To: u-boot

Hi Andre,

On Sat, 20 Feb 2021 at 17:08, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 7 Feb 2021 07:37:34 -0700
> Simon Glass <sjg@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@arm.com> wrote:
> > >
> > > From: Jagan Teki <jagan@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@amarulasolutions.com>
> > > [Andre: rebase and smaller fixes]
> > > Signed-off-by: Andre Przywara <andre.przywara@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@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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21  0:45     ` Tom Rini
@ 2021-02-21 16:47       ` Anatolij Gustschin
  2021-02-22  0:17         ` Andre Przywara
  0 siblings, 1 reply; 15+ messages in thread
From: Anatolij Gustschin @ 2021-02-21 16:47 UTC (permalink / raw)
  To: u-boot

On Sat, 20 Feb 2021 19:45:17 -0500
Tom Rini trini at konsulko.com wrote:

> On Sun, Feb 21, 2021 at 12:07:35AM +0000, Andre Przywara wrote:
> 
> > On Sun, 7 Feb 2021 07:37:34 -0700
> > Simon Glass <sjg@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.  
> 
> For the record, I'm OK with pulling this patch in as-is and improving
> the situation rather than requiring further changes right now.

OK, thanks! I'll queue it and will submit a pull request soon.

--
Anatolij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21 16:47       ` Anatolij Gustschin
@ 2021-02-22  0:17         ` Andre Przywara
  2021-02-22  7:37           ` Anatolij Gustschin
  0 siblings, 1 reply; 15+ messages in thread
From: Andre Przywara @ 2021-02-22  0:17 UTC (permalink / raw)
  To: u-boot

On Sun, 21 Feb 2021 17:47:53 +0100
Anatolij Gustschin <agust@denx.de> wrote:

Hi Anatolij,

> On Sat, 20 Feb 2021 19:45:17 -0500
> Tom Rini trini at konsulko.com wrote:
> 
> > On Sun, Feb 21, 2021 at 12:07:35AM +0000, Andre Przywara wrote:
> >   
> > > On Sun, 7 Feb 2021 07:37:34 -0700
> > > Simon Glass <sjg@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.    
> > 
> > For the record, I'm OK with pulling this patch in as-is and improving
> > the situation rather than requiring further changes right now.  
> 
> OK, thanks! I'll queue it and will submit a pull request soon.

Thanks for that. I just sent a v4 with some small fixes and all the
tags.

And just to be sure: with "queue it" you mean for -next, so the April
merge window? Because I would feel less comfortable with merging it now,
as this has not been widely tested yet (only on A10 and A20, but not on
all those other SoCs, especially in tablets with native LCDs).

Thanks!
Andre.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-22  0:17         ` Andre Przywara
@ 2021-02-22  7:37           ` Anatolij Gustschin
  0 siblings, 0 replies; 15+ messages in thread
From: Anatolij Gustschin @ 2021-02-22  7:37 UTC (permalink / raw)
  To: u-boot

Hi Andre,

On Mon, 22 Feb 2021 00:17:05 +0000
Andre Przywara andre.przywara at arm.com wrote:
...
> Thanks for that. I just sent a v4 with some small fixes and all the
> tags.
> 
> And just to be sure: with "queue it" you mean for -next, so the April
> merge window?

I meant to queue it for my build testing, to merge the patch for
v2021.04-rc3.

> Because I would feel less comfortable with merging it now,
> as this has not been widely tested yet (only on A10 and A20, but not on
> all those other SoCs, especially in tablets with native LCDs).

OK, then I'll postpone it and will merge in the April merge window.

Thanks!

--
Anatolij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21  0:07   ` Andre Przywara
  2021-02-21  0:45     ` Tom Rini
  2021-02-21 16:24     ` Simon Glass
@ 2021-02-22  8:59     ` Maxime Ripard
  2 siblings, 0 replies; 15+ messages in thread
From: Maxime Ripard @ 2021-02-22  8:59 UTC (permalink / raw)
  To: u-boot

Hi Andre,

On Sun, Feb 21, 2021 at 12:07:35AM +0000, Andre Przywara wrote:
> > >  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.

So the overscan will put the edges of any displayed image out of the
visible area of the screen, right? The amount of content non-visible due
to this isn't really standard and any display will behave differently
there.

One way to deal with this is to rescale the composited image to a
slightly smaller and center it in the middle of the screen.

We don't have that option though, so the code will instead render in a
slightly smaller resolution, centered in the framebuffer.

Note that composite isn't the only output that you can test this on:
HDMI TVs will also use overscan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/27d6640d/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] video: sunxi_display: Convert to DM_VIDEO
  2021-02-21 16:24     ` Simon Glass
@ 2021-02-22  9:02       ` Maxime Ripard
  0 siblings, 0 replies; 15+ messages in thread
From: Maxime Ripard @ 2021-02-22  9:02 UTC (permalink / raw)
  To: u-boot

On Sun, Feb 21, 2021 at 09:24:18AM -0700, Simon Glass wrote:
> > > > +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.

Seems Andre seems to be motivated to rework that driver, I guess we
could also rework how this is done.

Ideally, we should be describing the reserved buffer through a
reserved-memory node in the DT instead of carving out some memory for
Linux. That way, if we don't have simplefb support, or it's replaced by
something else eventually, we have a chance a claiming that memory back
at some point, instead of just ignoring it.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/bf7f06fc/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-02-22  9:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05  1:07 [PATCH v3] video: sunxi_display: Convert to DM_VIDEO Andre Przywara
2021-02-05 16:04 ` Maxime Ripard
2021-02-05 16:27 ` Jernej Škrabec
2021-02-05 19:31   ` Andre Przywara
2021-02-07 14:37 ` Simon Glass
2021-02-08  1:36   ` Andre Przywara
2021-02-08  4:21     ` Simon Glass
2021-02-21  0:07   ` Andre Przywara
2021-02-21  0:45     ` Tom Rini
2021-02-21 16:47       ` Anatolij Gustschin
2021-02-22  0:17         ` Andre Przywara
2021-02-22  7:37           ` Anatolij Gustschin
2021-02-21 16:24     ` Simon Glass
2021-02-22  9:02       ` Maxime Ripard
2021-02-22  8:59     ` Maxime Ripard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.