| Message ID | 20231109172449.1599262-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: Allow the damage helpers to handle buffer damage | expand |
Hi Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: > To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather > than per-plane uploads), since these type of drivers need to handle buffer > damages instead of frame damages. > > The drm_atomic_helper_buffer_damage_iter_init() has the same logic than > drm_atomic_helper_damage_iter_init() but it also takes into account if the > framebuffer attached to plane's state has changed since the last update. > > And the drm_atomic_helper_buffer_damage_merged() is just a version of the > drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper > that is mentioned above. > > Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") > Cc: <stable@vger.kernel.org> # v6.4+ > Reported-by: nerdopolis <bluescreen_avenger@verizon.net> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 > Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- > include/drm/drm_damage_helper.h | 7 +++ > 2 files changed, 80 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > index aa2325567918..b72062c9d31c 100644 > --- a/drivers/gpu/drm/drm_damage_helper.c > +++ b/drivers/gpu/drm/drm_damage_helper.c > @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); > static void > __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > - const struct drm_plane_state *state) > + const struct drm_plane_state *state, > + bool buffer_damage) I think it would be preferable to drop patches one and two and instead add this parameter directly to drm_atomic_helper_damage_iter_init() and drm_atomic_helper_damage_merged(). That's a bit of churn, but more readable code. Best regards Thomas > { > struct drm_rect src; > memset(iter, 0, sizeof(*iter)); > @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF); > iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF); > > - if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) { > + if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) || > + (buffer_damage && old_state->fb != state->fb)) { > iter->clips = NULL; > iter->num_clips = 0; > iter->full_update = true; > @@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > * update). Currently this iterator returns full plane src in case plane src > * changed but that can be changed in future to return damage. > * > + * Note that this helper is for drivers that do per-plane uploads and expect > + * to handle frame damages. Drivers that do per-buffer uploads instead should > + * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages. > + * > * For the case when plane is not visible or plane update should not happen the > * first call to iter_next will return false. Note that this helper use clipped > * &drm_plane_state.src, so driver calling this helper should have called > @@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > const struct drm_plane_state *state) > { > - __drm_atomic_helper_damage_iter_init(iter, old_state, state); > + __drm_atomic_helper_damage_iter_init(iter, old_state, state, false); > } > EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init); > > +/** > + * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator. > + * @iter: The iterator to initialize. > + * @old_state: Old plane state for validation. > + * @state: Plane state from which to iterate the damage clips. > + * > + * Initialize an iterator, which clips buffer damage > + * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator > + * returns full plane src in case buffer damage is not present because user-space > + * didn't sent, driver discarded it (it want to do full plane update) or the plane > + * @state has an attached framebuffer that is different than the one in @state (it > + * has changed since the last plane update). > + * > + * For the case when plane is not visible or plane update should not happen the > + * first call to iter_next will return false. Note that this helper use clipped > + * &drm_plane_state.src, so driver calling this helper should have called > + * drm_atomic_helper_check_plane_state() earlier. > + */ > +void > +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > + const struct drm_plane_state *old_state, > + const struct drm_plane_state *state) > +{ > + __drm_atomic_helper_damage_iter_init(iter, old_state, state, true); > +} > +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init); > + > /** > * drm_atomic_helper_damage_iter_next - Advance the damage iterator. > * @iter: The iterator to advance. > @@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); > > static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > - struct drm_rect *rect) > + struct drm_rect *rect, > + bool buffer_damage) > { > struct drm_atomic_helper_damage_iter iter; > struct drm_rect clip; > @@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ > rect->x2 = 0; > rect->y2 = 0; > > - drm_atomic_helper_damage_iter_init(&iter, old_state, state); > + __drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage); > drm_atomic_for_each_plane_damage(&iter, &clip) { > rect->x1 = min(rect->x1, clip.x1); > rect->y1 = min(rect->y1, clip.y1); > @@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ > * For details see: drm_atomic_helper_damage_iter_init() and > * drm_atomic_helper_damage_iter_next(). > * > + * Note that this helper is for drivers that do per-plane uploads and expect > + * to handle frame damages. Drivers that do per-buffer uploads instead should > + * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages. > + * > * Returns: > * True if there is valid plane damage otherwise false. > */ > @@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > struct drm_rect *rect) > { > - return __drm_atomic_helper_damage_merged(old_state, state, rect); > + return __drm_atomic_helper_damage_merged(old_state, state, rect, false); > } > EXPORT_SYMBOL(drm_atomic_helper_damage_merged); > + > +/** > + * drm_atomic_helper_buffer_damage_merged - Merged buffer damage > + * @old_state: Old plane state for validation. > + * @state: Plane state from which to iterate the damage clips. > + * @rect: Returns the merged buffer damage rectangle > + * > + * This function merges any valid buffer damage clips into one rectangle and > + * returns it in @rect. It checks if the framebuffers attached to @old_state > + * and @state are the same. If that is not the case then the returned damage > + * rectangle is the &drm_plane_state.src, since a full update should happen. > + * > + * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that > + * full plane update should happen. It also ensure helper iterator will return > + * &drm_plane_state.src as damage. > + * > + * For details see: drm_atomic_helper_buffer_damage_iter_init() and > + * drm_atomic_helper_damage_iter_next(). > + * > + * Returns: > + * True if there is valid buffer damage otherwise false. > + */ > +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, > + struct drm_plane_state *state, > + struct drm_rect *rect) > +{ > + return __drm_atomic_helper_damage_merged(old_state, state, rect, true); > +} > +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged); > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > index effda42cce31..328bb249d68f 100644 > --- a/include/drm/drm_damage_helper.h > +++ b/include/drm/drm_damage_helper.h > @@ -74,11 +74,18 @@ void > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > const struct drm_plane_state *new_state); > +void > +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > + const struct drm_plane_state *old_state, > + const struct drm_plane_state *new_state); > bool > drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter, > struct drm_rect *rect); > bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > struct drm_rect *rect); > +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, > + struct drm_plane_state *state, > + struct drm_rect *rect); > > #endif
Hi Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: > To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather > than per-plane uploads), since these type of drivers need to handle buffer > damages instead of frame damages. > > The drm_atomic_helper_buffer_damage_iter_init() has the same logic than > drm_atomic_helper_damage_iter_init() but it also takes into account if the > framebuffer attached to plane's state has changed since the last update. > > And the drm_atomic_helper_buffer_damage_merged() is just a version of the > drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper > that is mentioned above. > > Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") > Cc: <stable@vger.kernel.org> # v6.4+ > Reported-by: nerdopolis <bluescreen_avenger@verizon.net> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 > Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- > include/drm/drm_damage_helper.h | 7 +++ > 2 files changed, 80 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > index aa2325567918..b72062c9d31c 100644 > --- a/drivers/gpu/drm/drm_damage_helper.c > +++ b/drivers/gpu/drm/drm_damage_helper.c > @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); > static void > __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > - const struct drm_plane_state *state) > + const struct drm_plane_state *state, > + bool buffer_damage) > { > struct drm_rect src; > memset(iter, 0, sizeof(*iter)); > @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF); > iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF); > > - if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) { > + if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) || > + (buffer_damage && old_state->fb != state->fb)) { I'd assume that this change effectivly disables damage handling. AFAICT user space often does a page flip with a new framebuffer plus damage data. Now, with each change of the framebuffer we ignore the damage information. It's not a blocker as that's the behavior before 6.4, but we should be aware of it. Best regards Thomas > iter->clips = NULL; > iter->num_clips = 0; > iter->full_update = true; > @@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > * update). Currently this iterator returns full plane src in case plane src > * changed but that can be changed in future to return damage. > * > + * Note that this helper is for drivers that do per-plane uploads and expect > + * to handle frame damages. Drivers that do per-buffer uploads instead should > + * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages. > + * > * For the case when plane is not visible or plane update should not happen the > * first call to iter_next will return false. Note that this helper use clipped > * &drm_plane_state.src, so driver calling this helper should have called > @@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > const struct drm_plane_state *state) > { > - __drm_atomic_helper_damage_iter_init(iter, old_state, state); > + __drm_atomic_helper_damage_iter_init(iter, old_state, state, false); > } > EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init); > > +/** > + * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator. > + * @iter: The iterator to initialize. > + * @old_state: Old plane state for validation. > + * @state: Plane state from which to iterate the damage clips. > + * > + * Initialize an iterator, which clips buffer damage > + * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator > + * returns full plane src in case buffer damage is not present because user-space > + * didn't sent, driver discarded it (it want to do full plane update) or the plane > + * @state has an attached framebuffer that is different than the one in @state (it > + * has changed since the last plane update). > + * > + * For the case when plane is not visible or plane update should not happen the > + * first call to iter_next will return false. Note that this helper use clipped > + * &drm_plane_state.src, so driver calling this helper should have called > + * drm_atomic_helper_check_plane_state() earlier. > + */ > +void > +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > + const struct drm_plane_state *old_state, > + const struct drm_plane_state *state) > +{ > + __drm_atomic_helper_damage_iter_init(iter, old_state, state, true); > +} > +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init); > + > /** > * drm_atomic_helper_damage_iter_next - Advance the damage iterator. > * @iter: The iterator to advance. > @@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); > > static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > - struct drm_rect *rect) > + struct drm_rect *rect, > + bool buffer_damage) > { > struct drm_atomic_helper_damage_iter iter; > struct drm_rect clip; > @@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ > rect->x2 = 0; > rect->y2 = 0; > > - drm_atomic_helper_damage_iter_init(&iter, old_state, state); > + __drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage); > drm_atomic_for_each_plane_damage(&iter, &clip) { > rect->x1 = min(rect->x1, clip.x1); > rect->y1 = min(rect->y1, clip.y1); > @@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ > * For details see: drm_atomic_helper_damage_iter_init() and > * drm_atomic_helper_damage_iter_next(). > * > + * Note that this helper is for drivers that do per-plane uploads and expect > + * to handle frame damages. Drivers that do per-buffer uploads instead should > + * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages. > + * > * Returns: > * True if there is valid plane damage otherwise false. > */ > @@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > struct drm_rect *rect) > { > - return __drm_atomic_helper_damage_merged(old_state, state, rect); > + return __drm_atomic_helper_damage_merged(old_state, state, rect, false); > } > EXPORT_SYMBOL(drm_atomic_helper_damage_merged); > + > +/** > + * drm_atomic_helper_buffer_damage_merged - Merged buffer damage > + * @old_state: Old plane state for validation. > + * @state: Plane state from which to iterate the damage clips. > + * @rect: Returns the merged buffer damage rectangle > + * > + * This function merges any valid buffer damage clips into one rectangle and > + * returns it in @rect. It checks if the framebuffers attached to @old_state > + * and @state are the same. If that is not the case then the returned damage > + * rectangle is the &drm_plane_state.src, since a full update should happen. > + * > + * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that > + * full plane update should happen. It also ensure helper iterator will return > + * &drm_plane_state.src as damage. > + * > + * For details see: drm_atomic_helper_buffer_damage_iter_init() and > + * drm_atomic_helper_damage_iter_next(). > + * > + * Returns: > + * True if there is valid buffer damage otherwise false. > + */ > +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, > + struct drm_plane_state *state, > + struct drm_rect *rect) > +{ > + return __drm_atomic_helper_damage_merged(old_state, state, rect, true); > +} > +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged); > diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h > index effda42cce31..328bb249d68f 100644 > --- a/include/drm/drm_damage_helper.h > +++ b/include/drm/drm_damage_helper.h > @@ -74,11 +74,18 @@ void > drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > const struct drm_plane_state *old_state, > const struct drm_plane_state *new_state); > +void > +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > + const struct drm_plane_state *old_state, > + const struct drm_plane_state *new_state); > bool > drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter, > struct drm_rect *rect); > bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, > struct drm_plane_state *state, > struct drm_rect *rect); > +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, > + struct drm_plane_state *state, > + struct drm_rect *rect); > > #endif
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, Thanks a lot for your feedback. > Hi > > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: >> To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather >> than per-plane uploads), since these type of drivers need to handle buffer >> damages instead of frame damages. >> >> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than >> drm_atomic_helper_damage_iter_init() but it also takes into account if the >> framebuffer attached to plane's state has changed since the last update. >> >> And the drm_atomic_helper_buffer_damage_merged() is just a version of the >> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper >> that is mentioned above. >> >> Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") >> Cc: <stable@vger.kernel.org> # v6.4+ >> Reported-by: nerdopolis <bluescreen_avenger@verizon.net> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 >> Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- >> include/drm/drm_damage_helper.h | 7 +++ >> 2 files changed, 80 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c >> index aa2325567918..b72062c9d31c 100644 >> --- a/drivers/gpu/drm/drm_damage_helper.c >> +++ b/drivers/gpu/drm/drm_damage_helper.c >> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); >> static void >> __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, >> const struct drm_plane_state *old_state, >> - const struct drm_plane_state *state) >> + const struct drm_plane_state *state, >> + bool buffer_damage) > > I think it would be preferable to drop patches one and two and instead > add this parameter directly to drm_atomic_helper_damage_iter_init() and > drm_atomic_helper_damage_merged(). That's a bit of churn, but more > readable code. > Makes sense. I'll do that in v2. > Best regards > Thomas >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi > > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: [...] >> struct drm_rect src; >> memset(iter, 0, sizeof(*iter)); >> @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, >> iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF); >> iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF); >> >> - if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) { >> + if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) || >> + (buffer_damage && old_state->fb != state->fb)) { > > I'd assume that this change effectivly disables damage handling. AFAICT > user space often does a page flip with a new framebuffer plus damage > data. Now, with each change of the framebuffer we ignore the damage > information. It's not a blocker as that's the behavior before 6.4, but > we should be aware of it. > Yes, which is the goal of this patch since page flip with a new framebuffer attached to a plane plus damage information can't be supported by drivers that do per-buffer uploads. This was causing some weston and wlroots to have flickering artifacts, due the framebuffers being changed since the last plane update. For now it was decided with Sima, Simon and Pekka that is the best we can do and the reason why I add a TODO in patch #6.
Hi Am 14.11.23 um 16:58 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas, > > Thanks a lot for your feedback. > >> Hi >> >> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: >>> To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather >>> than per-plane uploads), since these type of drivers need to handle buffer >>> damages instead of frame damages. >>> >>> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than >>> drm_atomic_helper_damage_iter_init() but it also takes into account if the >>> framebuffer attached to plane's state has changed since the last update. >>> >>> And the drm_atomic_helper_buffer_damage_merged() is just a version of the >>> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper >>> that is mentioned above. >>> >>> Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") >>> Cc: <stable@vger.kernel.org> # v6.4+ >>> Reported-by: nerdopolis <bluescreen_avenger@verizon.net> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 >>> Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- >>> include/drm/drm_damage_helper.h | 7 +++ >>> 2 files changed, 80 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c >>> index aa2325567918..b72062c9d31c 100644 >>> --- a/drivers/gpu/drm/drm_damage_helper.c >>> +++ b/drivers/gpu/drm/drm_damage_helper.c >>> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); >>> static void >>> __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, >>> const struct drm_plane_state *old_state, >>> - const struct drm_plane_state *state) >>> + const struct drm_plane_state *state, >>> + bool buffer_damage) >> >> I think it would be preferable to drop patches one and two and instead >> add this parameter directly to drm_atomic_helper_damage_iter_init() and >> drm_atomic_helper_damage_merged(). That's a bit of churn, but more >> readable code. >> > > Makes sense. I'll do that in v2. Instead of modifying these function interfaces, it might be even better to introduce a state flag in struct drm_plane_state that you can modify in the plane's atomic_check helper. Something simple like this: if (old_fb != new_fb) plane_state->ignore_damage_clips = true; in the affected drivers/planes. In drm_atomic_helper_damage_iter_init() you can use it to generate a full update. This avoids the churn and is in line with the overall check/commit design of the DRM framework. Best regards Thomas > >> Best regards >> Thomas >> >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi > > Am 14.11.23 um 16:58 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >> Hello Thomas, >> >> Thanks a lot for your feedback. >> >>> Hi >>> >>> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: >>>> To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather >>>> than per-plane uploads), since these type of drivers need to handle buffer >>>> damages instead of frame damages. >>>> >>>> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than >>>> drm_atomic_helper_damage_iter_init() but it also takes into account if the >>>> framebuffer attached to plane's state has changed since the last update. >>>> >>>> And the drm_atomic_helper_buffer_damage_merged() is just a version of the >>>> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper >>>> that is mentioned above. >>>> >>>> Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") >>>> Cc: <stable@vger.kernel.org> # v6.4+ >>>> Reported-by: nerdopolis <bluescreen_avenger@verizon.net> >>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 >>>> Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>>> --- >>>> >>>> drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- >>>> include/drm/drm_damage_helper.h | 7 +++ >>>> 2 files changed, 80 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c >>>> index aa2325567918..b72062c9d31c 100644 >>>> --- a/drivers/gpu/drm/drm_damage_helper.c >>>> +++ b/drivers/gpu/drm/drm_damage_helper.c >>>> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); >>>> static void >>>> __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, >>>> const struct drm_plane_state *old_state, >>>> - const struct drm_plane_state *state) >>>> + const struct drm_plane_state *state, >>>> + bool buffer_damage) >>> >>> I think it would be preferable to drop patches one and two and instead >>> add this parameter directly to drm_atomic_helper_damage_iter_init() and >>> drm_atomic_helper_damage_merged(). That's a bit of churn, but more >>> readable code. >>> >> >> Makes sense. I'll do that in v2. > > Instead of modifying these function interfaces, it might be even better > to introduce a state flag in struct drm_plane_state that you can modify > in the plane's atomic_check helper. Something simple like this: > > if (old_fb != new_fb) > plane_state->ignore_damage_clips = true; > > in the affected drivers/planes. In drm_atomic_helper_damage_iter_init() > you can use it to generate a full update. This avoids the churn and is > in line with the overall check/commit design of the DRM framework. > Thanks. That indeed seems more aligned with the rest of the DRM framework.
On Tue, 14 Nov 2023 17:05:12 +0100 Javier Martinez Canillas <javierm@redhat.com> wrote: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > > Hi > > > > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: > > [...] > > >> struct drm_rect src; > >> memset(iter, 0, sizeof(*iter)); > >> @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, > >> iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF); > >> iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF); > >> > >> - if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) { > >> + if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) || > >> + (buffer_damage && old_state->fb != state->fb)) { > > > > I'd assume that this change effectivly disables damage handling. AFAICT > > user space often does a page flip with a new framebuffer plus damage > > data. Now, with each change of the framebuffer we ignore the damage > > information. It's not a blocker as that's the behavior before 6.4, but > > we should be aware of it. > > > > Yes, which is the goal of this patch since page flip with a new framebuffer > attached to a plane plus damage information can't be supported by drivers > that do per-buffer uploads. > > This was causing some weston and wlroots to have flickering artifacts, due > the framebuffers being changed since the last plane update. > > For now it was decided with Sima, Simon and Pekka that is the best we can > do and the reason why I add a TODO in patch #6. > Hi all, this made me thinking... The per-buffer damage accumulation that would be needed is per upload-buffer, not per KMS FB from userspace. So it should not make any difference whether userspace flips to another FB or not, the damage will need to be accumulated per upload-buffer anyway if the driver is flipping upload-buffers, in order to make the upload-buffer fully up-to-date. Why was that not more broken than it already was? Is there a fixed 1:1 relationship between userspace KMS FBs and the driver upload-buffers? Userspace is already taking care that the KMS FB is always fully up-to-date, FWIW, so the kernel can certainly read areas outside of FB_DAMAGE_CLIPS if it wants to. Thanks, pq
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index aa2325567918..b72062c9d31c 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb); static void __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, const struct drm_plane_state *old_state, - const struct drm_plane_state *state) + const struct drm_plane_state *state, + bool buffer_damage) { struct drm_rect src; memset(iter, 0, sizeof(*iter)); @@ -223,7 +224,8 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF); iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF); - if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) { + if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src) || + (buffer_damage && old_state->fb != state->fb)) { iter->clips = NULL; iter->num_clips = 0; iter->full_update = true; @@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, * update). Currently this iterator returns full plane src in case plane src * changed but that can be changed in future to return damage. * + * Note that this helper is for drivers that do per-plane uploads and expect + * to handle frame damages. Drivers that do per-buffer uploads instead should + * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages. + * * For the case when plane is not visible or plane update should not happen the * first call to iter_next will return false. Note that this helper use clipped * &drm_plane_state.src, so driver calling this helper should have called @@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, const struct drm_plane_state *old_state, const struct drm_plane_state *state) { - __drm_atomic_helper_damage_iter_init(iter, old_state, state); + __drm_atomic_helper_damage_iter_init(iter, old_state, state, false); } EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init); +/** + * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator. + * @iter: The iterator to initialize. + * @old_state: Old plane state for validation. + * @state: Plane state from which to iterate the damage clips. + * + * Initialize an iterator, which clips buffer damage + * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator + * returns full plane src in case buffer damage is not present because user-space + * didn't sent, driver discarded it (it want to do full plane update) or the plane + * @state has an attached framebuffer that is different than the one in @state (it + * has changed since the last plane update). + * + * For the case when plane is not visible or plane update should not happen the + * first call to iter_next will return false. Note that this helper use clipped + * &drm_plane_state.src, so driver calling this helper should have called + * drm_atomic_helper_check_plane_state() earlier. + */ +void +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, + const struct drm_plane_state *old_state, + const struct drm_plane_state *state) +{ + __drm_atomic_helper_damage_iter_init(iter, old_state, state, true); +} +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init); + /** * drm_atomic_helper_damage_iter_next - Advance the damage iterator. * @iter: The iterator to advance. @@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next); static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, struct drm_plane_state *state, - struct drm_rect *rect) + struct drm_rect *rect, + bool buffer_damage) { struct drm_atomic_helper_damage_iter iter; struct drm_rect clip; @@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ rect->x2 = 0; rect->y2 = 0; - drm_atomic_helper_damage_iter_init(&iter, old_state, state); + __drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage); drm_atomic_for_each_plane_damage(&iter, &clip) { rect->x1 = min(rect->x1, clip.x1); rect->y1 = min(rect->y1, clip.y1); @@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_ * For details see: drm_atomic_helper_damage_iter_init() and * drm_atomic_helper_damage_iter_next(). * + * Note that this helper is for drivers that do per-plane uploads and expect + * to handle frame damages. Drivers that do per-buffer uploads instead should + * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages. + * * Returns: * True if there is valid plane damage otherwise false. */ @@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, struct drm_plane_state *state, struct drm_rect *rect) { - return __drm_atomic_helper_damage_merged(old_state, state, rect); + return __drm_atomic_helper_damage_merged(old_state, state, rect, false); } EXPORT_SYMBOL(drm_atomic_helper_damage_merged); + +/** + * drm_atomic_helper_buffer_damage_merged - Merged buffer damage + * @old_state: Old plane state for validation. + * @state: Plane state from which to iterate the damage clips. + * @rect: Returns the merged buffer damage rectangle + * + * This function merges any valid buffer damage clips into one rectangle and + * returns it in @rect. It checks if the framebuffers attached to @old_state + * and @state are the same. If that is not the case then the returned damage + * rectangle is the &drm_plane_state.src, since a full update should happen. + * + * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that + * full plane update should happen. It also ensure helper iterator will return + * &drm_plane_state.src as damage. + * + * For details see: drm_atomic_helper_buffer_damage_iter_init() and + * drm_atomic_helper_damage_iter_next(). + * + * Returns: + * True if there is valid buffer damage otherwise false. + */ +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, + struct drm_plane_state *state, + struct drm_rect *rect) +{ + return __drm_atomic_helper_damage_merged(old_state, state, rect, true); +} +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged); diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h index effda42cce31..328bb249d68f 100644 --- a/include/drm/drm_damage_helper.h +++ b/include/drm/drm_damage_helper.h @@ -74,11 +74,18 @@ void drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, const struct drm_plane_state *old_state, const struct drm_plane_state *new_state); +void +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter, + const struct drm_plane_state *old_state, + const struct drm_plane_state *new_state); bool drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter, struct drm_rect *rect); bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state, struct drm_plane_state *state, struct drm_rect *rect); +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state, + struct drm_plane_state *state, + struct drm_rect *rect); #endif
To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather than per-plane uploads), since these type of drivers need to handle buffer damages instead of frame damages. The drm_atomic_helper_buffer_damage_iter_init() has the same logic than drm_atomic_helper_damage_iter_init() but it also takes into account if the framebuffer attached to plane's state has changed since the last update. And the drm_atomic_helper_buffer_damage_merged() is just a version of the drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper that is mentioned above. Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane") Cc: <stable@vger.kernel.org> # v6.4+ Reported-by: nerdopolis <bluescreen_avenger@verizon.net> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115 Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++--- include/drm/drm_damage_helper.h | 7 +++ 2 files changed, 80 insertions(+), 6 deletions(-)