[PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Thomas Zimmermann]

DRM's format-conversion helpers require temporary memory. Pass the
buffer from the caller and keep it allocated over several calls. Allow
the caller to preallocate the buffer memory.

The motivation for this patchset is the recent work on a DRM panic
handler. [1] The panic handler requires format conversion to display an
error to the screen. But allocating memory during kernel panics is
fragile. The changes in this patchset enable the DRM panic handler to
preallocate buffer storage before the panic occurs.

As an additonal benefit, drivers can now keep the temporary storage
across multiple updates. Avoiding memory allocation slightly reduces
the CPU overhead of the format helpers.

Patch 1 adds struct drm_format_conv_state, a simple interface to pass
around the buffer storage. Patch 2 adds an instance of the struct to
the shadow-plane state. Patch 3 moves the buffer's memory management
from the format helpers into their callers within the DRM drivers. Most
of the afected drivers use the state instance stored in their shadow-
plane state. The shadow-plane code releases the buffer memory automatically.

Patches 4 to 7 update three drivers to preallocate the format-conversion
buffer in their plane's atomic_check function. The driver thus detects OOM
errors before the display update begins.

Tested with simpledrm.

v4:
	* rename struct to drm_format_conv_state (Javier)
	* replace ARRAY_SIZE() with sizeof() (Jani)
	* store buffer in shadow-plane state (Javier, Maxime)
	* prealloc in atomic_check in several drivers
v3:
	* no changes
v2:
	* reserve storage during probing in the drivers

[1] https://patchwork.freedesktop.org/series/122244/

Thomas Zimmermann (7):
  drm/format-helper: Cache buffers with struct drm_format_conv_state
  drm/atomic-helper: Add format-conversion state to shadow-plane state
  drm/format-helper: Pass format-conversion state to helpers
  drm/ofdrm: Preallocate format-conversion buffer in atomic_check
  drm/simpledrm: Preallocate format-conversion buffer in atomic_check
  drm/ssd130x: Fix atomic_check for disabled planes
  drm/ssd130x: Preallocate format-conversion buffer in atomic_check

 drivers/gpu/drm/drm_format_helper.c           | 212 +++++++++++++-----
 drivers/gpu/drm/drm_gem_atomic_helper.c       |   9 +
 drivers/gpu/drm/drm_mipi_dbi.c                |  19 +-
 drivers/gpu/drm/gud/gud_pipe.c                |  30 ++-
 drivers/gpu/drm/solomon/ssd130x.c             |  36 ++-
 .../gpu/drm/tests/drm_format_helper_test.c    |  72 +++---
 drivers/gpu/drm/tiny/cirrus.c                 |   3 +-
 drivers/gpu/drm/tiny/ili9225.c                |  10 +-
 drivers/gpu/drm/tiny/ofdrm.c                  |  16 +-
 drivers/gpu/drm/tiny/repaper.c                |   8 +-
 drivers/gpu/drm/tiny/simpledrm.c              |  43 +++-
 drivers/gpu/drm/tiny/st7586.c                 |  19 +-
 include/drm/drm_format_helper.h               |  81 +++++--
 include/drm/drm_gem_atomic_helper.h           |  10 +
 include/drm/drm_mipi_dbi.h                    |   4 +-
 15 files changed, 428 insertions(+), 144 deletions(-)


base-commit: 57d3b83a83c5527325efb5bcaf594da09fe4a41b
-- 
2.42.0

[PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Thomas Zimmermann]

Hold temporary memory for format conversion in an instance of struct
drm_format_conv_state. Update internal helpers of DRM's format-conversion
code accordingly. Drivers will later be able to maintain this cache by
themselves.

Besides caching, struct drm_format_conv_state will be useful to hold
additional information for format conversion, such as palette data or
foreground/background colors. This will enable conversion from indexed
color formats to component-based formats.

v3:
	* rename struct drm_xfrm_buf to struct drm_format_conv_state
	  (Javier)
	* remove managed cleanup
	* add drm_format_conv_state_copy() for shadow-plane support

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_format_helper.c | 115 +++++++++++++++++++++++++---
 include/drm/drm_format_helper.h     |  51 ++++++++++++
 2 files changed, 155 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index f93a4efcee909..37c499ae4fe4f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -20,6 +20,94 @@
 #include <drm/drm_print.h>
 #include <drm/drm_rect.h>
 
+/**
+ * drm_format_conv_state_init - Initialize format-conversion state
+ * @state: The state to initialize
+ *
+ * Clears all fields in struct drm_format_conv_state and installs a DRM
+ * release action for the buffer. The buffer will be empty with no
+ * preallocated resources.
+ */
+void drm_format_conv_state_init(struct drm_format_conv_state *state)
+{
+	state->tmp.mem = NULL;
+	state->tmp.size = 0;
+	state->tmp.preallocated = false;
+}
+EXPORT_SYMBOL(drm_format_conv_state_init);
+
+/**
+ * drm_format_conv_state_copy - Copy format-conversion state
+ * @state: Destination state
+ * @old_state: Source state
+ *
+ * Copies format-conversion state from @old_state to @state; except for
+ * temporary storage.
+ */
+void drm_format_conv_state_copy(struct drm_format_conv_state *state,
+				const struct drm_format_conv_state *old_state)
+{
+	state->tmp.mem = NULL;
+	state->tmp.size = 0;
+	state->tmp.preallocated = false;
+}
+EXPORT_SYMBOL(drm_format_conv_state_copy);
+
+/**
+ * drm_format_conv_state_reserve - Allocates storage for format conversion
+ * @state: The format-conversion state
+ * @new_size: The minimum allocation size
+ * @flags: Flags for kmalloc()
+ *
+ * Allocates at least @new_size bytes and returns a pointer to the memory
+ * range. After calling this function, previously returned memory blocks
+ * are invalid. It's best to collect all memory requirements of a format
+ * conversion and call this function once to allocate the range.
+ *
+ * Returns:
+ * A pointer to the allocated memory range, or NULL otherwise.
+ */
+void *drm_format_conv_state_reserve(struct drm_format_conv_state *state,
+				    size_t new_size, gfp_t flags)
+{
+	void *mem;
+
+	if (new_size <= state->tmp.size)
+		goto out;
+	else if (state->tmp.preallocated)
+		return NULL;
+
+	mem = krealloc(state->tmp.mem, new_size, flags);
+	if (!mem)
+		return NULL;
+
+	state->tmp.mem = mem;
+	state->tmp.size = new_size;
+
+out:
+	return state->tmp.mem;
+}
+EXPORT_SYMBOL(drm_format_conv_state_reserve);
+
+/**
+ * drm_format_conv_state_release - Releases an format-conversion storage
+ * @state: The format-conversion state
+ *
+ * Releases the memory range references by the xfrm buffer. After
+ * this call, all pointers to the memory are invalid. Prefer
+ * drm_format_conv_state_init() for cleaning up and unloading a driver.
+ */
+void drm_format_conv_state_release(struct drm_format_conv_state *state)
+{
+	if (state->tmp.preallocated)
+		return;
+
+	kfree(state->tmp.mem);
+	state->tmp.mem = NULL;
+	state->tmp.size = 0;
+}
+EXPORT_SYMBOL(drm_format_conv_state_release);
+
 static unsigned int clip_offset(const struct drm_rect *clip, unsigned int pitch, unsigned int cpp)
 {
 	return clip->y1 * pitch + clip->x1 * cpp;
@@ -45,6 +133,7 @@ EXPORT_SYMBOL(drm_fb_clip_offset);
 static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
 			 const void *vaddr, const struct drm_framebuffer *fb,
 			 const struct drm_rect *clip, bool vaddr_cached_hint,
+			 struct drm_format_conv_state *state,
 			 void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
 {
 	unsigned long linepixels = drm_rect_width(clip);
@@ -60,7 +149,7 @@ static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_p
 	 * one line at a time.
 	 */
 	if (!vaddr_cached_hint) {
-		stmp = kmalloc(sbuf_len, GFP_KERNEL);
+		stmp = drm_format_conv_state_reserve(state, sbuf_len, GFP_KERNEL);
 		if (!stmp)
 			return -ENOMEM;
 	}
@@ -79,8 +168,6 @@ static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_p
 		dst += dst_pitch;
 	}
 
-	kfree(stmp);
-
 	return 0;
 }
 
@@ -88,6 +175,7 @@ static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long dst_p
 static int __drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsigned long dst_pixsize,
 			      const void *vaddr, const struct drm_framebuffer *fb,
 			      const struct drm_rect *clip, bool vaddr_cached_hint,
+			      struct drm_format_conv_state *state,
 			      void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
 {
 	unsigned long linepixels = drm_rect_width(clip);
@@ -101,9 +189,9 @@ static int __drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsign
 	void *dbuf;
 
 	if (vaddr_cached_hint) {
-		dbuf = kmalloc(dbuf_len, GFP_KERNEL);
+		dbuf = drm_format_conv_state_reserve(state, dbuf_len, GFP_KERNEL);
 	} else {
-		dbuf = kmalloc(stmp_off + sbuf_len, GFP_KERNEL);
+		dbuf = drm_format_conv_state_reserve(state, stmp_off + sbuf_len, GFP_KERNEL);
 		stmp = dbuf + stmp_off;
 	}
 	if (!dbuf)
@@ -124,8 +212,6 @@ static int __drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, unsign
 		dst += dst_pitch;
 	}
 
-	kfree(dbuf);
-
 	return 0;
 }
 
@@ -139,17 +225,24 @@ static int drm_fb_xfrm(struct iosys_map *dst,
 	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
 		0, 0, 0, 0
 	};
+	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
+	int ret;
 
 	if (!dst_pitch)
 		dst_pitch = default_dst_pitch;
 
 	/* TODO: handle src in I/O memory here */
 	if (dst[0].is_iomem)
-		return __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
-					  src[0].vaddr, fb, clip, vaddr_cached_hint, xfrm_line);
+		ret = __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
+					 src[0].vaddr, fb, clip, vaddr_cached_hint, &fmtcnv_state,
+					 xfrm_line);
 	else
-		return __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
-				     src[0].vaddr, fb, clip, vaddr_cached_hint, xfrm_line);
+		ret = __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
+				    src[0].vaddr, fb, clip, vaddr_cached_hint, &fmtcnv_state,
+				    xfrm_line);
+	drm_format_conv_state_release(&fmtcnv_state);
+
+	return ret;
 }
 
 /**
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 291deb09475bb..724a9baf7315d 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -15,6 +15,57 @@ struct drm_rect;
 
 struct iosys_map;
 
+/**
+ * struct drm_format_conv_state - Stores format-conversion state
+ *
+ * DRM helpers for format conversion store temporary state in
+ * struct drm_xfrm_buf. The buffer's resources can be reused
+ * among multiple conversion operations.
+ *
+ * All fields are considered private.
+ */
+struct drm_format_conv_state {
+	struct {
+		void *mem;
+		size_t size;
+		bool preallocated;
+	} tmp;
+};
+
+#define __DRM_FORMAT_CONV_STATE_INIT(_mem, _size, _preallocated) { \
+		.tmp = { \
+			.mem = (_mem), \
+			.size = (_size), \
+			.preallocated = (_preallocated), \
+		} \
+	}
+
+/**
+ * DRM_FORMAT_CONV_STATE_INIT - Initializer for struct drm_format_conv_state
+ *
+ * Initializes an instance of struct drm_format_conv_state to default values.
+ */
+#define DRM_FORMAT_CONV_STATE_INIT \
+	__DRM_FORMAT_CONV_STATE_INIT(NULL, 0, false)
+
+/**
+ * DRM_FORMAT_CONV_STATE_INIT_PREALLOCATED - Initializer for struct drm_format_conv_state
+ * @_mem: The preallocated memory area
+ * @_size: The number of bytes in _mem
+ *
+ * Initializes an instance of struct drm_format_conv_state to preallocated
+ * storage. The caller is responsible for releasing the provided memory range.
+ */
+#define DRM_FORMAT_CONV_STATE_INIT_PREALLOCATED(_mem, _size) \
+	__DRM_FORMAT_CONV_STATE_INIT(_mem, _size, true)
+
+void drm_format_conv_state_init(struct drm_format_conv_state *state);
+void drm_format_conv_state_copy(struct drm_format_conv_state *state,
+				const struct drm_format_conv_state *old_state);
+void *drm_format_conv_state_reserve(struct drm_format_conv_state *state,
+				    size_t new_size, gfp_t flags);
+void drm_format_conv_state_release(struct drm_format_conv_state *state);
+
 unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
 				const struct drm_rect *clip);
 
-- 
2.42.0

Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Noralf Trønnes]

On 10/5/23 11:04, Thomas Zimmermann wrote:
> Hold temporary memory for format conversion in an instance of struct
> drm_format_conv_state. Update internal helpers of DRM's format-conversion
> code accordingly. Drivers will later be able to maintain this cache by
> themselves.
> 
> Besides caching, struct drm_format_conv_state will be useful to hold
> additional information for format conversion, such as palette data or
> foreground/background colors. This will enable conversion from indexed
> color formats to component-based formats.
> 
> v3:
> 	* rename struct drm_xfrm_buf to struct drm_format_conv_state
> 	  (Javier)
> 	* remove managed cleanup
> 	* add drm_format_conv_state_copy() for shadow-plane support
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 115 +++++++++++++++++++++++++---
>  include/drm/drm_format_helper.h     |  51 ++++++++++++
>  2 files changed, 155 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index f93a4efcee909..37c499ae4fe4f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -20,6 +20,94 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_rect.h>
>  
> +/**
> + * drm_format_conv_state_init - Initialize format-conversion state
> + * @state: The state to initialize
> + *
> + * Clears all fields in struct drm_format_conv_state and installs a DRM
> + * release action for the buffer. The buffer will be empty with no
> + * preallocated resources.

You forgot to remove the release action note in this version.

With that fixed:

Acked-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hold temporary memory for format conversion in an instance of struct
> drm_format_conv_state. Update internal helpers of DRM's format-conversion
> code accordingly. Drivers will later be able to maintain this cache by
> themselves.
>
> Besides caching, struct drm_format_conv_state will be useful to hold
> additional information for format conversion, such as palette data or
> foreground/background colors. This will enable conversion from indexed
> color formats to component-based formats.
>
> v3:
> 	* rename struct drm_xfrm_buf to struct drm_format_conv_state
> 	  (Javier)
> 	* remove managed cleanup
> 	* add drm_format_conv_state_copy() for shadow-plane support
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> +/**
> + * drm_format_conv_state_init - Initialize format-conversion state
> + * @state: The state to initialize
> + *
> + * Clears all fields in struct drm_format_conv_state and installs a DRM
> + * release action for the buffer. The buffer will be empty with no
> + * preallocated resources.
> + */
> +void drm_format_conv_state_init(struct drm_format_conv_state *state)
> +{
> +	state->tmp.mem = NULL;
> +	state->tmp.size = 0;
> +	state->tmp.preallocated = false;
> +}
> +EXPORT_SYMBOL(drm_format_conv_state_init);
> +
> +/**
> + * drm_format_conv_state_copy - Copy format-conversion state
> + * @state: Destination state
> + * @old_state: Source state
> + *
> + * Copies format-conversion state from @old_state to @state; except for
> + * temporary storage.
> + */
> +void drm_format_conv_state_copy(struct drm_format_conv_state *state,
> +				const struct drm_format_conv_state *old_state)
> +{
> +	state->tmp.mem = NULL;
> +	state->tmp.size = 0;
> +	state->tmp.preallocated = false;
> +}
> +EXPORT_SYMBOL(drm_format_conv_state_copy);
> +

I'm confused, the copy helper is the same than init. What's the point of
this function ? Why not just call drm_format_conv_state_init() from the
__drm_gem_duplicate_shadow_plane_state() function in the next patch ?

Other than that the patch looks good to me. After fixing the issue that
Noralf pointed out:

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Thomas Zimmermann]

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

Hi

Am 05.10.23 um 15:18 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Hold temporary memory for format conversion in an instance of struct
>> drm_format_conv_state. Update internal helpers of DRM's format-conversion
>> code accordingly. Drivers will later be able to maintain this cache by
>> themselves.
>>
>> Besides caching, struct drm_format_conv_state will be useful to hold
>> additional information for format conversion, such as palette data or
>> foreground/background colors. This will enable conversion from indexed
>> color formats to component-based formats.
>>
>> v3:
>> 	* rename struct drm_xfrm_buf to struct drm_format_conv_state
>> 	  (Javier)
>> 	* remove managed cleanup
>> 	* add drm_format_conv_state_copy() for shadow-plane support
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +/**
>> + * drm_format_conv_state_init - Initialize format-conversion state
>> + * @state: The state to initialize
>> + *
>> + * Clears all fields in struct drm_format_conv_state and installs a DRM
>> + * release action for the buffer. The buffer will be empty with no
>> + * preallocated resources.
>> + */
>> +void drm_format_conv_state_init(struct drm_format_conv_state *state)
>> +{
>> +	state->tmp.mem = NULL;
>> +	state->tmp.size = 0;
>> +	state->tmp.preallocated = false;
>> +}
>> +EXPORT_SYMBOL(drm_format_conv_state_init);
>> +
>> +/**
>> + * drm_format_conv_state_copy - Copy format-conversion state
>> + * @state: Destination state
>> + * @old_state: Source state
>> + *
>> + * Copies format-conversion state from @old_state to @state; except for
>> + * temporary storage.
>> + */
>> +void drm_format_conv_state_copy(struct drm_format_conv_state *state,
>> +				const struct drm_format_conv_state *old_state)
>> +{
>> +	state->tmp.mem = NULL;
>> +	state->tmp.size = 0;
>> +	state->tmp.preallocated = false;
>> +}
>> +EXPORT_SYMBOL(drm_format_conv_state_copy);
>> +
> 
> I'm confused, the copy helper is the same than init. What's the point of
> this function ? Why not just call drm_format_conv_state_init() from the
> __drm_gem_duplicate_shadow_plane_state() function in the next patch ?

I guess that deserves a comment in the code. The reserved buffer is not 
to be copied to another state. So we just clear the fields. But in the 
future, we will likely be extra fields, such as the aforementioned 
palette data, that will be copied. It's just that these fields don't 
exist yet. Hence the copy function is different from the init.

Best regards
Thomas

> 
> Other than that the patch looks good to me. After fixing the issue that
> Noralf pointed out:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>

[...]

>> 
>> I'm confused, the copy helper is the same than init. What's the point of
>> this function ? Why not just call drm_format_conv_state_init() from the
>> __drm_gem_duplicate_shadow_plane_state() function in the next patch ?
>
> I guess that deserves a comment in the code. The reserved buffer is not 
> to be copied to another state. So we just clear the fields. But in the 
> future, we will likely be extra fields, such as the aforementioned 
> palette data, that will be copied. It's just that these fields don't 
> exist yet. Hence the copy function is different from the init.
>

Maybe the change is premature then and we could do once the palette data
(or any other field is added) ? But I am also OK with just a comment that
explains the reason for this function.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 2/7] drm/atomic-helper: Add format-conversion state to shadow-plane state

[Thomas Zimmermann]

Store an instance of struct drm_format_conv_state in the shadow-plane
state struct drm_shadow_plane_state. Many drivers with shadow planes
use DRM's format helpers to copy or convert the framebuffer data to
backing storage in the scanout buffer. The shadow plane provides the
necessary state and manages the conversion's intermediate buffer memory.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_atomic_helper.c |  9 +++++++++
 include/drm/drm_gem_atomic_helper.h     | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index 5d4b9cd077f7a..e440f458b6633 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -218,7 +218,14 @@ void
 __drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane,
 				       struct drm_shadow_plane_state *new_shadow_plane_state)
 {
+	struct drm_plane_state *plane_state = plane->state;
+	struct drm_shadow_plane_state *shadow_plane_state =
+		to_drm_shadow_plane_state(plane_state);
+
 	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
+
+	drm_format_conv_state_copy(&shadow_plane_state->fmtcnv_state,
+				   &new_shadow_plane_state->fmtcnv_state);
 }
 EXPORT_SYMBOL(__drm_gem_duplicate_shadow_plane_state);
 
@@ -266,6 +273,7 @@ EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
  */
 void __drm_gem_destroy_shadow_plane_state(struct drm_shadow_plane_state *shadow_plane_state)
 {
+	drm_format_conv_state_release(&shadow_plane_state->fmtcnv_state);
 	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
 }
 EXPORT_SYMBOL(__drm_gem_destroy_shadow_plane_state);
@@ -302,6 +310,7 @@ void __drm_gem_reset_shadow_plane(struct drm_plane *plane,
 				  struct drm_shadow_plane_state *shadow_plane_state)
 {
 	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
+	drm_format_conv_state_init(&shadow_plane_state->fmtcnv_state);
 }
 EXPORT_SYMBOL(__drm_gem_reset_shadow_plane);
 
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index 40b8b039518e0..3e01c619a25e0 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -5,6 +5,7 @@
 
 #include <linux/iosys-map.h>
 
+#include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_plane.h>
 
@@ -49,6 +50,15 @@ struct drm_shadow_plane_state {
 	/** @base: plane state */
 	struct drm_plane_state base;
 
+	/**
+	 * @fmtcnv_state: Format-conversion state
+	 *
+	 * Per-plane state for format conversion.
+	 * Flags for copying shadow buffers into backend storage. Also holds
+	 * temporary storage for format conversion.
+	 */
+	struct drm_format_conv_state fmtcnv_state;
+
 	/* Transitional state - do not export or duplicate */
 
 	/**
-- 
2.42.0

Re: [PATCH v4 2/7] drm/atomic-helper: Add format-conversion state to shadow-plane state

[Noralf Trønnes]

On 10/5/23 11:04, Thomas Zimmermann wrote:
> Store an instance of struct drm_format_conv_state in the shadow-plane
> state struct drm_shadow_plane_state. Many drivers with shadow planes
> use DRM's format helpers to copy or convert the framebuffer data to
> backing storage in the scanout buffer. The shadow plane provides the
> necessary state and manages the conversion's intermediate buffer memory.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Acked-by: Noralf Trønnes <noralf@tronnes.org>

Re: [PATCH v4 2/7] drm/atomic-helper: Add format-conversion state to shadow-plane state

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Store an instance of struct drm_format_conv_state in the shadow-plane
> state struct drm_shadow_plane_state. Many drivers with shadow planes
> use DRM's format helpers to copy or convert the framebuffer data to
> backing storage in the scanout buffer. The shadow plane provides the
> necessary state and manages the conversion's intermediate buffer memory.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[Thomas Zimmermann]

Pass an instance of struct drm_format_conv_state to DRM's format
conversion helpers. Update all callers.

Most drivers can use the format-conversion state from their shadow-
plane state. The shadow plane's destroy function releases the
allocated buffer. Drivers will later be able to allocate a buffer
of appropriate size in their plane's atomic_check code.

The gud driver uses a separate thread for committing updates. For
now, the update worker contains its own format-conversion state.

Images in the format-helper tests are small. The tests preallocate
a static page for the temporary buffer. Unloading the module releases
the memory.

v3:
	* store buffer in shadow-plane state (Javier, Maxime)
	* replace ARRAY_SIZE() with sizeof() (Jani)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Lechner <david@lechnology.com>
---
 drivers/gpu/drm/drm_format_helper.c           | 123 ++++++++++--------
 drivers/gpu/drm/drm_mipi_dbi.c                |  19 ++-
 drivers/gpu/drm/gud/gud_pipe.c                |  30 +++--
 drivers/gpu/drm/solomon/ssd130x.c             |   8 +-
 .../gpu/drm/tests/drm_format_helper_test.c    |  72 ++++++----
 drivers/gpu/drm/tiny/cirrus.c                 |   3 +-
 drivers/gpu/drm/tiny/ili9225.c                |  10 +-
 drivers/gpu/drm/tiny/ofdrm.c                  |   2 +-
 drivers/gpu/drm/tiny/repaper.c                |   8 +-
 drivers/gpu/drm/tiny/simpledrm.c              |   2 +-
 drivers/gpu/drm/tiny/st7586.c                 |  19 +--
 include/drm/drm_format_helper.h               |  30 +++--
 include/drm/drm_mipi_dbi.h                    |   4 +-
 13 files changed, 193 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 37c499ae4fe4f..b9b3dadf7b5f8 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -220,29 +220,25 @@ static int drm_fb_xfrm(struct iosys_map *dst,
 		       const unsigned int *dst_pitch, const u8 *dst_pixsize,
 		       const struct iosys_map *src, const struct drm_framebuffer *fb,
 		       const struct drm_rect *clip, bool vaddr_cached_hint,
+		       struct drm_format_conv_state *state,
 		       void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels))
 {
 	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
 		0, 0, 0, 0
 	};
-	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
-	int ret;
 
 	if (!dst_pitch)
 		dst_pitch = default_dst_pitch;
 
 	/* TODO: handle src in I/O memory here */
 	if (dst[0].is_iomem)
-		ret = __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
-					 src[0].vaddr, fb, clip, vaddr_cached_hint, &fmtcnv_state,
-					 xfrm_line);
+		return __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], dst_pixsize[0],
+					  src[0].vaddr, fb, clip, vaddr_cached_hint, state,
+					  xfrm_line);
 	else
-		ret = __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
-				    src[0].vaddr, fb, clip, vaddr_cached_hint, &fmtcnv_state,
-				    xfrm_line);
-	drm_format_conv_state_release(&fmtcnv_state);
-
-	return ret;
+		return __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
+				     src[0].vaddr, fb, clip, vaddr_cached_hint, state,
+				     xfrm_line);
 }
 
 /**
@@ -328,6 +324,7 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
  * @cached: Source buffer is mapped cached (eg. not write-combined)
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and swaps per-pixel
  * bytes during the process. Destination and framebuffer formats must match. The
@@ -342,7 +339,8 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
  */
 void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 		 const struct iosys_map *src, const struct drm_framebuffer *fb,
-		 const struct drm_rect *clip, bool cached)
+		 const struct drm_rect *clip, bool cached,
+		 struct drm_format_conv_state *state)
 {
 	const struct drm_format_info *format = fb->format;
 	u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);
@@ -361,7 +359,7 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 		return;
 	}
 
-	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, swab_line);
+	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, state, swab_line);
 }
 EXPORT_SYMBOL(drm_fb_swab);
 
@@ -388,6 +386,7 @@ static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigne
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. Destination and framebuffer formats must match. The
@@ -402,13 +401,13 @@ static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigne
  */
 void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip)
+			       const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		1,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_rgb332_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
@@ -457,6 +456,7 @@ static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
  * @src: Array of XRGB8888 source buffer
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  * @swab: Swap bytes
  *
  * This function copies parts of a framebuffer to display memory and converts the
@@ -472,7 +472,8 @@ static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
  */
 void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip, bool swab)
+			       const struct drm_rect *clip, struct drm_format_conv_state *state,
+			       bool swab)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		2,
@@ -485,7 +486,7 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
 	else
 		xfrm_line = drm_fb_xrgb8888_to_rgb565_line;
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, xfrm_line);
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state, xfrm_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
 
@@ -514,6 +515,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
  * @src: Array of XRGB8888 source buffer
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts
  * the color format during the process. The parameters @dst, @dst_pitch and
@@ -529,13 +531,13 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
  */
 void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip)
+				 const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		2,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_xrgb1555_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
@@ -566,6 +568,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
  * @src: Array of XRGB8888 source buffer
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts
  * the color format during the process. The parameters @dst, @dst_pitch and
@@ -581,13 +584,13 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
  */
 void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip)
+				 const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		2,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_argb1555_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
@@ -618,6 +621,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
  * @src: Array of XRGB8888 source buffer
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts
  * the color format during the process. The parameters @dst, @dst_pitch and
@@ -633,13 +637,13 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
  */
 void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip)
+				 const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		2,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_rgba5551_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
@@ -668,6 +672,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. Destination and framebuffer formats must match. The
@@ -683,13 +688,13 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
  */
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip)
+			       const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		3,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_rgb888_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
@@ -716,6 +721,7 @@ static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
  * @src: Array of XRGB8888 source buffer
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. The parameters @dst, @dst_pitch and @src refer
@@ -731,13 +737,13 @@ static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
  */
 void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip)
+				 const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		4,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_argb8888_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb8888);
@@ -762,13 +768,14 @@ static void drm_fb_xrgb8888_to_abgr8888_line(void *dbuf, const void *sbuf, unsig
 static void drm_fb_xrgb8888_to_abgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 					const struct iosys_map *src,
 					const struct drm_framebuffer *fb,
-					const struct drm_rect *clip)
+					const struct drm_rect *clip,
+					struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		4,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_abgr8888_line);
 }
 
@@ -792,13 +799,14 @@ static void drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsig
 static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 					const struct iosys_map *src,
 					const struct drm_framebuffer *fb,
-					const struct drm_rect *clip)
+					const struct drm_rect *clip,
+					struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		4,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_xbgr8888_line);
 }
 
@@ -828,6 +836,7 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. Destination and framebuffer formats must match. The
@@ -843,13 +852,14 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
  */
 void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
-				    const struct drm_rect *clip)
+				    const struct drm_rect *clip,
+				    struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		4,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_xrgb2101010_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
@@ -881,6 +891,7 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts
  * the color format during the process. The parameters @dst, @dst_pitch and
@@ -896,13 +907,14 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
  */
 void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
-				    const struct drm_rect *clip)
+				    const struct drm_rect *clip,
+				    struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		4,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_argb2101010_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010);
@@ -932,6 +944,7 @@ static void drm_fb_xrgb8888_to_gray8_line(void *dbuf, const void *sbuf, unsigned
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. Destination and framebuffer formats must match. The
@@ -951,13 +964,13 @@ static void drm_fb_xrgb8888_to_gray8_line(void *dbuf, const void *sbuf, unsigned
  */
 void drm_fb_xrgb8888_to_gray8(struct iosys_map *dst, const unsigned int *dst_pitch,
 			      const struct iosys_map *src, const struct drm_framebuffer *fb,
-			      const struct drm_rect *clip)
+			      const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
 		1,
 	};
 
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
 		    drm_fb_xrgb8888_to_gray8_line);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
@@ -971,6 +984,7 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
  * @src:	The framebuffer memory to copy from
  * @fb:		The framebuffer to copy from
  * @clip:	Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory. If the
  * formats of the display and the framebuffer mismatch, the blit function
@@ -989,7 +1003,7 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray8);
  */
 int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t dst_format,
 		const struct iosys_map *src, const struct drm_framebuffer *fb,
-		const struct drm_rect *clip)
+		const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	uint32_t fb_format = fb->format->format;
 
@@ -997,44 +1011,44 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		drm_fb_memcpy(dst, dst_pitch, src, fb, clip);
 		return 0;
 	} else if (fb_format == (dst_format | DRM_FORMAT_BIG_ENDIAN)) {
-		drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+		drm_fb_swab(dst, dst_pitch, src, fb, clip, false, state);
 		return 0;
 	} else if (fb_format == (dst_format & ~DRM_FORMAT_BIG_ENDIAN)) {
-		drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+		drm_fb_swab(dst, dst_pitch, src, fb, clip, false, state);
 		return 0;
 	} else if (fb_format == DRM_FORMAT_XRGB8888) {
 		if (dst_format == DRM_FORMAT_RGB565) {
-			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
+			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, state, false);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_XRGB1555) {
-			drm_fb_xrgb8888_to_xrgb1555(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_xrgb1555(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_ARGB1555) {
-			drm_fb_xrgb8888_to_argb1555(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_argb1555(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_RGBA5551) {
-			drm_fb_xrgb8888_to_rgba5551(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_rgba5551(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_RGB888) {
-			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_ARGB8888) {
-			drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_XBGR8888) {
-			drm_fb_xrgb8888_to_xbgr8888(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_xbgr8888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_ABGR8888) {
-			drm_fb_xrgb8888_to_abgr8888(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_abgr8888(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_XRGB2101010) {
-			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_ARGB2101010) {
-			drm_fb_xrgb8888_to_argb2101010(dst, dst_pitch, src, fb, clip);
+			drm_fb_xrgb8888_to_argb2101010(dst, dst_pitch, src, fb, clip, state);
 			return 0;
 		} else if (dst_format == DRM_FORMAT_BGRX8888) {
-			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+			drm_fb_swab(dst, dst_pitch, src, fb, clip, false, state);
 			return 0;
 		}
 	}
@@ -1071,6 +1085,7 @@ static void drm_fb_gray8_to_mono_line(void *dbuf, const void *sbuf, unsigned int
  * @src: Array of XRGB8888 source buffers
  * @fb: DRM framebuffer
  * @clip: Clip rectangle area to copy
+ * @xfrm: Transform and conversion state
  *
  * This function copies parts of a framebuffer to display memory and converts the
  * color format during the process. Destination and framebuffer formats must match. The
@@ -1095,7 +1110,7 @@ static void drm_fb_gray8_to_mono_line(void *dbuf, const void *sbuf, unsigned int
  */
 void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitch,
 			     const struct iosys_map *src, const struct drm_framebuffer *fb,
-			     const struct drm_rect *clip)
+			     const struct drm_rect *clip, struct drm_format_conv_state *state)
 {
 	static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
 		0, 0, 0, 0
@@ -1135,7 +1150,7 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 	 * Allocate a buffer to be used for both copying from the cma
 	 * memory and to store the intermediate grayscale line pixels.
 	 */
-	src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL);
+	src32 = drm_format_conv_state_reserve(state, len_src32 + linepixels, GFP_KERNEL);
 	if (!src32)
 		return;
 
@@ -1149,8 +1164,6 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 		vaddr += fb->pitches[0];
 		mono += dst_pitch_0;
 	}
-
-	kfree(src32);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
 
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index e90f0bf895b33..daac649aabdbe 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -197,12 +197,14 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
  * @fb: The source framebuffer
  * @clip: Clipping rectangle of the area to be copied
  * @swap: When true, swap MSB/LSB of 16-bit values
+ * @fmtcnv_state: Format-conversion state
  *
  * Returns:
  * Zero on success, negative error code on failure.
  */
 int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
-		      struct drm_rect *clip, bool swap)
+		      struct drm_rect *clip, bool swap,
+		      struct drm_format_conv_state *fmtcnv_state)
 {
 	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
 	struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
@@ -215,12 +217,13 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *
 	switch (fb->format->format) {
 	case DRM_FORMAT_RGB565:
 		if (swap)
-			drm_fb_swab(&dst_map, NULL, src, fb, clip, !gem->import_attach);
+			drm_fb_swab(&dst_map, NULL, src, fb, clip, !gem->import_attach,
+				    fmtcnv_state);
 		else
 			drm_fb_memcpy(&dst_map, NULL, src, fb, clip);
 		break;
 	case DRM_FORMAT_XRGB8888:
-		drm_fb_xrgb8888_to_rgb565(&dst_map, NULL, src, fb, clip, swap);
+		drm_fb_xrgb8888_to_rgb565(&dst_map, NULL, src, fb, clip, fmtcnv_state, swap);
 		break;
 	default:
 		drm_err_once(fb->dev, "Format is not supported: %p4cc\n",
@@ -252,7 +255,7 @@ static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
 }
 
 static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
-			      struct drm_rect *rect)
+			      struct drm_rect *rect, struct drm_format_conv_state *fmtcnv_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	unsigned int height = rect->y2 - rect->y1;
@@ -270,7 +273,7 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 	if (!dbi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = dbidev->tx_buf;
-		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
+		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap, fmtcnv_state);
 		if (ret)
 			goto err_msg;
 	} else {
@@ -332,7 +335,8 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+		mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+				  &shadow_plane_state->fmtcnv_state);
 
 	drm_dev_exit(idx);
 }
@@ -368,7 +372,8 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
 	if (!drm_dev_enter(&dbidev->drm, &idx))
 		return;
 
-	mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+	mipi_dbi_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+			  &shadow_plane_state->fmtcnv_state);
 	backlight_enable(dbidev->backlight);
 
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index d2f199ea3c111..28171de84718e 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -51,7 +51,8 @@ static bool gud_is_big_endian(void)
 
 static size_t gud_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format,
 				   void *src, struct drm_framebuffer *fb,
-				   struct drm_rect *rect)
+				   struct drm_rect *rect,
+				   struct drm_format_conv_state *fmtcnv_state)
 {
 	unsigned int block_width = drm_format_info_block_width(format, 0);
 	unsigned int bits_per_pixel = 8 / block_width;
@@ -75,7 +76,7 @@ static size_t gud_xrgb8888_to_r124(u8 *dst, const struct drm_format_info *format
 
 	iosys_map_set_vaddr(&dst_map, buf);
 	iosys_map_set_vaddr(&vmap, src);
-	drm_fb_xrgb8888_to_gray8(&dst_map, NULL, &vmap, fb, rect);
+	drm_fb_xrgb8888_to_gray8(&dst_map, NULL, &vmap, fb, rect, fmtcnv_state);
 	pix8 = buf;
 
 	for (y = 0; y < height; y++) {
@@ -152,7 +153,8 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
 static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 			  const struct iosys_map *src, bool cached_reads,
 			  const struct drm_format_info *format, struct drm_rect *rect,
-			  struct gud_set_buffer_req *req)
+			  struct gud_set_buffer_req *req,
+			  struct drm_format_conv_state *fmtcnv_state)
 {
 	u8 compression = gdrm->compression;
 	struct iosys_map dst;
@@ -178,23 +180,23 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 	 */
 	if (format != fb->format) {
 		if (format->format == GUD_DRM_FORMAT_R1) {
-			len = gud_xrgb8888_to_r124(buf, format, vaddr, fb, rect);
+			len = gud_xrgb8888_to_r124(buf, format, vaddr, fb, rect, fmtcnv_state);
 			if (!len)
 				return -ENOMEM;
 		} else if (format->format == DRM_FORMAT_R8) {
-			drm_fb_xrgb8888_to_gray8(&dst, NULL, src, fb, rect);
+			drm_fb_xrgb8888_to_gray8(&dst, NULL, src, fb, rect, fmtcnv_state);
 		} else if (format->format == DRM_FORMAT_RGB332) {
-			drm_fb_xrgb8888_to_rgb332(&dst, NULL, src, fb, rect);
+			drm_fb_xrgb8888_to_rgb332(&dst, NULL, src, fb, rect, fmtcnv_state);
 		} else if (format->format == DRM_FORMAT_RGB565) {
-			drm_fb_xrgb8888_to_rgb565(&dst, NULL, src, fb, rect,
+			drm_fb_xrgb8888_to_rgb565(&dst, NULL, src, fb, rect, fmtcnv_state,
 						  gud_is_big_endian());
 		} else if (format->format == DRM_FORMAT_RGB888) {
-			drm_fb_xrgb8888_to_rgb888(&dst, NULL, src, fb, rect);
+			drm_fb_xrgb8888_to_rgb888(&dst, NULL, src, fb, rect, fmtcnv_state);
 		} else {
 			len = gud_xrgb8888_to_color(buf, format, vaddr, fb, rect);
 		}
 	} else if (gud_is_big_endian() && format->cpp[0] > 1) {
-		drm_fb_swab(&dst, NULL, src, fb, rect, cached_reads);
+		drm_fb_swab(&dst, NULL, src, fb, rect, cached_reads, fmtcnv_state);
 	} else if (compression && cached_reads && pitch == fb->pitches[0]) {
 		/* can compress directly from the framebuffer */
 		buf = vaddr + rect->y1 * pitch;
@@ -266,7 +268,8 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
 
 static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
 			  const struct iosys_map *src, bool cached_reads,
-			  const struct drm_format_info *format, struct drm_rect *rect)
+			  const struct drm_format_info *format, struct drm_rect *rect,
+			  struct drm_format_conv_state *fmtcnv_state)
 {
 	struct gud_set_buffer_req req;
 	size_t len, trlen;
@@ -274,7 +277,7 @@ static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
 
 	drm_dbg(&gdrm->drm, "Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
-	ret = gud_prep_flush(gdrm, fb, src, cached_reads, format, rect, &req);
+	ret = gud_prep_flush(gdrm, fb, src, cached_reads, format, rect, &req, fmtcnv_state);
 	if (ret)
 		return ret;
 
@@ -318,6 +321,7 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 			     const struct iosys_map *src, bool cached_reads,
 			     struct drm_rect *damage)
 {
+	struct drm_format_conv_state fmtcnv_state = DRM_FORMAT_CONV_STATE_INIT;
 	const struct drm_format_info *format;
 	unsigned int i, lines;
 	size_t pitch;
@@ -340,7 +344,7 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 		rect.y1 += i * lines;
 		rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
 
-		ret = gud_flush_rect(gdrm, fb, src, cached_reads, format, &rect);
+		ret = gud_flush_rect(gdrm, fb, src, cached_reads, format, &rect, &fmtcnv_state);
 		if (ret) {
 			if (ret != -ENODEV && ret != -ECONNRESET &&
 			    ret != -ESHUTDOWN && ret != -EPROTO)
@@ -350,6 +354,8 @@ static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer *fb
 			break;
 		}
 	}
+
+	drm_format_conv_state_release(&fmtcnv_state);
 }
 
 void gud_flush_work(struct work_struct *work)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 6dcf3e0411136..3dd8e8a444b6f 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -602,7 +602,8 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
 				const struct iosys_map *vmap,
 				struct drm_rect *rect,
-				u8 *buf, u8 *data_array)
+				u8 *buf, u8 *data_array,
+				struct drm_format_conv_state *fmtcnv_state)
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
 	unsigned int page_height = ssd130x->device_info->page_height;
@@ -621,7 +622,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
 		return ret;
 
 	iosys_map_set_vaddr(&dst, buf);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
@@ -695,7 +696,8 @@ static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
 
 		ssd130x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
 				     ssd130x_plane_state->buffer,
-				     ssd130x_crtc_state->data_array);
+				     ssd130x_crtc_state->data_array,
+				     &shadow_plane_state->fmtcnv_state);
 	}
 
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index f6408e56f7861..08992636ec05f 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -20,6 +20,10 @@
 
 #define TEST_USE_DEFAULT_PITCH 0
 
+static unsigned char fmtcnv_state_mem[PAGE_SIZE];
+static struct drm_format_conv_state fmtcnv_state =
+	DRM_FORMAT_CONV_STATE_INIT_PREALLOCATED(fmtcnv_state_mem, sizeof(fmtcnv_state_mem));
+
 struct convert_to_gray8_result {
 	unsigned int dst_pitch;
 	const u8 expected[TEST_BUF_SIZE];
@@ -630,8 +634,7 @@ static void drm_test_fb_xrgb8888_to_gray8(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_gray8(&dst, dst_pitch, &src, &fb, &params->clip);
-
+	drm_fb_xrgb8888_to_gray8(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 }
 
@@ -664,7 +667,7 @@ static void drm_test_fb_xrgb8888_to_rgb332(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_rgb332(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_rgb332(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 }
 
@@ -697,12 +700,14 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_rgb565(&dst, dst_pitch, &src, &fb, &params->clip, false);
+	drm_fb_xrgb8888_to_rgb565(&dst, dst_pitch, &src, &fb, &params->clip,
+				  &fmtcnv_state, false);
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
 	buf = dst.vaddr; /* restore original value of buf */
-	drm_fb_xrgb8888_to_rgb565(&dst, &result->dst_pitch, &src, &fb, &params->clip, true);
+	drm_fb_xrgb8888_to_rgb565(&dst, &result->dst_pitch, &src, &fb, &params->clip,
+				  &fmtcnv_state, true);
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected_swab, dst_size);
 
@@ -711,7 +716,8 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGB565, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGB565, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 
@@ -748,7 +754,7 @@ static void drm_test_fb_xrgb8888_to_xrgb1555(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_xrgb1555(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_xrgb1555(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -757,7 +763,8 @@ static void drm_test_fb_xrgb8888_to_xrgb1555(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB1555, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB1555, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 
@@ -794,7 +801,7 @@ static void drm_test_fb_xrgb8888_to_argb1555(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_argb1555(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_argb1555(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -803,7 +810,8 @@ static void drm_test_fb_xrgb8888_to_argb1555(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ARGB1555, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ARGB1555, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 
@@ -840,7 +848,7 @@ static void drm_test_fb_xrgb8888_to_rgba5551(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_rgba5551(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_rgba5551(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -849,7 +857,8 @@ static void drm_test_fb_xrgb8888_to_rgba5551(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGBA5551, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGBA5551, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 
@@ -890,7 +899,7 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_rgb888(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_rgb888(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
 	buf = dst.vaddr; /* restore original value of buf */
@@ -898,7 +907,8 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGB888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_RGB888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
@@ -933,7 +943,7 @@ static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_argb8888(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_argb8888(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -942,7 +952,8 @@ static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ARGB8888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ARGB8888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
@@ -979,7 +990,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_xrgb2101010(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_xrgb2101010(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le32buf_to_cpu(test, buf, dst_size / sizeof(u32));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -989,7 +1000,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	int blit_result = 0;
 
 	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB2101010, &src, &fb,
-				  &params->clip);
+				  &params->clip, &fmtcnv_state);
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
@@ -1024,7 +1035,7 @@ static void drm_test_fb_xrgb8888_to_argb2101010(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_argb2101010(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_argb2101010(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -1034,7 +1045,7 @@ static void drm_test_fb_xrgb8888_to_argb2101010(struct kunit *test)
 	int blit_result = 0;
 
 	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ARGB2101010, &src, &fb,
-				  &params->clip);
+				  &params->clip, &fmtcnv_state);
 
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
@@ -1071,7 +1082,7 @@ static void drm_test_fb_xrgb8888_to_mono(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_xrgb8888_to_mono(&dst, dst_pitch, &src, &fb, &params->clip);
+	drm_fb_xrgb8888_to_mono(&dst, dst_pitch, &src, &fb, &params->clip, &fmtcnv_state);
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 }
 
@@ -1104,7 +1115,7 @@ static void drm_test_fb_swab(struct kunit *test)
 	const unsigned int *dst_pitch = (result->dst_pitch == TEST_USE_DEFAULT_PITCH) ?
 		NULL : &result->dst_pitch;
 
-	drm_fb_swab(&dst, dst_pitch, &src, &fb, &params->clip, false);
+	drm_fb_swab(&dst, dst_pitch, &src, &fb, &params->clip, false, &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 	KUNIT_EXPECT_MEMEQ(test, buf, result->expected, dst_size);
 
@@ -1114,7 +1125,7 @@ static void drm_test_fb_swab(struct kunit *test)
 	int blit_result;
 
 	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB8888 | DRM_FORMAT_BIG_ENDIAN,
-				  &src, &fb, &params->clip);
+				  &src, &fb, &params->clip, &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
@@ -1123,7 +1134,8 @@ static void drm_test_fb_swab(struct kunit *test)
 	buf = dst.vaddr;
 	memset(buf, 0, dst_size);
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_BGRX8888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_BGRX8888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
@@ -1137,7 +1149,8 @@ static void drm_test_fb_swab(struct kunit *test)
 	mock_format.format |= DRM_FORMAT_BIG_ENDIAN;
 	fb.format = &mock_format;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB8888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XRGB8888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
@@ -1175,7 +1188,8 @@ static void drm_test_fb_xrgb8888_to_abgr8888(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ABGR8888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_ABGR8888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
@@ -1214,7 +1228,8 @@ static void drm_test_fb_xrgb8888_to_xbgr8888(struct kunit *test)
 
 	int blit_result = 0;
 
-	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XBGR8888, &src, &fb, &params->clip);
+	blit_result = drm_fb_blit(&dst, dst_pitch, DRM_FORMAT_XBGR8888, &src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 
@@ -1817,7 +1832,8 @@ static void drm_test_fb_memcpy(struct kunit *test)
 
 	int blit_result;
 
-	blit_result = drm_fb_blit(dst, dst_pitches, params->format, src, &fb, &params->clip);
+	blit_result = drm_fb_blit(dst, dst_pitches, params->format, src, &fb, &params->clip,
+				  &fmtcnv_state);
 
 	KUNIT_EXPECT_FALSE(test, blit_result);
 	for (size_t i = 0; i < fb.format->num_planes; i++) {
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index c5c34cd2edc1f..4e3a152f897ac 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -411,7 +411,8 @@ static void cirrus_primary_plane_helper_atomic_update(struct drm_plane *plane,
 		unsigned int offset = drm_fb_clip_offset(pitch, format, &damage);
 		struct iosys_map dst = IOSYS_MAP_INIT_OFFSET(&vaddr, offset);
 
-		drm_fb_blit(&dst, &pitch, format->format, shadow_plane_state->data, fb, &damage);
+		drm_fb_blit(&dst, &pitch, format->format, shadow_plane_state->data, fb,
+			    &damage, &shadow_plane_state->fmtcnv_state);
 	}
 
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index 4ceb68ffac4be..dd8b0a181be94 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -78,7 +78,7 @@ static inline int ili9225_command(struct mipi_dbi *dbi, u8 cmd, u16 data)
 }
 
 static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
-			     struct drm_rect *rect)
+			     struct drm_rect *rect, struct drm_format_conv_state *fmtcnv_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	unsigned int height = rect->y2 - rect->y1;
@@ -98,7 +98,7 @@ static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 	if (!dbi->dc || !full || swap ||
 	    fb->format->format == DRM_FORMAT_XRGB8888) {
 		tr = dbidev->tx_buf;
-		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
+		ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap, fmtcnv_state);
 		if (ret)
 			goto err_msg;
 	} else {
@@ -171,7 +171,8 @@ static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+		ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+				 &shadow_plane_state->fmtcnv_state);
 
 	drm_dev_exit(idx);
 }
@@ -281,7 +282,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
 
-	ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+	ili9225_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+			 &shadow_plane_state->fmtcnv_state);
 
 out_exit:
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 2d999a0facdee..404c83032cecc 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -817,7 +817,7 @@ static void ofdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 
 		iosys_map_incr(&dst, drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip));
 		drm_fb_blit(&dst, &dst_pitch, dst_format->format, shadow_plane_state->data, fb,
-			    &damage);
+			    &damage, &shadow_plane_state->fmtcnv_state);
 	}
 
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 73dd4f4289c20..826fb20dbbf0d 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -509,7 +509,8 @@ static void repaper_get_temperature(struct repaper_epd *epd)
 	epd->factored_stage_time = epd->stage_time * factor10x / 10;
 }
 
-static int repaper_fb_dirty(struct drm_framebuffer *fb)
+static int repaper_fb_dirty(struct drm_framebuffer *fb,
+			    struct drm_format_conv_state *fmtcnv_state)
 {
 	struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
 	struct repaper_epd *epd = drm_to_epd(fb->dev);
@@ -545,7 +546,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
 
 	iosys_map_set_vaddr(&dst, buf);
 	iosys_map_set_vaddr(&vmap, dma_obj->vaddr);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, &vmap, fb, &clip);
+	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, &vmap, fb, &clip, fmtcnv_state);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
@@ -830,13 +831,14 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
 				struct drm_plane_state *old_state)
 {
 	struct drm_plane_state *state = pipe->plane.state;
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
 	struct drm_rect rect;
 
 	if (!pipe->crtc.state->active)
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		repaper_fb_dirty(state->fb);
+		repaper_fb_dirty(state->fb, &shadow_plane_state->fmtcnv_state);
 }
 
 static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 9c597461d1e2a..6788f465995b3 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -609,7 +609,7 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 
 		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
 		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data,
-			    fb, &damage);
+			    fb, &damage, &shadow_plane_state->fmtcnv_state);
 	}
 
 	drm_dev_exit(idx);
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index 3cf4eec16a813..7336fa1ddaed1 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -64,7 +64,8 @@ static const u8 st7586_lookup[] = { 0x7, 0x4, 0x2, 0x0 };
 
 static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 				       struct drm_framebuffer *fb,
-				       struct drm_rect *clip)
+				       struct drm_rect *clip,
+				       struct drm_format_conv_state *fmtcnv_state)
 {
 	size_t len = (clip->x2 - clip->x1) * (clip->y2 - clip->y1);
 	unsigned int x, y;
@@ -77,7 +78,7 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 
 	iosys_map_set_vaddr(&dst_map, buf);
 	iosys_map_set_vaddr(&vmap, vaddr);
-	drm_fb_xrgb8888_to_gray8(&dst_map, NULL, &vmap, fb, clip);
+	drm_fb_xrgb8888_to_gray8(&dst_map, NULL, &vmap, fb, clip, fmtcnv_state);
 	src = buf;
 
 	for (y = clip->y1; y < clip->y2; y++) {
@@ -93,7 +94,7 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
 }
 
 static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
-			   struct drm_rect *clip)
+			   struct drm_rect *clip, struct drm_format_conv_state *fmtcnv_state)
 {
 	int ret;
 
@@ -101,7 +102,7 @@ static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuf
 	if (ret)
 		return ret;
 
-	st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip);
+	st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip, fmtcnv_state);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
@@ -109,7 +110,7 @@ static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuf
 }
 
 static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
-			    struct drm_rect *rect)
+			    struct drm_rect *rect, struct drm_format_conv_state *fmtcnv_state)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
 	struct mipi_dbi *dbi = &dbidev->dbi;
@@ -121,7 +122,7 @@ static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
 
 	DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
 
-	ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
+	ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect, fmtcnv_state);
 	if (ret)
 		goto err_msg;
 
@@ -160,7 +161,8 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
 		return;
 
 	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
-		st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+		st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+				&shadow_plane_state->fmtcnv_state);
 
 	drm_dev_exit(idx);
 }
@@ -238,7 +240,8 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
 
 	msleep(100);
 
-	st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect);
+	st7586_fb_dirty(&shadow_plane_state->data[0], fb, &rect,
+			&shadow_plane_state->fmtcnv_state);
 
 	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
 out_exit:
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 724a9baf7315d..f13b34e0b752b 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -74,45 +74,49 @@ void drm_fb_memcpy(struct iosys_map *dst, const unsigned int *dst_pitch,
 		   const struct drm_rect *clip);
 void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 		 const struct iosys_map *src, const struct drm_framebuffer *fb,
-		 const struct drm_rect *clip, bool cached);
+		 const struct drm_rect *clip, bool cached,
+		 struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip);
+			       const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip, bool swab);
+			       const struct drm_rect *clip, struct drm_format_conv_state *state,
+			       bool swab);
 void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip);
+				 const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip);
+				 const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip);
+				 const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
-			       const struct drm_rect *clip);
+			       const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
-				 const struct drm_rect *clip);
+				 const struct drm_rect *clip, struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
-				    const struct drm_rect *clip);
+				    const struct drm_rect *clip,
+				    struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
-				    const struct drm_rect *clip);
+				    const struct drm_rect *clip,
+				    struct drm_format_conv_state *state);
 void drm_fb_xrgb8888_to_gray8(struct iosys_map *dst, const unsigned int *dst_pitch,
 			      const struct iosys_map *src, const struct drm_framebuffer *fb,
-			      const struct drm_rect *clip);
+			      const struct drm_rect *clip, struct drm_format_conv_state *state);
 
 int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t dst_format,
 		const struct iosys_map *src, const struct drm_framebuffer *fb,
-		const struct drm_rect *rect);
+		const struct drm_rect *clip, struct drm_format_conv_state *state);
 
 void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitch,
 			     const struct iosys_map *src, const struct drm_framebuffer *fb,
-			     const struct drm_rect *clip);
+			     const struct drm_rect *clip, struct drm_format_conv_state *state);
 
 size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 				const u32 *native_fourccs, size_t native_nfourccs,
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 816f196b3d4c5..e8e0f8d39f3a6 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -12,6 +12,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_simple_kms_helper.h>
 
+struct drm_format_conv_state;
 struct drm_rect;
 struct gpio_desc;
 struct iosys_map;
@@ -192,7 +193,8 @@ int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
 int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
 			      size_t len);
 int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
-		      struct drm_rect *clip, bool swap);
+		      struct drm_rect *clip, bool swap,
+		      struct drm_format_conv_state *fmtcnv_state);
 
 /**
  * mipi_dbi_command - MIPI DCS command with optional parameter(s)
-- 
2.42.0

Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 57d3b83a83c5527325efb5bcaf594da09fe4a41b]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-format-helper-Cache-buffers-with-struct-drm_format_conv_state/20231005-170643
base:   57d3b83a83c5527325efb5bcaf594da09fe4a41b
patch link:    https://lore.kernel.org/r/20231005090520.16511-4-tzimmermann%40suse.de
patch subject: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231005/202310051822.r1l9UoUq-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231005/202310051822.r1l9UoUq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310051822.r1l9UoUq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_format_helper.c:344: warning: Function parameter or member 'state' not described in 'drm_fb_swab'
>> drivers/gpu/drm/drm_format_helper.c:344: warning: Excess function parameter 'xfrm' description in 'drm_fb_swab'
>> drivers/gpu/drm/drm_format_helper.c:405: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb332'
>> drivers/gpu/drm/drm_format_helper.c:405: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb332'
>> drivers/gpu/drm/drm_format_helper.c:477: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb565'
>> drivers/gpu/drm/drm_format_helper.c:477: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb565'
>> drivers/gpu/drm/drm_format_helper.c:535: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_xrgb1555'
>> drivers/gpu/drm/drm_format_helper.c:535: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_xrgb1555'
>> drivers/gpu/drm/drm_format_helper.c:588: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb1555'
>> drivers/gpu/drm/drm_format_helper.c:588: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb1555'
>> drivers/gpu/drm/drm_format_helper.c:641: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgba5551'
>> drivers/gpu/drm/drm_format_helper.c:641: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgba5551'
>> drivers/gpu/drm/drm_format_helper.c:692: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb888'
>> drivers/gpu/drm/drm_format_helper.c:692: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb888'
>> drivers/gpu/drm/drm_format_helper.c:741: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb8888'
>> drivers/gpu/drm/drm_format_helper.c:741: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb8888'
>> drivers/gpu/drm/drm_format_helper.c:857: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_xrgb2101010'
>> drivers/gpu/drm/drm_format_helper.c:857: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_xrgb2101010'
>> drivers/gpu/drm/drm_format_helper.c:912: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb2101010'
>> drivers/gpu/drm/drm_format_helper.c:912: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb2101010'
   drivers/gpu/drm/drm_format_helper.c:968: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_gray8'
   drivers/gpu/drm/drm_format_helper.c:968: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_gray8'
   drivers/gpu/drm/drm_format_helper.c:1007: warning: Function parameter or member 'state' not described in 'drm_fb_blit'
   drivers/gpu/drm/drm_format_helper.c:1007: warning: Excess function parameter 'xfrm' description in 'drm_fb_blit'
   drivers/gpu/drm/drm_format_helper.c:1114: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_mono'
   drivers/gpu/drm/drm_format_helper.c:1114: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_mono'


vim +344 drivers/gpu/drm/drm_format_helper.c

41fd6f0a6dd62d Thomas Zimmermann  2022-04-27  317  
7415287e1f3675 Gerd Hoffmann      2019-04-05  318  /**
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  319   * drm_fb_swab - Swap bytes into clip buffer
ce582859ca7b33 Thomas Zimmermann  2022-08-08  320   * @dst: Array of destination buffers
ce582859ca7b33 Thomas Zimmermann  2022-08-08  321   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
ce582859ca7b33 Thomas Zimmermann  2022-08-08  322   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  323   * @src: Array of source buffers
7415287e1f3675 Gerd Hoffmann      2019-04-05  324   * @fb: DRM framebuffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  325   * @clip: Clip rectangle area to copy
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  326   * @cached: Source buffer is mapped cached (eg. not write-combined)
05864d296c0bfc Thomas Zimmermann  2023-10-05  327   * @xfrm: Transform and conversion state
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  328   *
ce582859ca7b33 Thomas Zimmermann  2022-08-08  329   * This function copies parts of a framebuffer to display memory and swaps per-pixel
ce582859ca7b33 Thomas Zimmermann  2022-08-08  330   * bytes during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  331   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
ce582859ca7b33 Thomas Zimmermann  2022-08-08  332   * least as many entries as there are planes in @fb's format. Each entry stores the
ce582859ca7b33 Thomas Zimmermann  2022-08-08  333   * value for the format's respective color plane at the same index. If @cached is
ce582859ca7b33 Thomas Zimmermann  2022-08-08  334   * false a temporary buffer is used to cache one pixel line at a time to speed up
ce582859ca7b33 Thomas Zimmermann  2022-08-08  335   * slow uncached reads.
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  336   *
ce582859ca7b33 Thomas Zimmermann  2022-08-08  337   * This function does not apply clipping on @dst (i.e. the destination is at the
ce582859ca7b33 Thomas Zimmermann  2022-08-08  338   * top-left corner).
7415287e1f3675 Gerd Hoffmann      2019-04-05  339   */
ce582859ca7b33 Thomas Zimmermann  2022-08-08  340  void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  341  		 const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  342  		 const struct drm_rect *clip, bool cached,
05864d296c0bfc Thomas Zimmermann  2023-10-05  343  		 struct drm_format_conv_state *state)
7415287e1f3675 Gerd Hoffmann      2019-04-05 @344  {
ce582859ca7b33 Thomas Zimmermann  2022-08-08  345  	const struct drm_format_info *format = fb->format;
ce582859ca7b33 Thomas Zimmermann  2022-08-08  346  	u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);
ce582859ca7b33 Thomas Zimmermann  2022-08-08  347  	void (*swab_line)(void *dbuf, const void *sbuf, unsigned int npixels);
3e3543c8a19cad Thomas Zimmermann  2021-11-10  348  
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  349  	switch (cpp) {
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  350  	case 4:
ce582859ca7b33 Thomas Zimmermann  2022-08-08  351  		swab_line = drm_fb_swab32_line;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  352  		break;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  353  	case 2:
ce582859ca7b33 Thomas Zimmermann  2022-08-08  354  		swab_line = drm_fb_swab16_line;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  355  		break;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  356  	default:
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  357  		drm_warn_once(fb->dev, "Format %p4cc has unsupported pixel size.\n",
ce582859ca7b33 Thomas Zimmermann  2022-08-08  358  			      &format->format);
ce582859ca7b33 Thomas Zimmermann  2022-08-08  359  		return;
f241b064426943 Thomas Zimmermann  2022-08-08  360  	}
ce582859ca7b33 Thomas Zimmermann  2022-08-08  361  
05864d296c0bfc Thomas Zimmermann  2023-10-05  362  	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, state, swab_line);
7415287e1f3675 Gerd Hoffmann      2019-04-05  363  }
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  364  EXPORT_SYMBOL(drm_fb_swab);
7415287e1f3675 Gerd Hoffmann      2019-04-05  365  
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  366  static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  367  {
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  368  	u8 *dbuf8 = dbuf;
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  369  	const __le32 *sbuf32 = sbuf;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  370  	unsigned int x;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  371  	u32 pix;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  372  
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  373  	for (x = 0; x < pixels; x++) {
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  374  		pix = le32_to_cpu(sbuf32[x]);
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  375  		dbuf8[x] = ((pix & 0x00e00000) >> 16) |
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  376  			   ((pix & 0x0000e000) >> 11) |
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  377  			   ((pix & 0x000000c0) >> 6);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  378  	}
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  379  }
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  380  
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  381  /**
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  382   * drm_fb_xrgb8888_to_rgb332 - Convert XRGB8888 to RGB332 clip buffer
e13140a0628314 Thomas Zimmermann  2022-08-08  383   * @dst: Array of RGB332 destination buffers
e13140a0628314 Thomas Zimmermann  2022-08-08  384   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
e13140a0628314 Thomas Zimmermann  2022-08-08  385   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  386   * @src: Array of XRGB8888 source buffers
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  387   * @fb: DRM framebuffer
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  388   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  389   * @xfrm: Transform and conversion state
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  390   *
e13140a0628314 Thomas Zimmermann  2022-08-08  391   * This function copies parts of a framebuffer to display memory and converts the
e13140a0628314 Thomas Zimmermann  2022-08-08  392   * color format during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  393   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
e13140a0628314 Thomas Zimmermann  2022-08-08  394   * least as many entries as there are planes in @fb's format. Each entry stores the
e13140a0628314 Thomas Zimmermann  2022-08-08  395   * value for the format's respective color plane at the same index.
e13140a0628314 Thomas Zimmermann  2022-08-08  396   *
e13140a0628314 Thomas Zimmermann  2022-08-08  397   * This function does not apply clipping on @dst (i.e. the destination is at the
e13140a0628314 Thomas Zimmermann  2022-08-08  398   * top-left corner).
e13140a0628314 Thomas Zimmermann  2022-08-08  399   *
e13140a0628314 Thomas Zimmermann  2022-08-08  400   * Drivers can use this function for RGB332 devices that don't support XRGB8888 natively.
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  401   */
e13140a0628314 Thomas Zimmermann  2022-08-08  402  void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  403  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  404  			       const struct drm_rect *clip, struct drm_format_conv_state *state)
cee0b7cbf1c02f Noralf Trønnes     2021-09-29 @405  {
f241b064426943 Thomas Zimmermann  2022-08-08  406  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
f241b064426943 Thomas Zimmermann  2022-08-08  407  		1,
e13140a0628314 Thomas Zimmermann  2022-08-08  408  	};
e13140a0628314 Thomas Zimmermann  2022-08-08  409  
05864d296c0bfc Thomas Zimmermann  2023-10-05  410  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
f241b064426943 Thomas Zimmermann  2022-08-08  411  		    drm_fb_xrgb8888_to_rgb332_line);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  412  }
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  413  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  414  
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  415  static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
bcc4442008aca0 Gerd Hoffmann      2019-04-05  416  {
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  417  	__le16 *dbuf16 = dbuf;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  418  	const __le32 *sbuf32 = sbuf;
d653bd39482377 Gerd Hoffmann      2019-04-10  419  	unsigned int x;
d653bd39482377 Gerd Hoffmann      2019-04-10  420  	u16 val16;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  421  	u32 pix;
bcc4442008aca0 Gerd Hoffmann      2019-04-05  422  
d653bd39482377 Gerd Hoffmann      2019-04-10  423  	for (x = 0; x < pixels; x++) {
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  424  		pix = le32_to_cpu(sbuf32[x]);
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  425  		val16 = ((pix & 0x00F80000) >> 8) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  426  			((pix & 0x0000FC00) >> 5) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  427  			((pix & 0x000000F8) >> 3);
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  428  		dbuf16[x] = cpu_to_le16(val16);
bcc4442008aca0 Gerd Hoffmann      2019-04-05  429  	}
bcc4442008aca0 Gerd Hoffmann      2019-04-05  430  }
bcc4442008aca0 Gerd Hoffmann      2019-04-05  431  
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  432  /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  433  static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
69add027fd2bac Thomas Zimmermann  2022-04-27  434  						unsigned int pixels)
69add027fd2bac Thomas Zimmermann  2022-04-27  435  {
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  436  	__le16 *dbuf16 = dbuf;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  437  	const __le32 *sbuf32 = sbuf;
69add027fd2bac Thomas Zimmermann  2022-04-27  438  	unsigned int x;
69add027fd2bac Thomas Zimmermann  2022-04-27  439  	u16 val16;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  440  	u32 pix;
69add027fd2bac Thomas Zimmermann  2022-04-27  441  
69add027fd2bac Thomas Zimmermann  2022-04-27  442  	for (x = 0; x < pixels; x++) {
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  443  		pix = le32_to_cpu(sbuf32[x]);
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  444  		val16 = ((pix & 0x00F80000) >> 8) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  445  			((pix & 0x0000FC00) >> 5) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  446  			((pix & 0x000000F8) >> 3);
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  447  		dbuf16[x] = cpu_to_le16(swab16(val16));
69add027fd2bac Thomas Zimmermann  2022-04-27  448  	}
69add027fd2bac Thomas Zimmermann  2022-04-27  449  }
69add027fd2bac Thomas Zimmermann  2022-04-27  450  
7415287e1f3675 Gerd Hoffmann      2019-04-05  451  /**
7415287e1f3675 Gerd Hoffmann      2019-04-05  452   * drm_fb_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer
ab298c29d414a5 Thomas Zimmermann  2022-08-08  453   * @dst: Array of RGB565 destination buffers
ab298c29d414a5 Thomas Zimmermann  2022-08-08  454   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
ab298c29d414a5 Thomas Zimmermann  2022-08-08  455   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  456   * @src: Array of XRGB8888 source buffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  457   * @fb: DRM framebuffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  458   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  459   * @xfrm: Transform and conversion state
d653bd39482377 Gerd Hoffmann      2019-04-10  460   * @swab: Swap bytes
7415287e1f3675 Gerd Hoffmann      2019-04-05  461   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  462   * This function copies parts of a framebuffer to display memory and converts the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  463   * color format during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  464   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
ab298c29d414a5 Thomas Zimmermann  2022-08-08  465   * least as many entries as there are planes in @fb's format. Each entry stores the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  466   * value for the format's respective color plane at the same index.
bcc4442008aca0 Gerd Hoffmann      2019-04-05  467   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  468   * This function does not apply clipping on @dst (i.e. the destination is at the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  469   * top-left corner).
ab298c29d414a5 Thomas Zimmermann  2022-08-08  470   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  471   * Drivers can use this function for RGB565 devices that don't support XRGB8888 natively.
bcc4442008aca0 Gerd Hoffmann      2019-04-05  472   */
ab298c29d414a5 Thomas Zimmermann  2022-08-08  473  void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  474  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  475  			       const struct drm_rect *clip, struct drm_format_conv_state *state,
05864d296c0bfc Thomas Zimmermann  2023-10-05  476  			       bool swab)
bcc4442008aca0 Gerd Hoffmann      2019-04-05 @477  {
f241b064426943 Thomas Zimmermann  2022-08-08  478  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
f241b064426943 Thomas Zimmermann  2022-08-08  479  		2,
ab298c29d414a5 Thomas Zimmermann  2022-08-08  480  	};
f241b064426943 Thomas Zimmermann  2022-08-08  481  
ab298c29d414a5 Thomas Zimmermann  2022-08-08  482  	void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels);
ab298c29d414a5 Thomas Zimmermann  2022-08-08  483  
69add027fd2bac Thomas Zimmermann  2022-04-27  484  	if (swab)
ab298c29d414a5 Thomas Zimmermann  2022-08-08  485  		xfrm_line = drm_fb_xrgb8888_to_rgb565_swab_line;
ab298c29d414a5 Thomas Zimmermann  2022-08-08  486  	else
ab298c29d414a5 Thomas Zimmermann  2022-08-08  487  		xfrm_line = drm_fb_xrgb8888_to_rgb565_line;
ab298c29d414a5 Thomas Zimmermann  2022-08-08  488  
05864d296c0bfc Thomas Zimmermann  2023-10-05  489  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state, xfrm_line);
7415287e1f3675 Gerd Hoffmann      2019-04-05  490  }
ab298c29d414a5 Thomas Zimmermann  2022-08-08  491  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
7415287e1f3675 Gerd Hoffmann      2019-04-05  492  
10cd592e639edc Thomas Zimmermann  2023-01-02  493  static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann  2023-01-02  494  {
10cd592e639edc Thomas Zimmermann  2023-01-02  495  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann  2023-01-02  496  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann  2023-01-02  497  	unsigned int x;
10cd592e639edc Thomas Zimmermann  2023-01-02  498  	u16 val16;
10cd592e639edc Thomas Zimmermann  2023-01-02  499  	u32 pix;
10cd592e639edc Thomas Zimmermann  2023-01-02  500  
10cd592e639edc Thomas Zimmermann  2023-01-02  501  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann  2023-01-02  502  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann  2023-01-02  503  		val16 = ((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann  2023-01-02  504  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann  2023-01-02  505  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann  2023-01-02  506  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann  2023-01-02  507  	}
10cd592e639edc Thomas Zimmermann  2023-01-02  508  }
10cd592e639edc Thomas Zimmermann  2023-01-02  509  
10cd592e639edc Thomas Zimmermann  2023-01-02  510  /**
10cd592e639edc Thomas Zimmermann  2023-01-02  511   * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
10cd592e639edc Thomas Zimmermann  2023-01-02  512   * @dst: Array of XRGB1555 destination buffers
10cd592e639edc Thomas Zimmermann  2023-01-02  513   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
10cd592e639edc Thomas Zimmermann  2023-01-02  514   *             within @dst; can be NULL if scanlines are stored next to each other.
10cd592e639edc Thomas Zimmermann  2023-01-02  515   * @src: Array of XRGB8888 source buffer
10cd592e639edc Thomas Zimmermann  2023-01-02  516   * @fb: DRM framebuffer
10cd592e639edc Thomas Zimmermann  2023-01-02  517   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  518   * @xfrm: Transform and conversion state
10cd592e639edc Thomas Zimmermann  2023-01-02  519   *
10cd592e639edc Thomas Zimmermann  2023-01-02  520   * This function copies parts of a framebuffer to display memory and converts
10cd592e639edc Thomas Zimmermann  2023-01-02  521   * the color format during the process. The parameters @dst, @dst_pitch and
10cd592e639edc Thomas Zimmermann  2023-01-02  522   * @src refer to arrays. Each array must have at least as many entries as
10cd592e639edc Thomas Zimmermann  2023-01-02  523   * there are planes in @fb's format. Each entry stores the value for the
10cd592e639edc Thomas Zimmermann  2023-01-02  524   * format's respective color plane at the same index.
10cd592e639edc Thomas Zimmermann  2023-01-02  525   *
10cd592e639edc Thomas Zimmermann  2023-01-02  526   * This function does not apply clipping on @dst (i.e. the destination is at the
10cd592e639edc Thomas Zimmermann  2023-01-02  527   * top-left corner).
10cd592e639edc Thomas Zimmermann  2023-01-02  528   *
10cd592e639edc Thomas Zimmermann  2023-01-02  529   * Drivers can use this function for XRGB1555 devices that don't support
10cd592e639edc Thomas Zimmermann  2023-01-02  530   * XRGB8888 natively.
10cd592e639edc Thomas Zimmermann  2023-01-02  531   */
10cd592e639edc Thomas Zimmermann  2023-01-02  532  void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
10cd592e639edc Thomas Zimmermann  2023-01-02  533  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  534  				 const struct drm_rect *clip, struct drm_format_conv_state *state)
10cd592e639edc Thomas Zimmermann  2023-01-02 @535  {
10cd592e639edc Thomas Zimmermann  2023-01-02  536  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
10cd592e639edc Thomas Zimmermann  2023-01-02  537  		2,
10cd592e639edc Thomas Zimmermann  2023-01-02  538  	};
10cd592e639edc Thomas Zimmermann  2023-01-02  539  
05864d296c0bfc Thomas Zimmermann  2023-10-05  540  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
10cd592e639edc Thomas Zimmermann  2023-01-02  541  		    drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann  2023-01-02  542  }
10cd592e639edc Thomas Zimmermann  2023-01-02  543  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
10cd592e639edc Thomas Zimmermann  2023-01-02  544  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 57d3b83a83c5527325efb5bcaf594da09fe4a41b]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-format-helper-Cache-buffers-with-struct-drm_format_conv_state/20231005-170643
base:   57d3b83a83c5527325efb5bcaf594da09fe4a41b
patch link:    https://lore.kernel.org/r/20231005090520.16511-4-tzimmermann%40suse.de
patch subject: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231005/202310051822.r1l9UoUq-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231005/202310051822.r1l9UoUq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310051822.r1l9UoUq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_format_helper.c:344: warning: Function parameter or member 'state' not described in 'drm_fb_swab'
>> drivers/gpu/drm/drm_format_helper.c:344: warning: Excess function parameter 'xfrm' description in 'drm_fb_swab'
>> drivers/gpu/drm/drm_format_helper.c:405: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb332'
>> drivers/gpu/drm/drm_format_helper.c:405: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb332'
>> drivers/gpu/drm/drm_format_helper.c:477: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb565'
>> drivers/gpu/drm/drm_format_helper.c:477: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb565'
>> drivers/gpu/drm/drm_format_helper.c:535: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_xrgb1555'
>> drivers/gpu/drm/drm_format_helper.c:535: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_xrgb1555'
>> drivers/gpu/drm/drm_format_helper.c:588: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb1555'
>> drivers/gpu/drm/drm_format_helper.c:588: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb1555'
>> drivers/gpu/drm/drm_format_helper.c:641: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgba5551'
>> drivers/gpu/drm/drm_format_helper.c:641: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgba5551'
>> drivers/gpu/drm/drm_format_helper.c:692: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_rgb888'
>> drivers/gpu/drm/drm_format_helper.c:692: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_rgb888'
>> drivers/gpu/drm/drm_format_helper.c:741: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb8888'
>> drivers/gpu/drm/drm_format_helper.c:741: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb8888'
>> drivers/gpu/drm/drm_format_helper.c:857: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_xrgb2101010'
>> drivers/gpu/drm/drm_format_helper.c:857: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_xrgb2101010'
>> drivers/gpu/drm/drm_format_helper.c:912: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_argb2101010'
>> drivers/gpu/drm/drm_format_helper.c:912: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_argb2101010'
   drivers/gpu/drm/drm_format_helper.c:968: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_gray8'
   drivers/gpu/drm/drm_format_helper.c:968: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_gray8'
   drivers/gpu/drm/drm_format_helper.c:1007: warning: Function parameter or member 'state' not described in 'drm_fb_blit'
   drivers/gpu/drm/drm_format_helper.c:1007: warning: Excess function parameter 'xfrm' description in 'drm_fb_blit'
   drivers/gpu/drm/drm_format_helper.c:1114: warning: Function parameter or member 'state' not described in 'drm_fb_xrgb8888_to_mono'
   drivers/gpu/drm/drm_format_helper.c:1114: warning: Excess function parameter 'xfrm' description in 'drm_fb_xrgb8888_to_mono'


vim +344 drivers/gpu/drm/drm_format_helper.c

41fd6f0a6dd62d Thomas Zimmermann  2022-04-27  317  
7415287e1f3675 Gerd Hoffmann      2019-04-05  318  /**
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  319   * drm_fb_swab - Swap bytes into clip buffer
ce582859ca7b33 Thomas Zimmermann  2022-08-08  320   * @dst: Array of destination buffers
ce582859ca7b33 Thomas Zimmermann  2022-08-08  321   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
ce582859ca7b33 Thomas Zimmermann  2022-08-08  322   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  323   * @src: Array of source buffers
7415287e1f3675 Gerd Hoffmann      2019-04-05  324   * @fb: DRM framebuffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  325   * @clip: Clip rectangle area to copy
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  326   * @cached: Source buffer is mapped cached (eg. not write-combined)
05864d296c0bfc Thomas Zimmermann  2023-10-05  327   * @xfrm: Transform and conversion state
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  328   *
ce582859ca7b33 Thomas Zimmermann  2022-08-08  329   * This function copies parts of a framebuffer to display memory and swaps per-pixel
ce582859ca7b33 Thomas Zimmermann  2022-08-08  330   * bytes during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  331   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
ce582859ca7b33 Thomas Zimmermann  2022-08-08  332   * least as many entries as there are planes in @fb's format. Each entry stores the
ce582859ca7b33 Thomas Zimmermann  2022-08-08  333   * value for the format's respective color plane at the same index. If @cached is
ce582859ca7b33 Thomas Zimmermann  2022-08-08  334   * false a temporary buffer is used to cache one pixel line at a time to speed up
ce582859ca7b33 Thomas Zimmermann  2022-08-08  335   * slow uncached reads.
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  336   *
ce582859ca7b33 Thomas Zimmermann  2022-08-08  337   * This function does not apply clipping on @dst (i.e. the destination is at the
ce582859ca7b33 Thomas Zimmermann  2022-08-08  338   * top-left corner).
7415287e1f3675 Gerd Hoffmann      2019-04-05  339   */
ce582859ca7b33 Thomas Zimmermann  2022-08-08  340  void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  341  		 const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  342  		 const struct drm_rect *clip, bool cached,
05864d296c0bfc Thomas Zimmermann  2023-10-05  343  		 struct drm_format_conv_state *state)
7415287e1f3675 Gerd Hoffmann      2019-04-05 @344  {
ce582859ca7b33 Thomas Zimmermann  2022-08-08  345  	const struct drm_format_info *format = fb->format;
ce582859ca7b33 Thomas Zimmermann  2022-08-08  346  	u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);
ce582859ca7b33 Thomas Zimmermann  2022-08-08  347  	void (*swab_line)(void *dbuf, const void *sbuf, unsigned int npixels);
3e3543c8a19cad Thomas Zimmermann  2021-11-10  348  
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  349  	switch (cpp) {
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  350  	case 4:
ce582859ca7b33 Thomas Zimmermann  2022-08-08  351  		swab_line = drm_fb_swab32_line;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  352  		break;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  353  	case 2:
ce582859ca7b33 Thomas Zimmermann  2022-08-08  354  		swab_line = drm_fb_swab16_line;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  355  		break;
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  356  	default:
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  357  		drm_warn_once(fb->dev, "Format %p4cc has unsupported pixel size.\n",
ce582859ca7b33 Thomas Zimmermann  2022-08-08  358  			      &format->format);
ce582859ca7b33 Thomas Zimmermann  2022-08-08  359  		return;
f241b064426943 Thomas Zimmermann  2022-08-08  360  	}
ce582859ca7b33 Thomas Zimmermann  2022-08-08  361  
05864d296c0bfc Thomas Zimmermann  2023-10-05  362  	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, state, swab_line);
7415287e1f3675 Gerd Hoffmann      2019-04-05  363  }
bd34cea2a0e4b0 Noralf Trønnes     2020-05-09  364  EXPORT_SYMBOL(drm_fb_swab);
7415287e1f3675 Gerd Hoffmann      2019-04-05  365  
cce6bedb38ed08 Thomas Zimmermann  2022-04-27  366  static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  367  {
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  368  	u8 *dbuf8 = dbuf;
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  369  	const __le32 *sbuf32 = sbuf;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  370  	unsigned int x;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  371  	u32 pix;
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  372  
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  373  	for (x = 0; x < pixels; x++) {
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  374  		pix = le32_to_cpu(sbuf32[x]);
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  375  		dbuf8[x] = ((pix & 0x00e00000) >> 16) |
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  376  			   ((pix & 0x0000e000) >> 11) |
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  377  			   ((pix & 0x000000c0) >> 6);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  378  	}
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  379  }
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  380  
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  381  /**
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  382   * drm_fb_xrgb8888_to_rgb332 - Convert XRGB8888 to RGB332 clip buffer
e13140a0628314 Thomas Zimmermann  2022-08-08  383   * @dst: Array of RGB332 destination buffers
e13140a0628314 Thomas Zimmermann  2022-08-08  384   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
e13140a0628314 Thomas Zimmermann  2022-08-08  385   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  386   * @src: Array of XRGB8888 source buffers
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  387   * @fb: DRM framebuffer
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  388   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  389   * @xfrm: Transform and conversion state
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  390   *
e13140a0628314 Thomas Zimmermann  2022-08-08  391   * This function copies parts of a framebuffer to display memory and converts the
e13140a0628314 Thomas Zimmermann  2022-08-08  392   * color format during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  393   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
e13140a0628314 Thomas Zimmermann  2022-08-08  394   * least as many entries as there are planes in @fb's format. Each entry stores the
e13140a0628314 Thomas Zimmermann  2022-08-08  395   * value for the format's respective color plane at the same index.
e13140a0628314 Thomas Zimmermann  2022-08-08  396   *
e13140a0628314 Thomas Zimmermann  2022-08-08  397   * This function does not apply clipping on @dst (i.e. the destination is at the
e13140a0628314 Thomas Zimmermann  2022-08-08  398   * top-left corner).
e13140a0628314 Thomas Zimmermann  2022-08-08  399   *
e13140a0628314 Thomas Zimmermann  2022-08-08  400   * Drivers can use this function for RGB332 devices that don't support XRGB8888 natively.
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  401   */
e13140a0628314 Thomas Zimmermann  2022-08-08  402  void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  403  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  404  			       const struct drm_rect *clip, struct drm_format_conv_state *state)
cee0b7cbf1c02f Noralf Trønnes     2021-09-29 @405  {
f241b064426943 Thomas Zimmermann  2022-08-08  406  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
f241b064426943 Thomas Zimmermann  2022-08-08  407  		1,
e13140a0628314 Thomas Zimmermann  2022-08-08  408  	};
e13140a0628314 Thomas Zimmermann  2022-08-08  409  
05864d296c0bfc Thomas Zimmermann  2023-10-05  410  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
f241b064426943 Thomas Zimmermann  2022-08-08  411  		    drm_fb_xrgb8888_to_rgb332_line);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  412  }
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  413  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
cee0b7cbf1c02f Noralf Trønnes     2021-09-29  414  
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  415  static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
bcc4442008aca0 Gerd Hoffmann      2019-04-05  416  {
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  417  	__le16 *dbuf16 = dbuf;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  418  	const __le32 *sbuf32 = sbuf;
d653bd39482377 Gerd Hoffmann      2019-04-10  419  	unsigned int x;
d653bd39482377 Gerd Hoffmann      2019-04-10  420  	u16 val16;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  421  	u32 pix;
bcc4442008aca0 Gerd Hoffmann      2019-04-05  422  
d653bd39482377 Gerd Hoffmann      2019-04-10  423  	for (x = 0; x < pixels; x++) {
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  424  		pix = le32_to_cpu(sbuf32[x]);
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  425  		val16 = ((pix & 0x00F80000) >> 8) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  426  			((pix & 0x0000FC00) >> 5) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  427  			((pix & 0x000000F8) >> 3);
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  428  		dbuf16[x] = cpu_to_le16(val16);
bcc4442008aca0 Gerd Hoffmann      2019-04-05  429  	}
bcc4442008aca0 Gerd Hoffmann      2019-04-05  430  }
bcc4442008aca0 Gerd Hoffmann      2019-04-05  431  
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  432  /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
a6fdb669bb6523 Thomas Zimmermann  2022-04-27  433  static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
69add027fd2bac Thomas Zimmermann  2022-04-27  434  						unsigned int pixels)
69add027fd2bac Thomas Zimmermann  2022-04-27  435  {
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  436  	__le16 *dbuf16 = dbuf;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  437  	const __le32 *sbuf32 = sbuf;
69add027fd2bac Thomas Zimmermann  2022-04-27  438  	unsigned int x;
69add027fd2bac Thomas Zimmermann  2022-04-27  439  	u16 val16;
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  440  	u32 pix;
69add027fd2bac Thomas Zimmermann  2022-04-27  441  
69add027fd2bac Thomas Zimmermann  2022-04-27  442  	for (x = 0; x < pixels; x++) {
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  443  		pix = le32_to_cpu(sbuf32[x]);
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  444  		val16 = ((pix & 0x00F80000) >> 8) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  445  			((pix & 0x0000FC00) >> 5) |
4d9db10576ff51 Geert Uytterhoeven 2022-07-08  446  			((pix & 0x000000F8) >> 3);
f21d62c9ce3db2 Thomas Zimmermann  2023-01-02  447  		dbuf16[x] = cpu_to_le16(swab16(val16));
69add027fd2bac Thomas Zimmermann  2022-04-27  448  	}
69add027fd2bac Thomas Zimmermann  2022-04-27  449  }
69add027fd2bac Thomas Zimmermann  2022-04-27  450  
7415287e1f3675 Gerd Hoffmann      2019-04-05  451  /**
7415287e1f3675 Gerd Hoffmann      2019-04-05  452   * drm_fb_xrgb8888_to_rgb565 - Convert XRGB8888 to RGB565 clip buffer
ab298c29d414a5 Thomas Zimmermann  2022-08-08  453   * @dst: Array of RGB565 destination buffers
ab298c29d414a5 Thomas Zimmermann  2022-08-08  454   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
ab298c29d414a5 Thomas Zimmermann  2022-08-08  455   *             within @dst; can be NULL if scanlines are stored next to each other.
504a51d70f86e3 Thomas Zimmermann  2022-08-08  456   * @src: Array of XRGB8888 source buffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  457   * @fb: DRM framebuffer
7415287e1f3675 Gerd Hoffmann      2019-04-05  458   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  459   * @xfrm: Transform and conversion state
d653bd39482377 Gerd Hoffmann      2019-04-10  460   * @swab: Swap bytes
7415287e1f3675 Gerd Hoffmann      2019-04-05  461   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  462   * This function copies parts of a framebuffer to display memory and converts the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  463   * color format during the process. Destination and framebuffer formats must match. The
504a51d70f86e3 Thomas Zimmermann  2022-08-08  464   * parameters @dst, @dst_pitch and @src refer to arrays. Each array must have at
ab298c29d414a5 Thomas Zimmermann  2022-08-08  465   * least as many entries as there are planes in @fb's format. Each entry stores the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  466   * value for the format's respective color plane at the same index.
bcc4442008aca0 Gerd Hoffmann      2019-04-05  467   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  468   * This function does not apply clipping on @dst (i.e. the destination is at the
ab298c29d414a5 Thomas Zimmermann  2022-08-08  469   * top-left corner).
ab298c29d414a5 Thomas Zimmermann  2022-08-08  470   *
ab298c29d414a5 Thomas Zimmermann  2022-08-08  471   * Drivers can use this function for RGB565 devices that don't support XRGB8888 natively.
bcc4442008aca0 Gerd Hoffmann      2019-04-05  472   */
ab298c29d414a5 Thomas Zimmermann  2022-08-08  473  void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
504a51d70f86e3 Thomas Zimmermann  2022-08-08  474  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  475  			       const struct drm_rect *clip, struct drm_format_conv_state *state,
05864d296c0bfc Thomas Zimmermann  2023-10-05  476  			       bool swab)
bcc4442008aca0 Gerd Hoffmann      2019-04-05 @477  {
f241b064426943 Thomas Zimmermann  2022-08-08  478  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
f241b064426943 Thomas Zimmermann  2022-08-08  479  		2,
ab298c29d414a5 Thomas Zimmermann  2022-08-08  480  	};
f241b064426943 Thomas Zimmermann  2022-08-08  481  
ab298c29d414a5 Thomas Zimmermann  2022-08-08  482  	void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned int npixels);
ab298c29d414a5 Thomas Zimmermann  2022-08-08  483  
69add027fd2bac Thomas Zimmermann  2022-04-27  484  	if (swab)
ab298c29d414a5 Thomas Zimmermann  2022-08-08  485  		xfrm_line = drm_fb_xrgb8888_to_rgb565_swab_line;
ab298c29d414a5 Thomas Zimmermann  2022-08-08  486  	else
ab298c29d414a5 Thomas Zimmermann  2022-08-08  487  		xfrm_line = drm_fb_xrgb8888_to_rgb565_line;
ab298c29d414a5 Thomas Zimmermann  2022-08-08  488  
05864d296c0bfc Thomas Zimmermann  2023-10-05  489  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state, xfrm_line);
7415287e1f3675 Gerd Hoffmann      2019-04-05  490  }
ab298c29d414a5 Thomas Zimmermann  2022-08-08  491  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
7415287e1f3675 Gerd Hoffmann      2019-04-05  492  
10cd592e639edc Thomas Zimmermann  2023-01-02  493  static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann  2023-01-02  494  {
10cd592e639edc Thomas Zimmermann  2023-01-02  495  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann  2023-01-02  496  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann  2023-01-02  497  	unsigned int x;
10cd592e639edc Thomas Zimmermann  2023-01-02  498  	u16 val16;
10cd592e639edc Thomas Zimmermann  2023-01-02  499  	u32 pix;
10cd592e639edc Thomas Zimmermann  2023-01-02  500  
10cd592e639edc Thomas Zimmermann  2023-01-02  501  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann  2023-01-02  502  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann  2023-01-02  503  		val16 = ((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann  2023-01-02  504  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann  2023-01-02  505  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann  2023-01-02  506  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann  2023-01-02  507  	}
10cd592e639edc Thomas Zimmermann  2023-01-02  508  }
10cd592e639edc Thomas Zimmermann  2023-01-02  509  
10cd592e639edc Thomas Zimmermann  2023-01-02  510  /**
10cd592e639edc Thomas Zimmermann  2023-01-02  511   * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
10cd592e639edc Thomas Zimmermann  2023-01-02  512   * @dst: Array of XRGB1555 destination buffers
10cd592e639edc Thomas Zimmermann  2023-01-02  513   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
10cd592e639edc Thomas Zimmermann  2023-01-02  514   *             within @dst; can be NULL if scanlines are stored next to each other.
10cd592e639edc Thomas Zimmermann  2023-01-02  515   * @src: Array of XRGB8888 source buffer
10cd592e639edc Thomas Zimmermann  2023-01-02  516   * @fb: DRM framebuffer
10cd592e639edc Thomas Zimmermann  2023-01-02  517   * @clip: Clip rectangle area to copy
05864d296c0bfc Thomas Zimmermann  2023-10-05  518   * @xfrm: Transform and conversion state
10cd592e639edc Thomas Zimmermann  2023-01-02  519   *
10cd592e639edc Thomas Zimmermann  2023-01-02  520   * This function copies parts of a framebuffer to display memory and converts
10cd592e639edc Thomas Zimmermann  2023-01-02  521   * the color format during the process. The parameters @dst, @dst_pitch and
10cd592e639edc Thomas Zimmermann  2023-01-02  522   * @src refer to arrays. Each array must have at least as many entries as
10cd592e639edc Thomas Zimmermann  2023-01-02  523   * there are planes in @fb's format. Each entry stores the value for the
10cd592e639edc Thomas Zimmermann  2023-01-02  524   * format's respective color plane at the same index.
10cd592e639edc Thomas Zimmermann  2023-01-02  525   *
10cd592e639edc Thomas Zimmermann  2023-01-02  526   * This function does not apply clipping on @dst (i.e. the destination is at the
10cd592e639edc Thomas Zimmermann  2023-01-02  527   * top-left corner).
10cd592e639edc Thomas Zimmermann  2023-01-02  528   *
10cd592e639edc Thomas Zimmermann  2023-01-02  529   * Drivers can use this function for XRGB1555 devices that don't support
10cd592e639edc Thomas Zimmermann  2023-01-02  530   * XRGB8888 natively.
10cd592e639edc Thomas Zimmermann  2023-01-02  531   */
10cd592e639edc Thomas Zimmermann  2023-01-02  532  void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
10cd592e639edc Thomas Zimmermann  2023-01-02  533  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
05864d296c0bfc Thomas Zimmermann  2023-10-05  534  				 const struct drm_rect *clip, struct drm_format_conv_state *state)
10cd592e639edc Thomas Zimmermann  2023-01-02 @535  {
10cd592e639edc Thomas Zimmermann  2023-01-02  536  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
10cd592e639edc Thomas Zimmermann  2023-01-02  537  		2,
10cd592e639edc Thomas Zimmermann  2023-01-02  538  	};
10cd592e639edc Thomas Zimmermann  2023-01-02  539  
05864d296c0bfc Thomas Zimmermann  2023-10-05  540  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
10cd592e639edc Thomas Zimmermann  2023-01-02  541  		    drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann  2023-01-02  542  }
10cd592e639edc Thomas Zimmermann  2023-01-02  543  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
10cd592e639edc Thomas Zimmermann  2023-01-02  544  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[Noralf Trønnes]

On 10/5/23 11:04, Thomas Zimmermann wrote:
> Pass an instance of struct drm_format_conv_state to DRM's format
> conversion helpers. Update all callers.
> 
> Most drivers can use the format-conversion state from their shadow-
> plane state. The shadow plane's destroy function releases the
> allocated buffer. Drivers will later be able to allocate a buffer
> of appropriate size in their plane's atomic_check code.
> 
> The gud driver uses a separate thread for committing updates. For
> now, the update worker contains its own format-conversion state.
> 
> Images in the format-helper tests are small. The tests preallocate
> a static page for the temporary buffer. Unloading the module releases
> the memory.
> 
> v3:
> 	* store buffer in shadow-plane state (Javier, Maxime)
> 	* replace ARRAY_SIZE() with sizeof() (Jani)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: David Lechner <david@lechnology.com>
> ---

> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 37c499ae4fe4f..b9b3dadf7b5f8 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c

> @@ -328,6 +324,7 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
>   * @fb: DRM framebuffer
>   * @clip: Clip rectangle area to copy
>   * @cached: Source buffer is mapped cached (eg. not write-combined)
> + * @xfrm: Transform and conversion state

Here and throughout the patch: xfrm does not match the argument name.

>   *
>   * This function copies parts of a framebuffer to display memory and swaps per-pixel
>   * bytes during the process. Destination and framebuffer formats must match. The
> @@ -342,7 +339,8 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
>   */
>  void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>  		 const struct iosys_map *src, const struct drm_framebuffer *fb,
> -		 const struct drm_rect *clip, bool cached)
> +		 const struct drm_rect *clip, bool cached,
> +		 struct drm_format_conv_state *state)
>  {
>  	const struct drm_format_info *format = fb->format;
>  	u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);


> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index 73dd4f4289c20..826fb20dbbf0d 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c

> @@ -830,13 +831,14 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
>  				struct drm_plane_state *old_state)
>  {
>  	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>  	struct drm_rect rect;
>  
>  	if (!pipe->crtc.state->active)
>  		return;
>  
>  	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> -		repaper_fb_dirty(state->fb);
> +		repaper_fb_dirty(state->fb, &shadow_plane_state->fmtcnv_state);

This won't work since repaper doesn't use the shadow plane helper.

Noralf.

>  }
>  
>  static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {

Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[Thomas Zimmermann]

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

Hi

Am 05.10.23 um 13:10 schrieb Noralf Trønnes:
> 
> 
> On 10/5/23 11:04, Thomas Zimmermann wrote:
>> Pass an instance of struct drm_format_conv_state to DRM's format
>> conversion helpers. Update all callers.
>>
>> Most drivers can use the format-conversion state from their shadow-
>> plane state. The shadow plane's destroy function releases the
>> allocated buffer. Drivers will later be able to allocate a buffer
>> of appropriate size in their plane's atomic_check code.
>>
>> The gud driver uses a separate thread for committing updates. For
>> now, the update worker contains its own format-conversion state.
>>
>> Images in the format-helper tests are small. The tests preallocate
>> a static page for the temporary buffer. Unloading the module releases
>> the memory.
>>
>> v3:
>> 	* store buffer in shadow-plane state (Javier, Maxime)
>> 	* replace ARRAY_SIZE() with sizeof() (Jani)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Noralf Trønnes <noralf@tronnes.org>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: David Lechner <david@lechnology.com>
>> ---
> 
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 37c499ae4fe4f..b9b3dadf7b5f8 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
> 
>> @@ -328,6 +324,7 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
>>    * @fb: DRM framebuffer
>>    * @clip: Clip rectangle area to copy
>>    * @cached: Source buffer is mapped cached (eg. not write-combined)
>> + * @xfrm: Transform and conversion state
> 
> Here and throughout the patch: xfrm does not match the argument name.

Thanks! I should be more careful when renaming.

> 
>>    *
>>    * This function copies parts of a framebuffer to display memory and swaps per-pixel
>>    * bytes during the process. Destination and framebuffer formats must match. The
>> @@ -342,7 +339,8 @@ static void drm_fb_swab32_line(void *dbuf, const void *sbuf, unsigned int pixels
>>    */
>>   void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>   		 const struct iosys_map *src, const struct drm_framebuffer *fb,
>> -		 const struct drm_rect *clip, bool cached)
>> +		 const struct drm_rect *clip, bool cached,
>> +		 struct drm_format_conv_state *state)
>>   {
>>   	const struct drm_format_info *format = fb->format;
>>   	u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);
> 
> 
>> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
>> index 73dd4f4289c20..826fb20dbbf0d 100644
>> --- a/drivers/gpu/drm/tiny/repaper.c
>> +++ b/drivers/gpu/drm/tiny/repaper.c
> 
>> @@ -830,13 +831,14 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
>>   				struct drm_plane_state *old_state)
>>   {
>>   	struct drm_plane_state *state = pipe->plane.state;
>> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>>   	struct drm_rect rect;
>>   
>>   	if (!pipe->crtc.state->active)
>>   		return;
>>   
>>   	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> -		repaper_fb_dirty(state->fb);
>> +		repaper_fb_dirty(state->fb, &shadow_plane_state->fmtcnv_state);
> 
> This won't work since repaper doesn't use the shadow plane helper.

Indeed. It can use a local buffer. But I'm a bit surprised to find that 
there are no shadow-plane helpers here. That sounds like a useful 
follow-up patch.

Best regards
Thomas

> 
> Noralf.
> 
>>   }
>>   
>>   static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Pass an instance of struct drm_format_conv_state to DRM's format
> conversion helpers. Update all callers.
>
> Most drivers can use the format-conversion state from their shadow-
> plane state. The shadow plane's destroy function releases the
> allocated buffer. Drivers will later be able to allocate a buffer
> of appropriate size in their plane's atomic_check code.
>
> The gud driver uses a separate thread for committing updates. For
> now, the update worker contains its own format-conversion state.
>
> Images in the format-helper tests are small. The tests preallocate
> a static page for the temporary buffer. Unloading the module releases
> the memory.
>
> v3:
> 	* store buffer in shadow-plane state (Javier, Maxime)
> 	* replace ARRAY_SIZE() with sizeof() (Jani)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: David Lechner <david@lechnology.com>
> ---

For the drm_format_helper.{h,c} and ssd130x driver changes:

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 4/7] drm/ofdrm: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

Preallocate the format-conversion state's storage in the plane's
atomic_check function if a format conversion is necessary. Allows
the update to fail if no memory is available. Avoids the same
allocation within atomic_update, which may not fail.

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

diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 404c83032cecc..05a72473cfc65 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -758,7 +758,11 @@ static const uint64_t ofdrm_primary_plane_format_modifiers[] = {
 static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 						   struct drm_atomic_state *new_state)
 {
+	struct drm_device *dev = plane->dev;
+	struct ofdrm_device *odev = ofdrm_device_of_dev(dev);
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_shadow_plane_state *new_shadow_plane_state =
+		to_drm_shadow_plane_state(new_plane_state);
 	struct drm_framebuffer *new_fb = new_plane_state->fb;
 	struct drm_crtc *new_crtc = new_plane_state->crtc;
 	struct drm_crtc_state *new_crtc_state = NULL;
@@ -777,6 +781,16 @@ static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	else if (!new_plane_state->visible)
 		return 0;
 
+	if (new_fb->format != odev->format) {
+		void *buf;
+
+		/* format conversion necessary; reserve buffer */
+		buf = drm_format_conv_state_reserve(&new_shadow_plane_state->fmtcnv_state,
+						    odev->pitch, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
 	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);
-- 
2.42.0

Re: [PATCH v4 4/7] drm/ofdrm: Preallocate format-conversion buffer in atomic_check

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Preallocate the format-conversion state's storage in the plane's
> atomic_check function if a format conversion is necessary. Allows
> the update to fail if no memory is available. Avoids the same
> allocation within atomic_update, which may not fail.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

Preallocate the format-conversion state's storage in the plane's
atomic_check function if a format conversion is necessary. Allows
the update to fail if no memory is available. Avoids the same
allocation within atomic_update, which may not fail.

Also inline drm_plane_helper_atomic_check() into the driver and thus
return early for invisible planes. Avoids memory allocation entirely
in this case.

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

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 6788f465995b3..d2280c81cbaae 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -19,6 +19,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_fbdev_generic.h>
 #include <drm/drm_format_helper.h>
+#include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
@@ -579,6 +580,44 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
+static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						       struct drm_atomic_state *state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *new_shadow_plane_state =
+		to_drm_shadow_plane_state(new_plane_state);
+	struct drm_framebuffer *new_fb = new_plane_state->fb;
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	struct drm_crtc_state *new_crtc_state = NULL;
+	struct drm_device *dev = plane->dev;
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
+	int ret;
+
+	if (new_crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
+
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, false);
+	if (ret)
+		return ret;
+	else if (!new_plane_state->visible)
+		return 0;
+
+	if (new_fb->format != sdev->format) {
+		void *buf;
+
+		/* format conversion necessary; reserve buffer */
+		buf = drm_format_conv_state_reserve(&new_shadow_plane_state->fmtcnv_state,
+						    sdev->pitch, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
 							 struct drm_atomic_state *state)
 {
@@ -635,7 +674,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 
 static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
-	.atomic_check = drm_plane_helper_atomic_check,
+	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
 	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
 	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
 };
-- 
2.42.0

Re: [PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Preallocate the format-conversion state's storage in the plane's
> atomic_check function if a format conversion is necessary. Allows
> the update to fail if no memory is available. Avoids the same
> allocation within atomic_update, which may not fail.
>
> Also inline drm_plane_helper_atomic_check() into the driver and thus
> return early for invisible planes. Avoids memory allocation entirely
> in this case.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 41 +++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 6788f465995b3..d2280c81cbaae 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -19,6 +19,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fbdev_generic.h>
>  #include <drm/drm_format_helper.h>
> +#include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
> @@ -579,6 +580,44 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> +						       struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *new_shadow_plane_state =
> +		to_drm_shadow_plane_state(new_plane_state);
> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> +	struct drm_crtc_state *new_crtc_state = NULL;
> +	struct drm_device *dev = plane->dev;
> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
> +	int ret;
> +
> +	if (new_crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
> +
> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);

Same comment that with the ssd130x driver. I think that we should use the
drm_plane_helper_atomic_check() helper instead of open coding it in each
driver. But regardless of what's decided on that, the change looks good:

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

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

Hi Javier

Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Preallocate the format-conversion state's storage in the plane's
>> atomic_check function if a format conversion is necessary. Allows
>> the update to fail if no memory is available. Avoids the same
>> allocation within atomic_update, which may not fail.
>>
>> Also inline drm_plane_helper_atomic_check() into the driver and thus
>> return early for invisible planes. Avoids memory allocation entirely
>> in this case.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/simpledrm.c | 41 +++++++++++++++++++++++++++++++-
>>   1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index 6788f465995b3..d2280c81cbaae 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -19,6 +19,7 @@
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fbdev_generic.h>
>>   #include <drm/drm_format_helper.h>
>> +#include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>>   #include <drm/drm_gem_shmem_helper.h>
>> @@ -579,6 +580,44 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>   	DRM_FORMAT_MOD_INVALID
>>   };
>>   
>> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> +						       struct drm_atomic_state *state)
>> +{
>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +	struct drm_shadow_plane_state *new_shadow_plane_state =
>> +		to_drm_shadow_plane_state(new_plane_state);
>> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>> +	struct drm_crtc_state *new_crtc_state = NULL;
>> +	struct drm_device *dev = plane->dev;
>> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>> +	int ret;
>> +
>> +	if (new_crtc)
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>> +
>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  false, false);
> 
> Same comment that with the ssd130x driver. I think that we should use the
> drm_plane_helper_atomic_check() helper instead of open coding it in each

I'm going to replace the call in simpledrm. 
drm_plane_helper_atomic_check() is useful to remove the entire 
atomic_check function from the driver; it does nothing apart from that. 
I've been called out before for such do-nothing helpers; deservedly so. [1]

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/aa3c4ad6-f99e-de48-e797-0748c9706e9e@amd.com/

> driver. But regardless of what's decided on that, the change looks good:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javier
>
> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:

[...]

>>> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>> +						       struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>>> +	struct drm_shadow_plane_state *new_shadow_plane_state =
>>> +		to_drm_shadow_plane_state(new_plane_state);
>>> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>> +	struct drm_crtc_state *new_crtc_state = NULL;
>>> +	struct drm_device *dev = plane->dev;
>>> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>> +	int ret;
>>> +
>>> +	if (new_crtc)
>>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>>> +
>>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>> +						  DRM_PLANE_NO_SCALING,
>>> +						  DRM_PLANE_NO_SCALING,
>>> +						  false, false);
>> 
>> Same comment that with the ssd130x driver. I think that we should use the
>> drm_plane_helper_atomic_check() helper instead of open coding it in each
>
> I'm going to replace the call in simpledrm. 
> drm_plane_helper_atomic_check() is useful to remove the entire 
> atomic_check function from the driver; it does nothing apart from that. 
> I've been called out before for such do-nothing helpers; deservedly so. [1]
>

The argument then is that drivers should open code *exactly* the same code
that the helper function already has just because they implement their own
.atomic_check callback?

And that the helper should only be used when is the .atomic_check callback
but not as a helper function?

I don't understand that rationale to be honest, but if there is one then
it should be very clear in the kernel-doc what functions are supposed to
be used only as callbacks and what functions can also be used as helpers.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

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

Hi Javier

Am 09.10.23 um 10:58 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Hi Javier
>>
>> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>>> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> [...]
> 
>>>> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>> +						       struct drm_atomic_state *state)
>>>> +{
>>>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>>>> +	struct drm_shadow_plane_state *new_shadow_plane_state =
>>>> +		to_drm_shadow_plane_state(new_plane_state);
>>>> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>>>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>> +	struct drm_crtc_state *new_crtc_state = NULL;
>>>> +	struct drm_device *dev = plane->dev;
>>>> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>>> +	int ret;
>>>> +
>>>> +	if (new_crtc)
>>>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>>>> +
>>>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>>> +						  DRM_PLANE_NO_SCALING,
>>>> +						  DRM_PLANE_NO_SCALING,
>>>> +						  false, false);
>>>
>>> Same comment that with the ssd130x driver. I think that we should use the
>>> drm_plane_helper_atomic_check() helper instead of open coding it in each
>>
>> I'm going to replace the call in simpledrm.
>> drm_plane_helper_atomic_check() is useful to remove the entire
>> atomic_check function from the driver; it does nothing apart from that.
>> I've been called out before for such do-nothing helpers; deservedly so. [1]
>>
> 
> The argument then is that drivers should open code *exactly* the same code
> that the helper function already has just because they implement their own
> .atomic_check callback?
> 
> And that the helper should only be used when is the .atomic_check callback
> but not as a helper function?

My point (and I think that's what Christian was also referring to) is 
that drm_plane_helper_atomic_check() does little more than pick a few 
default values for the parameters. It doesn't do anything in terms of 
algorithms. Hence there's no saving here that outweighs the cost of 
using this helper.

> 
> I don't understand that rationale to be honest, but if there is one then
> it should be very clear in the kernel-doc what functions are supposed to
> be used only as callbacks and what functions can also be used as helpers.

There's no clear rule AFAIK. We have to decide case by case. TBH I don't 
mind re-evaluating cases from time to time. At least, I'm going to 
revert the open-coded helper in ssd130x, as you asked me to.

Best regards
Thomas


> 
>> Best regards
>> Thomas
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 5/7] drm/simpledrm: Preallocate format-conversion buffer in atomic_check

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javier
>
> Am 09.10.23 um 10:58 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> Hello Thomas,
>> 
>>> Hi Javier
>>>
>>> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>>>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> [...]
>> 
>>>>> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>>> +						       struct drm_atomic_state *state)
>>>>> +{
>>>>> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>>>>> +	struct drm_shadow_plane_state *new_shadow_plane_state =
>>>>> +		to_drm_shadow_plane_state(new_plane_state);
>>>>> +	struct drm_framebuffer *new_fb = new_plane_state->fb;
>>>>> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>>> +	struct drm_crtc_state *new_crtc_state = NULL;
>>>>> +	struct drm_device *dev = plane->dev;
>>>>> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>>>> +	int ret;
>>>>> +
>>>>> +	if (new_crtc)
>>>>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>>>>> +
>>>>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>>>> +						  DRM_PLANE_NO_SCALING,
>>>>> +						  DRM_PLANE_NO_SCALING,
>>>>> +						  false, false);
>>>>
>>>> Same comment that with the ssd130x driver. I think that we should use the
>>>> drm_plane_helper_atomic_check() helper instead of open coding it in each
>>>
>>> I'm going to replace the call in simpledrm.
>>> drm_plane_helper_atomic_check() is useful to remove the entire
>>> atomic_check function from the driver; it does nothing apart from that.
>>> I've been called out before for such do-nothing helpers; deservedly so. [1]
>>>
>> 
>> The argument then is that drivers should open code *exactly* the same code
>> that the helper function already has just because they implement their own
>> .atomic_check callback?
>> 
>> And that the helper should only be used when is the .atomic_check callback
>> but not as a helper function?
>
> My point (and I think that's what Christian was also referring to) is 
> that drm_plane_helper_atomic_check() does little more than pick a few 
> default values for the parameters. It doesn't do anything in terms of 
> algorithms. Hence there's no saving here that outweighs the cost of 
> using this helper.
>

Got it.

>> 
>> I don't understand that rationale to be honest, but if there is one then
>> it should be very clear in the kernel-doc what functions are supposed to
>> be used only as callbacks and what functions can also be used as helpers.
>
> There's no clear rule AFAIK. We have to decide case by case. TBH I don't 
> mind re-evaluating cases from time to time. At least, I'm going to 
> revert the open-coded helper in ssd130x, as you asked me to.
>

No, that's OK. If you are going to revert also in simpledrm and the only
user will be a driver that has it as a callback, then I'm fine with your
original patch to ssd130x that open codes it in its .atomic_check as well.

> Best regards
> Thomas
>
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Thomas Zimmermann]

The plane's atomic_check returns -EINVAL if the CRTC has not been
set. This is the case for disabled planes, for which atomic_check
should return 0. For disabled planes, it also omits the mandatory
call to drm_atomic_helper_check_plane_state().

Replace the test with the boiler-plate code that first invokes
drm_atomic_helper_check_plane_state() and then tests for the plane
to be visible. Return early for non-visible planes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state")
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 3dd8e8a444b6f..dccbfe33edb5e 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -639,21 +639,22 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
 	struct drm_crtc *crtc = plane_state->crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state = NULL;
 	const struct drm_format_info *fi;
 	unsigned int pitch;
 	int ret;
 
-	if (!crtc)
-		return -EINVAL;
-
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
+	if (crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
-	ret = drm_plane_helper_atomic_check(plane, state);
+	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, false);
 	if (ret)
 		return ret;
+	else if (!plane_state->visible)
+		return 0;
 
 	fi = drm_format_info(DRM_FORMAT_R1);
 	if (!fi)
-- 
2.42.0

Re: [PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Oct 5, 2023 at 11:05 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The plane's atomic_check returns -EINVAL if the CRTC has not been
> set. This is the case for disabled planes, for which atomic_check
> should return 0. For disabled planes, it also omits the mandatory
> call to drm_atomic_helper_check_plane_state().
>
> Replace the test with the boiler-plate code that first invokes
> drm_atomic_helper_check_plane_state() and then tests for the plane
> to be visible. Return early for non-visible planes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state")

Thanks for your patch!

> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -639,21 +639,22 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>         struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>         struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
>         struct drm_crtc *crtc = plane_state->crtc;
> -       struct drm_crtc_state *crtc_state;
> +       struct drm_crtc_state *crtc_state = NULL;
>         const struct drm_format_info *fi;
>         unsigned int pitch;
>         int ret;
>
> -       if (!crtc)
> -               return -EINVAL;
> -
> -       crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -       if (IS_ERR(crtc_state))
> -               return PTR_ERR(crtc_state);
> +       if (crtc)
> +               crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> -       ret = drm_plane_helper_atomic_check(plane, state);
> +       ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +                                                 DRM_PLANE_NO_SCALING,
> +                                                 DRM_PLANE_NO_SCALING,
> +                                                 false, false);

Aren't the above 6 new lines identical to the call to
drm_plane_helper_atomic_check()? So why duplicate that?

>         if (ret)
>                 return ret;

Insert blank line?

> +       else if (!plane_state->visible)

"else" not needed.

> +               return 0;
>
>         fi = drm_format_info(DRM_FORMAT_R1);
>         if (!fi)

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 v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

Thanks for your patch.

> The plane's atomic_check returns -EINVAL if the CRTC has not been
> set. This is the case for disabled planes, for which atomic_check
> should return 0. For disabled planes, it also omits the mandatory
> call to drm_atomic_helper_check_plane_state().
>
> Replace the test with the boiler-plate code that first invokes
> drm_atomic_helper_check_plane_state() and then tests for the plane
> to be visible. Return early for non-visible planes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state")
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 3dd8e8a444b6f..dccbfe33edb5e 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -639,21 +639,22 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>  	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
>  	struct drm_crtc *crtc = plane_state->crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state = NULL;
>  	const struct drm_format_info *fi;
>  	unsigned int pitch;
>  	int ret;
>  
> -	if (!crtc)
> -		return -EINVAL;
> -
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> +	if (crtc)
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  
> -	ret = drm_plane_helper_atomic_check(plane, state);
> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);

As Geert mentioned you are open coding here what the called helper already
does. I prefer to keep doing that, instead of adding boiler plate code.

One question, the reason to return -EINVAL was to prevent the callback
ssd130x_primary_plane_atomic_update() to be executed, since that attempts
to get the CRTC state to pass the HW buffer to ssd130x_fb_blit_rect().

I believe this patch will introduce a regression and cause a NULL pointer
dereference when !plane_state->crtc and you should also add a check for
plane_state->visible in ssd130x_primary_plane_atomic_update() to bail ?

I haven't tested your patch yet though, so maybe I'm wrong about this.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Thomas Zimmermann]

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

Hi Javier

Am 05.10.23 um 13:37 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
> Thanks for your patch.
> 
>> The plane's atomic_check returns -EINVAL if the CRTC has not been
>> set. This is the case for disabled planes, for which atomic_check
>> should return 0. For disabled planes, it also omits the mandatory
>> call to drm_atomic_helper_check_plane_state().
>>
>> Replace the test with the boiler-plate code that first invokes
>> drm_atomic_helper_check_plane_state() and then tests for the plane
>> to be visible. Return early for non-visible planes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: d51f9fbd98b6 ("drm/ssd130x: Store the HW buffer in the driver-private CRTC state")
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> ---
>>   drivers/gpu/drm/solomon/ssd130x.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index 3dd8e8a444b6f..dccbfe33edb5e 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -639,21 +639,22 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
>>   	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
>>   	struct drm_crtc *crtc = plane_state->crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *crtc_state = NULL;
>>   	const struct drm_format_info *fi;
>>   	unsigned int pitch;
>>   	int ret;
>>   
>> -	if (!crtc)
>> -		return -EINVAL;
>> -
>> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> -	if (IS_ERR(crtc_state))
>> -		return PTR_ERR(crtc_state);
>> +	if (crtc)
>> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>   
>> -	ret = drm_plane_helper_atomic_check(plane, state);
>> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  DRM_PLANE_NO_SCALING,
>> +						  false, false);
> 
> As Geert mentioned you are open coding here what the called helper already
> does. I prefer to keep doing that, instead of adding boiler plate code.

Please see my other email.

> 
> One question, the reason to return -EINVAL was to prevent the callback
> ssd130x_primary_plane_atomic_update() to be executed, since that attempts
> to get the CRTC state to pass the HW buffer to ssd130x_fb_blit_rect().

Returning an errno code aborts the commit. [1] The CRTC can (maybe 
should?) be NULL to disable the plane. (It is in sync with 
plane_state->fb IIRC.)

So can you disable the plane now?

[1] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L997

> 
> I believe this patch will introduce a regression and cause a NULL pointer
> dereference when !plane_state->crtc and you should also add a check for
> plane_state->visible in ssd130x_primary_plane_atomic_update() to bail ?

You have a atomic_disable in that plane, so you're taking the branch at 
[2] for disabling the plane. No atomic_update then. If the plane has 
been enabled, you should take the branch at [3]. Without being able to 
move/scale the primary plane, I don't see how plane_state->visible could 
be false here. Right?

AFAIKT there should not be a NULL-deref here. Can you do a test?

[2] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L2745
[3] 
https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L2755

Best regards
Thomas

> 
> I haven't tested your patch yet though, so maybe I'm wrong about this.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javier
>
> Am 05.10.23 um 13:37 schrieb Javier Martinez Canillas:

[...]

>>> -	ret = drm_plane_helper_atomic_check(plane, state);
>>> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
>>> +						  DRM_PLANE_NO_SCALING,
>>> +						  DRM_PLANE_NO_SCALING,
>>> +						  false, false);
>> 
>> As Geert mentioned you are open coding here what the called helper already
>> does. I prefer to keep doing that, instead of adding boiler plate code.
>
> Please see my other email.
>

Sure, let's continue this discussion there.

>> 
>> One question, the reason to return -EINVAL was to prevent the callback
>> ssd130x_primary_plane_atomic_update() to be executed, since that attempts
>> to get the CRTC state to pass the HW buffer to ssd130x_fb_blit_rect().
>
> Returning an errno code aborts the commit. [1] The CRTC can (maybe 
> should?) be NULL to disable the plane. (It is in sync with 
> plane_state->fb IIRC.)
>

Thanks for the explanation.

> So can you disable the plane now?
>

It does not get disabled now indeed.

> [1] 
> https://elixir.bootlin.com/linux/v6.5/source/drivers/gpu/drm/drm_atomic_helper.c#L997
>
>> 
>> I believe this patch will introduce a regression and cause a NULL pointer
>> dereference when !plane_state->crtc and you should also add a check for
>> plane_state->visible in ssd130x_primary_plane_atomic_update() to bail ?
>
> You have a atomic_disable in that plane, so you're taking the branch at 
> [2] for disabling the plane. No atomic_update then. If the plane has 
> been enabled, you should take the branch at [3]. Without being able to 
> move/scale the primary plane, I don't see how plane_state->visible could 
> be false here. Right?
>
> AFAIKT there should not be a NULL-deref here. Can you do a test?
>

You are correct, there's no NULL-deref and in fact you are fixing the
plane not disable bug, so your fix is correct and should be applied.

I still prefer as mentioned keeping the drm_plane_helper_atomic_check()
call instead of open coding it, but regardless of what is decided:

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 6/7] drm/ssd130x: Fix atomic_check for disabled planes

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>

[...]

>>
>> You have a atomic_disable in that plane, so you're taking the branch at 
>> [2] for disabling the plane. No atomic_update then. If the plane has 
>> been enabled, you should take the branch at [3]. Without being able to 
>> move/scale the primary plane, I don't see how plane_state->visible could 
>> be false here. Right?
>>
>> AFAIKT there should not be a NULL-deref here. Can you do a test?
>>
>
> You are correct, there's no NULL-deref and in fact you are fixing the
> plane not disable bug, so your fix is correct and should be applied.
>
> I still prefer as mentioned keeping the drm_plane_helper_atomic_check()
> call instead of open coding it, but regardless of what is decided:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

And also for patches #1, #2, #3, #6 and #7, feel free to add:

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v4 7/7] drm/ssd130x: Preallocate format-conversion buffer in atomic_check

[Thomas Zimmermann]

Preallocate the format-conversion state's storage in the plane's
atomic_check function if a format conversion is necessary. Allows
the update to fail if no memory is available. Avoids the same
allocation within atomic_update, which may not fail.

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

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index dccbfe33edb5e..d3761e48bca12 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -638,6 +638,7 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
+	struct drm_shadow_plane_state *shadow_plane_state = &ssd130x_state->base;
 	struct drm_crtc *crtc = plane_state->crtc;
 	struct drm_crtc_state *crtc_state = NULL;
 	const struct drm_format_info *fi;
@@ -662,6 +663,16 @@ static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 
 	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
 
+	if (plane_state->fb->format != fi) {
+		void *buf;
+
+		/* format conversion necessary; reserve buffer */
+		buf = drm_format_conv_state_reserve(&shadow_plane_state->fmtcnv_state,
+						    pitch, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
 	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
 	if (!ssd130x_state->buffer)
 		return -ENOMEM;
-- 
2.42.0

Re: [PATCH v4 7/7] drm/ssd130x: Preallocate format-conversion buffer in atomic_check

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Preallocate the format-conversion state's storage in the plane's
> atomic_check function if a format conversion is necessary. Allows
> the update to fail if no memory is available. Avoids the same
> allocation within atomic_update, which may not fail.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

I like that the preallocated format conversion state is now part of the
shadow plane state. That makes a lot of sense to me.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Maxime Ripard]

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

Hi,

On Thu, Oct 05, 2023 at 11:04:20AM +0200, Thomas Zimmermann wrote:
> DRM's format-conversion helpers require temporary memory. Pass the
> buffer from the caller and keep it allocated over several calls. Allow
> the caller to preallocate the buffer memory.

I'm sorry... but why? Why do you need to keep it allocated over several
calls and preallocate the buffer? It's not clear to me at all.

> The motivation for this patchset is the recent work on a DRM panic
> handler. [1] The panic handler requires format conversion to display an
> error to the screen. But allocating memory during kernel panics is
> fragile.

We agree that we shouldn't allocate memory during the panic. I still
have concerns about how the panic handler will handle the driver
currently set up for a plane that isn't using an RGB format, or a buffer
not accessible by the kernel or CPU.

You can't expect to get away with just a copy to the current active
buffer.

If that's the assumption that underlines that patch series, then I don't
know why we need it at all, because that assumption is wrong to begin
with, and way too restrictive.

> The changes in this patchset enable the DRM panic handler to
> preallocate buffer storage before the panic occurs.
> 
> As an additonal benefit, drivers can now keep the temporary storage
> across multiple updates. Avoiding memory allocation slightly reduces
> the CPU overhead of the format helpers.

I'm sorry to go over that again, but you can't write a performance
improvement mechanism without some kind of benchmark. kmalloc has
built-in caching, why do we absolutely need our own cache on top of it?

If you never measured it, for all we know, we simply don't need it and
kmalloc is good enough.

Maxime

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

Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Thomas Zimmermann]

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

Hi Maxime

Am 06.10.23 um 16:49 schrieb Maxime Ripard:
> Hi,
> 
> On Thu, Oct 05, 2023 at 11:04:20AM +0200, Thomas Zimmermann wrote:
>> DRM's format-conversion helpers require temporary memory. Pass the
>> buffer from the caller and keep it allocated over several calls. Allow
>> the caller to preallocate the buffer memory.
> 
> I'm sorry... but why? Why do you need to keep it allocated over several
> calls and preallocate the buffer? It's not clear to me at all.
> 
>> The motivation for this patchset is the recent work on a DRM panic
>> handler. [1] The panic handler requires format conversion to display an
>> error to the screen. But allocating memory during kernel panics is
>> fragile.
> 
> We agree that we shouldn't allocate memory during the panic. I still
> have concerns about how the panic handler will handle the driver
> currently set up for a plane that isn't using an RGB format, or a buffer
> not accessible by the kernel or CPU.
> 
> You can't expect to get away with just a copy to the current active
> buffer.

In our current design, the panic handler calls get_scanout_buffer from 
struct drm_driver to retrieve a scanout buffer. What happens within that 
callback depends on the driver and hardware. Here are some of the 
expected scenarios:

  * simpledrm or ofdrm can return the firmware-provided scanout buffer. 
No further action is required.

  * Devices on a PCI-like bus:
      * With a working mode in RGB colors, drivers can return the 
current scanout buffer as well.
      * Without a working mode, drivers likely attempt to program a 
common display mode with RGB colors.

  * Drivers for devices behind other busses, such as USB, will probably 
not be able to reprogram during a panic or provide a useful scanout 
buffer at all.

  * The scanout buffer has to be mapped into kernel address space. This 
operation might be fragile during a panic. So drivers could set aside a 
slice of graphics memory and pre-map it; then use it during panic 
(requires some mode programming).

I expect that we will eventually have helpers for the various scenarios. 
Drivers will be able to implement their get_scanout_buffer with these 
helpers.

The font glyphs are 1-bit bitmaps. So we have to convert them to the 
scanout buffer's format in any case. We want to use the existing 
format-conversion helpers were possible.

> 
> If that's the assumption that underlines that patch series, then I don't
> know why we need it at all, because that assumption is wrong to begin
> with, and way too restrictive.
> 
>> The changes in this patchset enable the DRM panic handler to
>> preallocate buffer storage before the panic occurs.
>>
>> As an additonal benefit, drivers can now keep the temporary storage
>> across multiple updates. Avoiding memory allocation slightly reduces
>> the CPU overhead of the format helpers.
> 
> I'm sorry to go over that again, but you can't write a performance
> improvement mechanism without some kind of benchmark. kmalloc has
> built-in caching, why do we absolutely need our own cache on top of it?
> 
> If you never measured it, for all we know, we simply don't need it and
> kmalloc is good enough.

I'll remove that paragraph if you find it so annoying. Let me just say 
again that overhead is not the primary motivation behind these patches.

Best regards
Thomas


> 
> Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Maxime Ripard]

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

On Mon, Oct 09, 2023 at 10:23:02AM +0200, Thomas Zimmermann wrote:
> Hi Maxime
> 
> Am 06.10.23 um 16:49 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Thu, Oct 05, 2023 at 11:04:20AM +0200, Thomas Zimmermann wrote:
> > > DRM's format-conversion helpers require temporary memory. Pass the
> > > buffer from the caller and keep it allocated over several calls. Allow
> > > the caller to preallocate the buffer memory.
> > 
> > I'm sorry... but why? Why do you need to keep it allocated over several
> > calls and preallocate the buffer? It's not clear to me at all.
> >
> > > The motivation for this patchset is the recent work on a DRM panic
> > > handler. [1] The panic handler requires format conversion to display an
> > > error to the screen. But allocating memory during kernel panics is
> > > fragile.
> > 
> > We agree that we shouldn't allocate memory during the panic. I still
> > have concerns about how the panic handler will handle the driver
> > currently set up for a plane that isn't using an RGB format, or a buffer
> > not accessible by the kernel or CPU.
> > 
> > You can't expect to get away with just a copy to the current active
> > buffer.
> 
> In our current design, the panic handler calls get_scanout_buffer from
> struct drm_driver to retrieve a scanout buffer. What happens within that
> callback depends on the driver and hardware. Here are some of the expected
> scenarios:
> 
>  * simpledrm or ofdrm can return the firmware-provided scanout buffer. No
> further action is required.
> 
>  * Devices on a PCI-like bus:
>      * With a working mode in RGB colors, drivers can return the current
> scanout buffer as well.

Nothing guarantees that this is true. Even if in RGB, the buffer could
be unaccessible by the CPU, or still in an opaque format (when using
AFBC for example).

>      * Without a working mode, drivers likely attempt to program a common
> display mode with RGB colors.

Which would potentially require extra allocations, computations, etc.
that probably aren't doable in a panic handler path.

>  * Drivers for devices behind other busses, such as USB, will probably not
> be able to reprogram during a panic or provide a useful scanout buffer at
> all.
> 
>  * The scanout buffer has to be mapped into kernel address space. This
> operation might be fragile during a panic. So drivers could set aside a
> slice of graphics memory and pre-map it; then use it during panic (requires
> some mode programming).
> 
> I expect that we will eventually have helpers for the various scenarios.
> Drivers will be able to implement their get_scanout_buffer with these
> helpers.

What I'm trying to say is that it's not just about providing new
helpers. Sure, we can make drivers do whatever they want and provide a
scanout buffer. We still have to put that buffer into an active plane at
some point. The current design doesn't provide any way to do that
properly.

That's what I'd like to see addressed, and I will disagree with any
proposal that just ignores it.

> The font glyphs are 1-bit bitmaps. So we have to convert them to the scanout
> buffer's format in any case. We want to use the existing format-conversion
> helpers were possible.
> 
> > 
> > If that's the assumption that underlines that patch series, then I don't
> > know why we need it at all, because that assumption is wrong to begin
> > with, and way too restrictive.
> > 
> > > The changes in this patchset enable the DRM panic handler to
> > > preallocate buffer storage before the panic occurs.
> > > 
> > > As an additonal benefit, drivers can now keep the temporary storage
> > > across multiple updates. Avoiding memory allocation slightly reduces
> > > the CPU overhead of the format helpers.
> > 
> > I'm sorry to go over that again, but you can't write a performance
> > improvement mechanism without some kind of benchmark. kmalloc has
> > built-in caching, why do we absolutely need our own cache on top of it?
> > 
> > If you never measured it, for all we know, we simply don't need it and
> > kmalloc is good enough.
> 
> I'll remove that paragraph if you find it so annoying. Let me just say again
> that overhead is not the primary motivation behind these patches.

I mean, I don't want to sweep the code under the rug but keep it. I want
to know why we need that code in the first place. If there's no reason
then we just shouldn't have that caching at all.

Maxime

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

Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Noralf Trønnes]

On 10/5/23 11:04, Thomas Zimmermann wrote:
> DRM's format-conversion helpers require temporary memory. Pass the
> buffer from the caller and keep it allocated over several calls. Allow
> the caller to preallocate the buffer memory.
> 
> The motivation for this patchset is the recent work on a DRM panic
> handler. [1] The panic handler requires format conversion to display an
> error to the screen. But allocating memory during kernel panics is
> fragile. The changes in this patchset enable the DRM panic handler to
> preallocate buffer storage before the panic occurs.
> 

I've been thinking about this and afaiu this requires the display hw to
switch to the new panic buffer for scanout, right?
I don't think that is possible for any complex hw to do in a panic
situation. Or are you thinking that the driver should somehow "memcpy"
this buffer to the actual scanout buffer?

Noralf.

> As an additonal benefit, drivers can now keep the temporary storage
> across multiple updates. Avoiding memory allocation slightly reduces
> the CPU overhead of the format helpers.
> 
> Patch 1 adds struct drm_format_conv_state, a simple interface to pass
> around the buffer storage. Patch 2 adds an instance of the struct to
> the shadow-plane state. Patch 3 moves the buffer's memory management
> from the format helpers into their callers within the DRM drivers. Most
> of the afected drivers use the state instance stored in their shadow-
> plane state. The shadow-plane code releases the buffer memory automatically.
> 
> Patches 4 to 7 update three drivers to preallocate the format-conversion
> buffer in their plane's atomic_check function. The driver thus detects OOM
> errors before the display update begins.
> 
> Tested with simpledrm.
> 
> v4:
> 	* rename struct to drm_format_conv_state (Javier)
> 	* replace ARRAY_SIZE() with sizeof() (Jani)
> 	* store buffer in shadow-plane state (Javier, Maxime)
> 	* prealloc in atomic_check in several drivers
> v3:
> 	* no changes
> v2:
> 	* reserve storage during probing in the drivers
> 
> [1] https://patchwork.freedesktop.org/series/122244/
> 
> Thomas Zimmermann (7):
>   drm/format-helper: Cache buffers with struct drm_format_conv_state
>   drm/atomic-helper: Add format-conversion state to shadow-plane state
>   drm/format-helper: Pass format-conversion state to helpers
>   drm/ofdrm: Preallocate format-conversion buffer in atomic_check
>   drm/simpledrm: Preallocate format-conversion buffer in atomic_check
>   drm/ssd130x: Fix atomic_check for disabled planes
>   drm/ssd130x: Preallocate format-conversion buffer in atomic_check
> 
>  drivers/gpu/drm/drm_format_helper.c           | 212 +++++++++++++-----
>  drivers/gpu/drm/drm_gem_atomic_helper.c       |   9 +
>  drivers/gpu/drm/drm_mipi_dbi.c                |  19 +-
>  drivers/gpu/drm/gud/gud_pipe.c                |  30 ++-
>  drivers/gpu/drm/solomon/ssd130x.c             |  36 ++-
>  .../gpu/drm/tests/drm_format_helper_test.c    |  72 +++---
>  drivers/gpu/drm/tiny/cirrus.c                 |   3 +-
>  drivers/gpu/drm/tiny/ili9225.c                |  10 +-
>  drivers/gpu/drm/tiny/ofdrm.c                  |  16 +-
>  drivers/gpu/drm/tiny/repaper.c                |   8 +-
>  drivers/gpu/drm/tiny/simpledrm.c              |  43 +++-
>  drivers/gpu/drm/tiny/st7586.c                 |  19 +-
>  include/drm/drm_format_helper.h               |  81 +++++--
>  include/drm/drm_gem_atomic_helper.h           |  10 +
>  include/drm/drm_mipi_dbi.h                    |   4 +-
>  15 files changed, 428 insertions(+), 144 deletions(-)
> 
> 
> base-commit: 57d3b83a83c5527325efb5bcaf594da09fe4a41b

Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion

[Thomas Zimmermann]

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

Hi Noralf

Am 07.10.23 um 14:01 schrieb Noralf Trønnes:
> 
> 
> On 10/5/23 11:04, Thomas Zimmermann wrote:
>> DRM's format-conversion helpers require temporary memory. Pass the
>> buffer from the caller and keep it allocated over several calls. Allow
>> the caller to preallocate the buffer memory.
>>
>> The motivation for this patchset is the recent work on a DRM panic
>> handler. [1] The panic handler requires format conversion to display an
>> error to the screen. But allocating memory during kernel panics is
>> fragile. The changes in this patchset enable the DRM panic handler to
>> preallocate buffer storage before the panic occurs.
>>
> 
> I've been thinking about this and afaiu this requires the display hw to
> switch to the new panic buffer for scanout, right?
> I don't think that is possible for any complex hw to do in a panic
> situation. Or are you thinking that the driver should somehow "memcpy"
> this buffer to the actual scanout buffer?

It's all of that, but depends on the situation. In the easy case, we 
have a usable scanout buffer during the panic (e.g., XRGB888). We can 
use it for the panic screen.

If there's no such scanout programmed, we can attempt to set a new 
display mode. I think, it is best to leave these exact steps to the DRM 
driver. Some drivers might not be able to reprogram the display (e.g., 
with USB or I2C hardware).

In the panic handler, we copy glyphs into the scanout buffer and 
possibly convert the color format during that process. Without a working 
scanout buffer, the panic handler does nothing.

Best regards
Thomas


> 
> Noralf.
> 
>> As an additonal benefit, drivers can now keep the temporary storage
>> across multiple updates. Avoiding memory allocation slightly reduces
>> the CPU overhead of the format helpers.
>>
>> Patch 1 adds struct drm_format_conv_state, a simple interface to pass
>> around the buffer storage. Patch 2 adds an instance of the struct to
>> the shadow-plane state. Patch 3 moves the buffer's memory management
>> from the format helpers into their callers within the DRM drivers. Most
>> of the afected drivers use the state instance stored in their shadow-
>> plane state. The shadow-plane code releases the buffer memory automatically.
>>
>> Patches 4 to 7 update three drivers to preallocate the format-conversion
>> buffer in their plane's atomic_check function. The driver thus detects OOM
>> errors before the display update begins.
>>
>> Tested with simpledrm.
>>
>> v4:
>> 	* rename struct to drm_format_conv_state (Javier)
>> 	* replace ARRAY_SIZE() with sizeof() (Jani)
>> 	* store buffer in shadow-plane state (Javier, Maxime)
>> 	* prealloc in atomic_check in several drivers
>> v3:
>> 	* no changes
>> v2:
>> 	* reserve storage during probing in the drivers
>>
>> [1] https://patchwork.freedesktop.org/series/122244/
>>
>> Thomas Zimmermann (7):
>>    drm/format-helper: Cache buffers with struct drm_format_conv_state
>>    drm/atomic-helper: Add format-conversion state to shadow-plane state
>>    drm/format-helper: Pass format-conversion state to helpers
>>    drm/ofdrm: Preallocate format-conversion buffer in atomic_check
>>    drm/simpledrm: Preallocate format-conversion buffer in atomic_check
>>    drm/ssd130x: Fix atomic_check for disabled planes
>>    drm/ssd130x: Preallocate format-conversion buffer in atomic_check
>>
>>   drivers/gpu/drm/drm_format_helper.c           | 212 +++++++++++++-----
>>   drivers/gpu/drm/drm_gem_atomic_helper.c       |   9 +
>>   drivers/gpu/drm/drm_mipi_dbi.c                |  19 +-
>>   drivers/gpu/drm/gud/gud_pipe.c                |  30 ++-
>>   drivers/gpu/drm/solomon/ssd130x.c             |  36 ++-
>>   .../gpu/drm/tests/drm_format_helper_test.c    |  72 +++---
>>   drivers/gpu/drm/tiny/cirrus.c                 |   3 +-
>>   drivers/gpu/drm/tiny/ili9225.c                |  10 +-
>>   drivers/gpu/drm/tiny/ofdrm.c                  |  16 +-
>>   drivers/gpu/drm/tiny/repaper.c                |   8 +-
>>   drivers/gpu/drm/tiny/simpledrm.c              |  43 +++-
>>   drivers/gpu/drm/tiny/st7586.c                 |  19 +-
>>   include/drm/drm_format_helper.h               |  81 +++++--
>>   include/drm/drm_gem_atomic_helper.h           |  10 +
>>   include/drm/drm_mipi_dbi.h                    |   4 +-
>>   15 files changed, 428 insertions(+), 144 deletions(-)
>>
>>
>> base-commit: 57d3b83a83c5527325efb5bcaf594da09fe4a41b

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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