[PATCH v2 1/3] drm: writeback: Fix doc that says connector should be disconnected

[PATCH v2 1/3] drm: writeback: Fix doc that says connector should be disconnected

[Alexandru Gheorghe]

During iteration process one of the proposed mechanism for not
breaking existing userspace was to report writeback connectors as
disconnected, however the final version used
DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.

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

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 8273950..e7b6e5e 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -23,8 +23,8 @@
  * from a CRTC to a memory buffer. They are used and act similarly to other
  * types of connectors, with some important differences:
  *  - Writeback connectors don't provide a way to output visually to the user.
- *  - Writeback connectors should always report as "disconnected" (so that
- *    clients which don't understand them will ignore them).
+ *  - Writeback connectors are visible to userspace only when the client sets
+ *    DRM_CLIENT_CAP_WRITEBACK_CONNECTORS.
  *  - Writeback connectors don't have EDID.
  *
  * A framebuffer may only be attached to a writeback connector when the
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm: mali-dp: Report writeback connector as connected

[Alexandru Gheorghe]

Older version of this patch series reported writeback as disconnected
to avoid confusing userspace not aware of writeback connectors.
However, the version that got merged uses a special cap
(DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.

This helps us avoid some special handling of writeback connector
in drm_helper_probe_single_connector_modes, see [1].

https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html

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

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index cfd718e..ba6ae66 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
 static enum drm_connector_status
 malidp_mw_connector_detect(struct drm_connector *connector, bool force)
 {
-	return connector_status_disconnected;
+	return connector_status_connected;
 }
 
 static void malidp_mw_connector_destroy(struct drm_connector *connector)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm: mali-dp: Set encoder possible_clones

[Alexandru Gheorghe]

Set possible_clones field to report that the writeback connector and
the one driving the display could be enabled at the same time.

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

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 5b72605..08b5bb2 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
 	struct malidp_hw_device *hwdev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct of_device_id const *dev_id;
+	struct drm_encoder *encoder;
 	/* number of lines for the R, G and B output */
 	u8 output_width[MAX_OUTPUT_CHANNELS];
 	int ret = 0, i;
@@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev)
 		goto bind_fail;
 	}
 
+	/* We expect to have a maximum of two encoders one for the actual
+	 * display and a virtual one for the writeback connector
+	 */
+	WARN_ON(drm->mode_config.num_encoder > 2);
+	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
+		encoder->possible_clones =
+				(1 << drm->mode_config.num_encoder) -  1;
+	}
+
 	ret = malidp_irq_init(pdev);
 	if (ret < 0)
 		goto irq_init_fail;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: writeback: Fix doc that says connector should be disconnected

[Sean Paul]

On Fri, Jul 13, 2018 at 04:10:58PM +0100, Alexandru Gheorghe wrote:
> During iteration process one of the proposed mechanism for not
> breaking existing userspace was to report writeback connectors as
> disconnected, however the final version used
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/drm_writeback.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 8273950..e7b6e5e 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -23,8 +23,8 @@
>   * from a CRTC to a memory buffer. They are used and act similarly to other
>   * types of connectors, with some important differences:
>   *  - Writeback connectors don't provide a way to output visually to the user.
> - *  - Writeback connectors should always report as "disconnected" (so that
> - *    clients which don't understand them will ignore them).
> + *  - Writeback connectors are visible to userspace only when the client sets
> + *    DRM_CLIENT_CAP_WRITEBACK_CONNECTORS.
>   *  - Writeback connectors don't have EDID.
>   *
>   * A framebuffer may only be attached to a writeback connector when the
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: mali-dp: Report writeback connector as connected

[Sean Paul]

On Fri, Jul 13, 2018 at 04:10:59PM +0100, Alexandru Gheorghe wrote:
> Older version of this patch series reported writeback as disconnected
> to avoid confusing userspace not aware of writeback connectors.
> However, the version that got merged uses a special cap
> (DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.
> 
> This helps us avoid some special handling of writeback connector
> in drm_helper_probe_single_connector_modes, see [1].
> 
> https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/arm/malidp_mw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index cfd718e..ba6ae66 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
>  static enum drm_connector_status
>  malidp_mw_connector_detect(struct drm_connector *connector, bool force)
>  {
> -	return connector_status_disconnected;
> +	return connector_status_connected;
>  }
>  
>  static void malidp_mw_connector_destroy(struct drm_connector *connector)
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: mali-dp: Set encoder possible_clones

[Sean Paul]

On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
> Set possible_clones field to report that the writeback connector and
> the one driving the display could be enabled at the same time.
> 

In future, please include a "Changes in vX" section so it's easier to tell
what's changed.

> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 5b72605..08b5bb2 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
>  	struct malidp_hw_device *hwdev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct of_device_id const *dev_id;
> +	struct drm_encoder *encoder;
>  	/* number of lines for the R, G and B output */
>  	u8 output_width[MAX_OUTPUT_CHANNELS];
>  	int ret = 0, i;
> @@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev)
>  		goto bind_fail;
>  	}
>  
> +	/* We expect to have a maximum of two encoders one for the actual
> +	 * display and a virtual one for the writeback connector
> +	 */
> +	WARN_ON(drm->mode_config.num_encoder > 2);
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> +		encoder->possible_clones =
> +				(1 << drm->mode_config.num_encoder) -  1;
> +	}

This loop isn't necessary, you can just do the assignment once instead of
num_encoder times :-)

Sean

> +
>  	ret = malidp_irq_init(pdev);
>  	if (ret < 0)
>  		goto irq_init_fail;
> -- 
> 2.7.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: mali-dp: Set encoder possible_clones

[Sean Paul]

On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
> On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
> > Set possible_clones field to report that the writeback connector and
> > the one driving the display could be enabled at the same time.
> > 
> 
> In future, please include a "Changes in vX" section so it's easier to tell
> what's changed.
> 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index 5b72605..08b5bb2 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
> >  	struct malidp_hw_device *hwdev;
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct of_device_id const *dev_id;
> > +	struct drm_encoder *encoder;
> >  	/* number of lines for the R, G and B output */
> >  	u8 output_width[MAX_OUTPUT_CHANNELS];
> >  	int ret = 0, i;
> > @@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev)
> >  		goto bind_fail;
> >  	}
> >  
> > +	/* We expect to have a maximum of two encoders one for the actual

Also, while I'm here, drop the comment contents a line. ie:

/*
 * We expect...
 */

Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting

Sean

> > +	 * display and a virtual one for the writeback connector
> > +	 */
> > +	WARN_ON(drm->mode_config.num_encoder > 2);
> > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > +		encoder->possible_clones =
> > +				(1 << drm->mode_config.num_encoder) -  1;
> > +	}
> 
> This loop isn't necessary, you can just do the assignment once instead of
> num_encoder times :-)
> 
> Sean
> 
> > +
> >  	ret = malidp_irq_init(pdev);
> >  	if (ret < 0)
> >  		goto irq_init_fail;
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: mali-dp: Set encoder possible_clones

[Alexandru-Cosmin Gheorghe]

On Fri, Jul 13, 2018 at 11:47:33AM -0400, Sean Paul wrote:
> On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
> > On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
> > > Set possible_clones field to report that the writeback connector and
> > > the one driving the display could be enabled at the same time.
> > > 
> > 
> > In future, please include a "Changes in vX" section so it's easier to tell
> > what's changed.

Yeah, sorry about that, the patch was so small that it never crossed
my mind.

> > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---
> > >  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > > index 5b72605..08b5bb2 100644
> > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
> > >  	struct malidp_hw_device *hwdev;
> > >  	struct platform_device *pdev = to_platform_device(dev);
> > >  	struct of_device_id const *dev_id;
> > > +	struct drm_encoder *encoder;
> > >  	/* number of lines for the R, G and B output */
> > >  	u8 output_width[MAX_OUTPUT_CHANNELS];
> > >  	int ret = 0, i;
> > > @@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev)
> > >  		goto bind_fail;
> > >  	}
> > >  
> > > +	/* We expect to have a maximum of two encoders one for the actual
> 
> Also, while I'm here, drop the comment contents a line. ie:
> 
> /*
>  * We expect...
>  */
> 
> Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting

I blame checkpatch, it should've complain about it.

> 
> Sean
> 
> > > +	 * display and a virtual one for the writeback connector
> > > +	 */
> > > +	WARN_ON(drm->mode_config.num_encoder > 2);
> > > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > > +		encoder->possible_clones =
> > > +				(1 << drm->mode_config.num_encoder) -  1;
> > > +	}
> > 
> > This loop isn't necessary, you can just do the assignment once instead of
> > num_encoder times :-)
> > 
> > Sean
> > 

Not sure I get what you are suggesting, there are two encoders, so at least
two assignments and the loop does just that.

> > > +
> > >  	ret = malidp_irq_init(pdev);
> > >  	if (ret < 0)
> > >  		goto irq_init_fail;
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: mali-dp: Set encoder possible_clones

[Sean Paul]

On Fri, Jul 13, 2018 at 04:55:41PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Fri, Jul 13, 2018 at 11:47:33AM -0400, Sean Paul wrote:
> > On Fri, Jul 13, 2018 at 11:40:13AM -0400, Sean Paul wrote:
> > > On Fri, Jul 13, 2018 at 04:11:00PM +0100, Alexandru Gheorghe wrote:
> > > > Set possible_clones field to report that the writeback connector and
> > > > the one driving the display could be enabled at the same time.
> > > > 
> > > 
> > > In future, please include a "Changes in vX" section so it's easier to tell
> > > what's changed.
> 
> Yeah, sorry about that, the patch was so small that it never crossed
> my mind.
> 

Anything you can do to help reviewers is most appreciated.

> > > 
> > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > > ---
> > > >  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > > > index 5b72605..08b5bb2 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > > @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
> > > >  	struct malidp_hw_device *hwdev;
> > > >  	struct platform_device *pdev = to_platform_device(dev);
> > > >  	struct of_device_id const *dev_id;
> > > > +	struct drm_encoder *encoder;
> > > >  	/* number of lines for the R, G and B output */
> > > >  	u8 output_width[MAX_OUTPUT_CHANNELS];
> > > >  	int ret = 0, i;
> > > > @@ -737,6 +738,15 @@ static int malidp_bind(struct device *dev)
> > > >  		goto bind_fail;
> > > >  	}
> > > >  
> > > > +	/* We expect to have a maximum of two encoders one for the actual
> > 
> > Also, while I'm here, drop the comment contents a line. ie:
> > 
> > /*
> >  * We expect...
> >  */
> > 
> > Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
> 
> I blame checkpatch, it should've complain about it.
> 
> > 
> > Sean
> > 
> > > > +	 * display and a virtual one for the writeback connector
> > > > +	 */
> > > > +	WARN_ON(drm->mode_config.num_encoder > 2);
> > > > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > > > +		encoder->possible_clones =
> > > > +				(1 << drm->mode_config.num_encoder) -  1;
> > > > +	}
> > > 
> > > This loop isn't necessary, you can just do the assignment once instead of
> > > num_encoder times :-)
> > > 
> > > Sean
> > > 
> 
> Not sure I get what you are suggesting, there are two encoders, so at least
> two assignments and the loop does just that.

Yeah, temporarily (I hope) lapse on my part. You're right :-)

Sean

> 
> > > > +
> > > >  	ret = malidp_irq_init(pdev);
> > > >  	if (ret < 0)
> > > >  		goto irq_init_fail;
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > -- 
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> Cheers,
> Alex G

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] drm: mali-dp: Set encoder possible_clones

[Alexandru Gheorghe]

Set possible_clones field to report that the writeback connector and
the one driving the display could be enabled at the same time.

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

Changes since v2:
  - Use proper style for multi-line comments.
---
 drivers/gpu/drm/arm/malidp_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 5b72605..4169a72 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
 	struct malidp_hw_device *hwdev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct of_device_id const *dev_id;
+	struct drm_encoder *encoder;
 	/* number of lines for the R, G and B output */
 	u8 output_width[MAX_OUTPUT_CHANNELS];
 	int ret = 0, i;
@@ -737,6 +738,16 @@ static int malidp_bind(struct device *dev)
 		goto bind_fail;
 	}
 
+	/*
+	 * We expect to have a maximum of two encoders one for the actual
+	 * display and a virtual one for the writeback connector
+	 */
+	WARN_ON(drm->mode_config.num_encoder > 2);
+	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
+		encoder->possible_clones =
+				(1 << drm->mode_config.num_encoder) -  1;
+	}
+
 	ret = malidp_irq_init(pdev);
 	if (ret < 0)
 		goto irq_init_fail;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm: mali-dp: Set encoder possible_clones

[Liviu Dudau]

On Mon, Jul 16, 2018 at 10:56:13AM +0100, Alexandru Gheorghe wrote:
> Set possible_clones field to report that the writeback connector and
> the one driving the display could be enabled at the same time.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

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

> 
> Changes since v2:
>   - Use proper style for multi-line comments.
> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 5b72605..4169a72 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -616,6 +616,7 @@ static int malidp_bind(struct device *dev)
>  	struct malidp_hw_device *hwdev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct of_device_id const *dev_id;
> +	struct drm_encoder *encoder;
>  	/* number of lines for the R, G and B output */
>  	u8 output_width[MAX_OUTPUT_CHANNELS];
>  	int ret = 0, i;
> @@ -737,6 +738,16 @@ static int malidp_bind(struct device *dev)
>  		goto bind_fail;
>  	}
>  
> +	/*
> +	 * We expect to have a maximum of two encoders one for the actual
> +	 * display and a virtual one for the writeback connector
> +	 */
> +	WARN_ON(drm->mode_config.num_encoder > 2);
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> +		encoder->possible_clones =
> +				(1 << drm->mode_config.num_encoder) -  1;
> +	}
> +
>  	ret = malidp_irq_init(pdev);
>  	if (ret < 0)
>  		goto irq_init_fail;
> -- 
> 2.7.4
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: writeback: Fix doc that says connector should be disconnected

[Liviu Dudau]

On Fri, Jul 13, 2018 at 04:10:58PM +0100, Alexandru Gheorghe wrote:
> During iteration process one of the proposed mechanism for not
> breaking existing userspace was to report writeback connectors as
> disconnected, however the final version used
> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS for that purpose.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

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

> ---
>  drivers/gpu/drm/drm_writeback.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 8273950..e7b6e5e 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -23,8 +23,8 @@
>   * from a CRTC to a memory buffer. They are used and act similarly to other
>   * types of connectors, with some important differences:
>   *  - Writeback connectors don't provide a way to output visually to the user.
> - *  - Writeback connectors should always report as "disconnected" (so that
> - *    clients which don't understand them will ignore them).
> + *  - Writeback connectors are visible to userspace only when the client sets
> + *    DRM_CLIENT_CAP_WRITEBACK_CONNECTORS.
>   *  - Writeback connectors don't have EDID.
>   *
>   * A framebuffer may only be attached to a writeback connector when the
> -- 
> 2.7.4
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: mali-dp: Report writeback connector as connected

[Liviu Dudau]

On Fri, Jul 13, 2018 at 04:10:59PM +0100, Alexandru Gheorghe wrote:
> Older version of this patch series reported writeback as disconnected
> to avoid confusing userspace not aware of writeback connectors.
> However, the version that got merged uses a special cap
> (DRM_CLIENT_CAP_WRITEBACK_CONNECTORS) for this purpose.
> 
> This helps us avoid some special handling of writeback connector
> in drm_helper_probe_single_connector_modes, see [1].
> 
> https://lists.freedesktop.org/archives/dri-devel/2018-July/183144.html
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>

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

> ---
>  drivers/gpu/drm/arm/malidp_mw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index cfd718e..ba6ae66 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -73,7 +73,7 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
>  static enum drm_connector_status
>  malidp_mw_connector_detect(struct drm_connector *connector, bool force)
>  {
> -	return connector_status_disconnected;
> +	return connector_status_connected;
>  }
>  
>  static void malidp_mw_connector_destroy(struct drm_connector *connector)
> -- 
> 2.7.4
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

Re: [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);
>  	}
>  }
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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);
>  	}
>  }
>  

Re: [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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@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

[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

Re: [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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

RE: [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@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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1122 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@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

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

Re: [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?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [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!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@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!  /
  ---------------
    ?\_(?)_/?

Re: [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1617 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [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(-)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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(-)
>