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

[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 at suse.de>
> ---
>   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 at ideasonboard.com>

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

  Tomi