Re: [PATCH 6/6] drm/tidss: Implement struct drm_plane_helper_funcs.atomic_enable

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 17.02.23 um 15:42 schrieb Tomi Valkeinen:
> On 09/02/2023 17:41, Thomas Zimmermann wrote:
> > Enable the primary plane for tidss hardware via atomic_enable.
> > Atomic helpers invoke this callback only when the plane becomes
> > active.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > ---
> >   drivers/gpu/drm/tidss/tidss_plane.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tidss/tidss_plane.c 
> > b/drivers/gpu/drm/tidss/tidss_plane.c
> > index 0b12405edb47..6bdd6e4a955a 100644
> > --- a/drivers/gpu/drm/tidss/tidss_plane.c
> > +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> > @@ -124,6 +124,16 @@ static void tidss_plane_atomic_update(struct 
> > drm_plane *plane,
> >       hw_videoport = to_tidss_crtc(new_state->crtc)->hw_videoport;
> >       dispc_plane_setup(tidss->dispc, tplane->hw_plane_id, new_state, 
> > hw_videoport);
> > +}
> > +
> > +static void tidss_plane_atomic_enable(struct drm_plane *plane,
> > +                      struct drm_atomic_state *state)
> > +{
> > +    struct drm_device *ddev = plane->dev;
> > +    struct tidss_device *tidss = to_tidss(ddev);
> > +    struct tidss_plane *tplane = to_tidss_plane(plane);
> > +
> > +    dev_dbg(ddev->dev, "%s\n", __func__);
> >       dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, true);
> >   }
> > @@ -151,6 +161,7 @@ static void drm_plane_destroy(struct drm_plane 
> > *plane)
> >   static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
> >       .atomic_check = tidss_plane_atomic_check,
> >       .atomic_update = tidss_plane_atomic_update,
> > +    .atomic_enable = tidss_plane_atomic_enable,
> >       .atomic_disable = tidss_plane_atomic_disable,
> >   };
>
> I haven't tested this, but looks fine to me.
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>
> One thought, though, is that we still do dispc_plane_enable(false) in 
> tidss_plane_atomic_update() when the plane is not visible. Not a 
> problem, but it would be nice to only enable/disable the plane inside 
> atomic_enable/disable.
>
> Or maybe in cases like this the driver should only use atomic_update, 
> and do all the enabling and disabling there...

I agree. Drivers that have complex enable/disable semantics should 
probably handle everything in atomic_update.

Enabling/disabling is currently connected to the plane's framebuffer. As 
you said, it would be nice if this could be tied to visibility instead. 
The patch would be trivial, but some drivers might not like the change. 
I guess we could do an RFC patch and gather opinions.

Best regards
Thomas

>   Tomi

--
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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature