[PATCH v2 00/10] drm: Add driver for PowerPC OF displays

[PATCH v2 00/10] drm: Add driver for PowerPC OF displays

[Thomas Zimmermann]

(was: drm: Add driverof PowerPC OF displays)

PowerPC's Open Firmware offers a simple display buffer for graphics
output. Add ofdrm, a DRM driver for the device. As with the existing
simpledrm driver, the graphics hardware is pre-initialized by the
firmware. The driver only provides blitting, no actual DRM modesetting
is possible.

Version 2 of this patchset starts by cleaning up and refactoring
simpledrm, and moving some of the code in a helper library. These
functions are useful for ofdrm as well.

Patch 7 adds ofdrm, which has been significantly reworked since v1.
PCI is now optional and COMPILE_TEST is supported.

Patches 8 to 10 add support for color management. The code has been
taken from fbdev's offb. I have no hardware available for testing the
functionality. Qemu's stdvga apparently does not support gamma tables
in RGB modes. I verified that the color management code is executed
by running Gnome's night-mode settings, but the display's color tone
does not change.

Thomas Zimmermann (10):
  drm/simpledrm: Remove mem field from device structure
  drm/simpledrm: Inline device-init helpers
  drm/simpledrm: Remove pdev field from device structure
  drm/simpledrm: Compute framebuffer stride if not set
  drm/simpledrm: Convert to atomic helpers
  drm/simpledrm: Move some functionality into fwfb helper library
  drm/ofdrm: Add ofdrm for Open Firmware framebuffers
  drm/ofdrm: Add CRTC state
  drm/ofdrm: Add per-model device function
  drm/ofdrm: Support color management

 Documentation/gpu/drm-kms-helpers.rst |   12 +
 MAINTAINERS                           |    3 +
 drivers/gpu/drm/Kconfig               |    6 +
 drivers/gpu/drm/Makefile              |    3 +-
 drivers/gpu/drm/drm_fwfb_helper.c     |  301 ++++++
 drivers/gpu/drm/tiny/Kconfig          |   15 +
 drivers/gpu/drm/tiny/Makefile         |    1 +
 drivers/gpu/drm/tiny/ofdrm.c          | 1301 +++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c      |  588 +++++------
 drivers/video/fbdev/Kconfig           |    1 +
 include/drm/drm_fwfb_helper.h         |   51 +
 11 files changed, 1949 insertions(+), 333 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_fwfb_helper.c
 create mode 100644 drivers/gpu/drm/tiny/ofdrm.c
 create mode 100644 include/drm/drm_fwfb_helper.h

-- 
2.36.1

[PATCH v2 01/10] drm/simpledrm: Remove mem field from device structure

[Thomas Zimmermann]

Remove the unused mem field from struct simpledrm_device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 768242a78e2b..9fd507119372 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -217,7 +217,6 @@ struct simpledrm_device {
 	unsigned int pitch;
 
 	/* memory management */
-	struct resource *mem;
 	void __iomem *screen_base;
 
 	/* modesetting */
@@ -558,7 +557,6 @@ static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
 	if (!screen_base)
 		return -ENOMEM;
 
-	sdev->mem = mem;
 	sdev->screen_base = screen_base;
 
 	return 0;
-- 
2.36.1

Re: [PATCH v2 01/10] drm/simpledrm: Remove mem field from device structure

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Remove the unused mem field from struct simpledrm_device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 02/10] drm/simpledrm: Inline device-init helpers

[Thomas Zimmermann]

Inline the helpers for initializing the hardware FB, the memory
management and the modesetting into the device-creation function.
No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 291 ++++++++++++++-----------------
 1 file changed, 128 insertions(+), 163 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 9fd507119372..9bc9ecf6d964 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -449,119 +449,6 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
 }
 #endif
 
-/*
- *  Simplefb settings
- */
-
-static struct drm_display_mode simpledrm_mode(unsigned int width,
-					      unsigned int height)
-{
-	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
-
-	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
-	drm_mode_set_name(&mode);
-
-	return mode;
-}
-
-static int simpledrm_device_init_fb(struct simpledrm_device *sdev)
-{
-	int width, height, stride;
-	const struct drm_format_info *format;
-	struct drm_device *dev = &sdev->dev;
-	struct platform_device *pdev = sdev->pdev;
-	const struct simplefb_platform_data *pd = dev_get_platdata(&pdev->dev);
-	struct device_node *of_node = pdev->dev.of_node;
-
-	if (pd) {
-		width = simplefb_get_width_pd(dev, pd);
-		if (width < 0)
-			return width;
-		height = simplefb_get_height_pd(dev, pd);
-		if (height < 0)
-			return height;
-		stride = simplefb_get_stride_pd(dev, pd);
-		if (stride < 0)
-			return stride;
-		format = simplefb_get_format_pd(dev, pd);
-		if (IS_ERR(format))
-			return PTR_ERR(format);
-	} else if (of_node) {
-		width = simplefb_get_width_of(dev, of_node);
-		if (width < 0)
-			return width;
-		height = simplefb_get_height_of(dev, of_node);
-		if (height < 0)
-			return height;
-		stride = simplefb_get_stride_of(dev, of_node);
-		if (stride < 0)
-			return stride;
-		format = simplefb_get_format_of(dev, of_node);
-		if (IS_ERR(format))
-			return PTR_ERR(format);
-	} else {
-		drm_err(dev, "no simplefb configuration found\n");
-		return -ENODEV;
-	}
-
-	sdev->mode = simpledrm_mode(width, height);
-	sdev->format = format;
-	sdev->pitch = stride;
-
-	drm_dbg_kms(dev, "display mode={" DRM_MODE_FMT "}\n",
-		    DRM_MODE_ARG(&sdev->mode));
-	drm_dbg_kms(dev,
-		    "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
-		    &format->format, width, height, stride);
-
-	return 0;
-}
-
-/*
- * Memory management
- */
-
-static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
-{
-	struct drm_device *dev = &sdev->dev;
-	struct platform_device *pdev = sdev->pdev;
-	struct resource *res, *mem;
-	void __iomem *screen_base;
-	int ret;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-
-	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
-	if (ret) {
-		drm_err(dev, "could not acquire memory range %pr: error %d\n",
-			res, ret);
-		return ret;
-	}
-
-	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
-				      sdev->dev.driver->name);
-	if (!mem) {
-		/*
-		 * We cannot make this fatal. Sometimes this comes from magic
-		 * spaces our resource handlers simply don't know about. Use
-		 * the I/O-memory resource as-is and try to map that instead.
-		 */
-		drm_warn(dev, "could not acquire memory region %pr\n", res);
-		mem = res;
-	}
-
-	screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
-				      resource_size(mem));
-	if (!screen_base)
-		return -ENOMEM;
-
-	sdev->screen_base = screen_base;
-
-	return 0;
-}
-
 /*
  * Modesetting
  */
@@ -738,6 +625,21 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+/*
+ * Init / Cleanup
+ */
+
+static struct drm_display_mode simpledrm_mode(unsigned int width,
+					      unsigned int height)
+{
+	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
+
+	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
+	drm_mode_set_name(&mode);
+
+	return mode;
+}
+
 static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
 						size_t *nformats_out)
 {
@@ -777,88 +679,151 @@ static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
 	return sdev->formats;
 }
 
-static int simpledrm_device_init_modeset(struct simpledrm_device *sdev)
+static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
+							struct platform_device *pdev)
 {
-	struct drm_device *dev = &sdev->dev;
-	struct drm_display_mode *mode = &sdev->mode;
-	struct drm_connector *connector = &sdev->connector;
-	struct drm_simple_display_pipe *pipe = &sdev->pipe;
+	const struct simplefb_platform_data *pd = dev_get_platdata(&pdev->dev);
+	struct device_node *of_node = pdev->dev.of_node;
+	struct simpledrm_device *sdev;
+	struct drm_device *dev;
+	int width, height, stride;
+	const struct drm_format_info *format;
+	struct resource *res, *mem;
+	void __iomem *screen_base;
+	struct drm_connector *connector;
+	struct drm_simple_display_pipe *pipe;
 	unsigned long max_width, max_height;
 	const uint32_t *formats;
 	size_t nformats;
 	int ret;
 
+	sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
+	if (IS_ERR(sdev))
+		return ERR_CAST(sdev);
+	dev = &sdev->dev;
+	sdev->pdev = pdev;
+	platform_set_drvdata(pdev, sdev);
+
+	/*
+	 * Hardware settings
+	 */
+
+	ret = simpledrm_device_init_clocks(sdev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = simpledrm_device_init_regulators(sdev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (pd) {
+		width = simplefb_get_width_pd(dev, pd);
+		if (width < 0)
+			return ERR_PTR(width);
+		height = simplefb_get_height_pd(dev, pd);
+		if (height < 0)
+			return ERR_PTR(height);
+		stride = simplefb_get_stride_pd(dev, pd);
+		if (stride < 0)
+			return ERR_PTR(stride);
+		format = simplefb_get_format_pd(dev, pd);
+		if (IS_ERR(format))
+			return ERR_CAST(format);
+	} else if (of_node) {
+		width = simplefb_get_width_of(dev, of_node);
+		if (width < 0)
+			return ERR_PTR(width);
+		height = simplefb_get_height_of(dev, of_node);
+		if (height < 0)
+			return ERR_PTR(height);
+		stride = simplefb_get_stride_of(dev, of_node);
+		if (stride < 0)
+			return ERR_PTR(stride);
+		format = simplefb_get_format_of(dev, of_node);
+		if (IS_ERR(format))
+			return ERR_CAST(format);
+	} else {
+		drm_err(dev, "no simplefb configuration found\n");
+		return ERR_PTR(-ENODEV);
+	}
+	sdev->mode = simpledrm_mode(width, height);
+	sdev->format = format;
+	sdev->pitch = stride;
+
+	drm_dbg(dev, "display mode={" DRM_MODE_FMT "}\n", DRM_MODE_ARG(&sdev->mode));
+	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
+		&format->format, width, height, stride);
+
+	/*
+	 * Memory management
+	 */
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return ERR_PTR(-EINVAL);
+
+	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
+	if (ret) {
+		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
+		return ERR_PTR(ret);
+	}
+
+	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
+	if (!mem) {
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about. Use
+		 * the I/O-memory resource as-is and try to map that instead.
+		 */
+		drm_warn(dev, "could not acquire memory region %pr\n", res);
+		mem = res;
+	}
+
+	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
+	if (!screen_base)
+		return ERR_PTR(-ENOMEM);
+	sdev->screen_base = screen_base;
+
+	/*
+	 * Modesetting
+	 */
+
 	ret = drmm_mode_config_init(dev);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
-	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
-	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
+	max_width = max_t(unsigned long, width, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, height, DRM_SHADOW_PLANE_MAX_HEIGHT);
 
-	dev->mode_config.min_width = mode->hdisplay;
+	dev->mode_config.min_width = width;
 	dev->mode_config.max_width = max_width;
-	dev->mode_config.min_height = mode->vdisplay;
+	dev->mode_config.min_height = height;
 	dev->mode_config.max_height = max_height;
-	dev->mode_config.preferred_depth = sdev->format->cpp[0] * 8;
+	dev->mode_config.preferred_depth = format->cpp[0] * 8;
 	dev->mode_config.funcs = &simpledrm_mode_config_funcs;
 
+	connector = &sdev->connector;
 	ret = drm_connector_init(dev, connector, &simpledrm_connector_funcs,
 				 DRM_MODE_CONNECTOR_Unknown);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 	drm_connector_helper_add(connector, &simpledrm_connector_helper_funcs);
 	drm_connector_set_panel_orientation_with_quirk(connector,
 						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
-						       mode->hdisplay, mode->vdisplay);
+						       width, height);
 
 	formats = simpledrm_device_formats(sdev, &nformats);
 
+	pipe = &sdev->pipe;
 	ret = drm_simple_display_pipe_init(dev, pipe, &simpledrm_simple_display_pipe_funcs,
 					   formats, nformats, simpledrm_format_modifiers,
 					   connector);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	drm_plane_enable_fb_damage_clips(&pipe->plane);
 
 	drm_mode_config_reset(dev);
 
-	return 0;
-}
-
-/*
- * Init / Cleanup
- */
-
-static struct simpledrm_device *
-simpledrm_device_create(struct drm_driver *drv, struct platform_device *pdev)
-{
-	struct simpledrm_device *sdev;
-	int ret;
-
-	sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device,
-				  dev);
-	if (IS_ERR(sdev))
-		return ERR_CAST(sdev);
-	sdev->pdev = pdev;
-	platform_set_drvdata(pdev, sdev);
-
-	ret = simpledrm_device_init_clocks(sdev);
-	if (ret)
-		return ERR_PTR(ret);
-	ret = simpledrm_device_init_regulators(sdev);
-	if (ret)
-		return ERR_PTR(ret);
-	ret = simpledrm_device_init_fb(sdev);
-	if (ret)
-		return ERR_PTR(ret);
-	ret = simpledrm_device_init_mm(sdev);
-	if (ret)
-		return ERR_PTR(ret);
-	ret = simpledrm_device_init_modeset(sdev);
-	if (ret)
-		return ERR_PTR(ret);
-
 	return sdev;
 }
 
-- 
2.36.1

Re: [PATCH v2 02/10] drm/simpledrm: Inline device-init helpers

[Javier Martinez Canillas]

Hello Thomas,

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Inline the helpers for initializing the hardware FB, the memory
> management and the modesetting into the device-creation function.
> No functional changes.
>

Could you please elaborate in the commit message why this change is
desirable?  Without this additional context, this feels like going
backwards, since you are dropping few helpers that have quite self
contained code and making simpledrm_device_create() much larger.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 02/10] drm/simpledrm: Inline device-init helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1221 bytes --]

Hi

Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Inline the helpers for initializing the hardware FB, the memory
>> management and the modesetting into the device-creation function.
>> No functional changes.
>>
> 
> Could you please elaborate in the commit message why this change is
> desirable?  Without this additional context, this feels like going
> backwards, since you are dropping few helpers that have quite self
> contained code and making simpledrm_device_create() much larger.

To clarify: I want to make the init code more easy to follow. These old 
init functions still had to be called in the right order as each 
possibly depends on settings from the others. It also feels like it's 
easier to extract common code for ofdrm. And the pipeline is static, so 
it doesn't require complex chains of helper calls. Having everything in 
one helper seems beneficial. (It's a trade-off, I know.)

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 02/10] drm/simpledrm: Inline device-init helpers

[Javier Martinez Canillas]

Hello Thomas,

On 7/27/22 09:50, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas:
>> Hello Thomas,
>>
>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>> Inline the helpers for initializing the hardware FB, the memory
>>> management and the modesetting into the device-creation function.
>>> No functional changes.
>>>
>>
>> Could you please elaborate in the commit message why this change is
>> desirable?  Without this additional context, this feels like going
>> backwards, since you are dropping few helpers that have quite self
>> contained code and making simpledrm_device_create() much larger.
> 
> To clarify: I want to make the init code more easy to follow. These old 
> init functions still had to be called in the right order as each > possibly depends on settings from the others. It also feels like it's 
> easier to extract common code for ofdrm. And the pipeline is static, so 
> it doesn't require complex chains of helper calls. Having everything in 
> one helper seems beneficial. (It's a trade-off, I know.)
>

I see. That makes sense to me. Could you please add the explanation to
the commit message ? And feel free to add my Acked-by for this one too.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 03/10] drm/simpledrm: Remove pdev field from device structure

[Thomas Zimmermann]

