diff mbox series

[v2,13/13] drm/i915: Pass the plane to icl_program_input_csc_coeff()

Message ID 20181114210729.16185-14-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Program SKL+ watermarks/ddb more carefully | expand

Commit Message

Ville Syrjala Nov. 14, 2018, 9:07 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On icl+ the plane state that gets passed to update_slave() is not
the plane state of the plane we're programming. With NV12 the
plane state would be coming from the master (UV) plane whereas
the plane we're programming is the slave (Y) plane. For that reason
we need to explicitly pass around the slave plane (or we'd have to
otherwise deduce it by checking whether we were called via
.update_plane() or .update_slave()).

In the case of icl_program_input_csc_coeff() it's actually OK to
assume that we are always the master plane because the input CSC
only exists on HDR planes which can never be a slave plane. But
for consistency let's pass in the plane explicitly anyway.

While at it drop the "_coeff" from the function name since it's
kinda redundant, and this makes the name a bit shorter :)

Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Shankar, Uma Nov. 15, 2018, 11:18 a.m. UTC | #1 
>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Thursday, November 15, 2018 2:37 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@intel.com>; Maarten Lankhorst
><maarten.lankhorst@linux.intel.com>
>Subject: [PATCH v2 13/13] drm/i915: Pass the plane to
>icl_program_input_csc_coeff()
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>On icl+ the plane state that gets passed to update_slave() is not the plane state of
>the plane we're programming. With NV12 the plane state would be coming from
>the master (UV) plane whereas the plane we're programming is the slave (Y)
>plane. For that reason we need to explicitly pass around the slave plane (or we'd
>have to otherwise deduce it by checking whether we were called via
>.update_plane() or .update_slave()).
>
>In the case of icl_program_input_csc_coeff() it's actually OK to assume that we
>are always the master plane because the input CSC only exists on HDR planes
>which can never be a slave plane. But for consistency let's pass in the plane
>explicitly anyway.
>
>While at it drop the "_coeff" from the function name since it's kinda redundant,
>and this makes the name a bit shorter :)


Changes look ok to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_sprite.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>b/drivers/gpu/drm/i915/intel_sprite.c
>index 0262159e7084..ee4c37a613f7 100644
>--- a/drivers/gpu/drm/i915/intel_sprite.c
>+++ b/drivers/gpu/drm/i915/intel_sprite.c
>@@ -373,14 +373,12 @@ skl_program_scaler(struct intel_plane *plane,
> #define  BOFF(x)          (((x) & 0xffff) << 16)
>
> static void
>-icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>-			    const struct intel_plane_state *plane_state)
>+icl_program_input_csc(struct intel_plane *plane,
>+		      const struct intel_crtc_state *crtc_state,
>+		      const struct intel_plane_state *plane_state)
> {
>-	struct drm_i915_private *dev_priv =
>-		to_i915(plane_state->base.plane->dev);
>-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>-	enum pipe pipe = crtc->pipe;
>-	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>+	enum pipe pipe = plane->pipe;
> 	enum plane_id plane_id = plane->id;
>
> 	static const u16 input_csc_matrix[][9] = { @@ -540,7 +538,7 @@
>skl_program_plane(struct intel_plane *plane,
> 			      plane_state->color_ctl);
>
> 	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
>-		icl_program_input_csc_coeff(crtc_state, plane_state);
>+		icl_program_input_csc(plane, crtc_state, plane_state);
>
> 	skl_write_plane_wm(plane, crtc_state);
>
>--
>2.18.1
Maarten Lankhorst Nov. 15, 2018, 12:37 p.m. UTC | #2 
Op 14-11-18 om 22:07 schreef Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On icl+ the plane state that gets passed to update_slave() is not
> the plane state of the plane we're programming. With NV12 the
> plane state would be coming from the master (UV) plane whereas
> the plane we're programming is the slave (Y) plane. For that reason
> we need to explicitly pass around the slave plane (or we'd have to
> otherwise deduce it by checking whether we were called via
> .update_plane() or .update_slave()).
>
> In the case of icl_program_input_csc_coeff() it's actually OK to
> assume that we are always the master plane because the input CSC
> only exists on HDR planes which can never be a slave plane. But
> for consistency let's pass in the plane explicitly anyway.
>
> While at it drop the "_coeff" from the function name since it's
> kinda redundant, and this makes the name a bit shorter :)
>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0262159e7084..ee4c37a613f7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -373,14 +373,12 @@ skl_program_scaler(struct intel_plane *plane,
>  #define  BOFF(x)          (((x) & 0xffff) << 16)
>  
>  static void
> -icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> -			    const struct intel_plane_state *plane_state)
> +icl_program_input_csc(struct intel_plane *plane,
> +		      const struct intel_crtc_state *crtc_state,
> +		      const struct intel_plane_state *plane_state)
>  {
> -	struct drm_i915_private *dev_priv =
> -		to_i915(plane_state->base.plane->dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -	enum pipe pipe = crtc->pipe;
> -	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	enum pipe pipe = plane->pipe;
>  	enum plane_id plane_id = plane->id;
>  
>  	static const u16 input_csc_matrix[][9] = {
> @@ -540,7 +538,7 @@ skl_program_plane(struct intel_plane *plane,
>  			      plane_state->color_ctl);
>  
>  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> -		icl_program_input_csc_coeff(crtc_state, plane_state);
> +		icl_program_input_csc(plane, crtc_state, plane_state);
>  
>  	skl_write_plane_wm(plane, crtc_state);
>  

Whole series looks good to me.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Matt Roper Nov. 26, 2018, 11:38 p.m. UTC | #3 
On Wed, Nov 14, 2018 at 11:07:29PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On icl+ the plane state that gets passed to update_slave() is not
> the plane state of the plane we're programming. With NV12 the
> plane state would be coming from the master (UV) plane whereas
> the plane we're programming is the slave (Y) plane. For that reason
> we need to explicitly pass around the slave plane (or we'd have to
> otherwise deduce it by checking whether we were called via
> .update_plane() or .update_slave()).
> 
> In the case of icl_program_input_csc_coeff() it's actually OK to
> assume that we are always the master plane because the input CSC
> only exists on HDR planes which can never be a slave plane. But
> for consistency let's pass in the plane explicitly anyway.
> 
> While at it drop the "_coeff" from the function name since it's
> kinda redundant, and this makes the name a bit shorter :)
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0262159e7084..ee4c37a613f7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -373,14 +373,12 @@ skl_program_scaler(struct intel_plane *plane,
>  #define  BOFF(x)          (((x) & 0xffff) << 16)
>  
>  static void
> -icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
> -			    const struct intel_plane_state *plane_state)
> +icl_program_input_csc(struct intel_plane *plane,
> +		      const struct intel_crtc_state *crtc_state,
> +		      const struct intel_plane_state *plane_state)
>  {
> -	struct drm_i915_private *dev_priv =
> -		to_i915(plane_state->base.plane->dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -	enum pipe pipe = crtc->pipe;
> -	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	enum pipe pipe = plane->pipe;
>  	enum plane_id plane_id = plane->id;
>  
>  	static const u16 input_csc_matrix[][9] = {
> @@ -540,7 +538,7 @@ skl_program_plane(struct intel_plane *plane,
>  			      plane_state->color_ctl);
>  
>  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> -		icl_program_input_csc_coeff(crtc_state, plane_state);
> +		icl_program_input_csc(plane, crtc_state, plane_state);
>  
>  	skl_write_plane_wm(plane, crtc_state);
>  
> -- 
> 2.18.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0262159e7084..ee4c37a613f7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -373,14 +373,12 @@  skl_program_scaler(struct intel_plane *plane,
 #define  BOFF(x)          (((x) & 0xffff) << 16)
 
 static void
-icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
-			    const struct intel_plane_state *plane_state)
+icl_program_input_csc(struct intel_plane *plane,
+		      const struct intel_crtc_state *crtc_state,
+		      const struct intel_plane_state *plane_state)
 {
-	struct drm_i915_private *dev_priv =
-		to_i915(plane_state->base.plane->dev);
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
-	enum pipe pipe = crtc->pipe;
-	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	enum pipe pipe = plane->pipe;
 	enum plane_id plane_id = plane->id;
 
 	static const u16 input_csc_matrix[][9] = {
@@ -540,7 +538,7 @@  skl_program_plane(struct intel_plane *plane,
 			      plane_state->color_ctl);
 
 	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
-		icl_program_input_csc_coeff(crtc_state, plane_state);
+		icl_program_input_csc(plane, crtc_state, plane_state);
 
 	skl_write_plane_wm(plane, crtc_state);