Re: [PATCH 04/12] drm/format-helper: Rework XRGB8888-to-RGBG332 conversion

2022-08-04 Thread Sam Ravnborg
Hi Thomas,

On Wed, Jul 27, 2022 at 01:33:04PM +0200, Thomas Zimmermann wrote:
> Update XRGB-to-RGB332 conversion to support struct iosys_map
> and convert all users. Although these are single-plane color formats,
> the new interface supports multi-plane formats for consistency with
> drm_fb_blit().
> 
> Signed-off-by: Thomas Zimmermann 

I am not going to repeat my naming rant here, so
Reviewed-by: Sam Ravnborg 

> ---
>  drivers/gpu/drm/drm_format_helper.c   | 25 ++-
>  drivers/gpu/drm/gud/gud_pipe.c|  2 +-
>  .../gpu/drm/tests/drm_format_helper_test.c| 14 ++-
>  include/drm/drm_format_helper.h   |  5 ++--
>  4 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c 
> b/drivers/gpu/drm/drm_format_helper.c
> index fa22d3cb11e8..2b5c3746ff4a 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -265,18 +265,31 @@ static void drm_fb_xrgb_to_rgb332_line(void *dbuf, 
> const void *sbuf, unsigne
>  
>  /**
>   * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
> - * @dst: RGB332 destination buffer
> - * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> - * @src: XRGB source buffer
> + * @dst: Array of RGB332 destination buffers
> + * @dst_pitch: Array of numbers of bytes between two consecutive scanlines 
> within dst
> + * @vmap: Array of XRGB source buffers
>   * @fb: DRM framebuffer
>   * @clip: Clip rectangle area to copy
>   *
>   * Drivers can use this function for RGB332 devices that don't natively 
> support XRGB.
>   */
> -void drm_fb_xrgb_to_rgb332(void *dst, unsigned int dst_pitch, const void 
> *src,
> -const struct drm_framebuffer *fb, const struct 
> drm_rect *clip)
> +void drm_fb_xrgb_to_rgb332(struct iosys_map *dst, const unsigned int 
> *dst_pitch,
> +const struct iosys_map *vmap, const struct 
> drm_framebuffer *fb,
> +const struct drm_rect *clip)
>  {
> - drm_fb_xfrm(dst, dst_pitch, 1, src, fb, clip, false, 
> drm_fb_xrgb_to_rgb332_line);
> + static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
> + 0, 0, 0, 0
> + };
> +
> + if (!dst_pitch)
> + dst_pitch = default_dst_pitch;
> +
> + if (dst[0].is_iomem)
> + drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, 
> vmap[0].vaddr, fb, clip,
> +  false, drm_fb_xrgb_to_rgb332_line);
> + else
> + drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, 
> clip,
> + false, drm_fb_xrgb_to_rgb332_line);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
>  
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index a15cda9ba058..426a3ae6cc50 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -196,7 +196,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct 
> drm_framebuffer *fb,
>   } else if (format->format == DRM_FORMAT_R8) {
>   drm_fb_xrgb_to_gray8(buf, 0, vaddr, fb, rect);
>   } else if (format->format == DRM_FORMAT_RGB332) {
> - drm_fb_xrgb_to_rgb332(buf, 0, vaddr, fb, rect);
> + drm_fb_xrgb_to_rgb332(, NULL, map_data, fb, 
> rect);
>   } else if (format->format == DRM_FORMAT_RGB565) {
>   drm_fb_xrgb_to_rgb565(buf, 0, vaddr, fb, rect, 
> gud_is_big_endian());
>   } else if (format->format == DRM_FORMAT_RGB888) {
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c 
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 98583bf56044..b74dba06f704 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -124,7 +124,8 @@ static void xrgb_to_rgb332_test(struct kunit *test)
>  {
>   const struct xrgb_to_rgb332_case *params = test->param_value;
>   size_t dst_size;
> - __u8 *dst = NULL;
> + struct iosys_map dst, xrgb;
> + __u8 *buf = NULL;
>  
>   struct drm_framebuffer fb = {
>   .format = drm_format_info(DRM_FORMAT_XRGB),
> @@ -135,12 +136,13 @@ static void xrgb_to_rgb332_test(struct kunit *test)
>  >clip);
>   KUNIT_ASSERT_GT(test, dst_size, 0);
>  
> - dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> + buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
>  
> - drm_fb_xrgb_to_rgb332(dst, params->dst_pitch, params->xrgb,
> -   , >clip);
> - KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> + iosys_map_set_vaddr(, buf);
> + 

Re: [PATCH 04/12] drm/format-helper: Rework XRGB8888-to-RGBG332 conversion

2022-07-28 Thread José Expósito
On Thu, Jul 28, 2022 at 09:27:52AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.07.22 um 09:13 schrieb José Expósito:
> > Hi Thomas,
> > 
> > On Wed, Jul 27, 2022 at 01:33:04PM +0200, Thomas Zimmermann wrote:
> > > Update XRGB-to-RGB332 conversion to support struct iosys_map
> > > and convert all users. Although these are single-plane color formats,
> > > the new interface supports multi-plane formats for consistency with
> > > drm_fb_blit().
> > > 
> > > Signed-off-by: Thomas Zimmermann 
> > 
> > Tested-by: José Expósito 
> > Reviewed-by: José Expósito 
> > 
> > I just ran the tests in x86_64 and UML and they work as expected.
> > I need to find some time to review all patches, but this one LGTM.
> 
> Thanks a lot.
> 
> > 
> > This series will cause conflicts with [1]. Depending on which patchset
> > gets merged earlier, we will have to resolve the conflicts in one
> > series or the other.
> 
> I've seen this. Go ahead and commit your patches if they are ready. I can
> easily rebase on top.
> 
> Best reards
> Thomas

OK, I just merged the series in drm-misc-next... With some conflicts in
drm-tip in unreleated files I'm trying to figure out on IRC.

But I think you should be able to rebase your series if you want to.

Jose

> > 
> > Best wishes,
> > Jose
> > 
> > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=663266
> > 
> > > ---
> > >   drivers/gpu/drm/drm_format_helper.c   | 25 ++-
> > >   drivers/gpu/drm/gud/gud_pipe.c|  2 +-
> > >   .../gpu/drm/tests/drm_format_helper_test.c| 14 ++-
> > >   include/drm/drm_format_helper.h   |  5 ++--
> > >   4 files changed, 31 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_format_helper.c 
> > > b/drivers/gpu/drm/drm_format_helper.c
> > > index fa22d3cb11e8..2b5c3746ff4a 100644
> > > --- a/drivers/gpu/drm/drm_format_helper.c
> > > +++ b/drivers/gpu/drm/drm_format_helper.c
> > > @@ -265,18 +265,31 @@ static void drm_fb_xrgb_to_rgb332_line(void 
> > > *dbuf, const void *sbuf, unsigne
> > >   /**
> > >* drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
> > > - * @dst: RGB332 destination buffer
> > > - * @dst_pitch: Number of bytes between two consecutive scanlines within 
> > > dst
> > > - * @src: XRGB source buffer
> > > + * @dst: Array of RGB332 destination buffers
> > > + * @dst_pitch: Array of numbers of bytes between two consecutive 
> > > scanlines within dst
> > > + * @vmap: Array of XRGB source buffers
> > >* @fb: DRM framebuffer
> > >* @clip: Clip rectangle area to copy
> > >*
> > >* Drivers can use this function for RGB332 devices that don't natively 
> > > support XRGB.
> > >*/
> > > -void drm_fb_xrgb_to_rgb332(void *dst, unsigned int dst_pitch, const 
> > > void *src,
> > > -const struct drm_framebuffer *fb, const struct 
> > > drm_rect *clip)
> > > +void drm_fb_xrgb_to_rgb332(struct iosys_map *dst, const unsigned int 
> > > *dst_pitch,
> > > +const struct iosys_map *vmap, const struct 
> > > drm_framebuffer *fb,
> > > +const struct drm_rect *clip)
> > >   {
> > > - drm_fb_xfrm(dst, dst_pitch, 1, src, fb, clip, false, 
> > > drm_fb_xrgb_to_rgb332_line);
> > > + static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
> > > + 0, 0, 0, 0
> > > + };
> > > +
> > > + if (!dst_pitch)
> > > + dst_pitch = default_dst_pitch;
> > > +
> > > + if (dst[0].is_iomem)
> > > + drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, 
> > > vmap[0].vaddr, fb, clip,
> > > +  false, drm_fb_xrgb_to_rgb332_line);
> > > + else
> > > + drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, 
> > > clip,
> > > + false, drm_fb_xrgb_to_rgb332_line);
> > >   }
> > >   EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
> > > diff --git a/drivers/gpu/drm/gud/gud_pipe.c 
> > > b/drivers/gpu/drm/gud/gud_pipe.c
> > > index a15cda9ba058..426a3ae6cc50 100644
> > > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > > @@ -196,7 +196,7 @@ static int gud_prep_flush(struct gud_device *gdrm, 
> > > struct drm_framebuffer *fb,
> > >   } else if (format->format == DRM_FORMAT_R8) {
> > >   drm_fb_xrgb_to_gray8(buf, 0, vaddr, fb, 
> > > rect);
> > >   } else if (format->format == DRM_FORMAT_RGB332) {
> > > - drm_fb_xrgb_to_rgb332(buf, 0, vaddr, fb, rect);
> > > + drm_fb_xrgb_to_rgb332(, NULL, map_data, fb, 
> > > rect);
> > >   } else if (format->format == DRM_FORMAT_RGB565) {
> > >   drm_fb_xrgb_to_rgb565(buf, 0, vaddr, fb, 
> > > rect, gud_is_big_endian());
> > >   } else if (format->format == DRM_FORMAT_RGB888) {
> > > diff --git 

Re: [PATCH 04/12] drm/format-helper: Rework XRGB8888-to-RGBG332 conversion

2022-07-28 Thread Thomas Zimmermann

Hi

Am 28.07.22 um 09:13 schrieb José Expósito:

Hi Thomas,

On Wed, Jul 27, 2022 at 01:33:04PM +0200, Thomas Zimmermann wrote:

Update XRGB-to-RGB332 conversion to support struct iosys_map
and convert all users. Although these are single-plane color formats,
the new interface supports multi-plane formats for consistency with
drm_fb_blit().

Signed-off-by: Thomas Zimmermann 


Tested-by: José Expósito 
Reviewed-by: José Expósito 

I just ran the tests in x86_64 and UML and they work as expected.
I need to find some time to review all patches, but this one LGTM.


Thanks a lot.



This series will cause conflicts with [1]. Depending on which patchset
gets merged earlier, we will have to resolve the conflicts in one
series or the other.


I've seen this. Go ahead and commit your patches if they are ready. I 
can easily rebase on top.


Best reards
Thomas



Best wishes,
Jose

[1] https://patchwork.kernel.org/project/dri-devel/list/?series=663266


---
  drivers/gpu/drm/drm_format_helper.c   | 25 ++-
  drivers/gpu/drm/gud/gud_pipe.c|  2 +-
  .../gpu/drm/tests/drm_format_helper_test.c| 14 ++-
  include/drm/drm_format_helper.h   |  5 ++--
  4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c 
b/drivers/gpu/drm/drm_format_helper.c
index fa22d3cb11e8..2b5c3746ff4a 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -265,18 +265,31 @@ static void drm_fb_xrgb_to_rgb332_line(void *dbuf, 
const void *sbuf, unsigne
  
  /**

   * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
- * @dst: RGB332 destination buffer
- * @dst_pitch: Number of bytes between two consecutive scanlines within dst
- * @src: XRGB source buffer
+ * @dst: Array of RGB332 destination buffers
+ * @dst_pitch: Array of numbers of bytes between two consecutive scanlines 
within dst
+ * @vmap: Array of XRGB source buffers
   * @fb: DRM framebuffer
   * @clip: Clip rectangle area to copy
   *
   * Drivers can use this function for RGB332 devices that don't natively 
support XRGB.
   */
-void drm_fb_xrgb_to_rgb332(void *dst, unsigned int dst_pitch, const void 
*src,
-  const struct drm_framebuffer *fb, const struct 
drm_rect *clip)
+void drm_fb_xrgb_to_rgb332(struct iosys_map *dst, const unsigned int 
*dst_pitch,
+  const struct iosys_map *vmap, const struct 
drm_framebuffer *fb,
+  const struct drm_rect *clip)
  {
-   drm_fb_xfrm(dst, dst_pitch, 1, src, fb, clip, false, 
drm_fb_xrgb_to_rgb332_line);
+   static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
+   0, 0, 0, 0
+   };
+
+   if (!dst_pitch)
+   dst_pitch = default_dst_pitch;
+
+   if (dst[0].is_iomem)
+   drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, 
vmap[0].vaddr, fb, clip,
+false, drm_fb_xrgb_to_rgb332_line);
+   else
+   drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, 
clip,
+   false, drm_fb_xrgb_to_rgb332_line);
  }
  EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
  
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c

index a15cda9ba058..426a3ae6cc50 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -196,7 +196,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct 
drm_framebuffer *fb,
} else if (format->format == DRM_FORMAT_R8) {
drm_fb_xrgb_to_gray8(buf, 0, vaddr, fb, rect);
} else if (format->format == DRM_FORMAT_RGB332) {
-   drm_fb_xrgb_to_rgb332(buf, 0, vaddr, fb, rect);
+   drm_fb_xrgb_to_rgb332(, NULL, map_data, fb, 
rect);
} else if (format->format == DRM_FORMAT_RGB565) {
drm_fb_xrgb_to_rgb565(buf, 0, vaddr, fb, rect, 
gud_is_big_endian());
} else if (format->format == DRM_FORMAT_RGB888) {
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c 
b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 98583bf56044..b74dba06f704 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -124,7 +124,8 @@ static void xrgb_to_rgb332_test(struct kunit *test)
  {
const struct xrgb_to_rgb332_case *params = test->param_value;
size_t dst_size;
-   __u8 *dst = NULL;
+   struct iosys_map dst, xrgb;
+   __u8 *buf = NULL;
  
  	struct drm_framebuffer fb = {

.format = drm_format_info(DRM_FORMAT_XRGB),
@@ -135,12 +136,13 @@ static void xrgb_to_rgb332_test(struct kunit *test)
   >clip);
KUNIT_ASSERT_GT(test, dst_size, 0);
  
-	dst = 

Re: [PATCH 04/12] drm/format-helper: Rework XRGB8888-to-RGBG332 conversion

2022-07-28 Thread José Expósito
Hi Thomas,

On Wed, Jul 27, 2022 at 01:33:04PM +0200, Thomas Zimmermann wrote:
> Update XRGB-to-RGB332 conversion to support struct iosys_map
> and convert all users. Although these are single-plane color formats,
> the new interface supports multi-plane formats for consistency with
> drm_fb_blit().
> 
> Signed-off-by: Thomas Zimmermann 

Tested-by: José Expósito 
Reviewed-by: José Expósito 

I just ran the tests in x86_64 and UML and they work as expected.
I need to find some time to review all patches, but this one LGTM.

This series will cause conflicts with [1]. Depending on which patchset
gets merged earlier, we will have to resolve the conflicts in one
series or the other.

Best wishes,
Jose

[1] https://patchwork.kernel.org/project/dri-devel/list/?series=663266

> ---
>  drivers/gpu/drm/drm_format_helper.c   | 25 ++-
>  drivers/gpu/drm/gud/gud_pipe.c|  2 +-
>  .../gpu/drm/tests/drm_format_helper_test.c| 14 ++-
>  include/drm/drm_format_helper.h   |  5 ++--
>  4 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c 
> b/drivers/gpu/drm/drm_format_helper.c
> index fa22d3cb11e8..2b5c3746ff4a 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -265,18 +265,31 @@ static void drm_fb_xrgb_to_rgb332_line(void *dbuf, 
> const void *sbuf, unsigne
>  
>  /**
>   * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
> - * @dst: RGB332 destination buffer
> - * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> - * @src: XRGB source buffer
> + * @dst: Array of RGB332 destination buffers
> + * @dst_pitch: Array of numbers of bytes between two consecutive scanlines 
> within dst
> + * @vmap: Array of XRGB source buffers
>   * @fb: DRM framebuffer
>   * @clip: Clip rectangle area to copy
>   *
>   * Drivers can use this function for RGB332 devices that don't natively 
> support XRGB.
>   */
> -void drm_fb_xrgb_to_rgb332(void *dst, unsigned int dst_pitch, const void 
> *src,
> -const struct drm_framebuffer *fb, const struct 
> drm_rect *clip)
> +void drm_fb_xrgb_to_rgb332(struct iosys_map *dst, const unsigned int 
> *dst_pitch,
> +const struct iosys_map *vmap, const struct 
> drm_framebuffer *fb,
> +const struct drm_rect *clip)
>  {
> - drm_fb_xfrm(dst, dst_pitch, 1, src, fb, clip, false, 
> drm_fb_xrgb_to_rgb332_line);
> + static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
> + 0, 0, 0, 0
> + };
> +
> + if (!dst_pitch)
> + dst_pitch = default_dst_pitch;
> +
> + if (dst[0].is_iomem)
> + drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, 
> vmap[0].vaddr, fb, clip,
> +  false, drm_fb_xrgb_to_rgb332_line);
> + else
> + drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, 
> clip,
> + false, drm_fb_xrgb_to_rgb332_line);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
>  
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index a15cda9ba058..426a3ae6cc50 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -196,7 +196,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct 
> drm_framebuffer *fb,
>   } else if (format->format == DRM_FORMAT_R8) {
>   drm_fb_xrgb_to_gray8(buf, 0, vaddr, fb, rect);
>   } else if (format->format == DRM_FORMAT_RGB332) {
> - drm_fb_xrgb_to_rgb332(buf, 0, vaddr, fb, rect);
> + drm_fb_xrgb_to_rgb332(, NULL, map_data, fb, 
> rect);
>   } else if (format->format == DRM_FORMAT_RGB565) {
>   drm_fb_xrgb_to_rgb565(buf, 0, vaddr, fb, rect, 
> gud_is_big_endian());
>   } else if (format->format == DRM_FORMAT_RGB888) {
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c 
> b/drivers/gpu/drm/tests/drm_format_helper_test.c
> index 98583bf56044..b74dba06f704 100644
> --- a/drivers/gpu/drm/tests/drm_format_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -124,7 +124,8 @@ static void xrgb_to_rgb332_test(struct kunit *test)
>  {
>   const struct xrgb_to_rgb332_case *params = test->param_value;
>   size_t dst_size;
> - __u8 *dst = NULL;
> + struct iosys_map dst, xrgb;
> + __u8 *buf = NULL;
>  
>   struct drm_framebuffer fb = {
>   .format = drm_format_info(DRM_FORMAT_XRGB),
> @@ -135,12 +136,13 @@ static void xrgb_to_rgb332_test(struct kunit *test)
>  >clip);
>   KUNIT_ASSERT_GT(test, dst_size, 0);
>  
> - dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> -