[PATCH 00/17] drm: Start subclassing crtc_state.

[PATCH 00/17] drm: Start subclassing crtc_state.

[Maarten Lankhorst]

When we want to start adding default values to crtc_state, it makes
sense fix all drivers to call __drm_atomic_helper_crtc_reset, like
we do for connectors and planes.

Maarten Lankhorst (17):
  drm/vc4: Fix memory leak during gpu reset.
  drm/atomic: Create __drm_atomic_helper_crtc_reset() for subclassing crtc_state.
  drm/docs: Fix typo in __drm_atomic_helper_connector_reset
  drm/amd: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/mali: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/atmel-hlcdc: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/i915: Use the new __drm_atomic_helper_crtc_reset() helper.
  drm/imx: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/mediatek: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/msm: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/omap: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/rockchip: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/tegra: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/vc4: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/vkms: Convert to using __drm_atomic_helper_crtc_reset() for reset.
  drm/vmwgfx: Convert to using __drm_atomic_helper_crtc_reset() for reset.

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++----
 drivers/gpu/drm/arm/malidp_crtc.c             | 28 ++++++---------
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    | 29 ++++++---------
 drivers/gpu/drm/drm_atomic_state_helper.c     | 36 +++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c          |  5 ++-
 drivers/gpu/drm/imx/ipuv3-crtc.c              | 30 ++++++----------
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 29 ++++++---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 ++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c     | 28 ++++++---------
 drivers/gpu/drm/nouveau/dispnv50/head.c       | 13 ++-----
 drivers/gpu/drm/omapdrm/omap_crtc.c           | 11 +++---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        | 11 ++----
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 22 ++++++------
 drivers/gpu/drm/tegra/dc.c                    | 30 ++++++----------
 drivers/gpu/drm/vc4/vc4_crtc.c                |  9 ++---
 drivers/gpu/drm/vkms/vkms_crtc.c              | 33 +++++++----------
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           | 20 +++--------
 include/drm/drm_atomic_state_helper.h         |  2 ++
 18 files changed, 145 insertions(+), 207 deletions(-)

-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/17] drm/vc4: Fix memory leak during gpu reset.

[Maarten Lankhorst]

__drm_atomic_helper_crtc_destroy_state does not free memory, it only
cleans it up. Fix this by calling the functions own destroy function.

Fixes: 6d6e50039187 ("drm/vc4: Allocate the right amount of space for boot-time CRTC state.")
Cc: Eric Anholt <eric@anholt.net>
Cc: <stable@vger.kernel.org> # v4.6+
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 730008d3da76..e7c04a9eb219 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1042,7 +1042,7 @@ static void
 vc4_crtc_reset(struct drm_crtc *crtc)
 {
 	if (crtc->state)
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
+		vc4_crtc_destroy_state(crtc->state);
 
 	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
 	if (crtc->state)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/17] drm/vc4: Fix memory leak during gpu reset.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

Maarten Lankhorst <maarten.lankhorst@linux.intel.com> writes:

> __drm_atomic_helper_crtc_destroy_state does not free memory, it only
> cleans it up. Fix this by calling the functions own destroy function.

Reviewed-by: Eric Anholt <eric@anholt.net>

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

[PATCH 02/17] drm/atomic: Create __drm_atomic_helper_crtc_reset() for subclassing crtc_state.

[Maarten Lankhorst]

We already have __drm_atomic_helper_connector_reset() and
__drm_atomic_helper_plane_reset(), extend this to crtc as well.

This will allow us to set default values in the crtc_state, without
having to do it in each driver separately.

Of all drivers that need conversion, only nouveau is done in this
commit, because it wrote its own __drm_atomic_helper_crtc_reset(),
clashing with the drm core.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 34 +++++++++++++++++++----
 drivers/gpu/drm/nouveau/dispnv50/head.c   | 13 ++-------
 include/drm/drm_atomic_state_helper.h     |  2 ++
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 4985384e51f6..bc5ee66f75b3 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -55,6 +55,29 @@
  * for these functions.
  */
 
+/**
+ * __drm_atomic_helper_crtc_reset - reset state on CRTC
+ * @crtc: drm CRTC
+ * @crtc_state: CRTC state to assign
+ *
+ * Initializes the newly allocated @crtc_state and assigns it to
+ * the &drm_crtc->state pointer of @crtc, usually required when
+ * initializing the drivers or when called from the &drm_crtc_funcs.reset
+ * hook.
+ *
+ * This is useful for drivers that subclass the CRTC state.
+ */
+void
+__drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
+			       struct drm_crtc_state *crtc_state)
+{
+	if (crtc_state)
+		crtc_state->crtc = crtc;
+
+	crtc->state = crtc_state;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_crtc_reset);
+
 /**
  * drm_atomic_helper_crtc_reset - default &drm_crtc_funcs.reset hook for CRTCs
  * @crtc: drm CRTC
@@ -64,14 +87,13 @@
  */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 {
-	if (crtc->state)
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-
-	kfree(crtc->state);
-	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
+	struct drm_crtc_state *crtc_state =
+		kzalloc(sizeof(*crtc->state), GFP_KERNEL);
 
 	if (crtc->state)
-		crtc->state->crtc = crtc;
+		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, crtc_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
 
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 2e7a0c347ddb..93754743090f 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -419,16 +419,6 @@ nv50_head_atomic_duplicate_state(struct drm_crtc *crtc)
 	return &asyh->state;
 }
 
-static void
-__drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
-			       struct drm_crtc_state *state)
-{
-	if (crtc->state)
-		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
-	crtc->state = state;
-	crtc->state->crtc = crtc;
-}
-
 static void
 nv50_head_reset(struct drm_crtc *crtc)
 {
@@ -437,6 +427,9 @@ nv50_head_reset(struct drm_crtc *crtc)
 	if (WARN_ON(!(asyh = kzalloc(sizeof(*asyh), GFP_KERNEL))))
 		return;
 
+	if (crtc->state)
+		nv50_head_atomic_destroy_state(crtc, crtc->state);
+
 	__drm_atomic_helper_crtc_reset(crtc, &asyh->state);
 }
 
diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
index 66c92cbd8e16..4e6d2e7a40b8 100644
--- a/include/drm/drm_atomic_state_helper.h
+++ b/include/drm/drm_atomic_state_helper.h
@@ -37,6 +37,8 @@ struct drm_private_state;
 struct drm_modeset_acquire_ctx;
 struct drm_device;
 
+void __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
+				    struct drm_crtc_state *state);
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
 void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 					      struct drm_crtc_state *state);
-- 
2.20.1

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

[PATCH 03/17] drm/docs: Fix typo in __drm_atomic_helper_connector_reset

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index bc5ee66f75b3..172c8c698a46 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -335,7 +335,7 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
  * @conn_state: connector state to assign
  *
  * Initializes the newly allocated @conn_state and assigns it to
- * the &drm_conector->state pointer of @connector, usually required when
+ * the &drm_connector->state pointer of @connector, usually required when
  * initializing the drivers or when called from the &drm_connector_funcs.reset
  * hook.
  *
-- 
2.20.1

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

[PATCH 04/17] drm/amd: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert amd to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 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 0642dfe22582..975ed22e39d2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3072,18 +3072,12 @@ static void dm_crtc_destroy_state(struct drm_crtc *crtc,
 
 static void dm_crtc_reset_state(struct drm_crtc *crtc)
 {
-	struct dm_crtc_state *state;
+	struct dm_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
 
 	if (crtc->state)
 		dm_crtc_destroy_state(crtc, crtc->state);
 
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (WARN_ON(!state))
-		return;
-
-	crtc->state = &state->base;
-	crtc->state->crtc = crtc;
-
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
 }
 
 static struct drm_crtc_state *
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/17] drm/mali: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert mali to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
malidp_crtc_destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_crtc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 56aad288666e..d6690e016f0b 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -463,23 +463,6 @@ static struct drm_crtc_state *malidp_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &state->base;
 }
 
-static void malidp_crtc_reset(struct drm_crtc *crtc)
-{
-	struct malidp_crtc_state *state = NULL;
-
-	if (crtc->state) {
-		state = to_malidp_crtc_state(crtc->state);
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-	}
-
-	kfree(state);
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state) {
-		crtc->state = &state->base;
-		crtc->state->crtc = crtc;
-	}
-}
-
 static void malidp_crtc_destroy_state(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {
@@ -493,6 +476,17 @@ static void malidp_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(mali_state);
 }
 
+static void malidp_crtc_reset(struct drm_crtc *crtc)
+{
+	struct malidp_crtc_state *state =
+		kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (crtc->state)
+		malidp_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
+}
+
 static int malidp_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
-- 
2.20.1

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

Re: [PATCH 05/17] drm/mali: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Liviu Dudau]

Hi Maarten,

On Fri, Mar 01, 2019 at 01:56:15PM +0100, Maarten Lankhorst wrote:
> Convert mali to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding
> malidp_crtc_destroy_state(), call it directly for freeing the old state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>

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

If you need me to take this patch into malidp tree let me know,
otherwise I expect is going to show up in drm-misc-next at some moment.

Best regards,
Liviu

> Cc: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_crtc.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> index 56aad288666e..d6690e016f0b 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -463,23 +463,6 @@ static struct drm_crtc_state *malidp_crtc_duplicate_state(struct drm_crtc *crtc)
>  	return &state->base;
>  }
>  
> -static void malidp_crtc_reset(struct drm_crtc *crtc)
> -{
> -	struct malidp_crtc_state *state = NULL;
> -
> -	if (crtc->state) {
> -		state = to_malidp_crtc_state(crtc->state);
> -		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> -	}
> -
> -	kfree(state);
> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (state) {
> -		crtc->state = &state->base;
> -		crtc->state->crtc = crtc;
> -	}
> -}
> -
>  static void malidp_crtc_destroy_state(struct drm_crtc *crtc,
>  				      struct drm_crtc_state *state)
>  {
> @@ -493,6 +476,17 @@ static void malidp_crtc_destroy_state(struct drm_crtc *crtc,
>  	kfree(mali_state);
>  }
>  
> +static void malidp_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct malidp_crtc_state *state =
> +		kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (crtc->state)
> +		malidp_crtc_destroy_state(crtc, crtc->state);
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &state->base);
> +}
> +
>  static int malidp_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
====================
| 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 06/17] drm/atmel-hlcdc: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert atmel-hlcdc to using __drm_atomic_helper_crtc_reset(), instead
of writing its own version. Instead of open coding destroy_state(),
call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
---
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    | 29 +++++++------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 8070a558d7b1..816161d0a09d 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -400,24 +400,6 @@ void atmel_hlcdc_crtc_irq(struct drm_crtc *c)
 	atmel_hlcdc_crtc_finish_page_flip(drm_crtc_to_atmel_hlcdc_crtc(c));
 }
 
-static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc)
-{
-	struct atmel_hlcdc_crtc_state *state;
-
-	if (crtc->state) {
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-		state = drm_crtc_state_to_atmel_hlcdc_crtc_state(crtc->state);
-		kfree(state);
-		crtc->state = NULL;
-	}
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state) {
-		crtc->state = &state->base;
-		crtc->state->crtc = crtc;
-	}
-}
-
 static struct drm_crtc_state *
 atmel_hlcdc_crtc_duplicate_state(struct drm_crtc *crtc)
 {
@@ -447,6 +429,17 @@ static void atmel_hlcdc_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(state);
 }
 
+static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc)
+{
+	struct atmel_hlcdc_crtc_state *state =
+		kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (crtc->state)
+		atmel_hlcdc_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
+}
+
 static int atmel_hlcdc_crtc_enable_vblank(struct drm_crtc *c)
 {
 	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/17] drm/i915: Use the new __drm_atomic_helper_crtc_reset() helper.

[Maarten Lankhorst]

i915 has its own hw readout and doesn't use the reset helpers directly.
Still it has 2 places where it initialises the crtc_state. Fix those
by calling __drm_atomic_helper_crtc_reset().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7c5e84ef5171..2e14a50dbf6f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14447,9 +14447,8 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		ret = -ENOMEM;
 		goto fail;
 	}
+	__drm_atomic_helper_crtc_reset(&intel_crtc->base, &crtc_state->base);
 	intel_crtc->config = crtc_state;
-	intel_crtc->base.state = &crtc_state->base;
-	crtc_state->base.crtc = &intel_crtc->base;
 
 	primary = intel_primary_plane_create(dev_priv, pipe);
 	if (IS_ERR(primary)) {
@@ -15986,7 +15985,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
 		memset(crtc_state, 0, sizeof(*crtc_state));
-		crtc_state->base.crtc = &crtc->base;
+		__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->base);
 
 		crtc_state->base.active = crtc_state->base.enable =
 			dev_priv->display.get_pipe_config(crtc, crtc_state);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/17] drm/imx: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert imx to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index ec3602ebbc1c..54a32c6f2407 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -101,26 +101,6 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 	drm_crtc_vblank_off(crtc);
 }
 
-static void imx_drm_crtc_reset(struct drm_crtc *crtc)
-{
-	struct imx_crtc_state *state;
-
-	if (crtc->state) {
-		if (crtc->state->mode_blob)
-			drm_property_blob_put(crtc->state->mode_blob);
-
-		state = to_imx_crtc_state(crtc->state);
-		memset(state, 0, sizeof(*state));
-	} else {
-		state = kzalloc(sizeof(*state), GFP_KERNEL);
-		if (!state)
-			return;
-		crtc->state = &state->base;
-	}
-
-	state->base.crtc = crtc;
-}
-
 static struct drm_crtc_state *imx_drm_crtc_duplicate_state(struct drm_crtc *crtc)
 {
 	struct imx_crtc_state *state;
@@ -144,6 +124,16 @@ static void imx_drm_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(to_imx_crtc_state(state));
 }
 
+static void imx_drm_crtc_reset(struct drm_crtc *crtc)
+{
+	struct imx_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (crtc->state)
+		imx_drm_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
+}
+
 static int ipu_enable_vblank(struct drm_crtc *crtc)
 {
 	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/17] drm/mediatek: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert mediatek to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: CK Hu <ck.hu@mediatek.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 29 +++++++++----------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index acad088173da..3b18bbadd415 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -108,25 +108,6 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
-static void mtk_drm_crtc_reset(struct drm_crtc *crtc)
-{
-	struct mtk_crtc_state *state;
-
-	if (crtc->state) {
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-
-		state = to_mtk_crtc_state(crtc->state);
-		memset(state, 0, sizeof(*state));
-	} else {
-		state = kzalloc(sizeof(*state), GFP_KERNEL);
-		if (!state)
-			return;
-		crtc->state = &state->base;
-	}
-
-	state->base.crtc = crtc;
-}
-
 static struct drm_crtc_state *mtk_drm_crtc_duplicate_state(struct drm_crtc *crtc)
 {
 	struct mtk_crtc_state *state;
@@ -150,6 +131,16 @@ static void mtk_drm_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(to_mtk_crtc_state(state));
 }
 
+static void mtk_drm_crtc_reset(struct drm_crtc *crtc)
+{
+	struct mtk_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (crtc->state)
+		mtk_drm_crtc_destroy_state(crtc, &state->base);
+
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
+}
+
 static bool mtk_drm_crtc_mode_fixup(struct drm_crtc *crtc,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode)
-- 
2.20.1

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

[PATCH 10/17] drm/msm: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert msm to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 ++---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 28 +++++++++--------------
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index b776fca571f3..eb156cb73dd4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -753,14 +753,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
 
 static void dpu_crtc_reset(struct drm_crtc *crtc)
 {
-	struct dpu_crtc_state *cstate;
+	struct dpu_crtc_state *cstate = kzalloc(sizeof(*cstate), GFP_KERNEL);
 
 	if (crtc->state)
 		dpu_crtc_destroy_state(crtc, crtc->state);
 
-	crtc->state = kzalloc(sizeof(*cstate), GFP_KERNEL);
-	if (crtc->state)
-		crtc->state->crtc = crtc;
+	__drm_atomic_helper_crtc_reset(crtc, &cstate->base);
 }
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index b0cf63c4e3d7..bf24a08feab9 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -1002,23 +1002,6 @@ mdp5_crtc_atomic_print_state(struct drm_printer *p,
 	drm_printf(p, "\tcmd_mode=%d\n", mdp5_cstate->cmd_mode);
 }
 
-static void mdp5_crtc_reset(struct drm_crtc *crtc)
-{
-	struct mdp5_crtc_state *mdp5_cstate;
-
-	if (crtc->state) {
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-		kfree(to_mdp5_crtc_state(crtc->state));
-	}
-
-	mdp5_cstate = kzalloc(sizeof(*mdp5_cstate), GFP_KERNEL);
-
-	if (mdp5_cstate) {
-		mdp5_cstate->base.crtc = crtc;
-		crtc->state = &mdp5_cstate->base;
-	}
-}
-
 static struct drm_crtc_state *
 mdp5_crtc_duplicate_state(struct drm_crtc *crtc)
 {
@@ -1046,6 +1029,17 @@ static void mdp5_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state
 	kfree(mdp5_cstate);
 }
 
+static void mdp5_crtc_reset(struct drm_crtc *crtc)
+{
+	struct mdp5_crtc_state *mdp5_cstate =
+		mdp5_cstate = kzalloc(sizeof(*mdp5_cstate), GFP_KERNEL);
+
+	if (crtc->state)
+		mdp5_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &mdp5_cstate->base);
+}
+
 static const struct drm_crtc_funcs mdp5_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = mdp5_crtc_destroy,
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/17] drm/msm: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Sean Paul]

On Fri, Mar 01, 2019 at 01:56:20PM +0100, Maarten Lankhorst wrote:
> Convert msm to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding
> destroy_state(), call it directly for freeing the old state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 ++---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 28 +++++++++--------------
>  2 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index b776fca571f3..eb156cb73dd4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -753,14 +753,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
>  
>  static void dpu_crtc_reset(struct drm_crtc *crtc)
>  {
> -	struct dpu_crtc_state *cstate;
> +	struct dpu_crtc_state *cstate = kzalloc(sizeof(*cstate), GFP_KERNEL);
>  
>  	if (crtc->state)
>  		dpu_crtc_destroy_state(crtc, crtc->state);
>  
> -	crtc->state = kzalloc(sizeof(*cstate), GFP_KERNEL);
> -	if (crtc->state)
> -		crtc->state->crtc = crtc;
> +	__drm_atomic_helper_crtc_reset(crtc, &cstate->base);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index b0cf63c4e3d7..bf24a08feab9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -1002,23 +1002,6 @@ mdp5_crtc_atomic_print_state(struct drm_printer *p,
>  	drm_printf(p, "\tcmd_mode=%d\n", mdp5_cstate->cmd_mode);
>  }
>  
> -static void mdp5_crtc_reset(struct drm_crtc *crtc)
> -{
> -	struct mdp5_crtc_state *mdp5_cstate;
> -
> -	if (crtc->state) {
> -		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> -		kfree(to_mdp5_crtc_state(crtc->state));
> -	}
> -
> -	mdp5_cstate = kzalloc(sizeof(*mdp5_cstate), GFP_KERNEL);
> -
> -	if (mdp5_cstate) {
> -		mdp5_cstate->base.crtc = crtc;
> -		crtc->state = &mdp5_cstate->base;
> -	}
> -}
> -
>  static struct drm_crtc_state *
>  mdp5_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
> @@ -1046,6 +1029,17 @@ static void mdp5_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state
>  	kfree(mdp5_cstate);
>  }
>  
> +static void mdp5_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct mdp5_crtc_state *mdp5_cstate =
> +		mdp5_cstate = kzalloc(sizeof(*mdp5_cstate), GFP_KERNEL);

Assigned twice for good measure ;)

> +
> +	if (crtc->state)
> +		mdp5_crtc_destroy_state(crtc, crtc->state);
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &mdp5_cstate->base);
> +}
> +
>  static const struct drm_crtc_funcs mdp5_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = mdp5_crtc_destroy,
> -- 
> 2.20.1
> 

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

[PATCH 11/17] drm/omap: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert omap to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index d99e24dcc0bf..2546002b1c5c 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -549,14 +549,13 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
 
 static void omap_crtc_reset(struct drm_crtc *crtc)
 {
-	if (crtc->state)
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-
-	kfree(crtc->state);
-	crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL);
+	struct omap_crtc_state *crtc_state =
+		kzalloc(sizeof(*crtc_state), GFP_KERNEL);
 
 	if (crtc->state)
-		crtc->state->crtc = crtc;
+		drm_atomic_helper_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &crtc_state->base);
 }
 
 static struct drm_crtc_state *
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(), call
it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 4cdea14d552f..7766551e67fc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
 
 static void rcar_du_crtc_reset(struct drm_crtc *crtc)
 {
-	struct rcar_du_crtc_state *state;
+	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
 
-	if (crtc->state) {
+	if (crtc->state)
 		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
-		crtc->state = NULL;
-	}
 
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	__drm_atomic_helper_crtc_reset(crtc, &state->state);
 	if (state == NULL)
 		return;
 
 	state->crc.source = VSP1_DU_CRC_NONE;
 	state->crc.index = 0;
-
-	crtc->state = &state->state;
-	crtc->state->crtc = crtc;
 }
 
 static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
-- 
2.20.1

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

Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Laurent Pinchart]

Hi Maarten,

Thank you for the patch.

On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(), call
> it directly for freeing the old state.

I don't think the second sentence applies to this patch.

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 4cdea14d552f..7766551e67fc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>  
>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>  {
> -	struct rcar_du_crtc_state *state;
> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>  
> -	if (crtc->state) {
> +	if (crtc->state)
>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> -		crtc->state = NULL;
> -	}
>  
> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);

state may be NULL here if the above kzalloc() failed. Let's keep the
original order of the function, and simply call
__drm_atomic_helper_crtc_reset() after the NULL check below.

>  	if (state == NULL)
>  		return;
>  
>  	state->crc.source = VSP1_DU_CRC_NONE;
>  	state->crc.index = 0;
> -
> -	crtc->state = &state->state;
> -	crtc->state->crtc = crtc;
>  }
>  
>  static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)

-- 
Regards,

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

Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> Hi Maarten,
>
> Thank you for the patch.
>
> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
>> writing its own version. Instead of open coding destroy_state(), call
>> it directly for freeing the old state.
> I don't think the second sentence applies to this patch.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index 4cdea14d552f..7766551e67fc 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>>  
>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>>  {
>> -	struct rcar_du_crtc_state *state;
>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>>  
>> -	if (crtc->state) {
>> +	if (crtc->state)
>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
>> -		crtc->state = NULL;
>> -	}
>>  
>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> state may be NULL here if the above kzalloc() failed. Let's keep the
> original order of the function, and simply call
> __drm_atomic_helper_crtc_reset() after the NULL check below.

There were 10 different ways crtc was implemented, I felt it was good to settle on one.

We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.

Looking more closely, it's the same way that errors in rcar_du_plane_reset() are handled. :)

Cheers,

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Laurent Pinchart]

Hi Marteen,

On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> > On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> >> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> >> writing its own version. Instead of open coding destroy_state(), call
> >> it directly for freeing the old state.
> > I don't think the second sentence applies to this patch.
> >
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
> >>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> index 4cdea14d552f..7766551e67fc 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> >>  
> >>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> >>  {
> >> -	struct rcar_du_crtc_state *state;
> >> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>  
> >> -	if (crtc->state) {
> >> +	if (crtc->state)
> >>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> >> -		crtc->state = NULL;
> >> -	}
> >>  
> >> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> >> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> > 
> > state may be NULL here if the above kzalloc() failed. Let's keep the
> > original order of the function, and simply call
> > __drm_atomic_helper_crtc_reset() after the NULL check below.
> 
> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
> 
> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.

That's not the point. As state can be NULL, you could end up
dereferencing a NULL pointer. The fact that the base state is the first
field in the rcar_du_crtc_state structure is just luck, and shouldn't be
relied on.

> Looking more closely, it's the same way that errors in
> rcar_du_plane_reset() are handled. :)

It's not, the return value of kzalloc() is checked explicitly in
rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
Please copy the code flow of rcar_du_plane_reset() to implement
rcar_du_crtc_reset().

-- 
Regards,

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

Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Op 01-03-2019 om 15:36 schreef Laurent Pinchart:
> Hi Marteen,
>
> On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
>> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
>>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
>>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
>>>> writing its own version. Instead of open coding destroy_state(), call
>>>> it directly for freeing the old state.
>>> I don't think the second sentence applies to this patch.
>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> ---
>>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
>>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> index 4cdea14d552f..7766551e67fc 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
>>>>  
>>>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>>>>  {
>>>> -	struct rcar_du_crtc_state *state;
>>>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
>>>>  
>>>> -	if (crtc->state) {
>>>> +	if (crtc->state)
>>>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
>>>> -		crtc->state = NULL;
>>>> -	}
>>>>  
>>>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
>>>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
>>> state may be NULL here if the above kzalloc() failed. Let's keep the
>>> original order of the function, and simply call
>>> __drm_atomic_helper_crtc_reset() after the NULL check below.
>> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
>>
>> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.
> That's not the point. As state can be NULL, you could end up
> dereferencing a NULL pointer. The fact that the base state is the first
> field in the rcar_du_crtc_state structure is just luck, and shouldn't be
> relied on.

Would it be ok if I changed it to state ? &state->state : NULL and let the compiler deal with it?

Will probably fix up all other patches as well before committing.

>> Looking more closely, it's the same way that errors in
>> rcar_du_plane_reset() are handled. :)
> It's not, the return value of kzalloc() is checked explicitly in
> rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
> Please copy the code flow of rcar_du_plane_reset() to implement
> rcar_du_crtc_reset().
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/rcar-du: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Laurent Pinchart]

Hi Marteen,

