[4/5] drm/komeda: Remove layer_split property
diff mbox series

Message ID 20190705121006.26085-4-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/5] drm/komeda: Remove clock ratio property
Related show

Commit Message

Daniel Vetter July 5, 2019, 12:10 p.m. UTC
Properties are uapi like anything else, with all the usual rules
regarding review, testcases, open source userspace ... Furthermore
driver-private kms properties are highly discouraged, over the past
few years we've realized we need to make a serious effort at better
standardizing this stuff.

Again this probably needs multiple pieces to solve this properly:

- Instead of expecting userspace to compute this (and duplicating
  modeset code), the kernel driver should compute when it's necessary
  to enable layer_split mode to make a configuration possible. I.e. in
  komeda_plane_atomic_check() first try komeda_build_layer_data_flow()
  and if that fails, try komeda_build_layer_split_data_flow(), and set
  dflow.en_split accordingly. Assuming I understand somewhat correctly
  what this does.

- If this is needed for validation then you want a debugfs file to
  force this one way or the other, or alternatively  use
  ->atomic_print_state to dump such hidden driver-private state.
  Depends upon how you do your validation ofc.

Fixes: a407a6509393 ("drm/komeda: Add layer split support")
Cc: Lowry Li (Arm Technology China) <lowry.li@arm.com>
Cc: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mali DP Maintainers <malidp@foss.arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 -
 .../gpu/drm/arm/display/komeda/komeda_plane.c | 61 -------------------
 2 files changed, 64 deletions(-)

Comments

james qian wang (Arm Technology China) July 9, 2019, 8:08 a.m. UTC | #1
On Fri, Jul 05, 2019 at 02:10:05PM +0200, Daniel Vetter wrote:
> Properties are uapi like anything else, with all the usual rules
> regarding review, testcases, open source userspace ... Furthermore
> driver-private kms properties are highly discouraged, over the past
> few years we've realized we need to make a serious effort at better
> standardizing this stuff.
> 
> Again this probably needs multiple pieces to solve this properly:
> 
> - Instead of expecting userspace to compute this (and duplicating
>   modeset code), the kernel driver should compute when it's necessary
>   to enable layer_split mode to make a configuration possible. I.e. in
>   komeda_plane_atomic_check() first try komeda_build_layer_data_flow()
>   and if that fails, try komeda_build_layer_split_data_flow(), and set
>   dflow.en_split accordingly. Assuming I understand somewhat correctly
>   what this does.
> 
> - If this is needed for validation then you want a debugfs file to
>   force this one way or the other, or alternatively  use
>   ->atomic_print_state to dump such hidden driver-private state.
>   Depends upon how you do your validation ofc.
> 
> Fixes: a407a6509393 ("drm/komeda: Add layer split support")
> Cc: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> Cc: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mali DP Maintainers <malidp@foss.arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Hi Daniel:
Thank you for the patch!

Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

> ---
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 -
>  .../gpu/drm/arm/display/komeda/komeda_plane.c | 61 -------------------
>  2 files changed, 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index fb2adc233760..0c006576a25c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -33,9 +33,6 @@ struct komeda_plane {
>  	 * Layers with same capabilities.
>  	 */
>  	struct komeda_layer *layer;
> -
> -	/** @prop_layer_split: for on/off layer_split */
> -	struct drm_property *prop_layer_split;
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index 23db38650a46..5bb8553cc117 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -188,40 +188,6 @@ komeda_plane_atomic_destroy_state(struct drm_plane *plane,
>  	kfree(to_kplane_st(state));
>  }
>  
> -static int
> -komeda_plane_atomic_get_property(struct drm_plane *plane,
> -				 const struct drm_plane_state *state,
> -				 struct drm_property *property,
> -				 uint64_t *val)
> -{
> -	struct komeda_plane *kplane = to_kplane(plane);
> -	struct komeda_plane_state *st = to_kplane_st(state);
> -
> -	if (property == kplane->prop_layer_split)
> -		*val = st->layer_split;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int
> -komeda_plane_atomic_set_property(struct drm_plane *plane,
> -				 struct drm_plane_state *state,
> -				 struct drm_property *property,
> -				 uint64_t val)
> -{
> -	struct komeda_plane *kplane = to_kplane(plane);
> -	struct komeda_plane_state *st = to_kplane_st(state);
> -
> -	if (property == kplane->prop_layer_split)
> -		st->layer_split = !!val;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static bool
>  komeda_plane_format_mod_supported(struct drm_plane *plane,
>  				  u32 format, u64 modifier)
> @@ -241,32 +207,9 @@ static const struct drm_plane_funcs komeda_plane_funcs = {
>  	.reset			= komeda_plane_reset,
>  	.atomic_duplicate_state	= komeda_plane_atomic_duplicate_state,
>  	.atomic_destroy_state	= komeda_plane_atomic_destroy_state,
> -	.atomic_get_property	= komeda_plane_atomic_get_property,
> -	.atomic_set_property	= komeda_plane_atomic_set_property,
>  	.format_mod_supported	= komeda_plane_format_mod_supported,
>  };
>  
> -static int
> -komeda_plane_create_layer_properties(struct komeda_plane *kplane,
> -				     struct komeda_layer *layer)
> -{
> -	struct drm_device *drm = kplane->base.dev;
> -	struct drm_plane *plane = &kplane->base;
> -	struct drm_property *prop = NULL;
> -
> -	/* property: layer split */
> -	if (layer->right) {
> -		prop = drm_property_create_bool(drm, DRM_MODE_PROP_ATOMIC,
> -						"layer_split");
> -		if (!prop)
> -			return -ENOMEM;
> -		kplane->prop_layer_split = prop;
> -		drm_object_attach_property(&plane->base, prop, 0);
> -	}
> -
> -	return 0;
> -}
> -
>  /* for komeda, which is pipeline can be share between crtcs */
>  static u32 get_possible_crtcs(struct komeda_kms_dev *kms,
>  			      struct komeda_pipeline *pipe)
> @@ -360,10 +303,6 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>  	if (err)
>  		goto cleanup;
>  
> -	err = komeda_plane_create_layer_properties(kplane, layer);
> -	if (err)
> -		goto cleanup;
> -
>  	err = drm_plane_create_color_properties(plane,
>  			BIT(DRM_COLOR_YCBCR_BT601) |
>  			BIT(DRM_COLOR_YCBCR_BT709) |
> -- 
> 2.20.1

Patch
diff mbox series

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index fb2adc233760..0c006576a25c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -33,9 +33,6 @@  struct komeda_plane {
 	 * Layers with same capabilities.
 	 */
 	struct komeda_layer *layer;
-
-	/** @prop_layer_split: for on/off layer_split */
-	struct drm_property *prop_layer_split;
 };
 
 /**
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index 23db38650a46..5bb8553cc117 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -188,40 +188,6 @@  komeda_plane_atomic_destroy_state(struct drm_plane *plane,
 	kfree(to_kplane_st(state));
 }
 
-static int
-komeda_plane_atomic_get_property(struct drm_plane *plane,
-				 const struct drm_plane_state *state,
-				 struct drm_property *property,
-				 uint64_t *val)
-{
-	struct komeda_plane *kplane = to_kplane(plane);
-	struct komeda_plane_state *st = to_kplane_st(state);
-
-	if (property == kplane->prop_layer_split)
-		*val = st->layer_split;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
-static int
-komeda_plane_atomic_set_property(struct drm_plane *plane,
-				 struct drm_plane_state *state,
-				 struct drm_property *property,
-				 uint64_t val)
-{
-	struct komeda_plane *kplane = to_kplane(plane);
-	struct komeda_plane_state *st = to_kplane_st(state);
-
-	if (property == kplane->prop_layer_split)
-		st->layer_split = !!val;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
 static bool
 komeda_plane_format_mod_supported(struct drm_plane *plane,
 				  u32 format, u64 modifier)
@@ -241,32 +207,9 @@  static const struct drm_plane_funcs komeda_plane_funcs = {
 	.reset			= komeda_plane_reset,
 	.atomic_duplicate_state	= komeda_plane_atomic_duplicate_state,
 	.atomic_destroy_state	= komeda_plane_atomic_destroy_state,
-	.atomic_get_property	= komeda_plane_atomic_get_property,
-	.atomic_set_property	= komeda_plane_atomic_set_property,
 	.format_mod_supported	= komeda_plane_format_mod_supported,
 };
 
-static int
-komeda_plane_create_layer_properties(struct komeda_plane *kplane,
-				     struct komeda_layer *layer)
-{
-	struct drm_device *drm = kplane->base.dev;
-	struct drm_plane *plane = &kplane->base;
-	struct drm_property *prop = NULL;
-
-	/* property: layer split */
-	if (layer->right) {
-		prop = drm_property_create_bool(drm, DRM_MODE_PROP_ATOMIC,
-						"layer_split");
-		if (!prop)
-			return -ENOMEM;
-		kplane->prop_layer_split = prop;
-		drm_object_attach_property(&plane->base, prop, 0);
-	}
-
-	return 0;
-}
-
 /* for komeda, which is pipeline can be share between crtcs */
 static u32 get_possible_crtcs(struct komeda_kms_dev *kms,
 			      struct komeda_pipeline *pipe)
@@ -360,10 +303,6 @@  static int komeda_plane_add(struct komeda_kms_dev *kms,
 	if (err)
 		goto cleanup;
 
-	err = komeda_plane_create_layer_properties(kplane, layer);
-	if (err)
-		goto cleanup;
-
 	err = drm_plane_create_color_properties(plane,
 			BIT(DRM_COLOR_YCBCR_BT601) |
 			BIT(DRM_COLOR_YCBCR_BT709) |