drm/i915: Clean up HDMI deep color handling a bit
diff mbox series

Message ID 20190822180500.26608-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Clean up HDMI deep color handling a bit
Related show

Commit Message

Ville Syrjälä Aug. 22, 2019, 6:05 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reogranize the HDMI deep color state computation to just
loop over possible bpc values. Avoids having to maintain
so many variants of the clock etc.

The current code also looks confused w.r.t. port_clock vs.
bw_constrained. It would happily update port_clock for
deep color but then not actually enable deep color due to
bw_constrained being set. The new logic handles that case
correctly.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdmi.c | 82 ++++++++++-------------
 1 file changed, 37 insertions(+), 45 deletions(-)

Comments

Jani Nikula Aug. 27, 2019, 9:30 a.m. UTC | #1
On Thu, 22 Aug 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reogranize the HDMI deep color state computation to just
> loop over possible bpc values. Avoids having to maintain
> so many variants of the clock etc.
>
> The current code also looks confused w.r.t. port_clock vs.
> bw_constrained. It would happily update port_clock for
> deep color but then not actually enable deep color due to
> bw_constrained being set. The new logic handles that case
> correctly.

Care to elaborate on that please?

Some nitpicking below.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 82 ++++++++++-------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index e02f0faecf02..ed1a7afc1ffd 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2262,8 +2262,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state,
>  static bool
>  intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>  			   struct intel_crtc_state *config,
> -			   int *clock_12bpc, int *clock_10bpc,
> -			   int *clock_8bpc)
> +			   int *clock)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(config->base.crtc);
>  
> @@ -2273,10 +2272,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>  	}
>  
>  	/* YCBCR420 TMDS rate requirement is half the pixel clock */
> -	config->port_clock /= 2;
> -	*clock_12bpc /= 2;
> -	*clock_10bpc /= 2;
> -	*clock_8bpc /= 2;
> +	*clock /= 2;
>  	config->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>  
>  	/* YCBCR 420 output conversion needs a scaler */
> @@ -2302,10 +2298,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
>  	struct intel_digital_connector_state *intel_conn_state =
>  		to_intel_digital_connector_state(conn_state);
> -	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> -	int clock_10bpc = clock_8bpc * 5 / 4;
> -	int clock_12bpc = clock_8bpc * 3 / 2;
> -	int desired_bpp;
> +	int bpc, clock = pipe_config->base.adjusted_mode.crtc_clock;
>  	bool force_dvi = intel_conn_state->force_audio == HDMI_AUDIO_OFF_DVI;
>  
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
> @@ -2330,15 +2323,11 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
>  		pipe_config->pixel_multiplier = 2;
> -		clock_8bpc *= 2;
> -		clock_10bpc *= 2;
> -		clock_12bpc *= 2;
> +		clock *= 2;
>  	}
>  
>  	if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) {
> -		if (!intel_hdmi_ycbcr420_config(connector, pipe_config,
> -						&clock_12bpc, &clock_10bpc,
> -						&clock_8bpc)) {
> +		if (!intel_hdmi_ycbcr420_config(connector, pipe_config, &clock)) {
>  			DRM_ERROR("Can't support YCBCR420 output\n");
>  			return -EINVAL;
>  		}
> @@ -2355,41 +2344,44 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  				intel_conn_state->force_audio == HDMI_AUDIO_ON;
>  	}
>  
> -	/*
> -	 * Note that g4x/vlv don't support 12bpc hdmi outputs. We also need
> -	 * to check that the higher clock still fits within limits.
> -	 */
> -	if (hdmi_deep_color_possible(pipe_config, 12) &&
> -	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc,
> -				  true, force_dvi) == MODE_OK) {
> -		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> -		desired_bpp = 12*3;
> -
> -		/* Need to adjust the port link by 1.5x for 12bpc. */
> -		pipe_config->port_clock = clock_12bpc;
> -	} else if (hdmi_deep_color_possible(pipe_config, 10) &&
> -		   hdmi_port_clock_valid(intel_hdmi, clock_10bpc,
> -					 true, force_dvi) == MODE_OK) {
> -		DRM_DEBUG_KMS("picking bpc to 10 for HDMI output\n");
> -		desired_bpp = 10 * 3;
> -
> -		/* Need to adjust the port link by 1.25x for 10bpc. */
> -		pipe_config->port_clock = clock_10bpc;
> -	} else {
> -		DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> -		desired_bpp = 8*3;
> +	/* for 8bpc */
> +	pipe_config->port_clock = clock;
>  
> -		pipe_config->port_clock = clock_8bpc;
> -	}
> +	for (bpc = 12; bpc > 8; bpc -= 2) {

So I generally don't like loops like this. You enter the loop with 12
and 10, don't enter with 8 but assume in later code that's what you got
if not 12 or 10. And, of course, you need to initialize
pipe_config->port_clock beforehand for that case too.

Perhaps abstracting the loop to a function would help.

Now, I think this is still an improvement on the status quo, so up to
you whether to do that now or later.

> +		int port_clock;
> +
> +		if (!hdmi_deep_color_possible(pipe_config, bpc))
> +			continue;
> +
> +		/*
> +		 * Need to adjust the port link by:
> +		 *  1.5x for 12bpc
> +		 *  1.25x for 10bpc
> +		 */
> +		port_clock = clock * bpc / 8;
> +
> +		if (hdmi_port_clock_valid(intel_hdmi, port_clock,
> +					  true, force_dvi) != MODE_OK)
> +			continue;
>  
> -	if (!pipe_config->bw_constrained) {
> -		DRM_DEBUG_KMS("forcing pipe bpp to %i for HDMI\n", desired_bpp);
> -		pipe_config->pipe_bpp = desired_bpp;
> +		pipe_config->port_clock = port_clock;
> +		break;
>  	}
>  
> +	/*
> +	 * pipe_bpp could already be below 8bpc due to
> +	 * FDI bandwidth constraints. We shouldn't bump it
> +	 * back up to 8bpc in that case.
> +	 */
> +	if (pipe_config->pipe_bpp > bpc * 3)
> +		pipe_config->pipe_bpp = bpc * 3;
> +	DRM_DEBUG_KMS("picking %dbpc for HDMI output (pipe bpp: %d)\n",
                                 ^

Put a space there?

Regardless of the nitpicks,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



> +		      bpc, pipe_config->pipe_bpp);
> +
>  	if (hdmi_port_clock_valid(intel_hdmi, pipe_config->port_clock,
>  				  false, force_dvi) != MODE_OK) {
> -		DRM_DEBUG_KMS("unsupported HDMI clock, rejecting mode\n");
> +		DRM_DEBUG_KMS("unsupported HDMI clock (%d kHz), rejecting mode\n",
> +			      pipe_config->port_clock);
>  		return -EINVAL;
>  	}
Ville Syrjälä Aug. 28, 2019, 5:38 p.m. UTC | #2
On Tue, Aug 27, 2019 at 12:30:52PM +0300, Jani Nikula wrote:
> On Thu, 22 Aug 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Reogranize the HDMI deep color state computation to just
> > loop over possible bpc values. Avoids having to maintain
> > so many variants of the clock etc.
> >
> > The current code also looks confused w.r.t. port_clock vs.
> > bw_constrained. It would happily update port_clock for
> > deep color but then not actually enable deep color due to
> > bw_constrained being set. The new logic handles that case
> > correctly.
> 
> Care to elaborate on that please?

Actually I guess it can't happen because:
- bw_constrained is for fdi only atm and no 10bpc hdmi on those
- we can never come here with pipe_bpp==12 + bw_constrained==true anyway

But looks like before commit 3fad10dbb688 ("drm/i915/icl: Fix setting
10 bit deep color mode") this could have been a problem because we could
have legitimately come here with:

pipe_bpp=10*3
bw_constrained=true
deep_color_possible(12) -> true
 -> port_clock = clock * 12 / 8;
if (!bw_constained)
	pipe_bpp = 12*3;

Ie. we'd have come out with with pipe_bpp=10*3 +
port_clock set up for 12 bpc.

> 
> Some nitpicking below.
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_hdmi.c | 82 ++++++++++-------------
> >  1 file changed, 37 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index e02f0faecf02..ed1a7afc1ffd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2262,8 +2262,7 @@ static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state,
> >  static bool
> >  intel_hdmi_ycbcr420_config(struct drm_connector *connector,
> >  			   struct intel_crtc_state *config,
> > -			   int *clock_12bpc, int *clock_10bpc,
> > -			   int *clock_8bpc)
> > +			   int *clock)
> >  {
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(config->base.crtc);
> >  
> > @@ -2273,10 +2272,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
> >  	}
> >  
> >  	/* YCBCR420 TMDS rate requirement is half the pixel clock */
> > -	config->port_clock /= 2;
> > -	*clock_12bpc /= 2;
> > -	*clock_10bpc /= 2;
> > -	*clock_8bpc /= 2;
> > +	*clock /= 2;
> >  	config->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> >  
> >  	/* YCBCR 420 output conversion needs a scaler */
> > @@ -2302,10 +2298,7 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> >  	struct intel_digital_connector_state *intel_conn_state =
> >  		to_intel_digital_connector_state(conn_state);
> > -	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> > -	int clock_10bpc = clock_8bpc * 5 / 4;
> > -	int clock_12bpc = clock_8bpc * 3 / 2;
> > -	int desired_bpp;
> > +	int bpc, clock = pipe_config->base.adjusted_mode.crtc_clock;
> >  	bool force_dvi = intel_conn_state->force_audio == HDMI_AUDIO_OFF_DVI;
> >  
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
> > @@ -2330,15 +2323,11 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
> >  		pipe_config->pixel_multiplier = 2;
> > -		clock_8bpc *= 2;
> > -		clock_10bpc *= 2;
> > -		clock_12bpc *= 2;
> > +		clock *= 2;
> >  	}
> >  
> >  	if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) {
> > -		if (!intel_hdmi_ycbcr420_config(connector, pipe_config,
> > -						&clock_12bpc, &clock_10bpc,
> > -						&clock_8bpc)) {
> > +		if (!intel_hdmi_ycbcr420_config(connector, pipe_config, &clock)) {
> >  			DRM_ERROR("Can't support YCBCR420 output\n");
> >  			return -EINVAL;
> >  		}
> > @@ -2355,41 +2344,44 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  				intel_conn_state->force_audio == HDMI_AUDIO_ON;
> >  	}
> >  
> > -	/*
> > -	 * Note that g4x/vlv don't support 12bpc hdmi outputs. We also need
> > -	 * to check that the higher clock still fits within limits.
> > -	 */
> > -	if (hdmi_deep_color_possible(pipe_config, 12) &&
> > -	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc,
> > -				  true, force_dvi) == MODE_OK) {
> > -		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> > -		desired_bpp = 12*3;
> > -
> > -		/* Need to adjust the port link by 1.5x for 12bpc. */
> > -		pipe_config->port_clock = clock_12bpc;
> > -	} else if (hdmi_deep_color_possible(pipe_config, 10) &&
> > -		   hdmi_port_clock_valid(intel_hdmi, clock_10bpc,
> > -					 true, force_dvi) == MODE_OK) {
> > -		DRM_DEBUG_KMS("picking bpc to 10 for HDMI output\n");
> > -		desired_bpp = 10 * 3;
> > -
> > -		/* Need to adjust the port link by 1.25x for 10bpc. */
> > -		pipe_config->port_clock = clock_10bpc;
> > -	} else {
> > -		DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> > -		desired_bpp = 8*3;
> > +	/* for 8bpc */
> > +	pipe_config->port_clock = clock;
> >  
> > -		pipe_config->port_clock = clock_8bpc;
> > -	}
> > +	for (bpc = 12; bpc > 8; bpc -= 2) {
> 
> So I generally don't like loops like this. You enter the loop with 12
> and 10, don't enter with 8 but assume in later code that's what you got
> if not 12 or 10. And, of course, you need to initialize
> pipe_config->port_clock beforehand for that case too.
> 
> Perhaps abstracting the loop to a function would help.

Hmm. I guess. Though maybe I should split even more junk into a few
new functions...

> 
> Now, I think this is still an improvement on the status quo, so up to
> you whether to do that now or later.
> 
> > +		int port_clock;
> > +
> > +		if (!hdmi_deep_color_possible(pipe_config, bpc))
> > +			continue;
> > +
> > +		/*
> > +		 * Need to adjust the port link by:
> > +		 *  1.5x for 12bpc
> > +		 *  1.25x for 10bpc
> > +		 */
> > +		port_clock = clock * bpc / 8;
> > +
> > +		if (hdmi_port_clock_valid(intel_hdmi, port_clock,
> > +					  true, force_dvi) != MODE_OK)
> > +			continue;
> >  
> > -	if (!pipe_config->bw_constrained) {
> > -		DRM_DEBUG_KMS("forcing pipe bpp to %i for HDMI\n", desired_bpp);
> > -		pipe_config->pipe_bpp = desired_bpp;
> > +		pipe_config->port_clock = port_clock;
> > +		break;
> >  	}
> >  
> > +	/*
> > +	 * pipe_bpp could already be below 8bpc due to
> > +	 * FDI bandwidth constraints. We shouldn't bump it
> > +	 * back up to 8bpc in that case.
> > +	 */
> > +	if (pipe_config->pipe_bpp > bpc * 3)
> > +		pipe_config->pipe_bpp = bpc * 3;
> > +	DRM_DEBUG_KMS("picking %dbpc for HDMI output (pipe bpp: %d)\n",
>                                  ^
> 
> Put a space there?

Maybe. The comments in the code are w/o the space, but I guess we could
stick it in there anyway.

> 
> Regardless of the nitpicks,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> 
> > +		      bpc, pipe_config->pipe_bpp);
> > +
> >  	if (hdmi_port_clock_valid(intel_hdmi, pipe_config->port_clock,
> >  				  false, force_dvi) != MODE_OK) {
> > -		DRM_DEBUG_KMS("unsupported HDMI clock, rejecting mode\n");
> > +		DRM_DEBUG_KMS("unsupported HDMI clock (%d kHz), rejecting mode\n",
> > +			      pipe_config->port_clock);
> >  		return -EINVAL;
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index e02f0faecf02..ed1a7afc1ffd 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2262,8 +2262,7 @@  static bool hdmi_deep_color_possible(const struct intel_crtc_state *crtc_state,
 static bool
 intel_hdmi_ycbcr420_config(struct drm_connector *connector,
 			   struct intel_crtc_state *config,
-			   int *clock_12bpc, int *clock_10bpc,
-			   int *clock_8bpc)
+			   int *clock)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(config->base.crtc);
 
@@ -2273,10 +2272,7 @@  intel_hdmi_ycbcr420_config(struct drm_connector *connector,
 	}
 
 	/* YCBCR420 TMDS rate requirement is half the pixel clock */
-	config->port_clock /= 2;
-	*clock_12bpc /= 2;
-	*clock_10bpc /= 2;
-	*clock_8bpc /= 2;
+	*clock /= 2;
 	config->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
 
 	/* YCBCR 420 output conversion needs a scaler */
@@ -2302,10 +2298,7 @@  int intel_hdmi_compute_config(struct intel_encoder *encoder,
 	struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
 	struct intel_digital_connector_state *intel_conn_state =
 		to_intel_digital_connector_state(conn_state);
-	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
-	int clock_10bpc = clock_8bpc * 5 / 4;
-	int clock_12bpc = clock_8bpc * 3 / 2;
-	int desired_bpp;
+	int bpc, clock = pipe_config->base.adjusted_mode.crtc_clock;
 	bool force_dvi = intel_conn_state->force_audio == HDMI_AUDIO_OFF_DVI;
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
@@ -2330,15 +2323,11 @@  int intel_hdmi_compute_config(struct intel_encoder *encoder,
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) {
 		pipe_config->pixel_multiplier = 2;
-		clock_8bpc *= 2;
-		clock_10bpc *= 2;
-		clock_12bpc *= 2;
+		clock *= 2;
 	}
 
 	if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) {
-		if (!intel_hdmi_ycbcr420_config(connector, pipe_config,
-						&clock_12bpc, &clock_10bpc,
-						&clock_8bpc)) {
+		if (!intel_hdmi_ycbcr420_config(connector, pipe_config, &clock)) {
 			DRM_ERROR("Can't support YCBCR420 output\n");
 			return -EINVAL;
 		}
@@ -2355,41 +2344,44 @@  int intel_hdmi_compute_config(struct intel_encoder *encoder,
 				intel_conn_state->force_audio == HDMI_AUDIO_ON;
 	}
 
-	/*
-	 * Note that g4x/vlv don't support 12bpc hdmi outputs. We also need
-	 * to check that the higher clock still fits within limits.
-	 */
-	if (hdmi_deep_color_possible(pipe_config, 12) &&
-	    hdmi_port_clock_valid(intel_hdmi, clock_12bpc,
-				  true, force_dvi) == MODE_OK) {
-		DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
-		desired_bpp = 12*3;
-
-		/* Need to adjust the port link by 1.5x for 12bpc. */
-		pipe_config->port_clock = clock_12bpc;
-	} else if (hdmi_deep_color_possible(pipe_config, 10) &&
-		   hdmi_port_clock_valid(intel_hdmi, clock_10bpc,
-					 true, force_dvi) == MODE_OK) {
-		DRM_DEBUG_KMS("picking bpc to 10 for HDMI output\n");
-		desired_bpp = 10 * 3;
-
-		/* Need to adjust the port link by 1.25x for 10bpc. */
-		pipe_config->port_clock = clock_10bpc;
-	} else {
-		DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
-		desired_bpp = 8*3;
+	/* for 8bpc */
+	pipe_config->port_clock = clock;
 
-		pipe_config->port_clock = clock_8bpc;
-	}
+	for (bpc = 12; bpc > 8; bpc -= 2) {
+		int port_clock;
+
+		if (!hdmi_deep_color_possible(pipe_config, bpc))
+			continue;
+
+		/*
+		 * Need to adjust the port link by:
+		 *  1.5x for 12bpc
+		 *  1.25x for 10bpc
+		 */
+		port_clock = clock * bpc / 8;
+
+		if (hdmi_port_clock_valid(intel_hdmi, port_clock,
+					  true, force_dvi) != MODE_OK)
+			continue;
 
-	if (!pipe_config->bw_constrained) {
-		DRM_DEBUG_KMS("forcing pipe bpp to %i for HDMI\n", desired_bpp);
-		pipe_config->pipe_bpp = desired_bpp;
+		pipe_config->port_clock = port_clock;
+		break;
 	}
 
+	/*
+	 * pipe_bpp could already be below 8bpc due to
+	 * FDI bandwidth constraints. We shouldn't bump it
+	 * back up to 8bpc in that case.
+	 */
+	if (pipe_config->pipe_bpp > bpc * 3)
+		pipe_config->pipe_bpp = bpc * 3;
+	DRM_DEBUG_KMS("picking %dbpc for HDMI output (pipe bpp: %d)\n",
+		      bpc, pipe_config->pipe_bpp);
+
 	if (hdmi_port_clock_valid(intel_hdmi, pipe_config->port_clock,
 				  false, force_dvi) != MODE_OK) {
-		DRM_DEBUG_KMS("unsupported HDMI clock, rejecting mode\n");
+		DRM_DEBUG_KMS("unsupported HDMI clock (%d kHz), rejecting mode\n",
+			      pipe_config->port_clock);
 		return -EINVAL;
 	}