On Fri, Mar 01, 2019 at 03:47:02PM +0100, Maarten Lankhorst wrote:
> Op 01-03-2019 om 15:36 schreef Laurent Pinchart:
> > On Fri, Mar 01, 2019 at 03:08:20PM +0100, Maarten Lankhorst wrote:
> >> Op 01-03-2019 om 14:13 schreef Laurent Pinchart:
> >>> On Fri, Mar 01, 2019 at 01:56:22PM +0100, Maarten Lankhorst wrote:
> >>>> Convert rcar-du to using __drm_atomic_helper_crtc_reset(), instead of
> >>>> writing its own version. Instead of open coding destroy_state(), call
> >>>> it directly for freeing the old state.
> >>> I don't think the second sentence applies to this patch.
> >>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>> Cc: linux-renesas-soc@vger.kernel.org
> >>>> ---
> >>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 +++--------
> >>>>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> index 4cdea14d552f..7766551e67fc 100644
> >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >>>> @@ -891,22 +891,17 @@ static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> >>>>  
> >>>>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> >>>>  {
> >>>> -	struct rcar_du_crtc_state *state;
> >>>> +	struct rcar_du_crtc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>>>  
> >>>> -	if (crtc->state) {
> >>>> +	if (crtc->state)
> >>>>  		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> >>>> -		crtc->state = NULL;
> >>>> -	}
> >>>>  
> >>>> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> >>>> +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> >>> state may be NULL here if the above kzalloc() failed. Let's keep the
> >>> original order of the function, and simply call
> >>> __drm_atomic_helper_crtc_reset() after the NULL check below.
> >> There were 10 different ways crtc was implemented, I felt it was good to settle on one.
> >>
> >> We don't handle during reset at all, would need to start propagating this first before we should handle errors, imho.
> > That's not the point. As state can be NULL, you could end up
> > dereferencing a NULL pointer. The fact that the base state is the first
> > field in the rcar_du_crtc_state structure is just luck, and shouldn't be
> > relied on.
> 
> Would it be ok if I changed it to state ? &state->state : NULL and let
> the compiler deal with it?

What's wrong with a proper implementation ?

static void rcar_du_crtc_reset(struct drm_crtc *crtc)
{
	struct rcar_du_crtc_state *state;

	if (crtc->state) {
		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
		crtc->state = NULL;
	}

	state = kzalloc(sizeof(*state), GFP_KERNEL);
	if (state == NULL)
		return;

	__drm_atomic_helper_crtc_reset(crtc, &state->state);

	state->crc.source = VSP1_DU_CRC_NONE;
	state->crc.index = 0;
}

> Will probably fix up all other patches as well before committing.

You won't commit this one before I ack it, right ? :-)

> >> Looking more closely, it's the same way that errors in
> >> rcar_du_plane_reset() are handled. :)
> > It's not, the return value of kzalloc() is checked explicitly in
> > rcar_du_plane_reset() before calling __drm_atomic_helper_plane_reset().
> > Please copy the code flow of rcar_du_plane_reset() to implement
> > rcar_du_crtc_reset().

-- 
Regards,

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

[PATCH 13/17] drm/rockchip: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert rockchip to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding
destroy_state(), call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sandy Huang <hjc@rock-chips.com>
Cc: "Heiko Stübner" <heiko@sntech.de>
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 22 ++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c7d4c6073ea5..1cf1658f1c01 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1201,17 +1201,6 @@ static void vop_crtc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
-static void vop_crtc_reset(struct drm_crtc *crtc)
-{
-	if (crtc->state)
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-	kfree(crtc->state);
-
-	crtc->state = kzalloc(sizeof(struct rockchip_crtc_state), GFP_KERNEL);
-	if (crtc->state)
-		crtc->state->crtc = crtc;
-}
-
 static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
 {
 	struct rockchip_crtc_state *rockchip_state;
@@ -1233,6 +1222,17 @@ static void vop_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(s);
 }
 
+static void vop_crtc_reset(struct drm_crtc *crtc)
+{
+	struct rockchip_crtc_state *crtc_state =
+		kzalloc(sizeof(*crtc_state), GFP_KERNEL);
+
+	if (crtc->state)
+		vop_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &crtc_state->base);
+}
+
 #ifdef CONFIG_DRM_ANALOGIX_DP
 static struct drm_connector *vop_get_edp_connector(struct vop *vop)
 {
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 13/17] drm/rockchip: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Heiko Stübner]

Am Freitag, 1. März 2019, 13:56:23 CET schrieb Maarten Lankhorst:
> Convert rockchip to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding
> destroy_state(), call it directly for freeing the old state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sandy Huang <hjc@rock-chips.com>
> Cc: "Heiko Stübner" <heiko@sntech.de>
> Cc: linux-rockchip@lists.infradead.org

so I've looked up the patch2 that introduces __drm_atomic_helper_crtc_reset
in patchwork and compared results and everything looks as it should be
I think ;-) , so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>




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

[PATCH 14/17] drm/tegra: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert tegra to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(),
call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: linux-tegra@vger.kernel.org
---
 drivers/gpu/drm/tegra/dc.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 607a6ea17ecc..57c88d78cdaa 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1153,25 +1153,6 @@ static void tegra_dc_destroy(struct drm_crtc *crtc)
 	drm_crtc_cleanup(crtc);
 }
 
-static void tegra_crtc_reset(struct drm_crtc *crtc)
-{
-	struct tegra_dc_state *state;
-
-	if (crtc->state)
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-
-	kfree(crtc->state);
-	crtc->state = NULL;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (state) {
-		crtc->state = &state->base;
-		crtc->state->crtc = crtc;
-	}
-
-	drm_crtc_vblank_reset(crtc);
-}
-
 static struct drm_crtc_state *
 tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
 {
@@ -1198,6 +1179,17 @@ static void tegra_crtc_atomic_destroy_state(struct drm_crtc *crtc,
 	kfree(state);
 }
 
+static void tegra_crtc_reset(struct drm_crtc *crtc)
+{
+	struct tegra_dc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
+
+	if (crtc->state)
+		tegra_crtc_atomic_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &state->base);
+	drm_crtc_vblank_reset(crtc);
+}
+
 #define DEBUGFS_REG32(_name) { .name = #_name, .offset = _name }
 
 static const struct debugfs_reg32 tegra_dc_regs[] = {
-- 
2.20.1

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

Re: [PATCH 14/17] drm/tegra: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Thierry Reding]

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

On Fri, Mar 01, 2019 at 01:56:24PM +0100, Maarten Lankhorst wrote:
> Convert tegra to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(),
> call it directly for freeing the old state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: linux-tegra@vger.kernel.org
> ---
>  drivers/gpu/drm/tegra/dc.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 607a6ea17ecc..57c88d78cdaa 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1153,25 +1153,6 @@ static void tegra_dc_destroy(struct drm_crtc *crtc)
>  	drm_crtc_cleanup(crtc);
>  }
>  
> -static void tegra_crtc_reset(struct drm_crtc *crtc)
> -{
> -	struct tegra_dc_state *state;
> -
> -	if (crtc->state)
> -		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> -
> -	kfree(crtc->state);
> -	crtc->state = NULL;
> -
> -	state = kzalloc(sizeof(*state), GFP_KERNEL);
> -	if (state) {
> -		crtc->state = &state->base;
> -		crtc->state->crtc = crtc;
> -	}
> -
> -	drm_crtc_vblank_reset(crtc);
> -}
> -
>  static struct drm_crtc_state *
>  tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
>  {
> @@ -1198,6 +1179,17 @@ static void tegra_crtc_atomic_destroy_state(struct drm_crtc *crtc,
>  	kfree(state);
>  }
>  
> +static void tegra_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct tegra_dc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> +	if (crtc->state)
> +		tegra_crtc_atomic_destroy_state(crtc, crtc->state);
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &state->base);
> +	drm_crtc_vblank_reset(crtc);
> +}
> +

I would preferred a predeclaration of tegra_crtc_atomic_destroy_state()
in this case because the implementations are in the same order as their
use in tegra_crtc_funcs, but I think I can live with it, so either way:

Acked-by: Thierry Reding <treding@nvidia.com>

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/17] drm/vc4: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert vc4 to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(),
call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index e7c04a9eb219..fdf21594b050 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1041,12 +1041,13 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc,
 static void
 vc4_crtc_reset(struct drm_crtc *crtc)
 {
-	if (crtc->state)
-		vc4_crtc_destroy_state(crtc->state);
+	struct vc4_crtc_state *crtc_state =
+		kzalloc(sizeof(*crtc_state), GFP_KERNEL);
 
-	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
 	if (crtc->state)
-		crtc->state->crtc = crtc;
+		vc4_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &crtc_state->base);
 }
 
 static const struct drm_crtc_funcs vc4_crtc_funcs = {
-- 
2.20.1

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

Re: [PATCH 15/17] drm/vc4: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Eric Anholt]

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

Maarten Lankhorst <maarten.lankhorst@linux.intel.com> writes:

> Convert vc4 to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(),
> call it directly for freeing the old state.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e7c04a9eb219..fdf21594b050 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -1041,12 +1041,13 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>  static void
>  vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> -	if (crtc->state)
> -		vc4_crtc_destroy_state(crtc->state);
> +	struct vc4_crtc_state *crtc_state =
> +		kzalloc(sizeof(*crtc_state), GFP_KERNEL);
>  
> -	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
>  	if (crtc->state)
> -		crtc->state->crtc = crtc;
> +		vc4_crtc_destroy_state(crtc, crtc->state);
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &crtc_state->base);

Wouldn't it be much easier if __drm_atomic_helper_crtc_reset took in a
new state and destroyed the old state for you?  It seems like hardly a
helper as is.

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/17] drm/vc4: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Op 01-03-2019 om 23:47 schreef Eric Anholt:
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com> writes:
>
>> Convert vc4 to using __drm_atomic_helper_crtc_reset(), instead of
>> writing its own version. Instead of open coding destroy_state(),
>> call it directly for freeing the old state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/gpu/drm/vc4/vc4_crtc.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
>> index e7c04a9eb219..fdf21594b050 100644
>> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
>> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
>> @@ -1041,12 +1041,13 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>>  static void
>>  vc4_crtc_reset(struct drm_crtc *crtc)
>>  {
>> -	if (crtc->state)
>> -		vc4_crtc_destroy_state(crtc->state);
>> +	struct vc4_crtc_state *crtc_state =
>> +		kzalloc(sizeof(*crtc_state), GFP_KERNEL);
>>  
>> -	crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
>>  	if (crtc->state)
>> -		crtc->state->crtc = crtc;
>> +		vc4_crtc_destroy_state(crtc, crtc->state);
>> +
>> +	__drm_atomic_helper_crtc_reset(crtc, &crtc_state->base);
> Wouldn't it be much easier if __drm_atomic_helper_crtc_reset took in a
> new state and destroyed the old state for you?  It seems like hardly a
> helper as is.

Yes it would, but the plane/connector reset is the same. If you want to convert them all it would be nice. :)

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

[PATCH 16/17] drm/vkms: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert vkms to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(),
call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 33 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 8a9aeb0a9ea8..550888e72c96 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -83,26 +83,6 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 	return true;
 }
 
-static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
-{
-	struct vkms_crtc_state *vkms_state = NULL;
-
-	if (crtc->state) {
-		vkms_state = to_vkms_crtc_state(crtc->state);
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-		kfree(vkms_state);
-		crtc->state = NULL;
-	}
-
-	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
-	if (!vkms_state)
-		return;
-	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
-
-	crtc->state = &vkms_state->base;
-	crtc->state->crtc = crtc;
-}
-
 static struct drm_crtc_state *
 vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
 {
@@ -135,6 +115,19 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 	}
 }
 
+static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
+{
+	struct vkms_crtc_state *vkms_state =
+		kzalloc(sizeof(*vkms_state), GFP_KERNEL);
+
+	if (crtc->state)
+		vkms_atomic_crtc_destroy_state(crtc, crtc->state);
+
+	__drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
+	if (vkms_state)
+		INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
+}
+
 static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.set_config             = drm_atomic_helper_set_config,
 	.destroy                = drm_crtc_cleanup,
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 16/17] drm/vkms: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Rodrigo Siqueira]

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

On 03/01, Maarten Lankhorst wrote:
> Convert vkms to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(),
> call it directly for freeing the old state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 33 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..550888e72c96 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -83,26 +83,6 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>  	return true;
>  }
>  
> -static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
> -{
> -	struct vkms_crtc_state *vkms_state = NULL;
> -
> -	if (crtc->state) {
> -		vkms_state = to_vkms_crtc_state(crtc->state);
> -		__drm_atomic_helper_crtc_destroy_state(crtc->state);
> -		kfree(vkms_state);
> -		crtc->state = NULL;
> -	}
> -
> -	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
> -	if (!vkms_state)
> -		return;
> -	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> -
> -	crtc->state = &vkms_state->base;
> -	crtc->state->crtc = crtc;
> -}
> -
>  static struct drm_crtc_state *
>  vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
> @@ -135,6 +115,19 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
>  	}
>  }
>  
> +static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct vkms_crtc_state *vkms_state =
> +		kzalloc(sizeof(*vkms_state), GFP_KERNEL);
> +
> +	if (crtc->state)
> +		vkms_atomic_crtc_destroy_state(crtc, crtc->state);
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
> +	if (vkms_state)
> +		INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
> +}
> +
>  static const struct drm_crtc_funcs vkms_crtc_funcs = {
>  	.set_config             = drm_atomic_helper_set_config,
>  	.destroy                = drm_crtc_cleanup,
> -- 
> 2.20.1
> 

Hi Maarten,

First of all, thanks for the patch :)

I tested it on my VM with the IGT test, and everything looks fine.

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 16/17] drm/vkms: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Op 06-03-2019 om 23:43 schreef Rodrigo Siqueira:
> On 03/01, Maarten Lankhorst wrote:
>> Convert vkms to using __drm_atomic_helper_crtc_reset(), instead of
>> writing its own version. Instead of open coding destroy_state(),
>> call it directly for freeing the old state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> ---
>>  drivers/gpu/drm/vkms/vkms_crtc.c | 33 +++++++++++++-------------------
>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
>> index 8a9aeb0a9ea8..550888e72c96 100644
>> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
>> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>> @@ -83,26 +83,6 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>>  	return true;
>>  }
>>  
>> -static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>> -{
>> -	struct vkms_crtc_state *vkms_state = NULL;
>> -
>> -	if (crtc->state) {
>> -		vkms_state = to_vkms_crtc_state(crtc->state);
>> -		__drm_atomic_helper_crtc_destroy_state(crtc->state);
>> -		kfree(vkms_state);
>> -		crtc->state = NULL;
>> -	}
>> -
>> -	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>> -	if (!vkms_state)
>> -		return;
>> -	INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
>> -
>> -	crtc->state = &vkms_state->base;
>> -	crtc->state->crtc = crtc;
>> -}
>> -
>>  static struct drm_crtc_state *
>>  vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
>>  {
>> @@ -135,6 +115,19 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
>>  	}
>>  }
>>  
>> +static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
>> +{
>> +	struct vkms_crtc_state *vkms_state =
>> +		kzalloc(sizeof(*vkms_state), GFP_KERNEL);
>> +
>> +	if (crtc->state)
>> +		vkms_atomic_crtc_destroy_state(crtc, crtc->state);
>> +
>> +	__drm_atomic_helper_crtc_reset(crtc, &vkms_state->base);
>> +	if (vkms_state)
>> +		INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
>> +}
>> +
>>  static const struct drm_crtc_funcs vkms_crtc_funcs = {
>>  	.set_config             = drm_atomic_helper_set_config,
>>  	.destroy                = drm_crtc_cleanup,
>> -- 
>> 2.20.1
>>
> Hi Maarten,
>
> First of all, thanks for the patch :)
>
> I tested it on my VM with the IGT test, and everything looks fine.
>
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
>
Thanks, pushed patches 1-3, and rockchip, tegra, msm, vkms, mali and i915 patches with reviews. :)

Will resend the rest of the series and address the feedback.

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

[PATCH 17/17] drm/vmwgfx: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Maarten Lankhorst]

Convert vmwgfx to using __drm_atomic_helper_crtc_reset(), instead of
writing its own version. Instead of open coding destroy_state(),
call it directly for freeing the old state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index ed2f67822f45..602f549f09f6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -611,24 +611,12 @@ vmw_du_crtc_duplicate_state(struct drm_crtc *crtc)
  */
 void vmw_du_crtc_reset(struct drm_crtc *crtc)
 {
-	struct vmw_crtc_state *vcs;
-
-
-	if (crtc->state) {
-		__drm_atomic_helper_crtc_destroy_state(crtc->state);
-
-		kfree(vmw_crtc_state_to_vcs(crtc->state));
-	}
+	struct vmw_crtc_state *vcs = kzalloc(sizeof(*vcs), GFP_KERNEL);
 
-	vcs = kzalloc(sizeof(*vcs), GFP_KERNEL);
-
-	if (!vcs) {
-		DRM_ERROR("Cannot allocate vmw_crtc_state\n");
-		return;
-	}
+	if (crtc->state)
+		vmw_du_crtc_destroy_state(crtc, crtc->state);
 
-	crtc->state = &vcs->base;
-	crtc->state->crtc = crtc;
+	__drm_atomic_helper_crtc_reset(crtc, &vcs->base);
 }
 
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx