Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Thomas Zimmermann]

Hi

Am 14.11.23 um 16:58 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxxxxx> # v6.4+
> > > Reported-by: nerdopolis <bluescreen_avenger@xxxxxxxxxxx>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
> > > Suggested-by: Sima Vetter <daniel.vetter@xxxxxxxx>
> > > Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> > > ---
> > >
> > >    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
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature