Thomas Zimmermann
2021-Nov-09 12:38 UTC
[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property
Am 08.11.21 um 21:55 schrieb Noralf Tr?nnes:> > > Den 01.11.2021 15.15, skrev Thomas Zimmermann: >> 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 at 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. 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> > Noralf. > >> + 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; >>-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N?rnberg, Germany (HRB 36809, AG N?rnberg) Gesch?ftsf?hrer: Ivo Totev -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20211109/14f5e830/attachment.sig>
Noralf Trønnes
2021-Nov-09 13:04 UTC
[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property
Den 09.11.2021 13.38, skrev Thomas Zimmermann:> > > Am 08.11.21 um 21:55 schrieb Noralf Tr?nnes: >> >> >> Den 01.11.2021 15.15, skrev Thomas Zimmermann: >>> 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 at 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 > > >> >> Noralf. >> >>> +??? 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; >>> >