Am 07.12.21 um 10:02 schrieb Thomas Zimmermann:

Hi

Am 07.12.21 um 08:29 schrieb Hector Martin:

...

This code is required for both simplefb and simpledrm, so let's move it into the OF core instead of having it as an ad-hoc initcall in the drivers.

Signed-off-by: Hector Martin marcan@marcan.st

Acked-by: Thomas Zimmermann tzimmermann@suse.de

Well, please don't take this as a review. :)

This looks much better than before. Thank you.

...

---

drivers/of/platform.c          |  5 +++++   drivers/video/fbdev/simplefb.c | 21 +--------------------   2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b3faf89744aa..e097f40b03c0 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -540,6 +540,11 @@ static int __init of_platform_default_populate_init(void)           of_node_put(node);       } +    for_each_child_of_node(of_chosen, node) { +        if (of_device_is_compatible(node, "simple-framebuffer")) +            of_platform_device_create(node, NULL, NULL); +    }

/* Populate everything else. */       of_platform_default_populate(NULL, NULL, NULL); diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index b63074fd892e..57541887188b 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -541,26 +541,7 @@ static struct platform_driver simplefb_driver = {       .remove = simplefb_remove,   }; -static int __init simplefb_init(void) -{ -    int ret; -    struct device_node *np;

-    ret = platform_driver_register(&simplefb_driver); -    if (ret) -        return ret;

-    if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { -        for_each_child_of_node(of_chosen, np) { -            if (of_device_is_compatible(np, "simple-framebuffer")) -                of_platform_device_create(np, NULL, NULL); -        } -    }

-    return 0; -}

-fs_initcall(simplefb_init); +module_platform_driver(simplefb_driver);   MODULE_AUTHOR("Stephen Warren swarren@wwwdotorg.org");   MODULE_DESCRIPTION("Simple framebuffer driver");

Hi

Am 08.12.21 um 18:49 schrieb Rob Herring:

On Tue, Dec 7, 2021 at 1:31 AM Hector Martin marcan@marcan.st wrote:

...

This code is required for both simplefb and simpledrm, so let's move it into the OF core instead of having it as an ad-hoc initcall in the drivers.

Signed-off-by: Hector Martin marcan@marcan.st

drivers/of/platform.c | 5 +++++ drivers/video/fbdev/simplefb.c | 21 +-------------------- 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b3faf89744aa..e097f40b03c0 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -540,6 +540,11 @@ static int __init of_platform_default_populate_init(void) of_node_put(node); }

•   for_each_child_of_node(of_chosen, node) {
  

•           if (of_device_is_compatible(node, "simple-framebuffer"))
  

node = of_get_compatible_child(of_chosen, "simple-framebuffer"); of_platform_device_create(node, NULL, NULL); of_node_put(node);

Please Cc the DT list. Looks like this patch can be applied independently. (Better get the other 2 into drm-misc soon or it will miss 5.17).

Can we merge the whole patchset through drm-misc? Patches 2 and 3 are useless without the first one.

Best regards Thomas

Rob

Hi

Am 07.12.21 um 08:29 schrieb Hector Martin:

Add XRGB8888 emulation support for devices that can only do XRGB2101010.

This is chiefly useful for simpledrm on Apple devices where the bootloader-provided framebuffer is 10-bit.

Signed-off-by: Hector Martin marcan@marcan.st

drivers/gpu/drm/drm_format_helper.c | 62 +++++++++++++++++++++++++++++ include/drm/drm_format_helper.h | 3 ++ 2 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index dbe3e830096e..edd611d3ab6a 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -409,6 +409,59 @@ void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch, } EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio);

+static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,

• 				unsigned int pixels)
  

+{

• unsigned int x;
• for (x = 0; x < pixels; x++) {

• *dbuf++ = ((sbuf[x] & 0x000000FF) << 2) |
  

• 	  ((sbuf[x] & 0x0000FF00) << 4) |
  

• 	  ((sbuf[x] & 0x00FF0000) << 6);
  

This isn't quite right. The lowest two destination bits in each component will always be zero. You have to do the shifting as above and for each component the two highest source bits have to be OR'ed into the two lowest destination bits. For example the source bits in a component are numbered 7 to 0

| 7 6 5 4 3 2 1 0 |

then the destination bits should be

| 7 6 5 4 3 2 1 0 7 6 |

Best regards Thomas

• }

+}

+/**

• • drm_fb_xrgb8888_to_xrgb2101010_toio - Convert XRGB8888 to XRGB2101010 clip
• • buffer
• • @dst: XRGB2101010 destination buffer (iomem)
• • @dst_pitch: Number of bytes between two consecutive scanlines within dst
• • @vaddr: XRGB8888 source buffer
• • @fb: DRM framebuffer
• • @clip: Clip rectangle area to copy
• • Drivers can use this function for XRGB2101010 devices that don't natively
• • support XRGB8888.
• */

+void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst,

• 			 unsigned int dst_pitch, const void *vaddr,
  

• 			 const struct drm_framebuffer *fb,
  

• 			 const struct drm_rect *clip)
  

+{

• size_t linepixels = clip->x2 - clip->x1;
• size_t dst_len = linepixels * sizeof(u32);
• unsigned y, lines = clip->y2 - clip->y1;
• void *dbuf;
• if (!dst_pitch)

• dst_pitch = dst_len;
  

• dbuf = kmalloc(dst_len, GFP_KERNEL);
• if (!dbuf)

• return;
  

• vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
• for (y = 0; y < lines; y++) {

• drm_fb_xrgb8888_to_xrgb2101010_line(dbuf, vaddr, linepixels);
  

• memcpy_toio(dst, dbuf, dst_len);
  

• vaddr += fb->pitches[0];
  

• dst += dst_pitch;
  

• }
• kfree(dbuf);

+} +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio);

• /**
  • drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
  • @dst: 8-bit grayscale destination buffer

@@ -500,6 +553,10 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for fb_format = DRM_FORMAT_XRGB8888; if (dst_format == DRM_FORMAT_ARGB8888) dst_format = DRM_FORMAT_XRGB8888;

• if (fb_format == DRM_FORMAT_ARGB2101010)

• fb_format = DRM_FORMAT_XRGB2101010;
  

• if (dst_format == DRM_FORMAT_ARGB2101010)

• dst_format = DRM_FORMAT_XRGB2101010;
  
  

  if (dst_format == fb_format) { drm_fb_memcpy_toio(dst, dst_pitch, vmap, fb, clip);

@@ -515,6 +572,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for drm_fb_xrgb8888_to_rgb888_toio(dst, dst_pitch, vmap, fb, clip); return 0; }

• } else if (dst_format == DRM_FORMAT_XRGB2101010) {

• if (fb_format == DRM_FORMAT_XRGB8888) {
  

• 	drm_fb_xrgb8888_to_xrgb2101010_toio(dst, dst_pitch, vmap, fb, clip);
  

• 	return 0;
  

• }
  

  }

  return -EINVAL;

diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h index 97e4c3223af3..b30ed5de0a33 100644 --- a/include/drm/drm_format_helper.h +++ b/include/drm/drm_format_helper.h @@ -33,6 +33,9 @@ void drm_fb_xrgb8888_to_rgb888(void *dst, unsigned int dst_pitch, const void *sr void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch, const void *vaddr, const struct drm_framebuffer *fb, const struct drm_rect *clip); +void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst, unsigned int dst_pitch,

• 			 const void *vaddr, const struct drm_framebuffer *fb,
  

• 			 const struct drm_rect *clip);
  

  void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vaddr, const struct drm_framebuffer *fb, const struct drm_rect *clip);

Hi

Am 07.12.21 um 10:54 schrieb Hector Martin:

Hi, thanks for the review!

On 07/12/2021 18.40, Thomas Zimmermann wrote:

...

Hi

Am 07.12.21 um 08:29 schrieb Hector Martin:

...

Add XRGB8888 emulation support for devices that can only do XRGB2101010.

This is chiefly useful for simpledrm on Apple devices where the bootloader-provided framebuffer is 10-bit.

Signed-off-by: Hector Martin marcan@marcan.st

drivers/gpu/drm/drm_format_helper.c | 62 +++++++++++++++++++++++++++++    include/drm/drm_format_helper.h     |  3 ++    2 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index dbe3e830096e..edd611d3ab6a 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -409,6 +409,59 @@ void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,    }    EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio); +static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf, +                        unsigned int pixels) +{ +    unsigned int x;

+    for (x = 0; x < pixels; x++) { +        *dbuf++ = ((sbuf[x] & 0x000000FF) << 2) | +              ((sbuf[x] & 0x0000FF00) << 4) | +              ((sbuf[x] & 0x00FF0000) << 6);

This isn't quite right. The lowest two destination bits in each component will always be zero. You have to do the shifting as above and for each component the two highest source bits have to be OR'ed into the two lowest destination bits. For example the source bits in a component are numbered 7 to 0

| 7 6 5 4 3 2 1 0 |

then the destination bits should be

| 7 6 5 4 3 2 1 0 7 6 |

I think both approaches have pros and cons. Leaving the two LSBs always at 0 yields a fully linear transfer curve with no discontinuities, but means the maximum brightness is slightly less than full. Setting them fully maps the brightness range, but creates 4 double wide steps in the transfer curve (also it's potentially slightly slower CPU-wise).

If you prefer the latter I'll do that for v2.

We don't give guarantees for color output unless color spaces are involved. But the lack of LSB bits can be more visible than larger steps in the curve. With the current formats here, it's probably a non-issue. But there can be conversions, such as RGB444 to RGB88, where these missing LSBs make a visible difference.

Therefore, please change the algorithm. It produces more consistent results over a variety of format conversion. It's better to have the same (default) algorithm for all of them.

Best regards Thomas