Den 09.11.2021 13.38, skrev Thomas Zimmermann:

Am 08.11.21 um 21:55 schrieb Noralf Trønnes:

quoted

Den 01.11.2021 15.15, skrev Thomas Zimmermann:

quoted

Enable the FB_DAMAGE_CLIPS property to reduce display-update
overhead. Also fixes a warning in the kernel log.

   simple-framebuffer simple-framebuffer.0: [drm]
drm_plane_enable_fb_damage_clips() not called

Fix the computation of the blit rectangle. This wasn't an issue so
far, as simpledrm always blitted the full framebuffer. The code now
supports damage clipping and virtual screen sizes.

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

diff --git a/drivers/gpu/drm/tiny/simpledrm.c

b/drivers/gpu/drm/tiny/simpledrm.c
index 571f716ff427..e872121e9fb0 100644

--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c

@@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct

drm_simple_display_pipe *pipe,
      void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use
mapping abstraction */
      struct drm_device *dev = &sdev->dev;
      void __iomem *dst = sdev->screen_base;
-    struct drm_rect clip;
+    struct drm_rect src_clip, dst_clip;
      int idx;
        if (!fb)

@@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct

drm_simple_display_pipe *pipe,
      if (!drm_dev_enter(dev, &idx))
          return;
  -    drm_rect_init(&clip, 0, 0, fb->width, fb->height);
+    drm_rect_fp_to_int(&src_clip, &plane_state->src);
  -    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &clip);
-    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
fb, &clip);
+    dst_clip = plane_state->dst;

I assume that src_clip and dst_clip are of the same size, since scaling
is not supported. What prevents dst_clip from being bigger than the
buffer that's being blitted into? Where is that bounds checking done?

The value of dst_clip comes from plane_state->dst, which gets
initialized in drm_atomic_helper_check_plane_state(). [1] The fields are
taken from the crtc_{x,y,w,h} values by drm_plane_get_dest(). [2] For
primary planes, the crtc_{x,y,w,h} values are initialized in
__drm_atomic_helper_set_config() to the size of the display. [3] That
size comes directly from the current video mode. [4] From all I can see
this cannot overflow.

Ok, that takes care of the upper bound.

There's this in drm_atomic_helper_check_plane_state():

	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);

	if (!plane_state->visible)
		/*
		 * Plane isn't visible; some drivers can handle this
		 * so we just return success here.  Drivers that can't
		 * (including those that use the primary plane helper's
		 * update function) will return an error from their
		 * update_plane handler.
		 */
		return 0;

drm_atomic_helper_damage_merged() checks ->visible and returns false if
it is not set so update() is good on the lower bound.

Maybe it's necessary to check ->visible here in enable() before doing
the blit?

Noralf.

Best regards
Thomas

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L807

[2]
https://elixir.bootlin.com/linux/latest/source/include/drm/drm_plane.h#L257
[3]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic.c#L1590

[4]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L788

quoted

Noralf.

quoted

+    if (!drm_rect_intersect(&dst_clip, &src_clip))
+        return;
+
+    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip);
+    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
fb, &src_clip);
        drm_dev_exit(idx);
  }

@@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct

drm_simple_display_pipe *pipe,
      struct drm_framebuffer *fb = plane_state->fb;
      struct drm_device *dev = &sdev->dev;
      void __iomem *dst = sdev->screen_base;
-    struct drm_rect clip;
+    struct drm_rect damage_clip, src_clip, dst_clip;
      int idx;
        if (!fb)
          return;
  -    if (!drm_atomic_helper_damage_merged(old_plane_state,
plane_state, &clip))
+    if (!drm_atomic_helper_damage_merged(old_plane_state,
plane_state, &damage_clip))
+        return;
+
+    drm_rect_fp_to_int(&src_clip, &plane_state->src);
+    if (!drm_rect_intersect(&src_clip, &damage_clip))
+        return;
+
+    dst_clip = plane_state->dst;
+    if (!drm_rect_intersect(&dst_clip, &src_clip))
          return;
        if (!drm_dev_enter(dev, &idx))
          return;
  -    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &clip);
-    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
fb, &clip);
+    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip);
+    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
fb, &src_clip);
        drm_dev_exit(idx);
  }

@@ -794,6 +806,8 @@ static int simpledrm_device_init_modeset(struct

simpledrm_device *sdev)
      if (ret)
          return ret;
  +    drm_plane_enable_fb_damage_clips(&pipe->plane);
+
      drm_mode_config_reset(dev);
        return 0;