| Message ID | 20211101141532.26655-8-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/simpledrm: Enable damage clips and virtual screens | expand |
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@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; > + if (!drm_rect_intersect(&dst_clip, &src_clip)) > + return; You're inside drm_dev_enter here so can't just return. Move drm_dev_enter after this like you do in update(). > + > + 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; > + The following check, isn't that the same check that has just happened in drm_atomic_helper_damage_iter_next()? Noralf. > + 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; >
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@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? 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; >
Hi Am 08.11.21 um 19:57 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@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; >> + if (!drm_rect_intersect(&dst_clip, &src_clip)) >> + return; > > You're inside drm_dev_enter here so can't just return. Move > drm_dev_enter after this like you do in update(). Right. Will be fixed. > >> + >> + 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; >> + > > The following check, isn't that the same check that has just happened in > drm_atomic_helper_damage_iter_next()? Oh, indeed. It takes the plane's src rectangle [1] and clips against it. [2] The bounding box of all damage clips is the final rectangle. [3] It can never by outside of plane_state->src. So the extra test here and the damage_clip variable can be removed. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_damage_helper.c#L235 [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_damage_helper.c#L280 [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_damage_helper.c#L320 > > Noralf. > >> + 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; >>
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@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; >>
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@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; >>> >
Hi, thanks for looking through all this code. Am 09.11.21 um 14:04 schrieb Noralf Trønnes: > > > 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@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? Is it even possible to create an invisible primary plane here? We cannot scale [1] and if the primary plane is smaller than the framebuffer, we hit the case at [2]. The only way I can see this is that the visibility test at [3] succeeds. That would require a framebuffer of size 0. That's impossible from the code in frambuffer_check(). [4] Unless there's some corner case, we're good here. However, I think that drm_atomic_check_plane_state() could later be improved to handle the 'visible' field in a more sophisticated way. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_simple_kms_helper.c#L227 [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L854 [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L844 [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L185 > > 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; >>>> >>
Den 09.11.2021 15.56, skrev Thomas Zimmermann: > Hi, > > thanks for looking through all this code. > > Am 09.11.21 um 14:04 schrieb Noralf Trønnes: >> >> >> 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@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? > > Is it even possible to create an invisible primary plane here? We cannot > scale [1] and if the primary plane is smaller than the framebuffer, we > hit the case at [2]. The only way I can see this is that the visibility > test at [3] succeeds. That would require a framebuffer of size 0. That's > impossible from the code in frambuffer_check(). [4] Unless there's some > corner case, we're good here. Ok, thanks. Noralf. > > However, I think that drm_atomic_check_plane_state() could later be > improved to handle the 'visible' field in a more sophisticated way. > > Best regards > Thomas > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_simple_kms_helper.c#L227 > > [2] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L854 > > [3] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L844 > > [4] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L185 > > >> >> 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; >>>>> >>> >
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; + 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;
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(-)