Replace the remaining uses of the field pdev by upcasts from the Linux
device and remove the field.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 9bc9ecf6d964..7de477835d44 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -198,7 +198,6 @@ simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
 
 struct simpledrm_device {
 	struct drm_device dev;
-	struct platform_device *pdev;
 
 	/* clocks */
 #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
@@ -271,7 +270,7 @@ static void simpledrm_device_release_clocks(void *res)
 static int simpledrm_device_init_clocks(struct simpledrm_device *sdev)
 {
 	struct drm_device *dev = &sdev->dev;
-	struct platform_device *pdev = sdev->pdev;
+	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct device_node *of_node = pdev->dev.of_node;
 	struct clk *clock;
 	unsigned int i;
@@ -369,7 +368,7 @@ static void simpledrm_device_release_regulators(void *res)
 static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
 {
 	struct drm_device *dev = &sdev->dev;
-	struct platform_device *pdev = sdev->pdev;
+	struct platform_device *pdev = to_platform_device(dev->dev);
 	struct device_node *of_node = pdev->dev.of_node;
 	struct property *prop;
 	struct regulator *regulator;
@@ -701,7 +700,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	if (IS_ERR(sdev))
 		return ERR_CAST(sdev);
 	dev = &sdev->dev;
-	sdev->pdev = pdev;
 	platform_set_drvdata(pdev, sdev);
 
 	/*
-- 
2.36.1

Re: [PATCH v2 03/10] drm/simpledrm: Remove pdev field from device structure

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Replace the remaining uses of the field pdev by upcasts from the Linux
> device and remove the field.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Much better indeed.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Thomas Zimmermann]

Compute the framebuffer's scanline stride length if not given by
the simplefb data.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 7de477835d44..cff9e7f71f80 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 		drm_err(dev, "no simplefb configuration found\n");
 		return ERR_PTR(-ENODEV);
 	}
+	if (!stride)
+		stride = format->cpp[0] * width;
+
 	sdev->mode = simpledrm_mode(width, height);
 	sdev->format = format;
 	sdev->pitch = stride;
-- 
2.36.1

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Compute the framebuffer's scanline stride length if not given by
> the simplefb data.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your patch!

> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>                 drm_err(dev, "no simplefb configuration found\n");
>                 return ERR_PTR(-ENODEV);
>         }
> +       if (!stride)
> +               stride = format->cpp[0] * width;

DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)

> +
>         sdev->mode = simpledrm_mode(width, height);
>         sdev->format = format;
>         sdev->pitch = stride;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Javier Martinez Canillas]

Hello Geert,

On 7/21/22 16:46, Geert Uytterhoeven wrote:
> Hi Thomas,
> 
> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Compute the framebuffer's scanline stride length if not given by
>> the simplefb data.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks for your patch!
> 
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>                 drm_err(dev, "no simplefb configuration found\n");
>>                 return ERR_PTR(-ENODEV);
>>         }
>> +       if (!stride)
>> +               stride = format->cpp[0] * width;
> 
> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
>

I think you meant here:

DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?
 
With that change,

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1402 bytes --]

Hi

Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas:
> Hello Geert,
> 
> On 7/21/22 16:46, Geert Uytterhoeven wrote:
>> Hi Thomas,
>>
>> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Compute the framebuffer's scanline stride length if not given by
>>> the simplefb data.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>                  drm_err(dev, "no simplefb configuration found\n");
>>>                  return ERR_PTR(-ENODEV);
>>>          }
>>> +       if (!stride)
>>> +               stride = format->cpp[0] * width;
>>
>> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
>>
> 
> I think you meant here:
> 
> DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?

I guess, that's the right function. My original code is correct, but cpp 
is also deprecated.

Best regards
Thomas

>   
> With that change,
> 
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Daniel Vetter]

On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas:
> > Hello Geert,
> >
> > On 7/21/22 16:46, Geert Uytterhoeven wrote:
> >> Hi Thomas,
> >>
> >> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>> Compute the framebuffer's scanline stride length if not given by
> >>> the simplefb data.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/drivers/gpu/drm/tiny/simpledrm.c
> >>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> >>> @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >>>                  drm_err(dev, "no simplefb configuration found\n");
> >>>                  return ERR_PTR(-ENODEV);
> >>>          }
> >>> +       if (!stride)
> >>> +               stride = format->cpp[0] * width;
> >>
> >> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
> >>
> >
> > I think you meant here:
> >
> > DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?
>
> I guess, that's the right function. My original code is correct, but cpp
> is also deprecated.

You all mean drm_format_info_min_pitch().

I really don't want drivers to go grab any of the legacy format info
fields like bpp or depth. switch() statements on the fourcc code for
programming registers, or one of the real helper functions in
drm_fourcc.c (there might be some gaps), but not ever going through
legacy concepts. Anything else just leads to subtle bugs when new
formats get added and oops suddenly the assumptions don't hold.

Those should be strictly limited to legacy (i.e. not drm_fourcc aware)
interfaces. Heck I think even fbdev emulation should completely switch
over to drm_fourcc/drm_format_info, but alas that's a pile of work and
not much payoff.

I'm trying to volunteer Same to add a legacy_bpp tag to the above
helper and appropriately limit it, I think limiting to formats with
depth!=0 is probably the right thing. And then we should probably
remove a pile of the cargo-culted depth!=0 entries too.
-Daniel

>
> Best regards
> Thomas
>
> >
> > With that change,
> >
> > Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3028 bytes --]

Hi Daniel

Am 11.08.22 um 19:23 schrieb Daniel Vetter:
> On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas:
>>> Hello Geert,
>>>
>>> On 7/21/22 16:46, Geert Uytterhoeven wrote:
>>>> Hi Thomas,
>>>>
>>>> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>> Compute the framebuffer's scanline stride length if not given by
>>>>> the simplefb data.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>
>>>> Thanks for your patch!
>>>>
>>>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>>>> @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>>>                   drm_err(dev, "no simplefb configuration found\n");
>>>>>                   return ERR_PTR(-ENODEV);
>>>>>           }
>>>>> +       if (!stride)
>>>>> +               stride = format->cpp[0] * width;
>>>>
>>>> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
>>>>
>>>
>>> I think you meant here:
>>>
>>> DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?
>>
>> I guess, that's the right function. My original code is correct, but cpp
>> is also deprecated.
> 
> You all mean drm_format_info_min_pitch().

Thanks a lot. I wasn't even aware of this function, but I had almost 
written my own implementation of it.  I'll update the patch accordingly.

Best regards
Thomas

> 
> I really don't want drivers to go grab any of the legacy format info
> fields like bpp or depth. switch() statements on the fourcc code for
> programming registers, or one of the real helper functions in
> drm_fourcc.c (there might be some gaps), but not ever going through
> legacy concepts. Anything else just leads to subtle bugs when new
> formats get added and oops suddenly the assumptions don't hold.
> 
> Those should be strictly limited to legacy (i.e. not drm_fourcc aware)
> interfaces. Heck I think even fbdev emulation should completely switch
> over to drm_fourcc/drm_format_info, but alas that's a pile of work and
> not much payoff.
> 
> I'm trying to volunteer Same to add a legacy_bpp tag to the above
> helper and appropriately limit it, I think limiting to formats with
> depth!=0 is probably the right thing. And then we should probably
> remove a pile of the cargo-culted depth!=0 entries too.
> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> With that change,
>>>
>>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3312 bytes --]



Am 11.08.22 um 20:26 schrieb Thomas Zimmermann:
> Hi Daniel
> 
> Am 11.08.22 um 19:23 schrieb Daniel Vetter:
>> On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann <tzimmermann@suse.de> 
>> wrote:
>>>
>>> Hi
>>>
>>> Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas:
>>>> Hello Geert,
>>>>
>>>> On 7/21/22 16:46, Geert Uytterhoeven wrote:
>>>>> Hi Thomas,
>>>>>
>>>>> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann 
>>>>> <tzimmermann@suse.de> wrote:
>>>>>> Compute the framebuffer's scanline stride length if not given by
>>>>>> the simplefb data.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>
>>>>> Thanks for your patch!
>>>>>
>>>>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>>>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>>>>> @@ -743,6 +743,9 @@ static struct simpledrm_device 
>>>>>> *simpledrm_device_create(struct drm_driver *drv,
>>>>>>                   drm_err(dev, "no simplefb configuration found\n");
>>>>>>                   return ERR_PTR(-ENODEV);
>>>>>>           }
>>>>>> +       if (!stride)
>>>>>> +               stride = format->cpp[0] * width;
>>>>>
>>>>> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
>>>>>
>>>>
>>>> I think you meant here:
>>>>
>>>> DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?
>>>
>>> I guess, that's the right function. My original code is correct, but cpp
>>> is also deprecated.
>>
>> You all mean drm_format_info_min_pitch().
> 
> Thanks a lot. I wasn't even aware of this function, but I had almost 
> written my own implementation of it.  I'll update the patch accordingly.

Arghh, too late. I merged that patch already.

> 
> Best regards
> Thomas
> 
>>
>> I really don't want drivers to go grab any of the legacy format info
>> fields like bpp or depth. switch() statements on the fourcc code for
>> programming registers, or one of the real helper functions in
>> drm_fourcc.c (there might be some gaps), but not ever going through
>> legacy concepts. Anything else just leads to subtle bugs when new
>> formats get added and oops suddenly the assumptions don't hold.
>>
>> Those should be strictly limited to legacy (i.e. not drm_fourcc aware)
>> interfaces. Heck I think even fbdev emulation should completely switch
>> over to drm_fourcc/drm_format_info, but alas that's a pile of work and
>> not much payoff.
>>
>> I'm trying to volunteer Same to add a legacy_bpp tag to the above
>> helper and appropriately limit it, I think limiting to formats with
>> depth!=0 is probably the right thing. And then we should probably
>> remove a pile of the cargo-culted depth!=0 entries too.
>> -Daniel
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> With that change,
>>>>
>>>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>
>>>
>>> -- 
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> (HRB 36809, AG Nürnberg)
>>> Geschäftsführer: Ivo Totev
>>
>>
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set

[Daniel Vetter]

On Thu, Aug 11, 2022 at 08:27:42PM +0200, Thomas Zimmermann wrote:
> 
> 
> Am 11.08.22 um 20:26 schrieb Thomas Zimmermann:
> > Hi Daniel
> > 
> > Am 11.08.22 um 19:23 schrieb Daniel Vetter:
> > > On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann
> > > <tzimmermann@suse.de> wrote:
> > > > 
> > > > Hi
> > > > 
> > > > Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas:
> > > > > Hello Geert,
> > > > > 
> > > > > On 7/21/22 16:46, Geert Uytterhoeven wrote:
> > > > > > Hi Thomas,
> > > > > > 
> > > > > > On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann
> > > > > > <tzimmermann@suse.de> wrote:
> > > > > > > Compute the framebuffer's scanline stride length if not given by
> > > > > > > the simplefb data.
> > > > > > > 
> > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > > 
> > > > > > Thanks for your patch!
> > > > > > 
> > > > > > > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > > > > > > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > > > > > > @@ -743,6 +743,9 @@ static struct simpledrm_device
> > > > > > > *simpledrm_device_create(struct drm_driver *drv,
> > > > > > >                   drm_err(dev, "no simplefb configuration found\n");
> > > > > > >                   return ERR_PTR(-ENODEV);
> > > > > > >           }
> > > > > > > +       if (!stride)
> > > > > > > +               stride = format->cpp[0] * width;
> > > > > > 
> > > > > > DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
> > > > > > 
> > > > > 
> > > > > I think you meant here:
> > > > > 
> > > > > DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ?
> > > > 
> > > > I guess, that's the right function. My original code is correct, but cpp
> > > > is also deprecated.
> > > 
> > > You all mean drm_format_info_min_pitch().
> > 
> > Thanks a lot. I wasn't even aware of this function, but I had almost
> > written my own implementation of it.  I'll update the patch accordingly.
> 
> Arghh, too late. I merged that patch already.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Preemptively, if you can do the fixup patch (and it's not yet merged)?
-Daniel

> 
> > 
> > Best regards
> > Thomas
> > 
> > > 
> > > I really don't want drivers to go grab any of the legacy format info
> > > fields like bpp or depth. switch() statements on the fourcc code for
> > > programming registers, or one of the real helper functions in
> > > drm_fourcc.c (there might be some gaps), but not ever going through
> > > legacy concepts. Anything else just leads to subtle bugs when new
> > > formats get added and oops suddenly the assumptions don't hold.
> > > 
> > > Those should be strictly limited to legacy (i.e. not drm_fourcc aware)
> > > interfaces. Heck I think even fbdev emulation should completely switch
> > > over to drm_fourcc/drm_format_info, but alas that's a pile of work and
> > > not much payoff.
> > > 
> > > I'm trying to volunteer Same to add a legacy_bpp tag to the above
> > > helper and appropriately limit it, I think limiting to formats with
> > > depth!=0 is probably the right thing. And then we should probably
> > > remove a pile of the cargo-culted depth!=0 entries too.
> > > -Daniel
> > > 
> > > > 
> > > > Best regards
> > > > Thomas
> > > > 
> > > > > 
> > > > > With that change,
> > > > > 
> > > > > Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> > > > > 
> > > > 
> > > > -- 
> > > > Thomas Zimmermann
> > > > Graphics Driver Developer
> > > > SUSE Software Solutions Germany GmbH
> > > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > (HRB 36809, AG Nürnberg)
> > > > Geschäftsführer: Ivo Totev
> > > 
> > > 
> > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 05/10] drm/simpledrm: Convert to atomic helpers

[Thomas Zimmermann]

Replace the simple-KMS helpers with the regular atomic helpers. The
regular helpers are better architectured and therefore allow for easier
code sharing among drivers. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++-----------
 1 file changed, 180 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index cff9e7f71f80..727ab9d86bcc 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -8,6 +8,7 @@
 #include <linux/regulator/consumer.h>
 
 #include <drm/drm_aperture.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_damage_helper.h>
@@ -20,8 +21,8 @@
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #define DRIVER_NAME	"simpledrm"
 #define DRIVER_DESC	"DRM driver for simple-framebuffer platform devices"
@@ -221,8 +222,10 @@ struct simpledrm_device {
 	/* modesetting */
 	uint32_t formats[8];
 	size_t nformats;
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
 	struct drm_connector connector;
-	struct drm_simple_display_pipe pipe;
 };
 
 static struct simpledrm_device *simpledrm_device_of_dev(struct drm_device *dev)
@@ -460,7 +463,7 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
  * TODO: Add blit helpers for remaining formats and uncomment
  *       constants.
  */
-static const uint32_t simpledrm_default_formats[] = {
+static const uint32_t simpledrm_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
 	DRM_FORMAT_RGB565,
@@ -471,73 +474,43 @@ static const uint32_t simpledrm_default_formats[] = {
 	DRM_FORMAT_ARGB2101010,
 };
 
-static const uint64_t simpledrm_format_modifiers[] = {
+static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
 };
 
-static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
+static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						       struct drm_atomic_state *new_state)
 {
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
-	struct drm_display_mode *mode;
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_crtc_state *new_crtc_state;
+	int ret;
 
-	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
-	if (!mode)
+	if (!new_plane_state->fb)
 		return 0;
 
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	if (mode->width_mm)
-		connector->display_info.width_mm = mode->width_mm;
-	if (mode->height_mm)
-		connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
-}
+	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
 
-static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
-	.get_modes = simpledrm_connector_helper_get_modes,
-};
-
-static const struct drm_connector_funcs simpledrm_connector_funcs = {
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int
-simpledrm_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
-				    const struct drm_display_mode *mode)
-{
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(pipe->crtc.dev);
-
-	if (mode->hdisplay != sdev->mode.hdisplay &&
-	    mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_SIZE;
-	else if (mode->hdisplay != sdev->mode.hdisplay)
-		return MODE_ONE_WIDTH;
-	else if (mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_HEIGHT;
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	if (ret)
+		return ret;
 
-	return MODE_OK;
+	return 0;
 }
 
-static void
-simpledrm_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
-				     struct drm_crtc_state *crtc_state,
-				     struct drm_plane_state *plane_state)
+static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
+							 struct drm_atomic_state *old_state)
 {
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(pipe->crtc.dev);
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
-	struct drm_framebuffer *fb = plane_state->fb;
 	void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping abstraction */
-	struct drm_device *dev = &sdev->dev;
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_device *dev = plane->dev;
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
 	void __iomem *dst = sdev->screen_base;
 	struct drm_rect src_clip, dst_clip;
 	int idx;
@@ -545,7 +518,8 @@ simpledrm_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	if (!fb)
 		return;
 
-	drm_rect_fp_to_int(&src_clip, &plane_state->src);
+	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+		return;
 
 	dst_clip = plane_state->dst;
 	if (!drm_rect_intersect(&dst_clip, &src_clip))
@@ -560,11 +534,11 @@ simpledrm_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 	drm_dev_exit(idx);
 }
 
-static void
-simpledrm_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
+static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
+							  struct drm_atomic_state *old_state)
 {
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(pipe->crtc.dev);
-	struct drm_device *dev = &sdev->dev;
+	struct drm_device *dev = plane->dev;
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
 	int idx;
 
 	if (!drm_dev_enter(dev, &idx))
@@ -576,46 +550,120 @@ simpledrm_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe)
 	drm_dev_exit(idx);
 }
 
-static void
-simpledrm_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,
-				     struct drm_plane_state *old_plane_state)
+static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
+	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
+	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
+};
+
+static const struct drm_plane_funcs simpledrm_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
+							     const struct drm_display_mode *mode)
 {
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(pipe->crtc.dev);
-	struct drm_plane_state *plane_state = pipe->plane.state;
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
-	void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping abstraction */
-	struct drm_framebuffer *fb = plane_state->fb;
-	struct drm_device *dev = &sdev->dev;
-	void __iomem *dst = sdev->screen_base;
-	struct drm_rect src_clip, dst_clip;
-	int idx;
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
 
-	if (!fb)
-		return;
+	if (mode->hdisplay != sdev->mode.hdisplay &&
+	    mode->vdisplay != sdev->mode.vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != sdev->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != sdev->mode.vdisplay)
+		return MODE_ONE_HEIGHT;
 
-	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
-		return;
+	return MODE_OK;
+}
 
-	dst_clip = plane_state->dst;
-	if (!drm_rect_intersect(&dst_clip, &src_clip))
-		return;
+static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
+					      struct drm_atomic_state *new_state)
+{
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
+	int ret;
 
-	if (!drm_dev_enter(dev, &idx))
-		return;
+	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+	if (ret)
+		return ret;
 
-	dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip);
-	drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, &src_clip);
+	return drm_atomic_add_affected_planes(new_state, crtc);
+}
 
-	drm_dev_exit(idx);
+static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+						struct drm_atomic_state *old_state)
+{
+	/*
+	 * Always enabled; screen updates are performed by
+	 * the primary plane's atomic_update function.
+	 */
+}
+
+static void simpledrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+						 struct drm_atomic_state *old_state)
+{
+	/*
+	 * Always enabled; disabling clears the screen in the
+	 * primary plane's atomic_disable function.
+	 */
+}
+
+static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
+	.mode_valid = simpledrm_crtc_helper_mode_valid,
+	.atomic_check = simpledrm_crtc_helper_atomic_check,
+	.atomic_enable = simpledrm_crtc_helper_atomic_enable,
+	.atomic_disable = simpledrm_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs simpledrm_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
+	if (!mode)
+		return 0;
+
+	if (mode->name[0] == '\0')
+		drm_mode_set_name(mode);
+
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	if (mode->width_mm)
+		connector->display_info.width_mm = mode->width_mm;
+	if (mode->height_mm)
+		connector->display_info.height_mm = mode->height_mm;
+
+	return 1;
 }
 
