[PATCH 1/2] drm/sysfb: Use iosys_map_memset() to clear buffer

[PATCH 1/2] drm/sysfb: Use iosys_map_memset() to clear buffer

[Chen-Yu Tsai]

The atomic disable in drm/sysfb currently clears the buffer using
memset_io(). Since the mapping is provided in a |struct iosys_map|,
it is better to use the related API instead of extracting the vaddr.
This is mentioned as a TODO item.

Switch to iosys_map_memset(), which takes the mapping as well as an
offset into the mapping.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
index d2de29caf89e..99b13af4e391 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
@@ -375,13 +375,11 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(dev);
-	struct iosys_map dst = sysfb->fb_addr;
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
-	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
 	unsigned int dst_pitch = sysfb->fb_pitch;
 	const struct drm_format_info *dst_format = sysfb->fb_format;
 	struct drm_rect dst_clip;
-	unsigned long lines, linepixels, i;
+	unsigned long lines, linepixels, i, offset;
 	int idx;
 
 	drm_rect_init(&dst_clip,
@@ -395,11 +393,10 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
 		return;
 
 	/* Clear buffer to black if disabled */
-	dst_vmap += drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
-	for (i = 0; i < lines; ++i) {
-		memset_io(dst_vmap, 0, linepixels * dst_format->cpp[0]);
-		dst_vmap += dst_pitch;
-	}
+	offset = drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
+	for (i = 0; i < lines; ++i)
+		iosys_map_memset(&sysfb->fb_addr, offset + dst_pitch * i, 0,
+				 linepixels * dst_format->cpp[0]);
 
 	drm_dev_exit(idx);
 }
-- 
2.55.0.970.g62bdec98f9-goog

[PATCH 2/2] drm/sysfb: Use drm_format_info_min_pitch() to calculate line clear length

[Chen-Yu Tsai]

format->cpp[] is only for legacy formats that do not have block height /
width defined, and is replaced by format->chars_per_block[]. Users should
use helpers that transparently handle the migration.

Switch to drm_format_info_min_pitch(), i.e. convert from format->cpp
to format->chars_per_block and block height / width for calculating
the length of each line in the buffer to clear. This makes it future
proof with respect to format block size additions.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
index 99b13af4e391..acac94fdda7a 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
@@ -396,7 +396,7 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
 	offset = drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
 	for (i = 0; i < lines; ++i)
 		iosys_map_memset(&sysfb->fb_addr, offset + dst_pitch * i, 0,
-				 linepixels * dst_format->cpp[0]);
+				 drm_format_info_min_pitch(dst_format, 0, linepixels));
 
 	drm_dev_exit(idx);
 }
-- 
2.55.0.970.g62bdec98f9-goog

Re: [PATCH 2/2] drm/sysfb: Use drm_format_info_min_pitch() to calculate line clear length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete implementation of block format support, contradicting the commit message's claim of making the logic "future proof with respect to format block size additions".
--

commit 530a334e99c89cda78ed6306c7a7d6188cc1bf65
Author: Chen-Yu Tsai <wenst@chromium.org>

drm/sysfb: Use drm_format_info_min_pitch() to calculate line clear length

This commit updates drm_sysfb_plane_helper_atomic_disable() to use
drm_format_info_min_pitch() instead of format->cpp[] to calculate the
length of each line in the buffer to clear. This change is intended to
transparently handle the migration to format->chars_per_block[] and be
future-proof for format block size additions.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> @@ -396,7 +396,7 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
>  	offset = drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
>  	for (i = 0; i < lines; ++i)
>  		iosys_map_memset(&sysfb->fb_addr, offset + dst_pitch * i, 0,
> -				 linepixels * dst_format->cpp[0]);
> +				 drm_format_info_min_pitch(dst_format, 0, linepixels));

[Severity: Medium]
The commit message claims this change makes the logic "future proof with
respect to format block size additions", but is this implementation complete
for block formats?

For block formats where block_height > 1, lines represents the number
of pixel rows, but dst_pitch represents bytes per block row. Because the
loop iterates lines times and advances the pointer by dst_pitch each
iteration, wouldn't this advance the memory pointer block_height times
faster than the physical layout?

Additionally, it appears drm_fb_clip_offset() still multiplies by
format->cpp[0] internally, which is 0 for modern block formats,
potentially leading to an incorrect starting offset.

Although sysfb currently only exposes linear formats from firmware (meaning
block formats cannot reach this path today), could this cause an
out-of-bounds memory write and incorrect X-axis offset calculations if block
formats were ever used with sysfb in the future?

