Date:   Tue, 11 Oct 2022 15:21:10 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 3/3] drm/crtc-helper: Add a drm_crtc_helper_atomic_check()
 helper

Hi

Am 10.10.22 um 19:02 schrieb Javier Martinez Canillas:
> Provides a default CRTC state check handler for CRTCs that only have one
> primary plane attached.
> 
> There are some drivers that duplicate this logic in their helpers, such as
> simpledrm and ssd130x. Factor out this common code into a CRTC helper and
> make drivers use it.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>

There really isn't much here for now. I suspect that there are more 
drivers that could use this helper. If you merge this before ofdrm, I'll 
rebase ofdrm on top.

Please also see my comment below.


> ---
> 
>   drivers/gpu/drm/drm_crtc_helper.c | 24 ++++++++++++++++++++++++
>   drivers/gpu/drm/solomon/ssd130x.c | 14 ++------------
>   drivers/gpu/drm/tiny/simpledrm.c  | 14 ++------------
>   include/drm/drm_crtc_helper.h     |  2 ++
>   4 files changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 457448cc60f7..4ad3abaa98f4 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -421,6 +421,30 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>   }
>   EXPORT_SYMBOL(drm_crtc_helper_set_mode);
>   
> +/**
> + * drm_crtc_helper_atomic_check() - Helper to check CRTC atomic-state
> + * @crtc: CRTC to check
> + * @state: atomic state object
> + *
> + * Provides a default CRTC-state check handler for CRTCs that only have
> + * one primary plane attached to it.
> + *
> + * This is often the case for the CRTC of simple framebuffers.

I'd add a reference to drm_plane_helper_atomic_check() to this 
paragraph. Like

   See drm_plane_helper_atomic_check() for the respective plane helpers.

And also reference back from the plane-check helper to the CRTC-check 
helper.

Best regards
Thomas

> + *
> + * RETURNS:
> + * Zero on success, or an errno code otherwise.
> + */
> +int drm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	if (!new_crtc_state->enable)
> +		return 0;
> +
> +	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_atomic_check);
> +
>   static void
>   drm_crtc_helper_disable(struct drm_crtc *crtc)
>   {
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0d4ab65233db..f2795f90ea69 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -20,6 +20,7 @@
>   
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_edid.h>
>   #include <drm/drm_fb_helper.h>
> @@ -645,17 +646,6 @@ static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc
>   	return MODE_OK;
>   }
>   
> -static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
> -					    struct drm_atomic_state *new_state)
> -{
> -	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -
> -	if (!new_crtc_state->enable)
> -		return 0;
> -
> -	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -}
> -
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -663,7 +653,7 @@ static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
>    */
>   static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
>   	.mode_valid = ssd130x_crtc_helper_mode_valid,
> -	.atomic_check = ssd130x_crtc_helper_atomic_check,
> +	.atomic_check = drm_crtc_helper_atomic_check,
>   };
>   
>   static void ssd130x_crtc_reset(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index f03f17f62a56..cbb100753154 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -11,6 +11,7 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_connector.h>
> +#include <drm/drm_crtc_helper.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_device.h>
>   #include <drm/drm_drv.h>
> @@ -545,17 +546,6 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
>   	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &sdev->mode);
>   }
>   
> -static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> -					      struct drm_atomic_state *new_state)
> -{
> -	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> -
> -	if (!new_crtc_state->enable)
> -		return 0;
> -
> -	return drm_atomic_helper_check_crtc_primary_plane(new_crtc_state);
> -}
> -
>   /*
>    * The CRTC is always enabled. Screen updates are performed by
>    * the primary plane's atomic_update function. Disabling clears
> @@ -563,7 +553,7 @@ static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
>    */
>   static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = {
>   	.mode_valid = simpledrm_crtc_helper_mode_valid,
> -	.atomic_check = simpledrm_crtc_helper_atomic_check,
> +	.atomic_check = drm_crtc_helper_atomic_check,
>   };
>   
>   static const struct drm_crtc_funcs simpledrm_crtc_funcs = {
> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> index a6d520d5b6ca..1840db247f69 100644
> --- a/include/drm/drm_crtc_helper.h
> +++ b/include/drm/drm_crtc_helper.h
> @@ -50,6 +50,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>   			      struct drm_display_mode *mode,
>   			      int x, int y,
>   			      struct drm_framebuffer *old_fb);
> +int drm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> +				 struct drm_atomic_state *state);
>   bool drm_helper_crtc_in_use(struct drm_crtc *crtc);
>   bool drm_helper_encoder_in_use(struct drm_encoder *encoder);
>   

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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists