[PATCH 00/10] Add helper for plane reset

[PATCH 00/10] Add helper for plane reset

[Alexandru Gheorghe]

Drivers that subclass drm_plane need to copy the logic for linking the
drm_plane with its state and to initialize core properties to their
default values. E.g (alpha and rotation)

Having a helper to reset the plane_state makes sense because of multiple
reasons:
1. Eliminate code duplication.
2. Add a single place for initializing core properties to their
default values, no need for driver to do it if what the helper sets
makes sense for them.
3. No need to debug the driver when you enable a core property and
observe it doesn't have a proper default value.

Tested with mali-dp the other drivers are just built-tested.


Alexandru Gheorghe (10):
  drm/atomic: Add  __drm_atomic_helper_plane_reset
  drm/amd/display: Use __drm_atomic_helper_plane_reset instead of
    copying the logic
  drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying
    the logic
  drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of
    copying the logic
  drm/exynos: Use __drm_atomic_helper_plane_reset instead of copying the
    logic
  drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the
    logic
  drm: rcar-du: Use __drm_atomic_helper_plane_reset instead of copying
    the logic
  drm/sun4i: Use __drm_atomic_helper_plane_reset instead of copying the
    logic
  drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the
    logic
  drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the
    logic

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  7 ++--
 drivers/gpu/drm/arm/malidp_planes.c           |  7 ++--
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   |  5 +--
 drivers/gpu/drm/drm_atomic_helper.c           | 32 +++++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_plane.c     |  3 +-
 drivers/gpu/drm/imx/ipuv3-plane.c             |  8 ++---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c       |  4 +--
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c         |  4 +--
 drivers/gpu/drm/sun4i/sun4i_layer.c           |  4 +--
 drivers/gpu/drm/vc4/vc4_plane.c               |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |  4 +--
 include/drm/drm_atomic_helper.h               |  2 ++
 12 files changed, 39 insertions(+), 45 deletions(-)

-- 
2.18.0

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Alexandru Gheorghe]

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

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Ayan Halder]

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Philipp Zabel]

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

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Alexandru-Cosmin Gheorghe]

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

-- 
Cheers,
Alex G

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Boris Brezillon]

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?

[PATCH 01/10] drm/atomic: Add __drm_atomic_helper_plane_reset

[Alexandru-Cosmin Gheorghe]

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.

-- 
Cheers,
Alex G

[PATCH 02/10] drm/amd/display: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ca017c1dd4da..c08157686782 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3013,11 +3013,8 @@ static void dm_drm_plane_reset(struct drm_plane *plane)
 	amdgpu_state = kzalloc(sizeof(*amdgpu_state), GFP_KERNEL);
 	WARN_ON(amdgpu_state == NULL);
 	
-	if (amdgpu_state) {
-		plane->state = &amdgpu_state->base;
-		plane->state->plane = plane;
-		plane->state->rotation = DRM_MODE_ROTATE_0;
-	}
+	if (amdgpu_state)
+		__drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
 }
 
 static struct drm_plane_state *
-- 
2.18.0

[PATCH 03/10] drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/arm/malidp_planes.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 29409a65d864..49c37f6dd63e 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane)
 	kfree(state);
 	plane->state = NULL;
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state) {
-		state->base.plane = plane;
-		state->base.rotation = DRM_MODE_ROTATE_0;
-		plane->state = &state->base;
-	}
+	if (state)
+		__drm_atomic_helper_plane_reset(plane, &state->base);
 }
 
 static struct
-- 
2.18.0

[PATCH 03/10] drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Ayan Halder]

On Fri, Jul 20, 2018 at 10:15:02PM +0100, Alexandru Gheorghe wrote:
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_planes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 29409a65d864..49c37f6dd63e 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane)
>  	kfree(state);
>  	plane->state = NULL;
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (state) {
> -		state->base.plane = plane;
> -		state->base.rotation = DRM_MODE_ROTATE_0;
> -		plane->state = &state->base;
> -	}
> +	if (state)
> +		__drm_atomic_helper_plane_reset(plane, &state->base);
>  }
>  
>  static struct
> -- 
> 2.18.0
>
A commit description like the following might be useful (though the
patch is a part of series) :-
Invoke a newly added drm atomic helper function
(__drm_atomic_helper_plane_reset) to reset the 'drm_plane_state' to
its default values for rotation, alpha and plane.

Otherwise, 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/10] drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Liviu Dudau]

On Fri, Jul 20, 2018 at 10:15:02PM +0100, Alexandru Gheorghe wrote:

With a more detailed commit message:

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_planes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index 29409a65d864..49c37f6dd63e 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -78,11 +78,8 @@ static void malidp_plane_reset(struct drm_plane *plane)
>  	kfree(state);
>  	plane->state = NULL;
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (state) {
> -		state->base.plane = plane;
> -		state->base.rotation = DRM_MODE_ROTATE_0;
> -		plane->state = &state->base;
> -	}
> +	if (state)
> +		__drm_atomic_helper_plane_reset(plane, &state->base);
>  }
>  
>  static struct
> -- 
> 2.18.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH 04/10] drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 04440064b9b7..9330a076e15a 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -942,10 +942,7 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p)
 				"Failed to allocate initial plane state\n");
 			return;
 		}
-
-		p->state = &state->base;
-		p->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
-		p->state->plane = p;
+		__drm_atomic_helper_plane_reset(p, &state->base);
 	}
 }
 
-- 
2.18.0

[PATCH 04/10] drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Boris Brezillon]

Hi Alexandru,

On Fri, 20 Jul 2018 22:15:03 +0100
Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> wrote:

Please add a commit message here (even if it's just repeating what the
subject says).

> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

With this addressed:

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index 04440064b9b7..9330a076e15a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -942,10 +942,7 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p)
>  				"Failed to allocate initial plane state\n");
>  			return;
>  		}
> -
> -		p->state = &state->base;
> -		p->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> -		p->state->plane = p;
> +		__drm_atomic_helper_plane_reset(p, &state->base);
>  	}
>  }
>  

[PATCH 05/10] drm/exynos: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index eb9915da7dec..681328fbe7de 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -139,8 +139,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
 
 	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
 	if (exynos_state) {
-		plane->state = &exynos_state->base;
-		plane->state->plane = plane;
+		__drm_atomic_helper_plane_reset(plane, &exynos_state->base);
 		plane->state->zpos = exynos_plane->config->zpos;
 	}
 }
-- 
2.18.0

[PATCH 06/10] drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 203f247d4854..1bd4de03ce9e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane)
 		ipu_state = to_ipu_plane_state(plane->state);
 		__drm_atomic_helper_plane_destroy_state(plane->state);
 		kfree(ipu_state);
+		plane->state = NULL;
 	}
 
 	ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
 
-	if (ipu_state) {
-		ipu_state->base.plane = plane;
-		ipu_state->base.rotation = DRM_MODE_ROTATE_0;
-	}
+	if (ipu_state)
+		__drm_atomic_helper_plane_reset(plane, &ipu_state->base);
 
-	plane->state = &ipu_state->base;
 }
 
 static struct drm_plane_state *
-- 
2.18.0

[PATCH 06/10] drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Philipp Zabel]

Hi Alexandru,

On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 203f247d4854..1bd4de03ce9e 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane)
>  		ipu_state = to_ipu_plane_state(plane->state);
>  		__drm_atomic_helper_plane_destroy_state(plane->state);
>  		kfree(ipu_state);
> +		plane->state = NULL;

With __drm_atomic_helper_plane_reset as-is, this could be dropped ...

>  	}
>  
>  	ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
>  
> -	if (ipu_state) {
> -		ipu_state->base.plane = plane;
> -		ipu_state->base.rotation = DRM_MODE_ROTATE_0;
> -	}
> +	if (ipu_state)
> +		__drm_atomic_helper_plane_reset(plane, &ipu_state->base);

... if this is change to just

	__drm_atomic_helper_plane_reset(plane, &ipu_state->base);

>  
> -	plane->state = &ipu_state->base;
>  }
>  
>  static struct drm_plane_state *

regards
Philipp

[PATCH 06/10] drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Philipp Zabel]

On Tue, 2018-07-24 at 09:49 +0200, Philipp Zabel wrote:
> Hi Alexandru,
> 
> On Fri, 2018-07-20 at 22:15 +0100, Alexandru Gheorghe wrote:
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-plane.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 203f247d4854..1bd4de03ce9e 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -281,16 +281,14 @@ static void ipu_plane_state_reset(struct drm_plane *plane)
> >  		ipu_state = to_ipu_plane_state(plane->state);
> >  		__drm_atomic_helper_plane_destroy_state(plane->state);
> >  		kfree(ipu_state);
> > +		plane->state = NULL;
> 
> With __drm_atomic_helper_plane_reset as-is, this could be dropped ...
> 
> >  	}
> >  
> >  	ipu_state = kzalloc(sizeof(*ipu_state), GFP_KERNEL);
> >  
> > -	if (ipu_state) {
> > -		ipu_state->base.plane = plane;
> > -		ipu_state->base.rotation = DRM_MODE_ROTATE_0;
> > -	}
> > +	if (ipu_state)
> > +		__drm_atomic_helper_plane_reset(plane, &ipu_state->base);
> 
> ... if this is change to just
> 
> 	__drm_atomic_helper_plane_reset(plane, &ipu_state->base);

Euh, obviously that won't be possible, sorry for the noise.
I blame the heat.

regards
Philipp

[PATCH 07/10] drm: rcar-du: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 4 +---
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index c20f7ed48c8d..19a9d5f6db1c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -689,15 +689,13 @@ static void rcar_du_plane_reset(struct drm_plane *plane)
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state == NULL)
 		return;
+	__drm_atomic_helper_plane_reset(plane, &state->state);
 
 	state->hwindex = -1;
 	state->source = RCAR_DU_PLANE_MEMORY;
 	state->colorkey = RCAR_DU_COLORKEY_NONE;
 	state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
 
-	plane->state = &state->state;
-	plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
-	plane->state->plane = plane;
 }
 
 static int rcar_du_plane_atomic_set_property(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 72eebeda518e..0a0aa490f805 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -346,11 +346,9 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane)
 	if (state == NULL)
 		return;
 
-	state->state.alpha = DRM_BLEND_ALPHA_OPAQUE;
+	__drm_atomic_helper_plane_reset(plane, &state->state);
 	state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
 
-	plane->state = &state->state;
-	plane->state->plane = plane;
 }
 
 static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
-- 
2.18.0

[PATCH 08/10] drm/sun4i: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 750ad24de1d7..78f77af8805a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -35,9 +35,7 @@ static void sun4i_backend_layer_reset(struct drm_plane *plane)
 
 	state = kzalloc(sizeof(*state), GFP_KERNEL);
 	if (state) {
-		plane->state = &state->state;
-		plane->state->plane = plane;
-		plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
+		__drm_atomic_helper_plane_reset(plane, &state->state);
 		plane->state->zpos = layer->id;
 	}
 }
-- 
2.18.0

[PATCH 09/10] drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 9d7a36f148cf..688ad9bb0f08 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
 	if (!vc4_state)
 		return;
 
-	plane->state = &vc4_state->base;
-	plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
-	vc4_state->base.plane = plane;
+	__drm_atomic_helper_plane_reset(plane, &vc4_state->base);
 }
 
 static void vc4_dlist_write(struct vc4_plane_state *vc4_state, u32 val)
-- 
2.18.0

[PATCH 09/10] drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Maxime Ripard]

On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/vc4/vc4_plane.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 9d7a36f148cf..688ad9bb0f08 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
>  	if (!vc4_state)
>  		return;
>  
> -	plane->state = &vc4_state->base;
> -	plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> -	vc4_state->base.plane = plane;
> +	__drm_atomic_helper_plane_reset(plane, &vc4_state->base);

For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset
value of alpha from DRM_BLEND_ALPHA_OPAQUE to
plane->alpha_property->values[1].

This might or might not be a good idea, but you should definitely
explain why.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180724/68382632/attachment.sig>

[PATCH 09/10] drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru-Cosmin Gheorghe]

Hi Maxime,

On Tue, Jul 24, 2018 at 08:57:20AM +0200, Maxime Ripard wrote:
> On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_plane.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > index 9d7a36f148cf..688ad9bb0f08 100644
> > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
> >  	if (!vc4_state)
> >  		return;
> >  
> > -	plane->state = &vc4_state->base;
> > -	plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> > -	vc4_state->base.plane = plane;
> > +	__drm_atomic_helper_plane_reset(plane, &vc4_state->base);
> 
> For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset
> value of alpha from DRM_BLEND_ALPHA_OPAQUE to
> plane->alpha_property->values[1].

plane->alpha_property->values[1] holds the max value for alpha, and
it's initialized by the core to DRM_BLEND_ALPHA_OPAQUE.

> 
> This might or might not be a good idea, but you should definitely
> explain why.

Would a commit message suffice? Or do you have other ideas?

> 
> Maxime
> 
> -- 
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
Cheers,
Alex G

[PATCH 09/10] drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Maxime Ripard]

On Tue, Jul 24, 2018 at 09:11:21AM +0100, Alexandru-Cosmin Gheorghe wrote:
> Hi Maxime,
> 
> On Tue, Jul 24, 2018 at 08:57:20AM +0200, Maxime Ripard wrote:
> > On Fri, Jul 20, 2018 at 10:15:08PM +0100, Alexandru Gheorghe wrote:
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_plane.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> > > index 9d7a36f148cf..688ad9bb0f08 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> > > @@ -200,9 +200,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
> > >  	if (!vc4_state)
> > >  		return;
> > >  
> > > -	plane->state = &vc4_state->base;
> > > -	plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
> > > -	vc4_state->base.plane = plane;
> > > +	__drm_atomic_helper_plane_reset(plane, &vc4_state->base);
> > 
> > For vc4, rcar-du, atmel-hlcdc and sun4i, you're changing the reset
> > value of alpha from DRM_BLEND_ALPHA_OPAQUE to
> > plane->alpha_property->values[1].
> 
> plane->alpha_property->values[1] holds the max value for alpha, and
> it's initialized by the core to DRM_BLEND_ALPHA_OPAQUE.
> 
> > 
> > This might or might not be a good idea, but you should definitely
> > explain why.
> 
> Would a commit message suffice? Or do you have other ideas?

Yes, that would be enough.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180724/2848509e/attachment-0001.sig>

[PATCH 10/10] drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Alexandru Gheorghe]

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 466336b34fff..1e0fb3c79b50 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane)
 		return;
 	}
 
-	plane->state = &vps->base;
-	plane->state->plane = plane;
-	plane->state->rotation = DRM_MODE_ROTATE_0;
+	__drm_atomic_helper_plane_reset(plane, &vps->base);
 }
 
 
-- 
2.18.0

[PATCH 10/10] drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Deepak Singh Rawat]

Hi Alexandru,

Thanks for the patch, for the vmwgfx part:

Reviewed-by: Deepak Rawat <drawat@vmware.com>

> 
> Signed-off-by: Alexandru Gheorghe <alexandru-
> cosmin.gheorghe at arm.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 466336b34fff..1e0fb3c79b50 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane)
>  		return;
>  	}
> 
> -	plane->state = &vps->base;
> -	plane->state->plane = plane;
> -	plane->state->rotation = DRM_MODE_ROTATE_0;
> +	__drm_atomic_helper_plane_reset(plane, &vps->base);
>  }
> 
> 
> --
> 2.18.0
> 

[PATCH 10/10] drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the logic

[Sinclair Yeh]

Hello Alex,

other than adding a brief commit message to this patch,

Reviewed-by: Sinclair Yeh <syeh@vmware.com>

On Fri, Jul 20, 2018 at 10:15:09PM +0100, Alexandru Gheorghe wrote:
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 466336b34fff..1e0fb3c79b50 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -720,9 +720,7 @@ void vmw_du_plane_reset(struct drm_plane *plane)
>  		return;
>  	}
>  
> -	plane->state = &vps->base;
> -	plane->state->plane = plane;
> -	plane->state->rotation = DRM_MODE_ROTATE_0;
> +	__drm_atomic_helper_plane_reset(plane, &vps->base);
>  }
>  
>  
> -- 
> 2.18.0
> 

[PATCH 00/10] Add helper for plane reset

[Harry Wentland]

On 2018-07-20 05:14 PM, Alexandru Gheorghe wrote:
> Drivers that subclass drm_plane need to copy the logic for linking the
> drm_plane with its state and to initialize core properties to their
> default values. E.g (alpha and rotation)
> 
> Having a helper to reset the plane_state makes sense because of multiple
> reasons:
> 1. Eliminate code duplication.
> 2. Add a single place for initializing core properties to their
> default values, no need for driver to do it if what the helper sets
> makes sense for them.
> 3. No need to debug the driver when you enable a core property and
> observe it doesn't have a proper default value.
> 
> Tested with mali-dp the other drivers are just built-tested.
> 

For some reason I lost 02/10 hence I'm replying to the cover letter.

Patches 1 & 2 are

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> 
> Alexandru Gheorghe (10):
>   drm/atomic: Add  __drm_atomic_helper_plane_reset
>   drm/amd/display: Use __drm_atomic_helper_plane_reset instead of
>     copying the logic
>   drm: mali-dp: Use __drm_atomic_helper_plane_reset instead of copying
>     the logic
>   drm: atmel-hlcdc: Use __drm_atomic_helper_plane_reset instead of
>     copying the logic
>   drm/exynos: Use __drm_atomic_helper_plane_reset instead of copying the
>     logic
>   drm/imx: Use __drm_atomic_helper_plane_reset instead of copying the
>     logic
>   drm: rcar-du: Use __drm_atomic_helper_plane_reset instead of copying
>     the logic
>   drm/sun4i: Use __drm_atomic_helper_plane_reset instead of copying the
>     logic
>   drm/vc4: Use __drm_atomic_helper_plane_reset instead of copying the
>     logic
>   drm/vmwgfx: Use __drm_atomic_helper_plane_reset instead of copying the
>     logic
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  7 ++--
>  drivers/gpu/drm/arm/malidp_planes.c           |  7 ++--
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   |  5 +--
>  drivers/gpu/drm/drm_atomic_helper.c           | 32 +++++++++++++------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  3 +-
>  drivers/gpu/drm/imx/ipuv3-plane.c             |  8 ++---
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c       |  4 +--
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c         |  4 +--
>  drivers/gpu/drm/sun4i/sun4i_layer.c           |  4 +--
>  drivers/gpu/drm/vc4/vc4_plane.c               |  4 +--
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |  4 +--
>  include/drm/drm_atomic_helper.h               |  2 ++
>  12 files changed, 39 insertions(+), 45 deletions(-)
>