-static const struct drm_simple_display_pipe_funcs
-simpledrm_simple_display_pipe_funcs = {
-	.mode_valid = simpledrm_simple_display_pipe_mode_valid,
-	.enable = simpledrm_simple_display_pipe_enable,
-	.disable = simpledrm_simple_display_pipe_disable,
-	.update = simpledrm_simple_display_pipe_update,
-	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
+	.get_modes = simpledrm_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs simpledrm_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
@@ -653,10 +701,10 @@ static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
 	sdev->nformats = 1;
 
 	/* default formats go second */
-	for (i = 0; i < ARRAY_SIZE(simpledrm_default_formats); ++i) {
-		if (simpledrm_default_formats[i] == sdev->format->format)
+	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
+		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
 			continue; /* native format already went first */
-		sdev->formats[sdev->nformats] = simpledrm_default_formats[i];
+		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
 		sdev->nformats++;
 	}
 
@@ -689,8 +737,10 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	const struct drm_format_info *format;
 	struct resource *res, *mem;
 	void __iomem *screen_base;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
 	struct drm_connector *connector;
-	struct drm_simple_display_pipe *pipe;
 	unsigned long max_width, max_height;
 	const uint32_t *formats;
 	size_t nformats;
@@ -802,6 +852,40 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	dev->mode_config.preferred_depth = format->cpp[0] * 8;
 	dev->mode_config.funcs = &simpledrm_mode_config_funcs;
 
+	/* Primary plane */
+
+	formats = simpledrm_device_formats(sdev, &nformats);
+
+	primary_plane = &sdev->primary_plane;
+	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
+				       formats, nformats,
+				       simpledrm_primary_plane_format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+	drm_plane_helper_add(primary_plane, &simpledrm_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	/* CRTC */
+
+	crtc = &sdev->crtc;
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&simpledrm_crtc_funcs, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+	drm_crtc_helper_add(crtc, &simpledrm_crtc_helper_funcs);
+
+	/* Encoder */
+
+	encoder = &sdev->encoder;
+	ret = drm_encoder_init(dev, encoder, &simpledrm_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	/* Connector */
+
 	connector = &sdev->connector;
 	ret = drm_connector_init(dev, connector, &simpledrm_connector_funcs,
 				 DRM_MODE_CONNECTOR_Unknown);
@@ -812,17 +896,10 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
 						       width, height);
 
-	formats = simpledrm_device_formats(sdev, &nformats);
-
-	pipe = &sdev->pipe;
-	ret = drm_simple_display_pipe_init(dev, pipe, &simpledrm_simple_display_pipe_funcs,
-					   formats, nformats, simpledrm_format_modifiers,
-					   connector);
+	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ERR_PTR(ret);
 
-	drm_plane_enable_fb_damage_clips(&pipe->plane);
-
 	drm_mode_config_reset(dev);
 
 	return sdev;
-- 
2.36.1

Re: [PATCH v2 05/10] drm/simpledrm: Convert to atomic helpers

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Replace the simple-KMS helpers with the regular atomic helpers. The
> regular helpers are better architectured and therefore allow for easier
> code sharing among drivers. No functional changes.
>

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

But I've a question below...
 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++-----------
>  1 file changed, 180 insertions(+), 103 deletions(-)

[...]

> +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +						struct drm_atomic_state *old_state)
> +{
> +	/*
> +	 * Always enabled; screen updates are performed by
> +	 * the primary plane's atomic_update function.
> +	 */
> +}
> +
> +static void simpledrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +						 struct drm_atomic_state *old_state)
> +{
> +	/*
> +	 * Always enabled; disabling clears the screen in the
> +	 * primary plane's atomic_disable function.
> +	 */
> +}

...do we really need to have these ? Can't we just not set them ?

> +
> +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
> +	.mode_valid = simpledrm_crtc_helper_mode_valid,
> +	.atomic_check = simpledrm_crtc_helper_atomic_check,
> +	.atomic_enable = simpledrm_crtc_helper_atomic_enable,
> +	.atomic_disable = simpledrm_crtc_helper_atomic_disable,
> +};
> +
looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703
that says the .atomic_{en,dis}able handlers are optional.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 05/10] drm/simpledrm: Convert to atomic helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2148 bytes --]

Hi

Am 25.07.22 um 17:46 schrieb Javier Martinez Canillas:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Replace the simple-KMS helpers with the regular atomic helpers. The
>> regular helpers are better architectured and therefore allow for easier
>> code sharing among drivers. No functional changes.
>>
> 
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> But I've a question below...
>   
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++-----------
>>   1 file changed, 180 insertions(+), 103 deletions(-)
> 
> [...]
> 
>> +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>> +						struct drm_atomic_state *old_state)
>> +{
>> +	/*
>> +	 * Always enabled; screen updates are performed by
>> +	 * the primary plane's atomic_update function.
>> +	 */
>> +}
>> +
>> +static void simpledrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +						 struct drm_atomic_state *old_state)
>> +{
>> +	/*
>> +	 * Always enabled; disabling clears the screen in the
>> +	 * primary plane's atomic_disable function.
>> +	 */
>> +}
> 
> ...do we really need to have these ? Can't we just not set them ?
> 
>> +
>> +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
>> +	.mode_valid = simpledrm_crtc_helper_mode_valid,
>> +	.atomic_check = simpledrm_crtc_helper_atomic_check,
>> +	.atomic_enable = simpledrm_crtc_helper_atomic_enable,
>> +	.atomic_disable = simpledrm_crtc_helper_atomic_disable,
>> +};
>> +
> looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703
> that says the .atomic_{en,dis}able handlers are optional.
> 

The code also looks like we don't need the helpers. I mostly added them 
for the comments they contain, but I can also add those next to 
simpledrm_crtc_helper_funcs.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library

[Thomas Zimmermann]

Move some of simpledrm's functionality into a helper library. Other
drivers for firmware-provided framebuffers will also need functions
to handle fixed modes and color formats, or update the back buffer.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/drm-kms-helpers.rst |  12 +
 MAINTAINERS                           |   2 +
 drivers/gpu/drm/Kconfig               |   6 +
 drivers/gpu/drm/Makefile              |   3 +-
 drivers/gpu/drm/drm_fwfb_helper.c     | 301 ++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/Kconfig          |   1 +
 drivers/gpu/drm/tiny/simpledrm.c      | 163 ++------------
 include/drm/drm_fwfb_helper.h         |  51 +++++
 8 files changed, 398 insertions(+), 141 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_fwfb_helper.c
 create mode 100644 include/drm/drm_fwfb_helper.h

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 2d473bc64c9f..0a8815b4243e 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -421,6 +421,18 @@ OF/DT Helpers
 .. kernel-doc:: drivers/gpu/drm/drm_of.c
    :export:
 
+Firmware Framebuffer Helper Reference
+=====================================
+
+.. kernel-doc:: drivers/gpu/drm/drm_fwfb_helper.c
+   :doc: overview
+
+.. kernel-doc:: include/drm/drm_fwfb_helper.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_fwfb_helper.c
+   :export:
+
 Legacy Plane Helper Reference
 =============================
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 0f9366144d31..138680415e79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6572,9 +6572,11 @@ L:	dri-devel@lists.freedesktop.org
 S:	Maintained
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	drivers/gpu/drm/drm_aperture.c
+F:	drivers/gpu/drm/drm_fwfb_helper.c
 F:	drivers/gpu/drm/tiny/simpledrm.c
 F:	drivers/video/aperture.c
 F:	include/drm/drm_aperture.h
+F:	include/drm/drm_fwfb_helper.h
 F:	include/linux/aperture.h
 
 DRM DRIVER FOR SIS VIDEO CARDS
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1c91e1e861a5..47a82705e052 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -78,6 +78,12 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_FWFB_HELPER
+	bool
+	depends on DRM_KMS_HELPER
+	help
+	  Helpers for firmware-provided framebuffers.
+
 config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
         bool "Enable refcount backtrace history in the DP MST helpers"
 	depends on STACKTRACE_SUPPORT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 25016dcab55e..be51018a3cbf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -67,8 +67,9 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o \
 		drm_gem_framebuffer_helper.o \
 		drm_atomic_state_helper.o drm_damage_helper.o \
 		drm_format_helper.o drm_self_refresh_helper.o drm_rect.o
-drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
+drm_kms_helper-$(CONFIG_DRM_FWFB_HELPER) += drm_fwfb_helper.o
+drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
 #
diff --git a/drivers/gpu/drm/drm_fwfb_helper.c b/drivers/gpu/drm/drm_fwfb_helper.c
new file mode 100644
index 000000000000..522294f459c8
--- /dev/null
+++ b/drivers/gpu/drm/drm_fwfb_helper.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fwfb_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_print.h>
+
+/**
+ * DOC: overview
+ *
+ * The Firmware Framebuffer library FWFB provides helpers for devices with
+ * fixed-mode backing storage. It helps drivers to export a display mode of
+ * te correct size and copy updates to the backing storage.
+ */
+
+/*
+ * Assume a monitor resolution of 96 dpi to
+ * get a somewhat reasonable screen size.
+ */
+#define RES_MM(d)	\
+	(((d) * 254ul) / (96ul * 10ul))
+
+#define DRM_FWFB_MODE(hd, vd)	\
+	DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
+
+static struct drm_display_mode drm_fwfb_mode(unsigned int width, unsigned int height)
+{
+	struct drm_display_mode mode = { DRM_FWFB_MODE(width, height) };
+
+	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
+	drm_mode_set_name(&mode);
+
+	return mode;
+}
+
+/**
+ * drm_fwfb_init - Initializes an fwfb buffer
+ * @fwfb: fwfb buffer
+ * @screen_base: Address of the backing buffer in kernel address space
+ * @width: Number of pixels per scanline
+ * @height: Number of scanlines
+ * @format: Color format
+ * @pitch: Distance between two consecutive scanlines in bytes
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int drm_fwfb_init(struct drm_fwfb *fwfb, struct iosys_map *screen_base,
+		  unsigned int width, unsigned int height,
+		  const struct drm_format_info *format, unsigned int pitch)
+{
+	fwfb->screen_base = *screen_base;
+	fwfb->mode = drm_fwfb_mode(width, height);
+	fwfb->format = format;
+	fwfb->pitch = pitch;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_fwfb_init);
+
+static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
+{
+	const uint32_t *fourccs_end = fourccs + nfourccs;
+
+	while (fourccs < fourccs_end) {
+		if (*fourccs == fourcc)
+			return true;
+		++fourccs;
+	}
+	return false;
+}
+
+/**
+ * drm_fwfb_extra_formats - Filters a list of supported color formats against
+ *                          the device's native formats
+ * @dev: DRM device
+ * @native_fourccs: 4CC codes of natively supported color formats
+ * @native_nfourccs: The number of entries in @native_fourccs
+ * @extra_fourccs: 4CC codes of additionally supported color formats
+ * @extra_nfourccs: The number of entries in @extra_fourccs
+ * @fourccs_out: Returns 4CC codes of supported color formats
+ * @nfourccs_out: The number of available entries in @fourccs_out
+ *
+ * This function create a list of supported color format from the device's
+ * native formats and the driver's emulated formats. The returned list can
+ * be handed over to drm_universal_plane_init() et al.
+ *
+ * Returns:
+ * The number of color formats returned in @fourccs_out.
+ */
+size_t drm_fwfb_extra_formats(struct drm_device *dev,
+			      const uint32_t *native_fourccs, size_t native_nfourccs,
+			      const uint32_t *extra_fourccs, size_t extra_nfourccs,
+			      uint32_t *fourccs_out, size_t nfourccs_out)
+{
+	uint32_t *fourccs = fourccs_out;
+	const uint32_t *fourccs_end = fourccs_out + nfourccs_out;
+	bool found_native = false;
+	size_t nfourccs, i;
+
+	/* native formats go first */
+
+	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
+
+	for (i = 0; i < nfourccs; ++i) {
+		uint32_t fourcc = native_fourccs[i];
+
+		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
+
+		if (!found_native)
+			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+	/*
+	 * The plane's atomic_update helper converts the framebuffer's color format
+	 * to the native format when copying them to device memory.
+	 *
+	 * If there is not a single format supported by both, device and
+	 * plane, the native formats are likely not supported by the conversion
+	 * helpers. Therefore *only* support the native formats and add a
+	 * conversion helper ASAP.
+	 */
+	if (!found_native) {
+		drm_warn(dev, "format conversion helpers required to add extra formats\n");
+		goto out;
+	}
+
+	/* extra formats go second */
+
+	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
+
+	for (i = 0; i < nfourccs; ++i) {
+		uint32_t fourcc = extra_fourccs[i];
+
+		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
+			continue; /* native formats already went first */
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+out:
+	return fourccs - fourccs_out;
+}
+EXPORT_SYMBOL(drm_fwfb_extra_formats);
+
+/*
+ * Plane
+ */
+
+/**
+ * drm_fwfb_plane_helper_atomic_update - Helper for implementing atomic plane updates
+ * @fwfb: fwfb buffer
+ * @plane: the plane to update
+ * @old_state: the old state
+ *
+ * This function updates the plane's damaged areas in the fwfb buffer.
+ */
+void drm_fwfb_plane_helper_atomic_update(struct drm_fwfb *fwfb, struct drm_plane *plane,
+					 struct drm_atomic_state *old_state)
+{
+	struct drm_device *dev = plane->dev;
+	struct iosys_map dst = fwfb->screen_base;
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping abstraction */
+	struct drm_framebuffer *fb = plane_state->fb;
+	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
+	unsigned int dst_pitch = fwfb->pitch;
+	const struct drm_format_info *dst_format = fwfb->format;
+	struct drm_rect src_clip, dst_clip;
+	int idx;
+
+	if (!fb)
+		return;
+
+	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+		return;
+
+	dst_clip = plane_state->dst;
+	if (!drm_rect_intersect(&dst_clip, &src_clip))
+		return;
+
+	if (!drm_dev_enter(dev, &idx))
+		return;
+
+	dst_vmap += drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
+	drm_fb_blit_toio(dst_vmap, dst_pitch, dst_format->format, vmap, fb, &src_clip);
+
+	drm_dev_exit(idx);
+}
+EXPORT_SYMBOL(drm_fwfb_plane_helper_atomic_update);
+
+/**
+ * drm_fwfb_plane_helper_atomic_disable - Helper for disabling planes
+ * @fwfb: fwfb buffer
+ * @plane: the plane to disable
+ * @old_state: the old state
+ *
+ * This function clears the plane's fwfb buffer to zero.
+ */
+void drm_fwfb_plane_helper_atomic_disable(struct drm_fwfb *fwfb, struct drm_plane *plane,
+					  struct drm_atomic_state *old_state)
+{
+	struct drm_device *dev = plane->dev;
+	struct iosys_map dst = fwfb->screen_base;
+	struct drm_plane_state *plane_state = plane->state;
+	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
+	unsigned int dst_pitch = fwfb->pitch;
+	const struct drm_format_info *dst_format = fwfb->format;
+	struct drm_rect dst_clip;
+	unsigned long lines, linepixels, i;
+	int idx;
+
+	drm_rect_init(&dst_clip, plane_state->src_x >> 16, plane_state->src_y >> 16,
+		      plane_state->src_w >> 16, plane_state->src_h >> 16);
+	lines = drm_rect_height(&dst_clip);
+	linepixels = drm_rect_width(&dst_clip);
+
+	if (!drm_dev_enter(dev, &idx))
+		return;
+
+	/* Clear buffer to black if disabled */
+	dst_vmap += drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
+	for (i = 0; i < lines; ++i) {
+		memset_io(dst_vmap, 0, linepixels * dst_format->cpp[0]);
+		dst_vmap += dst_pitch;
+	}
+
+	drm_dev_exit(idx);
+}
+EXPORT_SYMBOL(drm_fwfb_plane_helper_atomic_disable);
+
+/*
+ * CRTC
+ */
+
+/**
+ * drm_fwfb_crtc_helper_mode_valid - Validates a display mode
+ * @fwfb: fwfb buffer
+ * @crtc: the crtc
+ * @mode: the mode to validate
+ */
+enum drm_mode_status drm_fwfb_crtc_helper_mode_valid(struct drm_fwfb *fwfb, struct drm_crtc *crtc,
+						     const struct drm_display_mode *mode)
+{
+
+	if (mode->hdisplay != fwfb->mode.hdisplay && mode->vdisplay != fwfb->mode.vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != fwfb->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != fwfb->mode.vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_fwfb_crtc_helper_mode_valid);
+
+/*
+ * Connector
+ */
+
+/**
+ * drm_fwfb_connector_helper_get_modes - Creates a list of display modes for a connector
+ * @fwfb: fwfb buffer
+ * @connector: the connector
+ *
+ * This function creates a list of display modes for a connector.
+ *
+ * Returns:
+ * The number of created modes.
+ */
+int drm_fwfb_connector_helper_get_modes(struct drm_fwfb *fwfb, struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, &fwfb->mode);
+	if (!mode)
+		return 0;
+
+	if (mode->name[0] == '\0')
+		drm_mode_set_name(mode);
+
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	if (mode->width_mm)
+		connector->display_info.width_mm = mode->width_mm;
+	if (mode->height_mm)
+		connector->display_info.height_mm = mode->height_mm;
+
+	return 1;
+}
+EXPORT_SYMBOL(drm_fwfb_connector_helper_get_modes);
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 027cd87c3d0d..f3d6e4713dc6 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -70,6 +70,7 @@ config DRM_SIMPLEDRM
 	tristate "Simple framebuffer driver"
 	depends on DRM && MMU
 	select APERTURE_HELPERS
+	select DRM_FWFB_HELPER
 	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
 	help
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 727ab9d86bcc..e2bf2f8abc74 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -16,6 +16,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_format_helper.h>
+#include <drm/drm_fwfb_helper.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
@@ -30,16 +31,6 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
-/*
- * Assume a monitor resolution of 96 dpi to
- * get a somewhat reasonable screen size.
- */
-#define RES_MM(d)	\
-	(((d) * 254ul) / (96ul * 10ul))
-
-#define SIMPLEDRM_MODE(hd, vd)	\
-	DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
-
 /*
  * Helpers for simplefb
  */
@@ -211,13 +202,8 @@ struct simpledrm_device {
 	struct regulator **regulators;
 #endif
 
-	/* simplefb settings */
-	struct drm_display_mode mode;
-	const struct drm_format_info *format;
-	unsigned int pitch;
-
-	/* memory management */
-	void __iomem *screen_base;
+	/* firmware framebuffer */
+	struct drm_fwfb fwfb;
 
 	/* modesetting */
 	uint32_t formats[8];
@@ -504,50 +490,17 @@ static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 							 struct drm_atomic_state *old_state)
 {
-	struct drm_plane_state *plane_state = plane->state;
-	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
-	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
-	void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping abstraction */
-	struct drm_framebuffer *fb = plane_state->fb;
-	struct drm_device *dev = plane->dev;
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
-	void __iomem *dst = sdev->screen_base;
-	struct drm_rect src_clip, dst_clip;
-	int idx;
-
-	if (!fb)
-		return;
-
-	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
-		return;
-
-	dst_clip = plane_state->dst;
-	if (!drm_rect_intersect(&dst_clip, &src_clip))
-		return;
-
-	if (!drm_dev_enter(dev, &idx))
-		return;
-
-	dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip);
-	drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, &src_clip);
-
-	drm_dev_exit(idx);
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev);
+
+	drm_fwfb_plane_helper_atomic_update(&sdev->fwfb, plane, old_state);
 }
 
 static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 							  struct drm_atomic_state *old_state)
 {
-	struct drm_device *dev = plane->dev;
-	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
-	int idx;
-
-	if (!drm_dev_enter(dev, &idx))
-		return;
-
-	/* Clear screen to black if disabled */
-	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev);
 
-	drm_dev_exit(idx);
+	drm_fwfb_plane_helper_atomic_disable(&sdev->fwfb, plane, old_state);
 }
 
 static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
@@ -569,15 +522,7 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
 
-	if (mode->hdisplay != sdev->mode.hdisplay &&
-	    mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_SIZE;
-	else if (mode->hdisplay != sdev->mode.hdisplay)
-		return MODE_ONE_WIDTH;
-	else if (mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_HEIGHT;
-
-	return MODE_OK;
+	return drm_fwfb_crtc_helper_mode_valid(&sdev->fwfb, crtc, mode);
 }
 
 static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
@@ -634,24 +579,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
 static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
-	struct drm_display_mode *mode;
-
-	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
-	if (!mode)
-		return 0;
-
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
 
-	if (mode->width_mm)
-		connector->display_info.width_mm = mode->width_mm;
-	if (mode->height_mm)
-		connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
+	return drm_fwfb_connector_helper_get_modes(&sdev->fwfb, connector);
 }
 
 static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
@@ -676,56 +605,6 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
  * Init / Cleanup
  */
 
-static struct drm_display_mode simpledrm_mode(unsigned int width,
-					      unsigned int height)
-{
-	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
-
-	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
-	drm_mode_set_name(&mode);
-
-	return mode;
-}
-
-static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
-						size_t *nformats_out)
-{
-	struct drm_device *dev = &sdev->dev;
-	size_t i;
-
-	if (sdev->nformats)
-		goto out; /* don't rebuild list on recurring calls */
-
-	/* native format goes first */
-	sdev->formats[0] = sdev->format->format;
-	sdev->nformats = 1;
-
-	/* default formats go second */
-	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
-		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
-			continue; /* native format already went first */
-		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
-		sdev->nformats++;
-	}
-
-	/*
-	 * TODO: The simpledrm driver converts framebuffers to the native
-	 * format when copying them to device memory. If there are more
-	 * formats listed than supported by the driver, the native format
-	 * is not supported by the conversion helpers. Therefore *only*
-	 * support the native format and add a conversion helper ASAP.
-	 */
-	if (drm_WARN_ONCE(dev, i != sdev->nformats,
-			  "format conversion helpers required for %p4cc",
-			  &sdev->format->format)) {
-		sdev->nformats = 1;
-	}
-
-out:
-	*nformats_out = sdev->nformats;
-	return sdev->formats;
-}
-
 static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 							struct platform_device *pdev)
 {
@@ -737,12 +616,13 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	const struct drm_format_info *format;
 	struct resource *res, *mem;
 	void __iomem *screen_base;
+	struct iosys_map screen_base_vmap;
+	struct drm_fwfb *fwfb;
 	struct drm_plane *primary_plane;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	unsigned long max_width, max_height;
-	const uint32_t *formats;
 	size_t nformats;
 	int ret;
 
@@ -796,11 +676,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	if (!stride)
 		stride = format->cpp[0] * width;
 
-	sdev->mode = simpledrm_mode(width, height);
-	sdev->format = format;
-	sdev->pitch = stride;
-
-	drm_dbg(dev, "display mode={" DRM_MODE_FMT "}\n", DRM_MODE_ARG(&sdev->mode));
 	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
 		&format->format, width, height, stride);
 
@@ -832,12 +707,17 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
 	if (!screen_base)
 		return ERR_PTR(-ENOMEM);
-	sdev->screen_base = screen_base;
+	iosys_map_set_vaddr_iomem(&screen_base_vmap, screen_base);
 
 	/*
 	 * Modesetting
 	 */
 
+	fwfb = &sdev->fwfb;
+	ret = drm_fwfb_init(fwfb, &screen_base_vmap, width, height, format, stride);
+	if (ret)
+		return ERR_PTR(ret);
+
 	ret = drmm_mode_config_init(dev);
 	if (ret)
 		return ERR_PTR(ret);
@@ -854,11 +734,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 
 	/* Primary plane */
 
-	formats = simpledrm_device_formats(sdev, &nformats);
+	nformats = drm_fwfb_extra_formats(dev, &format->format, 1,
+					  simpledrm_primary_plane_formats,
+					  ARRAY_SIZE(simpledrm_primary_plane_formats),
+					  sdev->formats, ARRAY_SIZE(sdev->formats));
 
 	primary_plane = &sdev->primary_plane;
 	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
-				       formats, nformats,
+				       sdev->formats, nformats,
 				       simpledrm_primary_plane_format_modifiers,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret)
diff --git a/include/drm/drm_fwfb_helper.h b/include/drm/drm_fwfb_helper.h
new file mode 100644
index 000000000000..2ad674e6564f
--- /dev/null
+++ b/include/drm/drm_fwfb_helper.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __DRM_FWFB_HELPER_H__
+#define __DRM_FWFB_HELPER_H__
+
+#include <linux/iosys-map.h>
+
+#include <drm/drm_modes.h>
+
+/**
+ * struct drm_fwfb - Represents fixed-mode backing storage
+ */
+struct drm_fwfb {
+	/**
+	 * @screen_base: Address of the screen memory
+	 */
+	struct iosys_map screen_base;
+	/**
+	 * @mode: Display mode
+	 */
+	struct drm_display_mode mode;
+	/**
+	 * @format: Color format
+	 */
+	const struct drm_format_info *format;
+	/**
+	 * @pitch: Distance between two consecutive scanlines in bytes
+	 */
+	unsigned int pitch;
+};
+
+int drm_fwfb_init(struct drm_fwfb *fwfb, struct iosys_map *screen_base,
+		  unsigned int width, unsigned int height,
+		  const struct drm_format_info *format, unsigned int pitch);
+
+size_t drm_fwfb_extra_formats(struct drm_device *dev,
+			      const uint32_t *native_fourccs, size_t native_nfourccs,
+			      const uint32_t *extra_fourccs, size_t extra_nfourccs,
+			      uint32_t *fourccs_out, size_t nfourccs_out);
+
+void drm_fwfb_plane_helper_atomic_update(struct drm_fwfb *fwfb, struct drm_plane *plane,
+					 struct drm_atomic_state *state);
+void drm_fwfb_plane_helper_atomic_disable(struct drm_fwfb *fwfb, struct drm_plane *plane,
+					  struct drm_atomic_state *state);
+
+enum drm_mode_status drm_fwfb_crtc_helper_mode_valid(struct drm_fwfb *fwfb, struct drm_crtc *crtc,
+						     const struct drm_display_mode *mode);
+
+int drm_fwfb_connector_helper_get_modes(struct drm_fwfb *fwfb, struct drm_connector *connector);
+
+#endif
-- 
2.36.1

Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Move some of simpledrm's functionality into a helper library. Other
> drivers for firmware-provided framebuffers will also need functions
> to handle fixed modes and color formats, or update the back buffer.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Nice patch!

[...]

> +
> +/**
> + * DOC: overview
> + *
> + * The Firmware Framebuffer library FWFB provides helpers for devices with
> + * fixed-mode backing storage. It helps drivers to export a display mode of
> + * te correct size and copy updates to the backing storage.

the

it is "backing storage" or "backing store" ? I always thought that storage was
used for non-volatile media while "store" could be volatile and non-volatile.

[...]

> +/**
> + * drm_fwfb_init - Initializes an fwfb buffer
> + * @fwfb: fwfb buffer
> + * @screen_base: Address of the backing buffer in kernel address space
> + * @width: Number of pixels per scanline
> + * @height: Number of scanlines
> + * @format: Color format
> + * @pitch: Distance between two consecutive scanlines in bytes
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int drm_fwfb_init(struct drm_fwfb *fwfb, struct iosys_map *screen_base,
> +		  unsigned int width, unsigned int height,
> +		  const struct drm_format_info *format, unsigned int pitch)
> +{
> +	fwfb->screen_base = *screen_base;
> +	fwfb->mode = drm_fwfb_mode(width, height);
> +	fwfb->format = format;

It seems a little bit arbitrary to me that format is the only field that's
a pointer and the other ones are embedded into the struct drm_fwfb. Any
reason for that or is just a consequence of how types were used by the
simpledrm_device_create() function before that code moved into helpers ?

[...]

> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
> +{
> +	const uint32_t *fourccs_end = fourccs + nfourccs;
> +
> +	while (fourccs < fourccs_end) {
> +		if (*fourccs == fourcc)
> +			return true;
> +		++fourccs;
> +	}
> +	return false;
> +}

This seems a helper that could be useful besides the drm_fwfb_helper.c file.

I believe patches 1-6 shouldn't wait for the others in this series and could
just be merged when ready. Patches 7-10 can follow later.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3888 bytes --]

Hi

Am 25.07.22 um 18:23 schrieb Javier Martinez Canillas:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Move some of simpledrm's functionality into a helper library. Other
>> drivers for firmware-provided framebuffers will also need functions
>> to handle fixed modes and color formats, or update the back buffer.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Nice patch!

TBH it took me 3 tries to get something done for this library and I'm 
still not happy with the result. I want to share code between simpledrm 
and ofdrm, but that turns out to be harder then expected. A good part of 
this code appears to belong into other libraries (you also mentioned 
this below).

I don't want to duplicated code between simpledrm and ofdrm without 
reason, but I expect that this library will somewhen be refactored and 
dissolved into existing libraries.


> 
> [...]
> 
>> +
>> +/**
>> + * DOC: overview
>> + *
>> + * The Firmware Framebuffer library FWFB provides helpers for devices with
>> + * fixed-mode backing storage. It helps drivers to export a display mode of
>> + * te correct size and copy updates to the backing storage.
> 
> the
> 
> it is "backing storage" or "backing store" ? I always thought that storage was
> used for non-volatile media while "store" could be volatile and non-volatile.

Why store? Isn't that a little shop for fashion or groceries? I'm no 
native speaker; I can't tell if either implies that we're sending 
pictures to a warehouse or bakery. :)

Would 'back buffer' (in contrast to 'shadow buffer') be clear?

> 
> [...]
> 
>> +/**
>> + * drm_fwfb_init - Initializes an fwfb buffer
>> + * @fwfb: fwfb buffer
>> + * @screen_base: Address of the backing buffer in kernel address space
>> + * @width: Number of pixels per scanline
>> + * @height: Number of scanlines
>> + * @format: Color format
>> + * @pitch: Distance between two consecutive scanlines in bytes
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise.
>> + */
>> +int drm_fwfb_init(struct drm_fwfb *fwfb, struct iosys_map *screen_base,
>> +		  unsigned int width, unsigned int height,
>> +		  const struct drm_format_info *format, unsigned int pitch)
>> +{
>> +	fwfb->screen_base = *screen_base;
>> +	fwfb->mode = drm_fwfb_mode(width, height);
>> +	fwfb->format = format;
> 
> It seems a little bit arbitrary to me that format is the only field that's
> a pointer and the other ones are embedded into the struct drm_fwfb. Any
> reason for that or is just a consequence of how types were used by the
> simpledrm_device_create() function before that code moved into helpers ?

Format is constant and comes from statically initialized memory in 
drm_fourcc.c. I'd expect to be able to compare formats by comparing the 
pointers. Copying the format here would break the assumption.

> 
> [...]
> 
>> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
>> +{
>> +	const uint32_t *fourccs_end = fourccs + nfourccs;
>> +
>> +	while (fourccs < fourccs_end) {
>> +		if (*fourccs == fourcc)
>> +			return true;
>> +		++fourccs;
>> +	}
>> +	return false;
>> +}
> 
> This seems a helper that could be useful besides the drm_fwfb_helper.c file.
> 
> I believe patches 1-6 shouldn't wait for the others in this series and could
> just be merged when ready. Patches 7-10 can follow later.

Yeah, I'd like to move patches 1 to 5 into a new series for merging. 
Patch 6 is only useful for ofdrm and as I said, maybe there's a better 
solution then this library. I'd rather keep it here for now.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 06/10] drm/simpledrm: Move some functionality into fwfb helper library

[Javier Martinez Canillas]

On 7/27/22 10:24, Thomas Zimmermann wrote:
> Hi
> 
> Am 25.07.22 um 18:23 schrieb Javier Martinez Canillas:
>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>> Move some of simpledrm's functionality into a helper library. Other
>>> drivers for firmware-provided framebuffers will also need functions
>>> to handle fixed modes and color formats, or update the back buffer.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>
>> Nice patch!
> 
> TBH it took me 3 tries to get something done for this library and I'm 
> still not happy with the result. I want to share code between simpledrm 
> and ofdrm, but that turns out to be harder then expected. A good part of 
> this code appears to belong into other libraries (you also mentioned 
> this below).
> 
> I don't want to duplicated code between simpledrm and ofdrm without 
> reason, but I expect that this library will somewhen be refactored and 
> dissolved into existing libraries.
>

Yes, I think is a step in the right direction and guess it would be even
more useful once/if a 3rd firmware-provided framebuffer driver is added.

> 
>>
>> [...]
>>
>>> +
>>> +/**
>>> + * DOC: overview
>>> + *
>>> + * The Firmware Framebuffer library FWFB provides helpers for devices with
>>> + * fixed-mode backing storage. It helps drivers to export a display mode of
>>> + * te correct size and copy updates to the backing storage.
>>
>> the
>>
>> it is "backing storage" or "backing store" ? I always thought that storage was
>> used for non-volatile media while "store" could be volatile and non-volatile.
> 
> Why store? Isn't that a little shop for fashion or groceries? I'm no 
> native speaker; I can't tell if either implies that we're sending 
> pictures to a warehouse or bakery. :)
> 

LOL.

> Would 'back buffer' (in contrast to 'shadow buffer') be clear?
>

Back buffer is more clear indeed.

[...]

>> It seems a little bit arbitrary to me that format is the only field that's
>> a pointer and the other ones are embedded into the struct drm_fwfb. Any
>> reason for that or is just a consequence of how types were used by the
>> simpledrm_device_create() function before that code moved into helpers ?
> 
> Format is constant and comes from statically initialized memory in 
> drm_fourcc.c. I'd expect to be able to compare formats by comparing the 
> pointers. Copying the format here would break the assumption.
>

I see. Makes sense.

>>
>> [...]
>>
>>> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
>>> +{
>>> +	const uint32_t *fourccs_end = fourccs + nfourccs;
>>> +
>>> +	while (fourccs < fourccs_end) {
>>> +		if (*fourccs == fourcc)
>>> +			return true;
>>> +		++fourccs;
>>> +	}
>>> +	return false;
>>> +}
>>
>> This seems a helper that could be useful besides the drm_fwfb_helper.c file.
>>
>> I believe patches 1-6 shouldn't wait for the others in this series and could
>> just be merged when ready. Patches 7-10 can follow later.
> 
> Yeah, I'd like to move patches 1 to 5 into a new series for merging. 
> Patch 6 is only useful for ofdrm and as I said, maybe there's a better 
> solution then this library. I'd rather keep it here for now.
>

OK.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH v2 07/10] drm/ofdrm: Add ofdrm for Open Firmware framebuffers

[Thomas Zimmermann]

Open Firmware provides basic display output via the 'display' node.
DT platform code already provides a device that represents the node's
framebuffer. Add a DRM driver for the device. The display mode and
color format is pre-initialized by the system's firmware. Runtime
modesetting via DRM is not possible. The display is useful during
early boot stages or as error fallback.

Similar functionality is already provided by fbdev's offb driver,
which is insufficient for modern userspace. The old driver includes
support for BootX device tree, which can be found on old 32-bit
PowerPC Macintosh systems. If these are still in use, the
functionality can be added to ofdrm or implemented in a new
driver. As with simepldrm, the fbdev driver cannot be selected is
ofdrm is already enabled.

Two noteable points about the driver:

 * Reading the framebuffer aperture from the device tree is not
reliable on all systems. Ofdrm takes the heuristics and a comment
from offb to pick the correct range.

 * No resource management may be tied to the underlying PCI device.
Otherwise the handover to the native driver will fail with a resource
conflict. PCI management is therefore done as part of the platform
device's cleanup.

The driver has been tested on qemu's ppc64le emulation. The device
hand-over has been tested with bochs.

v2:
	* removed simple-pipe helpers
	* built driver on top of FWFB helpers
	* merged all init code into single function
	* make PCI support optional (Michal)
	* support COMPILE_TEST (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 MAINTAINERS                   |   1 +
 drivers/gpu/drm/tiny/Kconfig  |  14 +
 drivers/gpu/drm/tiny/Makefile |   1 +
 drivers/gpu/drm/tiny/ofdrm.c  | 692 ++++++++++++++++++++++++++++++++++
 drivers/video/fbdev/Kconfig   |   1 +
 5 files changed, 709 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ofdrm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 138680415e79..9ba6853fd107 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6573,6 +6573,7 @@ S:	Maintained
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	drivers/gpu/drm/drm_aperture.c
 F:	drivers/gpu/drm/drm_fwfb_helper.c
+F:	drivers/gpu/drm/tiny/ofdrm.c
 F:	drivers/gpu/drm/tiny/simpledrm.c
 F:	drivers/video/aperture.c
 F:	include/drm/drm_aperture.h
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f3d6e4713dc6..320b370e685e 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -51,6 +51,20 @@ config DRM_GM12U320
 	 This is a KMS driver for projectors which use the GM12U320 chipset
 	 for video transfer over USB2/3, such as the Acer C120 mini projector.
 
+config DRM_OFDRM
+	tristate "Open Firmware display driver"
+	depends on DRM && OF && (PPC || COMPILE_TEST)
+	select APERTURE_HELPERS
+	select DRM_FWFB_HELPER
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	help
+	  DRM driver for Open Firmware framebuffers.
+
+	  This driver assumes that the display hardware has been initialized
+	  by the Open Firmware before the kernel boots. Scanout buffer, size,
+	  and display format must be provided via device tree.
+
 config DRM_PANEL_MIPI_DBI
 	tristate "DRM support for MIPI DBI compatible panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 1d9d6227e7ab..76dde89a044b 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
 obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
+obj-$(CONFIG_DRM_OFDRM)			+= ofdrm.o
 obj-$(CONFIG_DRM_PANEL_MIPI_DBI)	+= panel-mipi-dbi.o
 obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
new file mode 100644
index 000000000000..6c062b48d354
--- /dev/null
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -0,0 +1,692 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/of_address.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_aperture.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_device.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fwfb_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#define DRIVER_NAME	"ofdrm"
+#define DRIVER_DESC	"DRM driver for OF platform devices"
+#define DRIVER_DATE	"20220501"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+/*
+ * Helpers for display nodes
+ */
+
+static int display_get_validated_int(struct drm_device *dev, const char *name, uint32_t value)
+{
+	if (value > INT_MAX) {
+		drm_err(dev, "invalid framebuffer %s of %u\n", name, value);
+		return -EINVAL;
+	}
+	return (int)value;
+}
+
+static int display_get_validated_int0(struct drm_device *dev, const char *name, uint32_t value)
+{
+	if (!value) {
+		drm_err(dev, "invalid framebuffer %s of %u\n", name, value);
+		return -EINVAL;
+	}
+	return display_get_validated_int(dev, name, value);
+}
+
+static const struct drm_format_info *display_get_validated_format(struct drm_device *dev,
+								  u32 depth)
+{
+	const struct drm_format_info *info;
+	u32 format;
+
+	switch (depth) {
+	case 8:
+		format = drm_mode_legacy_fb_format(8, 8);
+		break;
+	case 15:
+	case 16:
+		format = drm_mode_legacy_fb_format(16, depth);
+		break;
+	case 32:
+		format = drm_mode_legacy_fb_format(32, 24);
+		break;
+	default:
+		drm_err(dev, "unsupported framebuffer depth %u\n", depth);
+		return ERR_PTR(-EINVAL);
+	}
+
+	info = drm_format_info(format);
+	if (!info) {
+		drm_err(dev, "cannot find framebuffer format for depth %u\n", depth);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return info;
+}
+
+static int display_read_u32_of(struct drm_device *dev, struct device_node *of_node,
+			       const char *name, u32 *value)
+{
+	int ret = of_property_read_u32(of_node, name, value);
+
+	if (ret)
+		drm_err(dev, "cannot parse framebuffer %s: error %d\n", name, ret);
+	return ret;
+}
+
+static int display_get_width_of(struct drm_device *dev, struct device_node *of_node)
+{
+	u32 width;
+	int ret = display_read_u32_of(dev, of_node, "width", &width);
+
+	if (ret)
+		return ret;
+	return display_get_validated_int0(dev, "width", width);
+}
+
+static int display_get_height_of(struct drm_device *dev, struct device_node *of_node)
+{
+	u32 height;
+	int ret = display_read_u32_of(dev, of_node, "height", &height);
+
+	if (ret)
+		return ret;
+	return display_get_validated_int0(dev, "height", height);
+}
+
+static const struct drm_format_info *display_get_format_of(struct drm_device *dev,
+							   struct device_node *of_node)
+{
+	u32 depth;
+	int ret = display_read_u32_of(dev, of_node, "depth", &depth);
+
+	if (ret)
+		return ERR_PTR(ret);
+	return display_get_validated_format(dev, depth);
+}
+
+static int display_get_linebytes_of(struct drm_device *dev, struct device_node *of_node)
+{
+	u32 linebytes;
+	int ret = display_read_u32_of(dev, of_node, "linebytes", &linebytes);
+
+	if (ret)
+		return ret;
+	return display_get_validated_int(dev, "linebytes", linebytes);
+}
+
+static u64 display_get_address_of(struct drm_device *dev, struct device_node *of_node)
+{
+	u32 address;
+	int ret;
+
+	/*
+	 * Not all devices provide an address property, it's not
+	 * a bug if this fails. The driver will try to find the
+	 * framebuffer base address from the device's memory regions.
+	 */
+	ret = of_property_read_u32(of_node, "address", &address);
+	if (ret)
+		return OF_BAD_ADDR;
+
+	return address;
+}
+
+/*
+ * Open Firmware display device
+ */
+
+struct ofdrm_device {
+	struct drm_device dev;
+	struct platform_device *pdev;
+
+	/* firmware framebuffer */
+	struct drm_fwfb fwfb;
+
+	/* modesetting */
+	uint32_t formats[8];
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+};
+
+static struct ofdrm_device *ofdrm_device_of_dev(struct drm_device *dev)
+{
+	return container_of(dev, struct ofdrm_device, dev);
+}
+
+/*
+ * Hardware
+ */
+
+#if defined(CONFIG_PCI)
+static struct pci_dev *display_get_pci_dev_of(struct drm_device *dev, struct device_node *of_node)
+{
+	const __be32 *vendor_p, *device_p;
+	u32 vendor, device;
+	struct pci_dev *pcidev;
+
+	vendor_p = of_get_property(of_node, "vendor-id", NULL);
+	if (!vendor_p)
+		return ERR_PTR(-ENODEV);
+	vendor = be32_to_cpup(vendor_p);
+
+	device_p = of_get_property(of_node, "device-id", NULL);
+	if (!device_p)
+		return ERR_PTR(-ENODEV);
+	device = be32_to_cpup(device_p);
+
+	pcidev = pci_get_device(vendor, device, NULL);
+	if (!pcidev)
+		return ERR_PTR(-ENODEV);
+
+	return pcidev;
+}
+
+static void ofdrm_pci_release(void *data)
+{
+	struct pci_dev *pcidev = data;
+
+	pci_disable_device(pcidev);
+}
+
+static int ofdrm_device_init_pci(struct ofdrm_device *odev)
+{
+	struct drm_device *dev = &odev->dev;
+	struct platform_device *pdev = to_platform_device(dev->dev);
+	struct device_node *of_node = pdev->dev.of_node;
+	struct pci_dev *pcidev;
+	int ret;
+
+	/*
+	 * Never use pcim_ or other managed helpers on the returned PCI
+	 * device. Otherwise, probing the native driver will fail for
+	 * resource conflicts. PCI-device management has to be tied to
+	 * the lifetime of the platform device until the native driver
+	 * takes over.
+	 */
+	pcidev = display_get_pci_dev_of(dev, of_node);
+	if (IS_ERR(pcidev))
+		return 0; /* no PCI device found; ignore the error */
+
+	ret = pci_enable_device(pcidev);
+	if (ret) {
+		drm_err(dev, "pci_enable_device(%s) failed: %d\n",
+			dev_name(&pcidev->dev), ret);
+		return ret;
+	}
+	ret = devm_add_action_or_reset(&pdev->dev, ofdrm_pci_release, pcidev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+#else
+static int ofdrm_device_init_pci(struct ofdrm_device *odev)
+{
+	return 0;
+}
+#endif
+
+/*
+ *  OF display settings
+ */
+
+static struct resource *ofdrm_find_fb_resource(struct ofdrm_device *odev,
+					       struct resource *fb_res)
+{
+	struct platform_device *pdev = to_platform_device(odev->dev.dev);
+	struct resource *res, *max_res = NULL;
+	u32 i;
+
+	for (i = 0; pdev->num_resources; ++i) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			break; /* all resources processed */
+		if (resource_size(res) < resource_size(fb_res))
+			continue; /* resource too small */
+		if (fb_res->start && resource_contains(res, fb_res))
+			return res; /* resource contains framebuffer */
+		if (!max_res || resource_size(res) > resource_size(max_res))
+			max_res = res; /* store largest resource as fallback */
+	}
+
+	return max_res;
+}
+
+/*
+ * Modesetting
+ */
+
+/*
+ * Support all formats of OF display and maybe more; in order
+ * of preference. The display's update function will do any
+ * conversion necessary.
+ *
+ * TODO: Add blit helpers for remaining formats and uncomment
+ *       constants.
+ */
+static const uint32_t ofdrm_primary_plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_RGB565,
+	//DRM_FORMAT_XRGB1555,
+	//DRM_FORMAT_C8,
+};
+
+static const uint64_t ofdrm_primary_plane_format_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						   struct drm_atomic_state *new_state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_crtc_state *new_crtc_state;
+	int ret;
+
+	if (!new_plane_state->fb)
+		return 0;
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
+
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  DRM_PLANE_HELPER_NO_SCALING,
+						  false, false);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						     struct drm_atomic_state *old_state)
+{
+	struct ofdrm_device *odev = ofdrm_device_of_dev(plane->dev);
+
+	drm_fwfb_plane_helper_atomic_update(&odev->fwfb, plane, old_state);
+}
+
+static void ofdrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
+						      struct drm_atomic_state *old_state)
+{
+	struct ofdrm_device *odev = ofdrm_device_of_dev(plane->dev);
+
+	drm_fwfb_plane_helper_atomic_disable(&odev->fwfb, plane, old_state);
+}
+
+static const struct drm_plane_helper_funcs ofdrm_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = ofdrm_primary_plane_helper_atomic_check,
+	.atomic_update = ofdrm_primary_plane_helper_atomic_update,
+	.atomic_disable = ofdrm_primary_plane_helper_atomic_disable,
+};
+
+static const struct drm_plane_funcs ofdrm_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+static enum drm_mode_status ofdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
+							 const struct drm_display_mode *mode)
+{
+	struct ofdrm_device *odev = ofdrm_device_of_dev(crtc->dev);
+
+	return drm_fwfb_crtc_helper_mode_valid(&odev->fwfb, crtc, mode);
+}
+
+static int ofdrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
+					  struct drm_atomic_state *new_state)
+{
+	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
+	int ret;
+
+	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
+	if (ret)
+		return ret;
+
+	return drm_atomic_add_affected_planes(new_state, crtc);
+}
+
+static void ofdrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+					    struct drm_atomic_state *old_state)
+{
+	/*
+	 * Always enabled; screen updates are performed by
+	 * the primary plane's atomic_update function.
+	 */
+}
+
+static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
+					     struct drm_atomic_state *old_state)
+{
+	/*
+	 * Always enabled; disabling clears the screen in the
+	 * primary plane's atomic_disable function.
+	 */
+}
+
+static const struct drm_crtc_helper_funcs ofdrm_crtc_helper_funcs = {
+	.mode_valid = ofdrm_crtc_helper_mode_valid,
+	.atomic_check = ofdrm_crtc_helper_atomic_check,
+	.atomic_enable = ofdrm_crtc_helper_atomic_enable,
+	.atomic_disable = ofdrm_crtc_helper_atomic_disable,
+};
+
+static const struct drm_crtc_funcs ofdrm_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+static int ofdrm_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct ofdrm_device *odev = ofdrm_device_of_dev(connector->dev);
+
+	return drm_fwfb_connector_helper_get_modes(&odev->fwfb, connector);
+}
+
+static const struct drm_connector_helper_funcs ofdrm_connector_helper_funcs = {
+	.get_modes = ofdrm_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ofdrm_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_mode_config_funcs ofdrm_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+/*
+ * Init / Cleanup
+ */
+
+static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
+						struct platform_device *pdev)
+{
+	struct device_node *of_node = pdev->dev.of_node;
+	struct ofdrm_device *odev;
+	struct drm_device *dev;
+	int width, height, linebytes;
+	const struct drm_format_info *format;
+	u64 address;
+	resource_size_t fb_size, fb_base, fb_pgbase, fb_pgsize;
+	struct resource *res, *mem;
+	void __iomem *screen_base;
+	struct iosys_map screen_base_vmap;
+	struct drm_fwfb *fwfb;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	unsigned long max_width, max_height;
+	size_t nformats;
+	int ret;
+
+	odev = devm_drm_dev_alloc(&pdev->dev, drv, struct ofdrm_device, dev);
+	if (IS_ERR(odev))
+		return ERR_CAST(odev);
+	dev = &odev->dev;
+	platform_set_drvdata(pdev, dev);
+
+	ret = ofdrm_device_init_pci(odev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/*
+	 * OF display-node settings
+	 */
+
+	width = display_get_width_of(dev, of_node);
+	if (width < 0)
+		return ERR_PTR(width);
+	height = display_get_height_of(dev, of_node);
+	if (height < 0)
+		return ERR_PTR(height);
+	format = display_get_format_of(dev, of_node);
+	if (IS_ERR(format))
+		return ERR_CAST(format);
+	linebytes = display_get_linebytes_of(dev, of_node);
+	if (linebytes < 0)
+		return ERR_PTR(linebytes);
+	else if (!linebytes)
+		linebytes = (width * format->cpp[0] + 7) / 8;
+	fb_size = linebytes * height;
+
+	/*
+	 * Try to figure out the address of the framebuffer. Unfortunately, Open
+	 * Firmware doesn't provide a standard way to do so. All we can do is a
+	 * dodgy heuristic that happens to work in practice.
+	 *
+	 * On most machines, the "address" property contains what we need, though
+	 * not on Matrox cards found in IBM machines. What appears to give good
+	 * results is to go through the PCI ranges and pick one that encloses the
+	 * "address" property. If none match, we pick the largest.
+	 */
+	address = display_get_address_of(dev, of_node);
+	if (address != OF_BAD_ADDR) {
+		struct resource fb_res = DEFINE_RES_MEM(address, fb_size);
+
+		res = ofdrm_find_fb_resource(odev, &fb_res);
+		if (!res)
+			return ERR_PTR(-EINVAL);
+		if (resource_contains(res, &fb_res))
+			fb_base = address;
+		else
+			fb_base = res->start;
+	} else {
+		struct resource fb_res = DEFINE_RES_MEM(0u, fb_size);
+
+		res = ofdrm_find_fb_resource(odev, &fb_res);
+		if (!res)
+			return ERR_PTR(-EINVAL);
+		fb_base = res->start;
+	}
+
+	/*
+	 * I/O resources
+	 */
+
+	fb_pgbase = round_down(fb_base, PAGE_SIZE);
+	fb_pgsize = fb_base - fb_pgbase + round_up(fb_size, PAGE_SIZE);
+
+	ret = devm_aperture_acquire_from_firmware(dev, fb_pgbase, fb_pgsize);
+	if (ret) {
+		drm_err(dev, "could not acquire memory range %pr: error %d\n", &res, ret);
+		return ERR_PTR(ret);
+	}
+
+	mem = devm_request_mem_region(&pdev->dev, fb_pgbase, fb_pgsize, drv->name);
+	if (!mem) {
+		drm_warn(dev, "could not acquire memory region %pr\n", &res);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	screen_base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
+	if (!screen_base)
+		return ERR_PTR(-ENOMEM);
+	iosys_map_set_vaddr_iomem(&screen_base_vmap, screen_base);
+
+	/*
+	 * Firmware framebuffer
+	 */
+
+	fwfb = &odev->fwfb;
+	ret = drm_fwfb_init(fwfb, &screen_base_vmap, width, height, format, linebytes);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/*
+	 * Mode-setting pipeline
+	 */
+
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	max_width = max_t(unsigned long, width, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, height, DRM_SHADOW_PLANE_MAX_HEIGHT);
+
+	dev->mode_config.min_width = width;
+	dev->mode_config.max_width = max_width;
+	dev->mode_config.min_height = height;
+	dev->mode_config.max_height = max_height;
+	dev->mode_config.preferred_depth = format->cpp[0] * 8;
+	dev->mode_config.funcs = &ofdrm_mode_config_funcs;
+
+	/* Primary plane */
+
+	nformats = drm_fwfb_extra_formats(dev, &format->format, 1,
+					  ofdrm_primary_plane_formats,
+					  ARRAY_SIZE(ofdrm_primary_plane_formats),
+					  odev->formats, ARRAY_SIZE(odev->formats));
+
+	primary_plane = &odev->primary_plane;
+	ret = drm_universal_plane_init(dev, primary_plane, 0, &ofdrm_primary_plane_funcs,
+				       odev->formats, nformats,
+				       ofdrm_primary_plane_format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+	drm_plane_helper_add(primary_plane, &ofdrm_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	/* CRTC */
+
+	crtc = &odev->crtc;
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&ofdrm_crtc_funcs, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+	drm_crtc_helper_add(crtc, &ofdrm_crtc_helper_funcs);
+
+	/* Encoder */
+
+	encoder = &odev->encoder;
+	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE);
+	if (ret)
+		return ERR_PTR(ret);
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	/* Connector */
+
+	connector = &odev->connector;
+	ret = drm_connector_init(dev, connector, &ofdrm_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret)
+		return ERR_PTR(ret);
+	drm_connector_helper_add(connector, &ofdrm_connector_helper_funcs);
+	drm_connector_set_panel_orientation_with_quirk(connector,
+						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+						       width, height);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ERR_PTR(ret);
+
+	drm_mode_config_reset(dev);
+
+	return odev;
+}
+
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_FOPS(ofdrm_fops);
+
+static struct drm_driver ofdrm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &ofdrm_fops,
+};
+
+/*
+ * Platform driver
+ */
+
+static int ofdrm_probe(struct platform_device *pdev)
+{
+	struct ofdrm_device *odev;
+	struct drm_device *dev;
+	int ret;
+
+	odev = ofdrm_device_create(&ofdrm_driver, pdev);
+	if (IS_ERR(odev))
+		return PTR_ERR(odev);
+	dev = &odev->dev;
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		return ret;
+
+	drm_fbdev_generic_setup(dev, 0);
+
+	return 0;
+}
+
+static int ofdrm_remove(struct platform_device *pdev)
+{
+	struct drm_device *dev = platform_get_drvdata(pdev);
+
+	drm_dev_unplug(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ofdrm_of_match_display[] = {
+	{ .compatible = "display", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ofdrm_of_match_display);
+
+static struct platform_driver ofdrm_platform_driver = {
+	.driver = {
+		.name = "of-display",
+		.of_match_table = ofdrm_of_match_display,
+	},
+	.probe = ofdrm_probe,
+	.remove = ofdrm_remove,
+};
+
+module_platform_driver(ofdrm_platform_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index cfc55273dc5d..a98987aa2784 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -455,6 +455,7 @@ config FB_ATARI
 config FB_OF
 	bool "Open Firmware frame buffer device support"
 	depends on (FB = y) && PPC && (!PPC_PSERIES || PCI)
+	depends on !DRM_OFDRM
 	select APERTURE_HELPERS
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
-- 
2.36.1

Re: [PATCH v2 07/10] drm/ofdrm: Add ofdrm for Open Firmware framebuffers

[Javier Martinez Canillas]

Hello Thomas,

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Open Firmware provides basic display output via the 'display' node.
> DT platform code already provides a device that represents the node's
> framebuffer. Add a DRM driver for the device. The display mode and
> color format is pre-initialized by the system's firmware. Runtime
> modesetting via DRM is not possible. The display is useful during
> early boot stages or as error fallback.
> 

I'm not familiar with OF display but the driver looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

I just have a few questions below.

[...]

> +static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> +						   struct drm_atomic_state *new_state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> +	struct drm_crtc_state *new_crtc_state;
> +	int ret;
> +
> +	if (!new_plane_state->fb)
> +		return 0;
> +
> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
> +
> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  DRM_PLANE_HELPER_NO_SCALING,
> +						  false, false);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

This seems to be exactly the same check than used in the simpledrm driver.
Maybe could be moved to the fwfb helper library too ?

[...]

> +
> +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					     struct drm_atomic_state *old_state)
> +{
> +	/*
> +	 * Always enabled; disabling clears the screen in the
> +	 * primary plane's atomic_disable function.
> +	 */
> +}
> +

Same comment than for simpledrm, are these no-op helpers really needed ?

[...]

> +static const struct of_device_id ofdrm_of_match_display[] = {
> +	{ .compatible = "display", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display);
> +

I don't see a binding for this in Documentation/devicetree/bindings/display.
Do we need one or it's that only required for FDT and not Open Firmware DT ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 07/10] drm/ofdrm: Add ofdrm for Open Firmware framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2878 bytes --]

Hi

Am 26.07.22 um 15:17 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Open Firmware provides basic display output via the 'display' node.
>> DT platform code already provides a device that represents the node's
>> framebuffer. Add a DRM driver for the device. The display mode and
>> color format is pre-initialized by the system's firmware. Runtime
>> modesetting via DRM is not possible. The display is useful during
>> early boot stages or as error fallback.
>>
> 
> I'm not familiar with OF display but the driver looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> I just have a few questions below.
> 
> [...]
> 
>> +static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> +						   struct drm_atomic_state *new_state)
>> +{
>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> +	struct drm_crtc_state *new_crtc_state;
>> +	int ret;
>> +
>> +	if (!new_plane_state->fb)
>> +		return 0;
>> +
>> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
>> +
>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  DRM_PLANE_HELPER_NO_SCALING,
>> +						  false, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
> 
> This seems to be exactly the same check than used in the simpledrm driver.
> Maybe could be moved to the fwfb helper library too ?

I've meanwhile dropped fwfb helpers. Color management requires specific 
code here, so there's no much to share anyway.

> 
> [...]
> 
>> +
>> +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +					     struct drm_atomic_state *old_state)
>> +{
>> +	/*
>> +	 * Always enabled; disabling clears the screen in the
>> +	 * primary plane's atomic_disable function.
>> +	 */
>> +}
>> +
> 
> Same comment than for simpledrm, are these no-op helpers really needed ?

In simpledrm, I've meanwhile removed them. I'll do so here as well.

> 
> [...]
> 
>> +static const struct of_device_id ofdrm_of_match_display[] = {
>> +	{ .compatible = "display", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display);
>> +
> 
> I don't see a binding for this in Documentation/devicetree/bindings/display.
> Do we need one or it's that only required for FDT and not Open Firmware DT ?
> 

No idea. The device is being created in drivers/of/platform.c. If offb 
didn't need these bindings, ofdrm probably won't need them either.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 08/10] drm/ofdrm: Add CRTC state

[Thomas Zimmermann]

Add a dedicated CRTC state to ofdrm to later store information for
palette updates.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 6c062b48d354..ad673c9b5502 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -277,6 +277,21 @@ static struct resource *ofdrm_find_fb_resource(struct ofdrm_device *odev,
  * Modesetting
  */
 
+struct ofdrm_crtc_state {
+	struct drm_crtc_state base;
+};
+
+static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base)
+{
+	return container_of(base, struct ofdrm_crtc_state, base);
+}
+
+static void ofdrm_crtc_state_destroy(struct ofdrm_crtc_state *ofdrm_crtc_state)
+{
+	__drm_atomic_helper_crtc_destroy_state(&ofdrm_crtc_state->base);
+	kfree(ofdrm_crtc_state);
+}
+
 /*
  * Support all formats of OF display and maybe more; in order
  * of preference. The display's update function will do any
@@ -395,13 +410,54 @@ static const struct drm_crtc_helper_funcs ofdrm_crtc_helper_funcs = {
 	.atomic_disable = ofdrm_crtc_helper_atomic_disable,
 };
 
+static void ofdrm_crtc_reset(struct drm_crtc *crtc)
+{
+	struct ofdrm_crtc_state *ofdrm_crtc_state;
+
+	if (crtc->state) {
+		ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
+		crtc->state = NULL; /* must be set to NULL here */
+	}
+
+	ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);
+	if (!ofdrm_crtc_state)
+		return;
+	__drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
+}
+
+static struct drm_crtc_state *ofdrm_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
+{
+	struct drm_crtc_state *crtc_state = crtc->state;
+	struct ofdrm_crtc_state *ofdrm_crtc_state;
+	struct ofdrm_crtc_state *new_ofdrm_crtc_state;
+
+	if (!crtc_state)
+		return NULL;
+
+	ofdrm_crtc_state = to_ofdrm_crtc_state(crtc_state);
+
+	new_ofdrm_crtc_state = kzalloc(sizeof(*new_ofdrm_crtc_state), GFP_KERNEL);
+	if (!new_ofdrm_crtc_state)
+		return NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &new_ofdrm_crtc_state->base);
+
+	return &new_ofdrm_crtc_state->base;
+}
+
+static void ofdrm_crtc_atomic_destroy_state(struct drm_crtc *crtc,
+					    struct drm_crtc_state *crtc_state)
+{
+	ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc_state));
+}
+
 static const struct drm_crtc_funcs ofdrm_crtc_funcs = {
-	.reset = drm_atomic_helper_crtc_reset,
+	.reset = ofdrm_crtc_reset,
 	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
-	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+	.atomic_duplicate_state = ofdrm_crtc_atomic_duplicate_state,
+	.atomic_destroy_state = ofdrm_crtc_atomic_destroy_state,
 };
 
 static int ofdrm_connector_helper_get_modes(struct drm_connector *connector)
-- 
2.36.1

Re: [PATCH v2 08/10] drm/ofdrm: Add CRTC state

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Add a dedicated CRTC state to ofdrm to later store information for
> palette updates.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
>  

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

> +static void ofdrm_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct ofdrm_crtc_state *ofdrm_crtc_state;
> +
> +	if (crtc->state) {
> +		ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
> +		crtc->state = NULL; /* must be set to NULL here */
> +	}
> +
> +	ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);
> +	if (!ofdrm_crtc_state)
> +		return;
> +	__drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
> +}
> +

IMO this function is hard to read, I would instead write it as following:

static void ofdrm_crtc_reset(struct drm_crtc *crtc)
{
        struct ofdrm_crtc_state *ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);

	if (!ofdrm_crtc_state)
		return;

        if (crtc->state) {
                ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
		crtc->state = NULL; /* must be set to NULL here */
	}

        __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
}

Also with that form I think that the crtc->state = NULL could just be dropped ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 08/10] drm/ofdrm: Add CRTC state

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1925 bytes --]

Hi

Am 26.07.22 um 15:36 schrieb Javier Martinez Canillas:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Add a dedicated CRTC state to ofdrm to later store information for
>> palette updates.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
>>   
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [...]
> 
>> +static void ofdrm_crtc_reset(struct drm_crtc *crtc)
>> +{
>> +	struct ofdrm_crtc_state *ofdrm_crtc_state;
>> +
>> +	if (crtc->state) {
>> +		ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
>> +		crtc->state = NULL; /* must be set to NULL here */
>> +	}
>> +
>> +	ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);
>> +	if (!ofdrm_crtc_state)
>> +		return;
>> +	__drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
>> +}
>> +
> 
> IMO this function is hard to read, I would instead write it as following:
> 
> static void ofdrm_crtc_reset(struct drm_crtc *crtc)
> {
>          struct ofdrm_crtc_state *ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);
> 
> 	if (!ofdrm_crtc_state)
> 		return;
> 
>          if (crtc->state) {
>                  ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
> 		crtc->state = NULL; /* must be set to NULL here */
> 	}
> 
>          __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
> }
> 
> Also with that form I think that the crtc->state = NULL could just be dropped ?

I once had to add this line to a driver to make the DRM helpers work. 
But I cannot find any longer why. Maybe it's been resolved meanwhile.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Thomas Zimmermann]

Add a per-model device-function structure in preparation of adding
color-management support. Detection of the individual models has been
taken from fbdev's offb.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/ofdrm.c | 121 +++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index ad673c9b5502..62136b8ab975 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -29,6 +29,18 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
+enum ofdrm_model {
+	OFDRM_MODEL_UNKNOWN,
+	OFDRM_MODEL_MACH64, /* ATI Mach64 */
+	OFDRM_MODEL_RAGE128, /* ATI Rage128 */
+	OFDRM_MODEL_RAGE_M3A, /* ATI Rage Mobility M3 Head A */
+	OFDRM_MODEL_RAGE_M3B, /* ATI Rage Mobility M3 Head B */
+	OFDRM_MODEL_RADEON, /* ATI Radeon */
+	OFDRM_MODEL_GXT2000, /* IBM GXT2000 */
+	OFDRM_MODEL_AVIVO, /* ATI R5xx */
+	OFDRM_MODEL_QEMU, /* QEMU VGA */
+};
+
 /*
  * Helpers for display nodes
  */
@@ -150,14 +162,62 @@ static u64 display_get_address_of(struct drm_device *dev, struct device_node *of
 	return address;
 }
 
+static bool is_avivo(__be32 vendor, __be32 device)
+{
+	/* This will match most R5xx */
+	return (vendor == 0x1002) &&
+	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
+}
+
+static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct device_node *of_node)
+{
+	enum ofdrm_model model = OFDRM_MODEL_UNKNOWN;
+
+	if (of_node_name_prefix(of_node, "ATY,Rage128")) {
+		model = OFDRM_MODEL_RAGE128;
+	} else if (of_node_name_prefix(of_node, "ATY,RageM3pA") ||
+		   of_node_name_prefix(of_node, "ATY,RageM3p12A")) {
+		model = OFDRM_MODEL_RAGE_M3A;
+	} else if (of_node_name_prefix(of_node, "ATY,RageM3pB")) {
+		model = OFDRM_MODEL_RAGE_M3B;
+	} else if (of_node_name_prefix(of_node, "ATY,Rage6")) {
+		model = OFDRM_MODEL_RADEON;
+	} else if (of_node_name_prefix(of_node, "ATY,")) {
+		return OFDRM_MODEL_MACH64;
+	} else if (of_device_is_compatible(of_node, "pci1014,b7") ||
+		   of_device_is_compatible(of_node, "pci1014,21c")) {
+		model = OFDRM_MODEL_GXT2000;
+	} else if (of_node_name_prefix(of_node, "vga,Display-")) {
+		struct device_node *of_parent;
+		const __be32 *vendor_p, *device_p;
+
+		/* Look for AVIVO initialized by SLOF */
+		of_parent = of_get_parent(of_node);
+		vendor_p = of_get_property(of_parent, "vendor-id", NULL);
+		device_p = of_get_property(of_parent, "device-id", NULL);
+		if (vendor_p && device_p && is_avivo(*vendor_p, *device_p))
+			model = OFDRM_MODEL_AVIVO;
+		of_node_put(of_parent);
+	} else if (of_device_is_compatible(of_node, "qemu,std-vga")) {
+		model = OFDRM_MODEL_QEMU;
+	}
+
+	return model;
+}
+
 /*
  * Open Firmware display device
  */
 
+struct ofdrm_device_funcs {
+};
+
 struct ofdrm_device {
 	struct drm_device dev;
 	struct platform_device *pdev;
 
+	const struct ofdrm_device_funcs *funcs;
+
 	/* firmware framebuffer */
 	struct drm_fwfb fwfb;
 
@@ -489,12 +549,40 @@ static const struct drm_mode_config_funcs ofdrm_mode_config_funcs = {
  * Init / Cleanup
  */
 
+static const struct ofdrm_device_funcs ofdrm_unknown_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_mach64_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_rage128_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_rage_m3a_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_rage_m3b_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_radeon_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_gxt2000_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_avivo_device_funcs = {
+};
+
+static const struct ofdrm_device_funcs ofdrm_qemu_device_funcs = {
+};
+
 static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 						struct platform_device *pdev)
 {
 	struct device_node *of_node = pdev->dev.of_node;
 	struct ofdrm_device *odev;
 	struct drm_device *dev;
+	enum ofdrm_model model;
 	int width, height, linebytes;
 	const struct drm_format_info *format;
 	u64 address;
@@ -525,6 +613,39 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 	 * OF display-node settings
 	 */
 
+	model = display_get_model_of(dev, of_node);
+	drm_dbg(dev, "detected model %d\n", model);
+
+	switch (model) {
+	case OFDRM_MODEL_UNKNOWN:
+		odev->funcs = &ofdrm_unknown_device_funcs;
+		break;
+	case OFDRM_MODEL_MACH64:
+		odev->funcs = &ofdrm_mach64_device_funcs;
+		break;
+	case OFDRM_MODEL_RAGE128:
+		odev->funcs = &ofdrm_rage128_device_funcs;
+		break;
+	case OFDRM_MODEL_RAGE_M3A:
+		odev->funcs = &ofdrm_rage_m3a_device_funcs;
+		break;
+	case OFDRM_MODEL_RAGE_M3B:
+		odev->funcs = &ofdrm_rage_m3b_device_funcs;
+		break;
+	case OFDRM_MODEL_RADEON:
+		odev->funcs = &ofdrm_radeon_device_funcs;
+		break;
+	case OFDRM_MODEL_GXT2000:
+		odev->funcs = &ofdrm_gxt2000_device_funcs;
+		break;
+	case OFDRM_MODEL_AVIVO:
+		odev->funcs = &ofdrm_avivo_device_funcs;
+		break;
+	case OFDRM_MODEL_QEMU:
+		odev->funcs = &ofdrm_qemu_device_funcs;
+		break;
+	}
+
 	width = display_get_width_of(dev, of_node);
 	if (width < 0)
 		return ERR_PTR(width);
-- 
2.36.1

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Add a per-model device-function structure in preparation of adding
> color-management support. Detection of the individual models has been
> taken from fbdev's offb.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

> +static bool is_avivo(__be32 vendor, __be32 device)
> +{
> +	/* This will match most R5xx */
> +	return (vendor == 0x1002) &&
> +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
> +}

Maybe add some constant macros to not have these magic numbers ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Michal Suchánek]

Hello,

On Tue, Jul 26, 2022 at 03:38:37PM +0200, Javier Martinez Canillas wrote:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
> > Add a per-model device-function structure in preparation of adding
> > color-management support. Detection of the individual models has been
> > taken from fbdev's offb.
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [...]
> 
> > +static bool is_avivo(__be32 vendor, __be32 device)
> > +{
> > +	/* This will match most R5xx */
> > +	return (vendor == 0x1002) &&
> > +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
> > +}
> 
> Maybe add some constant macros to not have these magic numbers ?

This is based on the existing fbdev implementation's magic numbers:

drivers/video/fbdev/offb.c:                 ((*did >= 0x7100 && *did < 0x7800) ||

Of course, it would be great if somebody knowledgeable could clarify
those.

Thanks

Michal

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Javier Martinez Canillas]

Hello Michal,

On 7/26/22 16:40, Michal Suchánek wrote:
> Hello,
> 
> On Tue, Jul 26, 2022 at 03:38:37PM +0200, Javier Martinez Canillas wrote:
>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>> Add a per-model device-function structure in preparation of adding
>>> color-management support. Detection of the individual models has been
>>> taken from fbdev's offb.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> [...]
>>
>>> +static bool is_avivo(__be32 vendor, __be32 device)
>>> +{
>>> +	/* This will match most R5xx */
>>> +	return (vendor == 0x1002) &&
>>> +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
>>> +}
>>
>> Maybe add some constant macros to not have these magic numbers ?
> 
> This is based on the existing fbdev implementation's magic numbers:
> 
> drivers/video/fbdev/offb.c:                 ((*did >= 0x7100 && *did < 0x7800) ||
>

Ah, I see. Then we might have to go with the magic numbers...
 
> Of course, it would be great if somebody knowledgeable could clarify
> those.
>

Indeed.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1574 bytes --]

Hi

Am 26.07.22 um 21:22 schrieb Javier Martinez Canillas:
> Hello Michal,
> 
> On 7/26/22 16:40, Michal Suchánek wrote:
>> Hello,
>>
>> On Tue, Jul 26, 2022 at 03:38:37PM +0200, Javier Martinez Canillas wrote:
>>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>>> Add a per-model device-function structure in preparation of adding
>>>> color-management support. Detection of the individual models has been
>>>> taken from fbdev's offb.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>
>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>>
>>> [...]
>>>
>>>> +static bool is_avivo(__be32 vendor, __be32 device)
>>>> +{
>>>> +	/* This will match most R5xx */
>>>> +	return (vendor == 0x1002) &&
>>>> +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
>>>> +}
>>>
>>> Maybe add some constant macros to not have these magic numbers ?
>>
>> This is based on the existing fbdev implementation's magic numbers:
>>
>> drivers/video/fbdev/offb.c:                 ((*did >= 0x7100 && *did < 0x7800) ||
>>
> 
> Ah, I see. Then we might have to go with the magic numbers...
>   
>> Of course, it would be great if somebody knowledgeable could clarify
>> those.

Those are PCI ids. If I find them already defined, I'll use the macros 
instead.

Best regards
Thomas

>>
> 
> Indeed.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Benjamin Herrenschmidt]

On Tue, 2022-07-26 at 16:40 +0200, Michal Suchánek wrote:
> Hello,
> 
> On Tue, Jul 26, 2022 at 03:38:37PM +0200, Javier Martinez Canillas wrote:
> > On 7/20/22 16:27, Thomas Zimmermann wrote:
> > > Add a per-model device-function structure in preparation of adding
> > > color-management support. Detection of the individual models has been
> > > taken from fbdev's offb.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > ---
> > 
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > 
> > [...]
> > 
> > > +static bool is_avivo(__be32 vendor, __be32 device)
> > > +{
> > > +	/* This will match most R5xx */
> > > +	return (vendor == 0x1002) &&
> > > +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
> > > +}
> > 
> > Maybe add some constant macros to not have these magic numbers ?
> 
> This is based on the existing fbdev implementation's magic numbers:
> 
> drivers/video/fbdev/offb.c:                 ((*did >= 0x7100 && *did < 0x7800) ||
> 
> Of course, it would be great if somebody knowledgeable could clarify
> those.

I don't think anybody remembers :-) Vendor 0x1002 is PCI_VENDOR_ID_ATI,
but the rest is basically ranges of PCI IDs for which we don't have
symbolic constants.

Cheers,
Ben.

Re: [PATCH v2 09/10] drm/ofdrm: Add per-model device function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1710 bytes --]

Hi

Am 05.08.22 um 02:22 schrieb Benjamin Herrenschmidt:
> On Tue, 2022-07-26 at 16:40 +0200, Michal Suchánek wrote:
>> Hello,
>>
>> On Tue, Jul 26, 2022 at 03:38:37PM +0200, Javier Martinez Canillas wrote:
>>> On 7/20/22 16:27, Thomas Zimmermann wrote:
>>>> Add a per-model device-function structure in preparation of adding
>>>> color-management support. Detection of the individual models has been
>>>> taken from fbdev's offb.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>
>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>>
>>> [...]
>>>
>>>> +static bool is_avivo(__be32 vendor, __be32 device)
>>>> +{
>>>> +	/* This will match most R5xx */
>>>> +	return (vendor == 0x1002) &&
>>>> +	       ((device >= 0x7100 && device < 0x7800) || (device >= 0x9400));
>>>> +}
>>>
>>> Maybe add some constant macros to not have these magic numbers ?
>>
>> This is based on the existing fbdev implementation's magic numbers:
>>
>> drivers/video/fbdev/offb.c:                 ((*did >= 0x7100 && *did < 0x7800) ||
>>
>> Of course, it would be great if somebody knowledgeable could clarify
>> those.
> 
> I don't think anybody remembers :-) Vendor 0x1002 is PCI_VENDOR_ID_ATI,

I do :)

> but the rest is basically ranges of PCI IDs for which we don't have
> symbolic constants.

Should we add them to the official list in pci_ids.h?  I cannot find 
0x7800. The others are R520 and R600.

Best regards
Thomas

> 
> Cheers,
> Ben.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 10/10] drm/ofdrm: Support color management

[Thomas Zimmermann]

Support the CRTC's color-management property and implement each model's
palette support.

The OF hardware has different methods of setting the palette. The
respective code has been taken from fbdev's offb and refactored into
per-model device functions. The device functions integrate this
functionality into the overall modesetting.

As palette handling is a CRTC property that depends on the primary
plane's color format, the plane's atomic_check helper now updates the
format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
helper updates the palette for the format as needed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/ofdrm.c | 434 ++++++++++++++++++++++++++++++++++-
 1 file changed, 433 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 62136b8ab975..2964eb9951c5 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -13,6 +13,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_format_helper.h>
+#include <drm/drm_framebuffer.h>
 #include <drm/drm_fwfb_helper.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -29,6 +30,44 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
+#if !defined(CONFIG_PPC)
+static inline void out_8(void __iomem *addr, int val)
+{ }
+static inline void out_le32(void __iomem *addr, int val)
+{ }
+static inline unsigned int in_le32(const void __iomem *addr)
+{
+	return 0;
+}
+#endif
+
+#define OFDRM_GAMMA_LUT_SIZE	256
+
+/* Definitions used by the Avivo palette  */
+#define AVIVO_DC_LUT_RW_SELECT                  0x6480
+#define AVIVO_DC_LUT_RW_MODE                    0x6484
+#define AVIVO_DC_LUT_RW_INDEX                   0x6488
+#define AVIVO_DC_LUT_SEQ_COLOR                  0x648c
+#define AVIVO_DC_LUT_PWL_DATA                   0x6490
+#define AVIVO_DC_LUT_30_COLOR                   0x6494
+#define AVIVO_DC_LUT_READ_PIPE_SELECT           0x6498
+#define AVIVO_DC_LUT_WRITE_EN_MASK              0x649c
+#define AVIVO_DC_LUT_AUTOFILL                   0x64a0
+#define AVIVO_DC_LUTA_CONTROL                   0x64c0
+#define AVIVO_DC_LUTA_BLACK_OFFSET_BLUE         0x64c4
+#define AVIVO_DC_LUTA_BLACK_OFFSET_GREEN        0x64c8
+#define AVIVO_DC_LUTA_BLACK_OFFSET_RED          0x64cc
+#define AVIVO_DC_LUTA_WHITE_OFFSET_BLUE         0x64d0
+#define AVIVO_DC_LUTA_WHITE_OFFSET_GREEN        0x64d4
+#define AVIVO_DC_LUTA_WHITE_OFFSET_RED          0x64d8
+#define AVIVO_DC_LUTB_CONTROL                   0x6cc0
+#define AVIVO_DC_LUTB_BLACK_OFFSET_BLUE         0x6cc4
+#define AVIVO_DC_LUTB_BLACK_OFFSET_GREEN        0x6cc8
+#define AVIVO_DC_LUTB_BLACK_OFFSET_RED          0x6ccc
+#define AVIVO_DC_LUTB_WHITE_OFFSET_BLUE         0x6cd0
+#define AVIVO_DC_LUTB_WHITE_OFFSET_GREEN        0x6cd4
+#define AVIVO_DC_LUTB_WHITE_OFFSET_RED          0x6cd8
+
 enum ofdrm_model {
 	OFDRM_MODEL_UNKNOWN,
 	OFDRM_MODEL_MACH64, /* ATI Mach64 */
@@ -209,7 +248,14 @@ static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct devi
  * Open Firmware display device
  */
 
+struct ofdrm_device;
+
 struct ofdrm_device_funcs {
+	void __iomem *(*cmap_ioremap)(struct ofdrm_device *odev,
+				      struct device_node *of_node,
+				      u64 fb_bas);
+	void (*cmap_write)(struct ofdrm_device *odev, unsigned char index,
+			   unsigned char r, unsigned char g, unsigned char b);
 };
 
 struct ofdrm_device {
@@ -221,6 +267,9 @@ struct ofdrm_device {
 	/* firmware framebuffer */
 	struct drm_fwfb fwfb;
 
+	/* colormap */
+	void __iomem *cmap_base;
+
 	/* modesetting */
 	uint32_t formats[8];
 	struct drm_plane primary_plane;
@@ -333,12 +382,322 @@ static struct resource *ofdrm_find_fb_resource(struct ofdrm_device *odev,
 	return max_res;
 }
 
+/*
+ * Colormap / Palette
+ */
+
+static void __iomem *get_cmap_address_of(struct ofdrm_device *odev, struct device_node *of_node,
+					 int bar_no, unsigned long offset, unsigned long size)
+{
+	struct drm_device *dev = &odev->dev;
+	const __be32 *addr_p;
+	u64 max_size, address;
+	unsigned int flags;
+	void __iomem *mem;
+
+	addr_p = of_get_pci_address(of_node, bar_no, &max_size, &flags);
+	if (!addr_p)
+		addr_p = of_get_address(of_node, bar_no, &max_size, &flags);
+	if (!addr_p)
+		return ERR_PTR(-ENODEV);
+
+	if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
+		return ERR_PTR(-ENODEV);
+
+	if ((offset + size) >= max_size)
+		return ERR_PTR(-ENODEV);
+
+	address = of_translate_address(of_node, addr_p);
+	if (address == OF_BAD_ADDR)
+		return ERR_PTR(-ENODEV);
+
+	mem = devm_ioremap(dev->dev, address + offset, size);
+	if (!mem)
+		return ERR_PTR(-ENOMEM);
+
+	return mem;
+}
+
+static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
+					       struct device_node *of_node,
+					       u64 fb_base)
+{
+	struct drm_device *dev = &odev->dev;
+	u64 address;
+	void __iomem *cmap_base;
+
+	address = fb_base & 0xff000000ul;
+	address += 0x7ff000;
+
+	cmap_base = devm_ioremap(dev->dev, address, 0x1000);
+	if (!cmap_base)
+		return ERR_PTR(-ENOMEM);
+
+	return cmap_base;
+}
+
+static void ofdrm_mach64_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				    unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *addr = odev->cmap_base + 0xcc0;
+	void __iomem *data = odev->cmap_base + 0xcc0 + 1;
+
+	writeb(index, addr);
+	writeb(r, data);
+	writeb(g, data);
+	writeb(b, data);
+}
+
+static void __iomem *ofdrm_rage128_cmap_ioremap(struct ofdrm_device *odev,
+						struct device_node *of_node,
+						u64 fb_base)
+{
+	return get_cmap_address_of(odev, of_node, 2, 0, 0x1fff);
+}
+
+static void ofdrm_rage128_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				     unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *addr = odev->cmap_base + 0xb0;
+	void __iomem *data = odev->cmap_base + 0xb4;
+	int color = (r << 16) | (g << 8) | b;
+
+	out_8(addr, index);
+	out_le32(data, color);
+}
+
+static void __iomem *ofdrm_rage_m3a_cmap_ioremap(struct ofdrm_device *odev,
+						 struct device_node *of_node,
+						 u64 fb_base)
+{
+	return get_cmap_address_of(odev, of_node, 2, 0, 0x1fff);
+}
+
+static void ofdrm_rage_m3a_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				      unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *dac_ctl = odev->cmap_base + 0x58;
+	void __iomem *addr = odev->cmap_base + 0xb0;
+	void __iomem *data = odev->cmap_base + 0xb4;
+	int color = (r << 16) | (g << 8) | b;
+	unsigned int val;
+
+	/* Clear PALETTE_ACCESS_CNTL in DAC_CNTL */
+	val = in_le32(dac_ctl);
+	val &= ~0x20;
+	out_le32(dac_ctl, val);
+
+	/* Set color at palette index */
+	out_8(addr, index);
+	out_le32(data, color);
+}
+
+static void __iomem *ofdrm_rage_m3b_cmap_ioremap(struct ofdrm_device *odev,
+						 struct device_node *of_node,
+						 u64 fb_base)
+{
+	return get_cmap_address_of(odev, of_node, 2, 0, 0x1fff);
+}
+
+static void ofdrm_rage_m3b_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				      unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *dac_ctl = odev->cmap_base + 0x58;
+	void __iomem *addr = odev->cmap_base + 0xb0;
+	void __iomem *data = odev->cmap_base + 0xb4;
+	int color = (r << 16) | (g << 8) | b;
+	unsigned int val;
+
+	/* Set PALETTE_ACCESS_CNTL in DAC_CNTL */
+	val = in_le32(dac_ctl);
+	val |= 0x20;
+	out_le32(dac_ctl, val);
+
+	/* Set color at palette index */
+	out_8(addr, index);
+	out_le32(data, color);
+}
+
+static void __iomem *ofdrm_radeon_cmap_ioremap(struct ofdrm_device *odev,
+					       struct device_node *of_node,
+					       u64 fb_base)
+{
+	return get_cmap_address_of(odev, of_node, 1, 0, 0x1fff);
+}
+
+static void __iomem *ofdrm_gxt2000_cmap_ioremap(struct ofdrm_device *odev,
+						struct device_node *of_node,
+						u64 fb_base)
+{
+	return get_cmap_address_of(odev, of_node, 0, 0x6000, 0x1000);
+}
+
+static void ofdrm_gxt2000_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				     unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *data = ((unsigned int __iomem *)odev->cmap_base) + index;
+	int color = (r << 16) | (g << 8) | b;
+
+	out_le32(data, color);
+}
+
+static void __iomem *ofdrm_avivo_cmap_ioremap(struct ofdrm_device *odev,
+					      struct device_node *of_node,
+					      u64 fb_base)
+{
+	struct device_node *of_parent;
+	void __iomem *cmap_base;
+
+	of_parent = of_get_parent(of_node);
+	cmap_base = get_cmap_address_of(odev, of_parent, 0, 0, 0x10000);
+	of_node_put(of_parent);
+
+	return cmap_base;
+}
+
+static void ofdrm_avivo_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				   unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *lutsel = odev->cmap_base + AVIVO_DC_LUT_RW_SELECT;
+	void __iomem *addr = odev->cmap_base + AVIVO_DC_LUT_RW_INDEX;
+	void __iomem *data = odev->cmap_base + AVIVO_DC_LUT_30_COLOR;
+	u32 color = (r << 22) | (g << 12) | (b << 2);
+
+	/* Write to both LUTs for now */
+
+	writel(1, lutsel);
+	writeb(index, addr);
+	writel(color, data);
+
+	writel(0, lutsel);
+	writeb(index, addr);
+	writel(color, data);
+}
+
+static void __iomem *ofdrm_qemu_cmap_ioremap(struct ofdrm_device *odev,
+					     struct device_node *of_node,
+					     u64 fb_base)
+{
+#ifdef __BIG_ENDIAN
+	static const __be32 io_of_addr[3] = { 0x01000000, 0x0, 0x0 };
+#else
+	static const __be32 io_of_addr[3] = { 0x00000001, 0x0, 0x0 };
+#endif
+
+	struct drm_device *dev = &odev->dev;
+	u64 address;
+	void __iomem *cmap_base;
+
+	address = of_translate_address(of_node, io_of_addr);
+	if (address == OF_BAD_ADDR)
+		return ERR_PTR(-ENODEV);
+
+	cmap_base = devm_ioremap(dev->dev, address + 0x3c8, 2);
+	if (!cmap_base)
+		return ERR_PTR(-ENOMEM);
+
+	return cmap_base;
+}
+
+static void ofdrm_qemu_cmap_write(struct ofdrm_device *odev, unsigned char index,
+				  unsigned char r, unsigned char g, unsigned char b)
+{
+	void __iomem *addr = odev->cmap_base;
+	void __iomem *data = odev->cmap_base + 1;
+
+	writeb(index, addr);
+	writeb(r, data);
+	writeb(g, data);
+	writeb(b, data);
+}
+
+static void ofdrm_device_set_gamma_linear(struct ofdrm_device *odev,
+					  const struct drm_format_info *format)
+{
+	struct drm_device *dev = &odev->dev;
+	int i;
+
+	switch (format->format) {
+	case DRM_FORMAT_RGB565:
+		/* Use better interpolation, to take 32 values from 0 to 255 */
+		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
+			unsigned char r = i * 8 + i / 4;
+			unsigned char g = i * 4 + i / 16;
+			unsigned char b = i * 8 + i / 4;
+
+			odev->funcs->cmap_write(odev, i, r, g, b);
+		}
+		/* Green has one more bit, so add padding with 0 for red and blue. */
+		for (i = OFDRM_GAMMA_LUT_SIZE / 8; i < OFDRM_GAMMA_LUT_SIZE / 4; i++) {
+			unsigned char r = 0;
+			unsigned char g = i * 4 + i / 16;
+			unsigned char b = 0;
+
+			odev->funcs->cmap_write(odev, i, r, g, b);
+		}
+		break;
+	case DRM_FORMAT_XRGB8888:
+		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++)
+			odev->funcs->cmap_write(odev, i, i, i, i);
+		break;
+	default:
+		drm_warn_once(dev, "Unsupported format %p4cc for gamma correction\n",
+			      &format->format);
+		break;
+	}
+}
+
+static void ofdrm_device_set_gamma(struct ofdrm_device *odev,
+				   const struct drm_format_info *format,
+				   struct drm_color_lut *lut)
+{
+	struct drm_device *dev = &odev->dev;
+	int i;
+
+	switch (format->format) {
+	case DRM_FORMAT_RGB565:
+		/* Use better interpolation, to take 32 values from lut[0] to lut[255] */
+		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
+			unsigned char r = lut[i * 8 + i / 4].red >> 8;
+			unsigned char g = lut[i * 4 + i / 16].green >> 8;
+			unsigned char b = lut[i * 8 + i / 4].blue >> 8;
+
+			odev->funcs->cmap_write(odev, i, r, g, b);
+		}
+		/* Green has one more bit, so add padding with 0 for red and blue. */
+		for (i = OFDRM_GAMMA_LUT_SIZE / 8; i < OFDRM_GAMMA_LUT_SIZE / 4; i++) {
+			unsigned char r = 0;
+			unsigned char g = lut[i * 4 + i / 16].green >> 8;
+			unsigned char b = 0;
+
+			odev->funcs->cmap_write(odev, i, r, g, b);
+		}
+		break;
+	case DRM_FORMAT_XRGB8888:
+		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++) {
+			unsigned char r = lut[i].red >> 8;
+			unsigned char g = lut[i].green >> 8;
+			unsigned char b = lut[i].blue >> 8;
+
+			odev->funcs->cmap_write(odev, i, r, g, b);
+		}
+		break;
+	default:
+		drm_warn_once(dev, "Unsupported format %p4cc for gamma correction\n",
+			      &format->format);
+		break;
+	}
+}
+
 /*
  * Modesetting
  */
 
 struct ofdrm_crtc_state {
 	struct drm_crtc_state base;
+
+	/* Primary-plane format; required for color mgmt. */
+	const struct drm_format_info *format;
 };
 
 static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base)
@@ -376,10 +735,12 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						   struct drm_atomic_state *new_state)
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_framebuffer *new_fb = new_plane_state->fb;
 	struct drm_crtc_state *new_crtc_state;
+	struct ofdrm_crtc_state *new_ofdrm_crtc_state;
 	int ret;
 
-	if (!new_plane_state->fb)
+	if (!new_fb)
 		return 0;
 
 	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
@@ -391,6 +752,14 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	if (!new_plane_state->visible)
+		return 0;
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
+
+	new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
+	new_ofdrm_crtc_state->format = new_fb->format;
+
 	return 0;
 }
 
@@ -435,6 +804,9 @@ static enum drm_mode_status ofdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
 static int ofdrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 					  struct drm_atomic_state *new_state)
 {
+	static const size_t gamma_lut_length = OFDRM_GAMMA_LUT_SIZE * sizeof(struct drm_color_lut);
+
+	struct drm_device *dev = crtc->dev;
 	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
 	int ret;
 
@@ -442,9 +814,35 @@ static int ofdrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	if (new_crtc_state->color_mgmt_changed) {
+		struct drm_property_blob *gamma_lut = new_crtc_state->gamma_lut;
+
+		if (gamma_lut && (gamma_lut->length != gamma_lut_length)) {
+			drm_dbg(dev, "Incorrect gamma_lut length %zu\n", gamma_lut->length);
+			return -EINVAL;
+		}
+	}
+
 	return drm_atomic_add_affected_planes(new_state, crtc);
 }
 
+static void ofdrm_crtc_helper_atomic_flush(struct drm_crtc *crtc,
+					   struct drm_atomic_state *old_state)
+{
+	struct ofdrm_device *odev = ofdrm_device_of_dev(crtc->dev);
+	struct drm_crtc_state *crtc_state = crtc->state;
+	struct ofdrm_crtc_state *ofdrm_crtc_state = to_ofdrm_crtc_state(crtc_state);
+
+	if (crtc_state->enable && crtc_state->color_mgmt_changed) {
+		const struct drm_format_info *format = ofdrm_crtc_state->format;
+
+		if (crtc_state->gamma_lut)
+			ofdrm_device_set_gamma(odev, format, crtc_state->gamma_lut->data);
+		else
+			ofdrm_device_set_gamma_linear(odev, format);
+	}
+}
+
 static void ofdrm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 					    struct drm_atomic_state *old_state)
 {
@@ -466,6 +864,7 @@ static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs ofdrm_crtc_helper_funcs = {
 	.mode_valid = ofdrm_crtc_helper_mode_valid,
 	.atomic_check = ofdrm_crtc_helper_atomic_check,
+	.atomic_flush = ofdrm_crtc_helper_atomic_flush,
 	.atomic_enable = ofdrm_crtc_helper_atomic_enable,
 	.atomic_disable = ofdrm_crtc_helper_atomic_disable,
 };
@@ -501,6 +900,7 @@ static struct drm_crtc_state *ofdrm_crtc_atomic_duplicate_state(struct drm_crtc
 		return NULL;
 
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &new_ofdrm_crtc_state->base);
+	new_ofdrm_crtc_state->format = ofdrm_crtc_state->format;
 
 	return &new_ofdrm_crtc_state->base;
 }
@@ -553,27 +953,43 @@ static const struct ofdrm_device_funcs ofdrm_unknown_device_funcs = {
 };
 
 static const struct ofdrm_device_funcs ofdrm_mach64_device_funcs = {
+	.cmap_ioremap = ofdrm_mach64_cmap_ioremap,
+	.cmap_write = ofdrm_mach64_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_rage128_device_funcs = {
+	.cmap_ioremap = ofdrm_rage128_cmap_ioremap,
+	.cmap_write = ofdrm_rage128_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_rage_m3a_device_funcs = {
+	.cmap_ioremap = ofdrm_rage_m3a_cmap_ioremap,
+	.cmap_write = ofdrm_rage_m3a_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_rage_m3b_device_funcs = {
+	.cmap_ioremap = ofdrm_rage_m3b_cmap_ioremap,
+	.cmap_write = ofdrm_rage_m3b_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_radeon_device_funcs = {
+	.cmap_ioremap = ofdrm_radeon_cmap_ioremap,
+	.cmap_write = ofdrm_rage128_cmap_write, /* same as Rage128 */
 };
 
 static const struct ofdrm_device_funcs ofdrm_gxt2000_device_funcs = {
+	.cmap_ioremap = ofdrm_gxt2000_cmap_ioremap,
+	.cmap_write = ofdrm_gxt2000_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_avivo_device_funcs = {
+	.cmap_ioremap = ofdrm_avivo_cmap_ioremap,
+	.cmap_write = ofdrm_avivo_cmap_write,
 };
 
 static const struct ofdrm_device_funcs ofdrm_qemu_device_funcs = {
+	.cmap_ioremap = ofdrm_qemu_cmap_ioremap,
+	.cmap_write = ofdrm_qemu_cmap_write,
 };
 
 static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
@@ -716,6 +1132,17 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 		return ERR_PTR(-ENOMEM);
 	iosys_map_set_vaddr_iomem(&screen_base_vmap, screen_base);
 
+	if (odev->funcs->cmap_ioremap) {
+		void __iomem *cmap_base = odev->funcs->cmap_ioremap(odev, of_node, fb_base);
+
+		if (IS_ERR(cmap_base)) {
+			/* Don't fail; continue without colormap */
+			ret = PTR_ERR(cmap_base);
+		} else {
+			odev->cmap_base = cmap_base;
+		}
+	}
+
 	/*
 	 * Firmware framebuffer
 	 */
@@ -769,6 +1196,11 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 		return ERR_PTR(ret);
 	drm_crtc_helper_add(crtc, &ofdrm_crtc_helper_funcs);
 
+	if (odev->cmap_base) {
+		drm_mode_crtc_set_gamma_size(crtc, OFDRM_GAMMA_LUT_SIZE);
+		drm_crtc_enable_color_mgmt(crtc, 0, false, OFDRM_GAMMA_LUT_SIZE);
+	}
+
 	/* Encoder */
 
 	encoder = &odev->encoder;
-- 
2.36.1

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Javier Martinez Canillas]

On 7/20/22 16:27, Thomas Zimmermann wrote:
> Support the CRTC's color-management property and implement each model's
> palette support.
> 
> The OF hardware has different methods of setting the palette. The
> respective code has been taken from fbdev's offb and refactored into
> per-model device functions. The device functions integrate this
> functionality into the overall modesetting.
> 
> As palette handling is a CRTC property that depends on the primary
> plane's color format, the plane's atomic_check helper now updates the
> format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
> helper updates the palette for the format as needed.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[...]

> +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
> +					       struct device_node *of_node,
> +					       u64 fb_base)
> +{
> +	struct drm_device *dev = &odev->dev;
> +	u64 address;
> +	void __iomem *cmap_base;
> +
> +	address = fb_base & 0xff000000ul;
> +	address += 0x7ff000;
> +

It would be good to know where these addresses are coming from. Maybe some
constant macros or a comment ? Same for the other places where addresses
and offsets are used.

[...]

>  static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base)
> @@ -376,10 +735,12 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  						   struct drm_atomic_state *new_state)
>  {
>  	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>  	struct drm_crtc_state *new_crtc_state;
> +	struct ofdrm_crtc_state *new_ofdrm_crtc_state;
>  	int ret;
>  
> -	if (!new_plane_state->fb)
> +	if (!new_fb)
>  		return 0;
>  
>  	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
> @@ -391,6 +752,14 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> +	if (!new_plane_state->visible)
> +		return 0;
> +
> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
> +
> +	new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
> +	new_ofdrm_crtc_state->format = new_fb->format;
> +

Ah, I understand now why you didn't factor out the .atomic_check callbacks
for the two drivers in a fwfb helper. Maybe you can also add a comment to
mention that this updates the format so the CRTC palette can be applied in
the .atomic_flush callback ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3580 bytes --]

Hi

Am 26.07.22 um 15:49 schrieb Javier Martinez Canillas:
> On 7/20/22 16:27, Thomas Zimmermann wrote:
>> Support the CRTC's color-management property and implement each model's
>> palette support.
>>
>> The OF hardware has different methods of setting the palette. The
>> respective code has been taken from fbdev's offb and refactored into
>> per-model device functions. The device functions integrate this
>> functionality into the overall modesetting.
>>
>> As palette handling is a CRTC property that depends on the primary
>> plane's color format, the plane's atomic_check helper now updates the
>> format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
>> helper updates the palette for the format as needed.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [...]
> 
>> +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
>> +					       struct device_node *of_node,
>> +					       u64 fb_base)
>> +{
>> +	struct drm_device *dev = &odev->dev;
>> +	u64 address;
>> +	void __iomem *cmap_base;
>> +
>> +	address = fb_base & 0xff000000ul;
>> +	address += 0x7ff000;
>> +
> 
> It would be good to know where these addresses are coming from. Maybe some
> constant macros or a comment ? Same for the other places where addresses
> and offsets are used.

I have no idea where these values come from. I took them from offb. And 
I suspect that some of these CMAP helpers could be further merged if 
only it was clear where the numbers come from.  But as i don't have the 
equipment for testing, I took most of this literally as-is from offb.

> 
> [...]
> 
>>   static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state *base)
>> @@ -376,10 +735,12 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>   						   struct drm_atomic_state *new_state)
>>   {
>>   	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
>> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>>   	struct drm_crtc_state *new_crtc_state;
>> +	struct ofdrm_crtc_state *new_ofdrm_crtc_state;
>>   	int ret;
>>   
>> -	if (!new_plane_state->fb)
>> +	if (!new_fb)
>>   		return 0;
>>   
>>   	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
>> @@ -391,6 +752,14 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>   	if (ret)
>>   		return ret;
>>   
>> +	if (!new_plane_state->visible)
>> +		return 0;
>> +
>> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
>> +
>> +	new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
>> +	new_ofdrm_crtc_state->format = new_fb->format;
>> +
> 
> Ah, I understand now why you didn't factor out the .atomic_check callbacks
> for the two drivers in a fwfb helper. Maybe you can also add a comment to
> mention that this updates the format so the CRTC palette can be applied in
> the .atomic_flush callback ?

Yeah, this code is one reason for not sharing atomic_check in fwfb.  The 
other reason is that the fwfb code is only a wrapper around the atomic 
helpers with little extra value.  I did have such fwfb helpers a some 
point, but removed them.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Javier Martinez Canillas]

On 7/27/22 10:41, Thomas Zimmermann wrote:

[...]

>>
>>> +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
>>> +					       struct device_node *of_node,
>>> +					       u64 fb_base)
>>> +{
>>> +	struct drm_device *dev = &odev->dev;
>>> +	u64 address;
>>> +	void __iomem *cmap_base;
>>> +
>>> +	address = fb_base & 0xff000000ul;
>>> +	address += 0x7ff000;
>>> +
>>
>> It would be good to know where these addresses are coming from. Maybe some
>> constant macros or a comment ? Same for the other places where addresses
>> and offsets are used.
> 
> I have no idea where these values come from. I took them from offb. And 
> I suspect that some of these CMAP helpers could be further merged if 
> only it was clear where the numbers come from.  But as i don't have the 
> equipment for testing, I took most of this literally as-is from offb.
>

I see. As Michal mentioned maybe someone more familiar with this platform
could shed some light about these but in any case that could be done later.

[...]

>>> +
>>> +	new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
>>> +
>>> +	new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
>>> +	new_ofdrm_crtc_state->format = new_fb->format;
>>> +
>>
>> Ah, I understand now why you didn't factor out the .atomic_check callbacks
>> for the two drivers in a fwfb helper. Maybe you can also add a comment to
>> mention that this updates the format so the CRTC palette can be applied in
>> the .atomic_flush callback ?
> 
> Yeah, this code is one reason for not sharing atomic_check in fwfb.  The 
> other reason is that the fwfb code is only a wrapper around the atomic 
> helpers with little extra value.  I did have such fwfb helpers a some 
> point, but removed them.
>

Got it.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Benjamin Herrenschmidt]

On Wed, 2022-07-27 at 10:41 +0200, Thomas Zimmermann wrote:
> 
> > > +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
> > > +					       struct device_node *of_node,
> > > +					       u64 fb_base)
> > > +{
> > > +	struct drm_device *dev = &odev->dev;
> > > +	u64 address;
> > > +	void __iomem *cmap_base;
> > > +
> > > +	address = fb_base & 0xff000000ul;
> > > +	address += 0x7ff000;
> > > +
> > 
> > It would be good to know where these addresses are coming from. Maybe some
> > constant macros or a comment ? Same for the other places where addresses
> > and offsets are used.
> 
> I have no idea where these values come from. I took them from offb. And 
> I suspect that some of these CMAP helpers could be further merged if 
> only it was clear where the numbers come from.  But as i don't have the 
> equipment for testing, I took most of this literally as-is from offb.

Ancient black magic :-) Old ATI mach64 chips had the registers sitting
at the end of the framebuffer. You can find an equivalent in
drivers/video/aty/atyfb_base.c:atyfb_setup_generic():

	raddr = addr + 0x7ff000UL;

Cheers,
Ben.

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Benjamin Herrenschmidt]

On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> +#if !defined(CONFIG_PPC)
> +static inline void out_8(void __iomem *addr, int val)
> +{ }
> +static inline void out_le32(void __iomem *addr, int val)
> +{ }
> +static inline unsigned int in_le32(const void __iomem *addr)
> +{
> +       return 0;
> +}
> +#endif

These guys could just be replaced with readb/writel/readl respectively
(beware of the argument swap).

Cheers,
Ben.

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 885 bytes --]

Hi

Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
> On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
>> +#if !defined(CONFIG_PPC)
>> +static inline void out_8(void __iomem *addr, int val)
>> +{ }
>> +static inline void out_le32(void __iomem *addr, int val)
>> +{ }
>> +static inline unsigned int in_le32(const void __iomem *addr)
>> +{
>> +       return 0;
>> +}
>> +#endif
> 
> These guys could just be replaced with readb/writel/readl respectively
> (beware of the argument swap).

I only added them for COMPILE_TEST. There appears to be no portable 
interface that implements out_le32() and in_le32()?

Best regards
Thomas

> 
> Cheers,
> Ben.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
> > On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> >> +#if !defined(CONFIG_PPC)
> >> +static inline void out_8(void __iomem *addr, int val)
> >> +{ }
> >> +static inline void out_le32(void __iomem *addr, int val)
> >> +{ }
> >> +static inline unsigned int in_le32(const void __iomem *addr)
> >> +{
> >> +       return 0;
> >> +}
> >> +#endif
> >
> > These guys could just be replaced with readb/writel/readl respectively
> > (beware of the argument swap).
>
> I only added them for COMPILE_TEST. There appears to be no portable
> interface that implements out_le32() and in_le32()?

iowrite32() and ioread32()?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1560 bytes --]

Hi

Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
>>> On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
>>>> +#if !defined(CONFIG_PPC)
>>>> +static inline void out_8(void __iomem *addr, int val)
>>>> +{ }
>>>> +static inline void out_le32(void __iomem *addr, int val)
>>>> +{ }
>>>> +static inline unsigned int in_le32(const void __iomem *addr)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>> +#endif
>>>
>>> These guys could just be replaced with readb/writel/readl respectively
>>> (beware of the argument swap).
>>
>> I only added them for COMPILE_TEST. There appears to be no portable
>> interface that implements out_le32() and in_le32()?
> 
> iowrite32() and ioread32()?

Do they always use little endian, as these *_le32 helpers do? I though 
they use host byte order.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Thu, Sep 22, 2022 at 08:42:23AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
> > Hi Thomas,
> > 
> > On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
> > > > On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> > > > > +#if !defined(CONFIG_PPC)
> > > > > +static inline void out_8(void __iomem *addr, int val)
> > > > > +{ }
> > > > > +static inline void out_le32(void __iomem *addr, int val)
> > > > > +{ }
> > > > > +static inline unsigned int in_le32(const void __iomem *addr)
> > > > > +{
> > > > > +       return 0;
> > > > > +}
> > > > > +#endif
> > > > 
> > > > These guys could just be replaced with readb/writel/readl respectively
> > > > (beware of the argument swap).
> > > 
> > > I only added them for COMPILE_TEST. There appears to be no portable
> > > interface that implements out_le32() and in_le32()?
> > 
> > iowrite32() and ioread32()?
> 
> Do they always use little endian, as these *_le32 helpers do? I though they
> use host byte order.

They use either outl or writel under the hood, which are always little-endian

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 10/10] drm/ofdrm: Support color management

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1504 bytes --]

Hi

Am 22.09.22 um 09:28 schrieb Maxime Ripard:
> On Thu, Sep 22, 2022 at 08:42:23AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
>>> Hi Thomas,
>>>
>>> On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
>>>>> On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
>>>>>> +#if !defined(CONFIG_PPC)
>>>>>> +static inline void out_8(void __iomem *addr, int val)
>>>>>> +{ }
>>>>>> +static inline void out_le32(void __iomem *addr, int val)
>>>>>> +{ }
>>>>>> +static inline unsigned int in_le32(const void __iomem *addr)
>>>>>> +{
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +#endif
>>>>>
>>>>> These guys could just be replaced with readb/writel/readl respectively
>>>>> (beware of the argument swap).
>>>>
>>>> I only added them for COMPILE_TEST. There appears to be no portable
>>>> interface that implements out_le32() and in_le32()?
>>>
>>> iowrite32() and ioread32()?
>>
>> Do they always use little endian, as these *_le32 helpers do? I though they
>> use host byte order.
> 
> They use either outl or writel under the hood, which are always little-endian

I see. I'll replace the custom helpers.

Best regards
Thomas

> 
> Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 00/10] drm: Add driver for PowerPC OF displays

[Michael Ellerman]

Thomas Zimmermann <tzimmermann@suse.de> writes:
> (was: drm: Add driverof PowerPC OF displays)
>
> PowerPC's Open Firmware offers a simple display buffer for graphics
> output. Add ofdrm, a DRM driver for the device. As with the existing
> simpledrm driver, the graphics hardware is pre-initialized by the
> firmware. The driver only provides blitting, no actual DRM modesetting
> is possible.

Hi Thomas,

I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck.
But I'm probably doing something wrong because I'm a graphics noob.

The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and
turned DRM_OFDRM on.

When I boot I get boot messages but only one screen worth, the messages
don't scroll at all, which is unusual. But I'm not sure if that's
related to ofdrm or something else.

The machine does come up, I can login via SSH. Is there some way to
start X to exercise the driver from an SSH login?

cheers

Re: [PATCH v2 00/10] drm: Add driver for PowerPC OF displays

[Michal Suchánek]

Hello,

On Thu, Jul 28, 2022 at 09:13:59PM +1000, Michael Ellerman wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> > (was: drm: Add driverof PowerPC OF displays)
> >
> > PowerPC's Open Firmware offers a simple display buffer for graphics
> > output. Add ofdrm, a DRM driver for the device. As with the existing
> > simpledrm driver, the graphics hardware is pre-initialized by the
> > firmware. The driver only provides blitting, no actual DRM modesetting
> > is possible.
> 
> Hi Thomas,
> 
> I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck.
> But I'm probably doing something wrong because I'm a graphics noob.
> 
> The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and
> turned DRM_OFDRM on.
> 
> When I boot I get boot messages but only one screen worth, the messages
> don't scroll at all, which is unusual. But I'm not sure if that's
> related to ofdrm or something else.

A somewhat interesting datapoint might be how this works with offb.

> The machine does come up, I can login via SSH. Is there some way to
> start X to exercise the driver from an SSH login?

The startx script provided by distribution usually works.

It's basically a very convoluted way to do something like

X :0&
DISPLAY=:0 xterm&

Thanks

Michal