[01/10] drm/atomic: Add __drm_atomic_helper_plane_reset
diff mbox

Message ID 20180720211509.23605-2-alexandru-cosmin.gheorghe@arm.com
State Not Applicable
Headers show

Commit Message

Alexandru-Cosmin Gheorghe July 20, 2018, 9:15 p.m. UTC
There are a lot of drivers that subclass drm_plane_state, all of them
duplicate the code that links toghether the plane with plane_state.

On top of that, drivers that enable core properties also have to
duplicate the code for initializing the properties to their default
values, which in all cases are the same as the defaults from core.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 25 insertions(+), 9 deletions(-)

Comments

Ayan Halder July 23, 2018, 9:37 a.m. UTC | #1
On Fri, Jul 20, 2018 at 10:15:00PM +0100, Alexandru Gheorghe wrote:
> There are a lot of drivers that subclass drm_plane_state, all of them
> duplicate the code that links toghether the plane with plane_state.
> 
> On top of that, drivers that enable core properties also have to
> duplicate the code for initializing the properties to their default
> values, which in all cases are the same as the defaults from core.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8008a7de2e10..e1c6f101652e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
>  
> +/**
> + * __drm_atomic_helper_plane_reset - resets planes state to default values
> + * @plane: plane object
> + * @new_state: atomic plane state
> + *
> + * Initializes plane state to default. This is useful for drivers that subclass
> + * the plane state.
> + */
> +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> +				     struct drm_plane_state *state)
> +{
> +	if (state) {
> +		state->plane = plane;
> +		state->rotation = DRM_MODE_ROTATE_0;
> +		/* Reset the alpha value to fully opaque if it matters */
> +		if (plane->alpha_property)
> +			state->alpha = plane->alpha_property->values[1];
> +	}
> +	plane->state = state;
> +}
> +EXPORT_SYMBOL(__drm_atomic_helper_plane_reset);
> +
>  /**
>   * drm_atomic_helper_plane_reset - default &drm_plane_funcs.reset hook for planes
>   * @plane: drm plane
> @@ -3521,15 +3543,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>  
>  	kfree(plane->state);
>  	plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
> -
> -	if (plane->state) {
> -		plane->state->plane = plane;
> -		plane->state->rotation = DRM_MODE_ROTATE_0;
> -
> -		/* Reset the alpha value to fully opaque if it matters */
> -		if (plane->alpha_property)
> -			plane->state->alpha = plane->alpha_property->values[1];
> -	}
> +	__drm_atomic_helper_plane_reset(plane, plane->state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 26aaba58d6ce..2dd40c761dfd 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -155,6 +155,8 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state);
>  void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					  struct drm_crtc_state *state);
>  
> +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> +				     struct drm_plane_state *state);
>  void drm_atomic_helper_plane_reset(struct drm_plane *plane);
>  void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  					       struct drm_plane_state *state);
> -- 
> 2.18.0
> 
With my limited knowledge on DRM, this looks OK to me:-
Reviewed-by :- Ayan Kumar halder <ayan.halder@arm.com>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel July 24, 2018, 7:48 a.m. UTC | #2
Hi Alexandru,

On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
> There are a lot of drivers that subclass drm_plane_state, all of them
> duplicate the code that links toghether the plane with plane_state.
> 
> On top of that, drivers that enable core properties also have to
> duplicate the code for initializing the properties to their default
> values, which in all cases are the same as the defaults from core.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8008a7de2e10..e1c6f101652e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
>  
> +/**
> + * __drm_atomic_helper_plane_reset - resets planes state to default values
> + * @plane: plane object
> + * @new_state: atomic plane state
> + *
> + * Initializes plane state to default. This is useful for drivers that subclass
> + * the plane state.
> + */
> +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> +				     struct drm_plane_state *state)
> +{
> +	if (state) {
> +		state->plane = plane;
> +		state->rotation = DRM_MODE_ROTATE_0;
> +		/* Reset the alpha value to fully opaque if it matters */
> +		if (plane->alpha_property)
> +			state->alpha = plane->alpha_property->values[1];
> +	}

Is this function supposed to be callable with state == NULL ?

> +	plane->state = state;

If so, the comment could mention that this sets plane->state to NULL if
state == NULL, and a few of the call sites could be simplified.

If not, I would remove the conditional if (state) {} part and also
mention this in the comment.

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandru-Cosmin Gheorghe July 24, 2018, 8:14 a.m. UTC | #3
On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
> Hi Alexandru,
> 
> On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
> > There are a lot of drivers that subclass drm_plane_state, all of them
> > duplicate the code that links toghether the plane with plane_state.
> > 
> > On top of that, drivers that enable core properties also have to
> > duplicate the code for initializing the properties to their default
> > values, which in all cases are the same as the defaults from core.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  2 files changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 8008a7de2e10..e1c6f101652e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
> >  
> > +/**
> > + * __drm_atomic_helper_plane_reset - resets planes state to default values
> > + * @plane: plane object
> > + * @new_state: atomic plane state
> > + *
> > + * Initializes plane state to default. This is useful for drivers that subclass
> > + * the plane state.
> > + */
> > +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> > +				     struct drm_plane_state *state)
> > +{
> > +	if (state) {
> > +		state->plane = plane;
> > +		state->rotation = DRM_MODE_ROTATE_0;
> > +		/* Reset the alpha value to fully opaque if it matters */
> > +		if (plane->alpha_property)
> > +			state->alpha = plane->alpha_property->values[1];
> > +	}
> 
> Is this function supposed to be callable with state == NULL ?
> 
> > +	plane->state = state;
> 
> If so, the comment could mention that this sets plane->state to NULL if
> state == NULL, and a few of the call sites could be simplified.
> 
> If not, I would remove the conditional if (state) {} part and also
> mention this in the comment.

Yes, It's intended to be callable with null. I will update the
comment.

> 
> regards
> Philipp
Boris Brezillon July 24, 2018, 8:16 a.m. UTC | #4
On Tue, 24 Jul 2018 09:14:02 +0100
Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> wrote:

> On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
> > Hi Alexandru,
> > 
> > On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:  
> > > There are a lot of drivers that subclass drm_plane_state, all of them
> > > duplicate the code that links toghether the plane with plane_state.
> > > 
> > > On top of that, drivers that enable core properties also have to
> > > duplicate the code for initializing the properties to their default
> > > values, which in all cases are the same as the defaults from core.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
> > >  include/drm/drm_atomic_helper.h     |  2 ++
> > >  2 files changed, 25 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 8008a7de2e10..e1c6f101652e 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
> > >  
> > > +/**
> > > + * __drm_atomic_helper_plane_reset - resets planes state to default values
> > > + * @plane: plane object
> > > + * @new_state: atomic plane state
> > > + *
> > > + * Initializes plane state to default. This is useful for drivers that subclass
> > > + * the plane state.
> > > + */
> > > +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> > > +				     struct drm_plane_state *state)
> > > +{
> > > +	if (state) {
> > > +		state->plane = plane;
> > > +		state->rotation = DRM_MODE_ROTATE_0;
> > > +		/* Reset the alpha value to fully opaque if it matters */
> > > +		if (plane->alpha_property)
> > > +			state->alpha = plane->alpha_property->values[1];
> > > +	}  
> > 
> > Is this function supposed to be callable with state == NULL ?
> >   
> > > +	plane->state = state;  
> > 
> > If so, the comment could mention that this sets plane->state to NULL if
> > state == NULL, and a few of the call sites could be simplified.
> > 
> > If not, I would remove the conditional if (state) {} part and also
> > mention this in the comment.  
> 
> Yes, It's intended to be callable with null.

May I ask why? I'd assume drivers that call this function to pass a
non-NULL plane state. What's the use case for passing a NULL state here?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandru-Cosmin Gheorghe July 24, 2018, 9:28 a.m. UTC | #5
On Tue, Jul 24, 2018 at 10:16:56AM +0200, Boris Brezillon wrote:
> On Tue, 24 Jul 2018 09:14:02 +0100
> Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> wrote:
> 
> > On Tue, Jul 24, 2018 at 09:48:53AM +0200, Philipp Zabel wrote:
> > > Hi Alexandru,
> > > 
> > > On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:  
> > > > There are a lot of drivers that subclass drm_plane_state, all of them
> > > > duplicate the code that links toghether the plane with plane_state.
> > > > 
> > > > On top of that, drivers that enable core properties also have to
> > > > duplicate the code for initializing the properties to their default
> > > > values, which in all cases are the same as the defaults from core.
> > > > 
> > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++--------
> > > >  include/drm/drm_atomic_helper.h     |  2 ++
> > > >  2 files changed, 25 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 8008a7de2e10..e1c6f101652e 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -3507,6 +3507,28 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
> > > >  }
> > > >  EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
> > > >  
> > > > +/**
> > > > + * __drm_atomic_helper_plane_reset - resets planes state to default values
> > > > + * @plane: plane object
> > > > + * @new_state: atomic plane state
> > > > + *
> > > > + * Initializes plane state to default. This is useful for drivers that subclass
> > > > + * the plane state.
> > > > + */
> > > > +void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
> > > > +				     struct drm_plane_state *state)
> > > > +{
> > > > +	if (state) {
> > > > +		state->plane = plane;
> > > > +		state->rotation = DRM_MODE_ROTATE_0;
> > > > +		/* Reset the alpha value to fully opaque if it matters */
> > > > +		if (plane->alpha_property)
> > > > +			state->alpha = plane->alpha_property->values[1];
> > > > +	}  
> > > 
> > > Is this function supposed to be callable with state == NULL ?
> > >   
> > > > +	plane->state = state;  
> > > 
> > > If so, the comment could mention that this sets plane->state to NULL if
> > > state == NULL, and a few of the call sites could be simplified.
> > > 
> > > If not, I would remove the conditional if (state) {} part and also
> > > mention this in the comment.  
> > 
> > Yes, It's intended to be callable with null.
> 
> May I ask why? I'd assume drivers that call this function to pass a
> non-NULL plane state. What's the use case for passing a NULL state here?

Drivers check it, drm_atomic_helper_plane_reset doesn't.
Looking at the existing __drm_atomic_helper_plane_* it seems they all
assume state not null, so I think it probably makes more sense to do
that for this function as well.

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8008a7de2e10..e1c6f101652e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3507,6 +3507,28 @@  void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
 
+/**
+ * __drm_atomic_helper_plane_reset - resets planes state to default values
+ * @plane: plane object
+ * @new_state: atomic plane state
+ *
+ * Initializes plane state to default. This is useful for drivers that subclass
+ * the plane state.
+ */
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
+				     struct drm_plane_state *state)
+{
+	if (state) {
+		state->plane = plane;
+		state->rotation = DRM_MODE_ROTATE_0;
+		/* Reset the alpha value to fully opaque if it matters */
+		if (plane->alpha_property)
+			state->alpha = plane->alpha_property->values[1];
+	}
+	plane->state = state;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_plane_reset);
+
 /**
  * drm_atomic_helper_plane_reset - default &drm_plane_funcs.reset hook for planes
  * @plane: drm plane
@@ -3521,15 +3543,7 @@  void drm_atomic_helper_plane_reset(struct drm_plane *plane)
 
 	kfree(plane->state);
 	plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
-
-	if (plane->state) {
-		plane->state->plane = plane;
-		plane->state->rotation = DRM_MODE_ROTATE_0;
-
-		/* Reset the alpha value to fully opaque if it matters */
-		if (plane->alpha_property)
-			plane->state->alpha = plane->alpha_property->values[1];
-	}
+	__drm_atomic_helper_plane_reset(plane, plane->state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 26aaba58d6ce..2dd40c761dfd 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -155,6 +155,8 @@  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state);
 void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					  struct drm_crtc_state *state);
 
+void __drm_atomic_helper_plane_reset(struct drm_plane *plane,
+				     struct drm_plane_state *state);
 void drm_atomic_helper_plane_reset(struct drm_plane *plane);
 void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 					       struct drm_plane_state *state);