>  
>  	drm_dev_exit(idx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903060651.3724163-1-wenst@chromium.org?part=2

Re: [PATCH 1/2] drm/sysfb: Use iosys_map_memset() to clear buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The atomic_disable callback uses the new plane state to determine the screen area to clear, which may be zeroed out and fail to clear the screen.
--

commit cdbf0de82b5bf761696ff2cc3c658a3d090a02c8
Author: Chen-Yu Tsai <wenst@chromium.org>

drm/sysfb: Use iosys_map_memset() to clear buffer

This patch updates the sysfb atomic disable routine to use
iosys_map_memset() with an offset, rather than directly extracting the
virtual address for memory clearing.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index d2de29caf89eb..99b13af4e391c 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> @@ -375,13 +375,11 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(dev);
> -	struct iosys_map dst = sysfb->fb_addr;
>  	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
>  	unsigned int dst_pitch = sysfb->fb_pitch;
>  	const struct drm_format_info *dst_format = sysfb->fb_format;
>  	struct drm_rect dst_clip;
> -	unsigned long lines, linepixels, i;
> +	unsigned long lines, linepixels, i, offset;
>  	int idx;
>  
>  	drm_rect_init(&dst_clip,
>  		      plane_state->src_x >> 16, plane_state->src_y >> 16,
>  		      plane_state->src_w >> 16, plane_state->src_h >> 16);
>  
>  	lines = drm_rect_height(&dst_clip);
>  	linepixels = drm_rect_width(&dst_clip);

[Severity: High]
This isn't a bug introduced by this patch, but does using the new plane state
to calculate the clear area result in a failure to clear the screen?

Because drm_sysfb_plane_helper_atomic_disable() fetches the new plane state
above, it uses the state properties after the plane is disabled. If a
userspace display server zeroes out SRC_W and SRC_H when setting FB_ID = 0
to disable the plane, both lines and linepixels will evaluate to 0 here.

Could this cause the clearing loop to be completely skipped, leaving stale
framebuffer data visible on the screen?

Should this code use drm_atomic_get_old_plane_state() instead to ensure it
always correctly clears the area previously occupied by the plane?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903060651.3724163-1-wenst@chromium.org?part=1

Re: [PATCH 1/2] drm/sysfb: Use iosys_map_memset() to clear buffer

[Thomas Zimmermann]

Hi

Am 03.09.26 um 08:06 schrieb Chen-Yu Tsai:
> The atomic disable in drm/sysfb currently clears the buffer using
> memset_io(). Since the mapping is provided in a |struct iosys_map|,
> it is better to use the related API instead of extracting the vaddr.
> This is mentioned as a TODO item.
>
> Switch to iosys_map_memset(), which takes the mapping as well as an
> offset into the mapping.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index d2de29caf89e..99b13af4e391 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> @@ -375,13 +375,11 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,

There's a newer version of this function in the drm-misc-next branch. 
Please rebase your patch onto this code.

Best regards
Thomas

>   {
>   	struct drm_device *dev = plane->dev;
>   	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(dev);
> -	struct iosys_map dst = sysfb->fb_addr;
>   	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> -	void __iomem *dst_vmap = dst.vaddr_iomem; /* TODO: Use mapping abstraction */
>   	unsigned int dst_pitch = sysfb->fb_pitch;
>   	const struct drm_format_info *dst_format = sysfb->fb_format;
>   	struct drm_rect dst_clip;
> -	unsigned long lines, linepixels, i;
> +	unsigned long lines, linepixels, i, offset;
>   	int idx;
>   
>   	drm_rect_init(&dst_clip,
> @@ -395,11 +393,10 @@ void drm_sysfb_plane_helper_atomic_disable(struct drm_plane *plane,
>   		return;
>   
>   	/* Clear buffer to black if disabled */
> -	dst_vmap += drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
> -	for (i = 0; i < lines; ++i) {
> -		memset_io(dst_vmap, 0, linepixels * dst_format->cpp[0]);
> -		dst_vmap += dst_pitch;
> -	}
> +	offset = drm_fb_clip_offset(dst_pitch, dst_format, &dst_clip);
> +	for (i = 0; i < lines; ++i)
> +		iosys_map_memset(&sysfb->fb_addr, offset + dst_pitch * i, 0,
> +				 linepixels * dst_format->cpp[0]);
>   
>   	drm_dev_exit(idx);
>   }

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)