| Message ID | 20190822180500.26608-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Clean up HDMI deep color handling a bit | expand |
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; > }
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
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; }