| Message ID | 1539325394-20788-8-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v12,1/8] drm/i915: Introduce CRTC output format | expand |
On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote: > From: Shashank Sharma <shashank.sharma@linux.intel.com> > > LSPCON chips can generate YCBCR outputs, if asked nicely :). > > In order to generate YCBCR 4:2:0 outputs, a source must: > - send YCBCR 4:4:4 signals to LSPCON > - program color space as 4:2:0 in AVI infoframes > > Whereas for YCBCR 4:4:4 outputs, the source must: > - send YCBCR 4:4:4 signals to LSPCON > - program color space as 4:4:4 in AVI infoframes > > So for both 4:2:0 as well as 4:4:4 outputs, we are driving the > pipe for YCBCR 4:4:4 output, but AVI infoframe's color space > information indicates LSPCON FW to start scaling down from YCBCR > 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by > LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. > > V2: rebase > V3: Addressed review comments from Ville > - add enum crtc_output_format instead of bool ycbcr420 > - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output > cases in this way we will have YCBCR 4:4:4 framework ready (except > the ABI part) > V4: Added r-b from Maarten (for v3) > Addressed review comments from Ville: > - Do not add a non-atomic state variable to determine lspcon output. > Instead add bool in CRTC state to indicate lspcon based scaling. > V5: Addressed review comments from Ville: > - Change the state bool name from external scaling to something more > relavent. > - Keep the info and adjusted_mode structures const. > - use crtc_state instead of pipe_config. > - Push all the config change into lspcon_ycbcr420_config function. > V6: Rebase, small changes to accommodate changes in patch 2. > V7: Fixed checkpatch warnings for alignment > V8: Rebase > > PS: Ignored following warnings to match the current formatting: > drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON > -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721: > +#define TRANS_MSA_SAMPLING_444 (2<<1) > ^ > -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722: > +#define TRANS_MSA_CLRSP_YCBCR (2<<3) > V9: Rebase > V10: Rebase > V11: Rebase > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ > drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++ > 6 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2078541..1e13e51 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9229,6 +9229,8 @@ enum skl_power_gate { > #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) > > #define TRANS_MSA_SYNC_CLK (1 << 0) > +#define TRANS_MSA_SAMPLING_444 (2 << 1) > +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) > #define TRANS_MSA_6_BPC (0 << 5) > #define TRANS_MSA_8_BPC (1 << 5) > #define TRANS_MSA_10_BPC (2 << 5) > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index be21131..186111a 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) > break; > } > > + /* > + * As per DP 1.2 spec section 2.3.4.3 while sending > + * YCBCR 444 signals we should program MSA MISC1/0 fields with > + * colorspace information. The output colorspace encoding is BT601. > + */ > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) > + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3317c6c..41abd03 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; > > + pipe_config->lspcon_downsampling = false; > + > if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > > @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > else > output = INTEL_OUTPUT_FORMAT_YCBCR420; > } else { > + /* > + * Currently there is no interface defined to > + * check user preference between RGB/YCBCR444 > + * or YCBCR420. So the only possible case for > + * YCBCR444 usage is driving YCBCR420 output > + * with LSPCON, when pipe is configured for > + * YCBCR444 output and LSPCON takes care of > + * downsampling it. > + */ > + pipe_config->lspcon_downsampling = true; > output = INTEL_OUTPUT_FORMAT_YCBCR444; > } > } > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c3f6330..1156d59 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); > enum port port = encoder->port; > struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); > struct intel_connector *intel_connector = intel_dp->attached_connector; > @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, > pipe_config->has_pch_encoder = true; > > pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; > + if (lspcon->active) > + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); > + > pipe_config->has_drrs = false; > if (IS_G4X(dev_priv) || port == PORT_A) > pipe_config->has_audio = false; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index b8e5f95..95e252e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -908,6 +908,9 @@ struct intel_crtc_state { > > /* Output format RGB/YCBCR etc */ > enum intel_output_format output_format; > + > + /* Output down scaling is done in LSPCON device */ > + bool lspcon_downsampling; > }; > > struct intel_crtc { > @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > const struct drm_connector_state *conn_state); > bool lspcon_infoframe_enabled(struct intel_encoder *encoder, > const struct intel_crtc_state *pipe_config); > +void lspcon_ycbcr420_config(struct drm_connector *connector, > + struct intel_crtc_state *crtc_state); > > /* intel_pipe_crc.c */ > #ifdef CONFIG_DEBUG_FS > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > index 829c40a..fff32b3 100644 > --- a/drivers/gpu/drm/i915/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) > return true; > } > > +void lspcon_ycbcr420_config(struct drm_connector *connector, > + struct intel_crtc_state *crtc_state) > +{ > + const struct drm_display_info *info = &connector->display_info; > + const struct drm_display_mode *adjusted_mode = > + &crtc_state->base.adjusted_mode; > + > + if (drm_mode_is_420_only(info, adjusted_mode) && > + connector->ycbcr_420_allowed) { > + crtc_state->port_clock /= 2; This looks bogus. We're talking about DP here so we should not be frobbing port_clock. And since we output 4:4:4 from the port anyway we don't even have to take 4:2:0 into consideration when calculating port_clock. Ah, this guy is called before we even calculate port_clock. That explains why this didn't cause any problems. So this is just dead code here. And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() also dead code since the caller overwrites it there as well. > + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; > + crtc_state->lspcon_downsampling = true; > + } > +} > + > static bool lspcon_probe(struct intel_lspcon *lspcon) > { > int retry; > @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > return; > } > > + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { > + if (crtc_state->lspcon_downsampling) > + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > + else > + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; > + } else { > + frame.avi.colorspace = HDMI_COLORSPACE_RGB; > + } > + > drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, > crtc_state->limited_color_range ? > HDMI_QUANTIZATION_RANGE_LIMITED : > @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > struct intel_lspcon *lspcon = &intel_dig_port->lspcon; > struct drm_device *dev = intel_dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_connector *connector = &dp->attached_connector->base; > > if (!HAS_LSPCON(dev_priv)) { > DRM_ERROR("LSPCON is not supported on this platform\n"); > @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > return false; > } > > + connector->ycbcr_420_allowed = true; > lspcon->active = true; > DRM_DEBUG_KMS("Success: LSPCON init\n"); > return true; > -- > 2.7.4
Regards Shashank On 10/12/2018 7:28 PM, Ville Syrjälä wrote: > On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote: >> From: Shashank Sharma <shashank.sharma@linux.intel.com> >> >> LSPCON chips can generate YCBCR outputs, if asked nicely :). >> >> In order to generate YCBCR 4:2:0 outputs, a source must: >> - send YCBCR 4:4:4 signals to LSPCON >> - program color space as 4:2:0 in AVI infoframes >> >> Whereas for YCBCR 4:4:4 outputs, the source must: >> - send YCBCR 4:4:4 signals to LSPCON >> - program color space as 4:4:4 in AVI infoframes >> >> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the >> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space >> information indicates LSPCON FW to start scaling down from YCBCR >> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by >> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. >> >> V2: rebase >> V3: Addressed review comments from Ville >> - add enum crtc_output_format instead of bool ycbcr420 >> - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output >> cases in this way we will have YCBCR 4:4:4 framework ready (except >> the ABI part) >> V4: Added r-b from Maarten (for v3) >> Addressed review comments from Ville: >> - Do not add a non-atomic state variable to determine lspcon output. >> Instead add bool in CRTC state to indicate lspcon based scaling. >> V5: Addressed review comments from Ville: >> - Change the state bool name from external scaling to something more >> relavent. >> - Keep the info and adjusted_mode structures const. >> - use crtc_state instead of pipe_config. >> - Push all the config change into lspcon_ycbcr420_config function. >> V6: Rebase, small changes to accommodate changes in patch 2. >> V7: Fixed checkpatch warnings for alignment >> V8: Rebase >> >> PS: Ignored following warnings to match the current formatting: >> drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON >> -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) >> #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721: >> +#define TRANS_MSA_SAMPLING_444 (2<<1) >> ^ >> -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) >> #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722: >> +#define TRANS_MSA_CLRSP_YCBCR (2<<3) >> V9: Rebase >> V10: Rebase >> V11: Rebase >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ >> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 4 ++++ >> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ >> drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++ >> 6 files changed, 56 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 2078541..1e13e51 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -9229,6 +9229,8 @@ enum skl_power_gate { >> #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) >> >> #define TRANS_MSA_SYNC_CLK (1 << 0) >> +#define TRANS_MSA_SAMPLING_444 (2 << 1) >> +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) >> #define TRANS_MSA_6_BPC (0 << 5) >> #define TRANS_MSA_8_BPC (1 << 5) >> #define TRANS_MSA_10_BPC (2 << 5) >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index be21131..186111a 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) >> break; >> } >> >> + /* >> + * As per DP 1.2 spec section 2.3.4.3 while sending >> + * YCBCR 444 signals we should program MSA MISC1/0 fields with >> + * colorspace information. The output colorspace encoding is BT601. >> + */ >> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) >> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 3317c6c..41abd03 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; >> >> + pipe_config->lspcon_downsampling = false; >> + >> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { >> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); >> >> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, >> else >> output = INTEL_OUTPUT_FORMAT_YCBCR420; >> } else { >> + /* >> + * Currently there is no interface defined to >> + * check user preference between RGB/YCBCR444 >> + * or YCBCR420. So the only possible case for >> + * YCBCR444 usage is driving YCBCR420 output >> + * with LSPCON, when pipe is configured for >> + * YCBCR444 output and LSPCON takes care of >> + * downsampling it. >> + */ >> + pipe_config->lspcon_downsampling = true; >> output = INTEL_OUTPUT_FORMAT_YCBCR444; >> } >> } >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index c3f6330..1156d59 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); >> enum port port = encoder->port; >> struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); >> struct intel_connector *intel_connector = intel_dp->attached_connector; >> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, >> pipe_config->has_pch_encoder = true; >> >> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; >> + if (lspcon->active) >> + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); >> + >> pipe_config->has_drrs = false; >> if (IS_G4X(dev_priv) || port == PORT_A) >> pipe_config->has_audio = false; >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index b8e5f95..95e252e 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -908,6 +908,9 @@ struct intel_crtc_state { >> >> /* Output format RGB/YCBCR etc */ >> enum intel_output_format output_format; >> + >> + /* Output down scaling is done in LSPCON device */ >> + bool lspcon_downsampling; >> }; >> >> struct intel_crtc { >> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, >> const struct drm_connector_state *conn_state); >> bool lspcon_infoframe_enabled(struct intel_encoder *encoder, >> const struct intel_crtc_state *pipe_config); >> +void lspcon_ycbcr420_config(struct drm_connector *connector, >> + struct intel_crtc_state *crtc_state); >> >> /* intel_pipe_crc.c */ >> #ifdef CONFIG_DEBUG_FS >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >> index 829c40a..fff32b3 100644 >> --- a/drivers/gpu/drm/i915/intel_lspcon.c >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) >> return true; >> } >> >> +void lspcon_ycbcr420_config(struct drm_connector *connector, >> + struct intel_crtc_state *crtc_state) >> +{ >> + const struct drm_display_info *info = &connector->display_info; >> + const struct drm_display_mode *adjusted_mode = >> + &crtc_state->base.adjusted_mode; >> + >> + if (drm_mode_is_420_only(info, adjusted_mode) && >> + connector->ycbcr_420_allowed) { >> + crtc_state->port_clock /= 2; > This looks bogus. We're talking about DP here so we should not be > frobbing port_clock. And since we output 4:4:4 from the port > anyway we don't even have to take 4:2:0 into consideration > when calculating port_clock. I agree on this. > Ah, this guy is called before we even calculate port_clock. > That explains why this didn't cause any problems. So this is > just dead code here. > > And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() > also dead code since the caller overwrites it there as well. I am not sure if that's the case for Native HDMI 2.0. In intel_hdmi_420_config we are updating the config->port_clock, which is the same thing being updated for 8/12/16 BPC deep color too, just after this function. I have tested the output with HDMI 2.0 analyzer, which reported right pixel clock (297Mhz). Now, as any of the port clock calculations do not get information about HDMI output type, there is no other way they will calculate the right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is active. - Shashank >> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; >> + crtc_state->lspcon_downsampling = true; >> + } >> +} >> + >> static bool lspcon_probe(struct intel_lspcon *lspcon) >> { >> int retry; >> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, >> return; >> } >> >> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { >> + if (crtc_state->lspcon_downsampling) >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >> + else >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; >> + } else { >> + frame.avi.colorspace = HDMI_COLORSPACE_RGB; >> + } >> + >> drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, >> crtc_state->limited_color_range ? >> HDMI_QUANTIZATION_RANGE_LIMITED : >> @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >> struct intel_lspcon *lspcon = &intel_dig_port->lspcon; >> struct drm_device *dev = intel_dig_port->base.base.dev; >> struct drm_i915_private *dev_priv = to_i915(dev); >> + struct drm_connector *connector = &dp->attached_connector->base; >> >> if (!HAS_LSPCON(dev_priv)) { >> DRM_ERROR("LSPCON is not supported on this platform\n"); >> @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >> return false; >> } >> >> + connector->ycbcr_420_allowed = true; >> lspcon->active = true; >> DRM_DEBUG_KMS("Success: LSPCON init\n"); >> return true; >> -- >> 2.7.4
On Fri, Oct 12, 2018 at 11:21:56PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 10/12/2018 7:28 PM, Ville Syrjälä wrote: > > On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote: > >> From: Shashank Sharma <shashank.sharma@linux.intel.com> > >> > >> LSPCON chips can generate YCBCR outputs, if asked nicely :). > >> > >> In order to generate YCBCR 4:2:0 outputs, a source must: > >> - send YCBCR 4:4:4 signals to LSPCON > >> - program color space as 4:2:0 in AVI infoframes > >> > >> Whereas for YCBCR 4:4:4 outputs, the source must: > >> - send YCBCR 4:4:4 signals to LSPCON > >> - program color space as 4:4:4 in AVI infoframes > >> > >> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the > >> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space > >> information indicates LSPCON FW to start scaling down from YCBCR > >> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by > >> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. > >> > >> V2: rebase > >> V3: Addressed review comments from Ville > >> - add enum crtc_output_format instead of bool ycbcr420 > >> - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output > >> cases in this way we will have YCBCR 4:4:4 framework ready (except > >> the ABI part) > >> V4: Added r-b from Maarten (for v3) > >> Addressed review comments from Ville: > >> - Do not add a non-atomic state variable to determine lspcon output. > >> Instead add bool in CRTC state to indicate lspcon based scaling. > >> V5: Addressed review comments from Ville: > >> - Change the state bool name from external scaling to something more > >> relavent. > >> - Keep the info and adjusted_mode structures const. > >> - use crtc_state instead of pipe_config. > >> - Push all the config change into lspcon_ycbcr420_config function. > >> V6: Rebase, small changes to accommodate changes in patch 2. > >> V7: Fixed checkpatch warnings for alignment > >> V8: Rebase > >> > >> PS: Ignored following warnings to match the current formatting: > >> drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON > >> -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > >> #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721: > >> +#define TRANS_MSA_SAMPLING_444 (2<<1) > >> ^ > >> -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > >> #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722: > >> +#define TRANS_MSA_CLRSP_YCBCR (2<<3) > >> V9: Rebase > >> V10: Rebase > >> V11: Rebase > >> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ > >> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ > >> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ > >> drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > >> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > >> drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++ > >> 6 files changed, 56 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 2078541..1e13e51 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -9229,6 +9229,8 @@ enum skl_power_gate { > >> #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) > >> > >> #define TRANS_MSA_SYNC_CLK (1 << 0) > >> +#define TRANS_MSA_SAMPLING_444 (2 << 1) > >> +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) > >> #define TRANS_MSA_6_BPC (0 << 5) > >> #define TRANS_MSA_8_BPC (1 << 5) > >> #define TRANS_MSA_10_BPC (2 << 5) > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index be21131..186111a 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) > >> break; > >> } > >> > >> + /* > >> + * As per DP 1.2 spec section 2.3.4.3 while sending > >> + * YCBCR 444 signals we should program MSA MISC1/0 fields with > >> + * colorspace information. The output colorspace encoding is BT601. > >> + */ > >> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) > >> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > >> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 3317c6c..41abd03 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; > >> > >> + pipe_config->lspcon_downsampling = false; > >> + > >> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > >> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > >> > >> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > >> else > >> output = INTEL_OUTPUT_FORMAT_YCBCR420; > >> } else { > >> + /* > >> + * Currently there is no interface defined to > >> + * check user preference between RGB/YCBCR444 > >> + * or YCBCR420. So the only possible case for > >> + * YCBCR444 usage is driving YCBCR420 output > >> + * with LSPCON, when pipe is configured for > >> + * YCBCR444 output and LSPCON takes care of > >> + * downsampling it. > >> + */ > >> + pipe_config->lspcon_downsampling = true; > >> output = INTEL_OUTPUT_FORMAT_YCBCR444; > >> } > >> } > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index c3f6330..1156d59 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, > >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; > >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); > >> enum port port = encoder->port; > >> struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); > >> struct intel_connector *intel_connector = intel_dp->attached_connector; > >> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, > >> pipe_config->has_pch_encoder = true; > >> > >> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; > >> + if (lspcon->active) > >> + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); > >> + > >> pipe_config->has_drrs = false; > >> if (IS_G4X(dev_priv) || port == PORT_A) > >> pipe_config->has_audio = false; > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index b8e5f95..95e252e 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -908,6 +908,9 @@ struct intel_crtc_state { > >> > >> /* Output format RGB/YCBCR etc */ > >> enum intel_output_format output_format; > >> + > >> + /* Output down scaling is done in LSPCON device */ > >> + bool lspcon_downsampling; > >> }; > >> > >> struct intel_crtc { > >> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > >> const struct drm_connector_state *conn_state); > >> bool lspcon_infoframe_enabled(struct intel_encoder *encoder, > >> const struct intel_crtc_state *pipe_config); > >> +void lspcon_ycbcr420_config(struct drm_connector *connector, > >> + struct intel_crtc_state *crtc_state); > >> > >> /* intel_pipe_crc.c */ > >> #ifdef CONFIG_DEBUG_FS > >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > >> index 829c40a..fff32b3 100644 > >> --- a/drivers/gpu/drm/i915/intel_lspcon.c > >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c > >> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) > >> return true; > >> } > >> > >> +void lspcon_ycbcr420_config(struct drm_connector *connector, > >> + struct intel_crtc_state *crtc_state) > >> +{ > >> + const struct drm_display_info *info = &connector->display_info; > >> + const struct drm_display_mode *adjusted_mode = > >> + &crtc_state->base.adjusted_mode; > >> + > >> + if (drm_mode_is_420_only(info, adjusted_mode) && > >> + connector->ycbcr_420_allowed) { > >> + crtc_state->port_clock /= 2; > > This looks bogus. We're talking about DP here so we should not be > > frobbing port_clock. And since we output 4:4:4 from the port > > anyway we don't even have to take 4:2:0 into consideration > > when calculating port_clock. > I agree on this. > > Ah, this guy is called before we even calculate port_clock. > > That explains why this didn't cause any problems. So this is > > just dead code here. > > > > And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() > > also dead code since the caller overwrites it there as well. > I am not sure if that's the case for Native HDMI 2.0. > In intel_hdmi_420_config we are updating the config->port_clock, which > is the same thing being updated for 8/12/16 BPC deep color too, just > after this function. > I have tested the output with HDMI 2.0 analyzer, which reported right > pixel clock (297Mhz). Now, as any of the port clock calculations do not > get information > about HDMI output type, there is no other way they will calculate the > right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is > active. It effectively does this: 1. port_clock = who knows 2. port_clock /= 2; 3. if (12bpc) port_clock = clock_12bpc; else if (10bpc) port_clock = clock_10bpc; else port_clock = clock_8bpc; Ie. step 2. is dead code. > > - Shashank > >> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; > >> + crtc_state->lspcon_downsampling = true; > >> + } > >> +} > >> + > >> static bool lspcon_probe(struct intel_lspcon *lspcon) > >> { > >> int retry; > >> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > >> return; > >> } > >> > >> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { > >> + if (crtc_state->lspcon_downsampling) > >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > >> + else > >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; > >> + } else { > >> + frame.avi.colorspace = HDMI_COLORSPACE_RGB; > >> + } > >> + > >> drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, > >> crtc_state->limited_color_range ? > >> HDMI_QUANTIZATION_RANGE_LIMITED : > >> @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > >> struct intel_lspcon *lspcon = &intel_dig_port->lspcon; > >> struct drm_device *dev = intel_dig_port->base.base.dev; > >> struct drm_i915_private *dev_priv = to_i915(dev); > >> + struct drm_connector *connector = &dp->attached_connector->base; > >> > >> if (!HAS_LSPCON(dev_priv)) { > >> DRM_ERROR("LSPCON is not supported on this platform\n"); > >> @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > >> return false; > >> } > >> > >> + connector->ycbcr_420_allowed = true; > >> lspcon->active = true; > >> DRM_DEBUG_KMS("Success: LSPCON init\n"); > >> return true; > >> -- > >> 2.7.4
Regards Shashank On 10/12/2018 11:39 PM, Ville Syrjälä wrote: > On Fri, Oct 12, 2018 at 11:21:56PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 10/12/2018 7:28 PM, Ville Syrjälä wrote: >>> On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote: >>>> From: Shashank Sharma <shashank.sharma@linux.intel.com> >>>> >>>> LSPCON chips can generate YCBCR outputs, if asked nicely :). >>>> >>>> In order to generate YCBCR 4:2:0 outputs, a source must: >>>> - send YCBCR 4:4:4 signals to LSPCON >>>> - program color space as 4:2:0 in AVI infoframes >>>> >>>> Whereas for YCBCR 4:4:4 outputs, the source must: >>>> - send YCBCR 4:4:4 signals to LSPCON >>>> - program color space as 4:4:4 in AVI infoframes >>>> >>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the >>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space >>>> information indicates LSPCON FW to start scaling down from YCBCR >>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by >>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. >>>> >>>> V2: rebase >>>> V3: Addressed review comments from Ville >>>> - add enum crtc_output_format instead of bool ycbcr420 >>>> - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output >>>> cases in this way we will have YCBCR 4:4:4 framework ready (except >>>> the ABI part) >>>> V4: Added r-b from Maarten (for v3) >>>> Addressed review comments from Ville: >>>> - Do not add a non-atomic state variable to determine lspcon output. >>>> Instead add bool in CRTC state to indicate lspcon based scaling. >>>> V5: Addressed review comments from Ville: >>>> - Change the state bool name from external scaling to something more >>>> relavent. >>>> - Keep the info and adjusted_mode structures const. >>>> - use crtc_state instead of pipe_config. >>>> - Push all the config change into lspcon_ycbcr420_config function. >>>> V6: Rebase, small changes to accommodate changes in patch 2. >>>> V7: Fixed checkpatch warnings for alignment >>>> V8: Rebase >>>> >>>> PS: Ignored following warnings to match the current formatting: >>>> drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON >>>> -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) >>>> #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721: >>>> +#define TRANS_MSA_SAMPLING_444 (2<<1) >>>> ^ >>>> -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) >>>> #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722: >>>> +#define TRANS_MSA_CLRSP_YCBCR (2<<3) >>>> V9: Rebase >>>> V10: Rebase >>>> V11: Rebase >>>> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ >>>> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ >>>> drivers/gpu/drm/i915/intel_dp.c | 4 ++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ >>>> drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++ >>>> 6 files changed, 56 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 2078541..1e13e51 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -9229,6 +9229,8 @@ enum skl_power_gate { >>>> #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) >>>> >>>> #define TRANS_MSA_SYNC_CLK (1 << 0) >>>> +#define TRANS_MSA_SAMPLING_444 (2 << 1) >>>> +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) >>>> #define TRANS_MSA_6_BPC (0 << 5) >>>> #define TRANS_MSA_8_BPC (1 << 5) >>>> #define TRANS_MSA_10_BPC (2 << 5) >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>> index be21131..186111a 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) >>>> break; >>>> } >>>> >>>> + /* >>>> + * As per DP 1.2 spec section 2.3.4.3 while sending >>>> + * YCBCR 444 signals we should program MSA MISC1/0 fields with >>>> + * colorspace information. The output colorspace encoding is BT601. >>>> + */ >>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 3317c6c..41abd03 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, >>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>>> enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; >>>> >>>> + pipe_config->lspcon_downsampling = false; >>>> + >>>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { >>>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); >>>> >>>> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, >>>> else >>>> output = INTEL_OUTPUT_FORMAT_YCBCR420; >>>> } else { >>>> + /* >>>> + * Currently there is no interface defined to >>>> + * check user preference between RGB/YCBCR444 >>>> + * or YCBCR420. So the only possible case for >>>> + * YCBCR444 usage is driving YCBCR420 output >>>> + * with LSPCON, when pipe is configured for >>>> + * YCBCR444 output and LSPCON takes care of >>>> + * downsampling it. >>>> + */ >>>> + pipe_config->lspcon_downsampling = true; >>>> output = INTEL_OUTPUT_FORMAT_YCBCR444; >>>> } >>>> } >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index c3f6330..1156d59 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, >>>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >>>> struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; >>>> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >>>> + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); >>>> enum port port = encoder->port; >>>> struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); >>>> struct intel_connector *intel_connector = intel_dp->attached_connector; >>>> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, >>>> pipe_config->has_pch_encoder = true; >>>> >>>> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; >>>> + if (lspcon->active) >>>> + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); >>>> + >>>> pipe_config->has_drrs = false; >>>> if (IS_G4X(dev_priv) || port == PORT_A) >>>> pipe_config->has_audio = false; >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index b8e5f95..95e252e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -908,6 +908,9 @@ struct intel_crtc_state { >>>> >>>> /* Output format RGB/YCBCR etc */ >>>> enum intel_output_format output_format; >>>> + >>>> + /* Output down scaling is done in LSPCON device */ >>>> + bool lspcon_downsampling; >>>> }; >>>> >>>> struct intel_crtc { >>>> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, >>>> const struct drm_connector_state *conn_state); >>>> bool lspcon_infoframe_enabled(struct intel_encoder *encoder, >>>> const struct intel_crtc_state *pipe_config); >>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, >>>> + struct intel_crtc_state *crtc_state); >>>> >>>> /* intel_pipe_crc.c */ >>>> #ifdef CONFIG_DEBUG_FS >>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >>>> index 829c40a..fff32b3 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c >>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >>>> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) >>>> return true; >>>> } >>>> >>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, >>>> + struct intel_crtc_state *crtc_state) >>>> +{ >>>> + const struct drm_display_info *info = &connector->display_info; >>>> + const struct drm_display_mode *adjusted_mode = >>>> + &crtc_state->base.adjusted_mode; >>>> + >>>> + if (drm_mode_is_420_only(info, adjusted_mode) && >>>> + connector->ycbcr_420_allowed) { >>>> + crtc_state->port_clock /= 2; >>> This looks bogus. We're talking about DP here so we should not be >>> frobbing port_clock. And since we output 4:4:4 from the port >>> anyway we don't even have to take 4:2:0 into consideration >>> when calculating port_clock. >> I agree on this. >>> Ah, this guy is called before we even calculate port_clock. >>> That explains why this didn't cause any problems. So this is >>> just dead code here. >>> >>> And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() >>> also dead code since the caller overwrites it there as well. >> I am not sure if that's the case for Native HDMI 2.0. >> In intel_hdmi_420_config we are updating the config->port_clock, which >> is the same thing being updated for 8/12/16 BPC deep color too, just >> after this function. >> I have tested the output with HDMI 2.0 analyzer, which reported right >> pixel clock (297Mhz). Now, as any of the port clock calculations do not >> get information >> about HDMI output type, there is no other way they will calculate the >> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is >> active. > It effectively does this: > > 1. port_clock = who knows > 2. port_clock /= 2; Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally doing this also: clock_8bpc /= 2; clock_10bpc /= 2; clock_12bpc /= 2; > 3. if (12bpc) > port_clock = clock_12bpc; > else if (10bpc) > port_clock = clock_10bpc; > else > port_clock = clock_8bpc; This means effectively the selected clock is /2, so the code is not dead (thankfully :-)) - Shashank > > Ie. step 2. is dead code. > >> - Shashank >>>> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; >>>> + crtc_state->lspcon_downsampling = true; >>>> + } >>>> +} >>>> + >>>> static bool lspcon_probe(struct intel_lspcon *lspcon) >>>> { >>>> int retry; >>>> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, >>>> return; >>>> } >>>> >>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { >>>> + if (crtc_state->lspcon_downsampling) >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >>>> + else >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; >>>> + } else { >>>> + frame.avi.colorspace = HDMI_COLORSPACE_RGB; >>>> + } >>>> + >>>> drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, >>>> crtc_state->limited_color_range ? >>>> HDMI_QUANTIZATION_RANGE_LIMITED : >>>> @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >>>> struct intel_lspcon *lspcon = &intel_dig_port->lspcon; >>>> struct drm_device *dev = intel_dig_port->base.base.dev; >>>> struct drm_i915_private *dev_priv = to_i915(dev); >>>> + struct drm_connector *connector = &dp->attached_connector->base; >>>> >>>> if (!HAS_LSPCON(dev_priv)) { >>>> DRM_ERROR("LSPCON is not supported on this platform\n"); >>>> @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >>>> return false; >>>> } >>>> >>>> + connector->ycbcr_420_allowed = true; >>>> lspcon->active = true; >>>> DRM_DEBUG_KMS("Success: LSPCON init\n"); >>>> return true; >>>> -- >>>> 2.7.4
On Sat, Oct 13, 2018 at 12:02:25AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 10/12/2018 11:39 PM, Ville Syrjälä wrote: > > On Fri, Oct 12, 2018 at 11:21:56PM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> > >> On 10/12/2018 7:28 PM, Ville Syrjälä wrote: > >>> On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote: > >>>> From: Shashank Sharma <shashank.sharma@linux.intel.com> > >>>> > >>>> LSPCON chips can generate YCBCR outputs, if asked nicely :). > >>>> > >>>> In order to generate YCBCR 4:2:0 outputs, a source must: > >>>> - send YCBCR 4:4:4 signals to LSPCON > >>>> - program color space as 4:2:0 in AVI infoframes > >>>> > >>>> Whereas for YCBCR 4:4:4 outputs, the source must: > >>>> - send YCBCR 4:4:4 signals to LSPCON > >>>> - program color space as 4:4:4 in AVI infoframes > >>>> > >>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the > >>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space > >>>> information indicates LSPCON FW to start scaling down from YCBCR > >>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by > >>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. > >>>> > >>>> V2: rebase > >>>> V3: Addressed review comments from Ville > >>>> - add enum crtc_output_format instead of bool ycbcr420 > >>>> - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output > >>>> cases in this way we will have YCBCR 4:4:4 framework ready (except > >>>> the ABI part) > >>>> V4: Added r-b from Maarten (for v3) > >>>> Addressed review comments from Ville: > >>>> - Do not add a non-atomic state variable to determine lspcon output. > >>>> Instead add bool in CRTC state to indicate lspcon based scaling. > >>>> V5: Addressed review comments from Ville: > >>>> - Change the state bool name from external scaling to something more > >>>> relavent. > >>>> - Keep the info and adjusted_mode structures const. > >>>> - use crtc_state instead of pipe_config. > >>>> - Push all the config change into lspcon_ycbcr420_config function. > >>>> V6: Rebase, small changes to accommodate changes in patch 2. > >>>> V7: Fixed checkpatch warnings for alignment > >>>> V8: Rebase > >>>> > >>>> PS: Ignored following warnings to match the current formatting: > >>>> drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON > >>>> -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > >>>> #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721: > >>>> +#define TRANS_MSA_SAMPLING_444 (2<<1) > >>>> ^ > >>>> -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > >>>> #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722: > >>>> +#define TRANS_MSA_CLRSP_YCBCR (2<<3) > >>>> V9: Rebase > >>>> V10: Rebase > >>>> V11: Rebase > >>>> > >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/i915_reg.h | 2 ++ > >>>> drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ > >>>> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ > >>>> drivers/gpu/drm/i915/intel_dp.c | 4 ++++ > >>>> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > >>>> drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++ > >>>> 6 files changed, 56 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>>> index 2078541..1e13e51 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_reg.h > >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h > >>>> @@ -9229,6 +9229,8 @@ enum skl_power_gate { > >>>> #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) > >>>> > >>>> #define TRANS_MSA_SYNC_CLK (1 << 0) > >>>> +#define TRANS_MSA_SAMPLING_444 (2 << 1) > >>>> +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) > >>>> #define TRANS_MSA_6_BPC (0 << 5) > >>>> #define TRANS_MSA_8_BPC (1 << 5) > >>>> #define TRANS_MSA_10_BPC (2 << 5) > >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >>>> index be21131..186111a 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c > >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >>>> @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) > >>>> break; > >>>> } > >>>> > >>>> + /* > >>>> + * As per DP 1.2 spec section 2.3.4.3 while sending > >>>> + * YCBCR 444 signals we should program MSA MISC1/0 fields with > >>>> + * colorspace information. The output colorspace encoding is BT601. > >>>> + */ > >>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) > >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > >>>> } > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>> index 3317c6c..41abd03 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > >>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >>>> enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; > >>>> > >>>> + pipe_config->lspcon_downsampling = false; > >>>> + > >>>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > >>>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > >>>> > >>>> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, > >>>> else > >>>> output = INTEL_OUTPUT_FORMAT_YCBCR420; > >>>> } else { > >>>> + /* > >>>> + * Currently there is no interface defined to > >>>> + * check user preference between RGB/YCBCR444 > >>>> + * or YCBCR420. So the only possible case for > >>>> + * YCBCR444 usage is driving YCBCR420 output > >>>> + * with LSPCON, when pipe is configured for > >>>> + * YCBCR444 output and LSPCON takes care of > >>>> + * downsampling it. > >>>> + */ > >>>> + pipe_config->lspcon_downsampling = true; > >>>> output = INTEL_OUTPUT_FORMAT_YCBCR444; > >>>> } > >>>> } > >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >>>> index c3f6330..1156d59 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_dp.c > >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c > >>>> @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, > >>>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >>>> struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; > >>>> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >>>> + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); > >>>> enum port port = encoder->port; > >>>> struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); > >>>> struct intel_connector *intel_connector = intel_dp->attached_connector; > >>>> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, > >>>> pipe_config->has_pch_encoder = true; > >>>> > >>>> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; > >>>> + if (lspcon->active) > >>>> + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); > >>>> + > >>>> pipe_config->has_drrs = false; > >>>> if (IS_G4X(dev_priv) || port == PORT_A) > >>>> pipe_config->has_audio = false; > >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >>>> index b8e5f95..95e252e 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_drv.h > >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h > >>>> @@ -908,6 +908,9 @@ struct intel_crtc_state { > >>>> > >>>> /* Output format RGB/YCBCR etc */ > >>>> enum intel_output_format output_format; > >>>> + > >>>> + /* Output down scaling is done in LSPCON device */ > >>>> + bool lspcon_downsampling; > >>>> }; > >>>> > >>>> struct intel_crtc { > >>>> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > >>>> const struct drm_connector_state *conn_state); > >>>> bool lspcon_infoframe_enabled(struct intel_encoder *encoder, > >>>> const struct intel_crtc_state *pipe_config); > >>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, > >>>> + struct intel_crtc_state *crtc_state); > >>>> > >>>> /* intel_pipe_crc.c */ > >>>> #ifdef CONFIG_DEBUG_FS > >>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > >>>> index 829c40a..fff32b3 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c > >>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c > >>>> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) > >>>> return true; > >>>> } > >>>> > >>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, > >>>> + struct intel_crtc_state *crtc_state) > >>>> +{ > >>>> + const struct drm_display_info *info = &connector->display_info; > >>>> + const struct drm_display_mode *adjusted_mode = > >>>> + &crtc_state->base.adjusted_mode; > >>>> + > >>>> + if (drm_mode_is_420_only(info, adjusted_mode) && > >>>> + connector->ycbcr_420_allowed) { > >>>> + crtc_state->port_clock /= 2; > >>> This looks bogus. We're talking about DP here so we should not be > >>> frobbing port_clock. And since we output 4:4:4 from the port > >>> anyway we don't even have to take 4:2:0 into consideration > >>> when calculating port_clock. > >> I agree on this. > >>> Ah, this guy is called before we even calculate port_clock. > >>> That explains why this didn't cause any problems. So this is > >>> just dead code here. > >>> > >>> And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() > >>> also dead code since the caller overwrites it there as well. > >> I am not sure if that's the case for Native HDMI 2.0. > >> In intel_hdmi_420_config we are updating the config->port_clock, which > >> is the same thing being updated for 8/12/16 BPC deep color too, just > >> after this function. > >> I have tested the output with HDMI 2.0 analyzer, which reported right > >> pixel clock (297Mhz). Now, as any of the port clock calculations do not > >> get information > >> about HDMI output type, there is no other way they will calculate the > >> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is > >> active. > > It effectively does this: > > > > 1. port_clock = who knows > > 2. port_clock /= 2; > Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally > doing this also: > > clock_8bpc /= 2; > clock_10bpc /= 2; > clock_12bpc /= 2; > > > 3. if (12bpc) > > port_clock = clock_12bpc; > > else if (10bpc) > > port_clock = clock_10bpc; > > else > > port_clock = clock_8bpc; > This means effectively the selected clock is /2, so the code is not dead > (thankfully :-)) Only the port_clock/=2 part. I never claimed the rest was dead. > - Shashank > > > > Ie. step 2. is dead code. > > > >> - Shashank > >>>> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; > >>>> + crtc_state->lspcon_downsampling = true; > >>>> + } > >>>> +} > >>>> + > >>>> static bool lspcon_probe(struct intel_lspcon *lspcon) > >>>> { > >>>> int retry; > >>>> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, > >>>> return; > >>>> } > >>>> > >>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { > >>>> + if (crtc_state->lspcon_downsampling) > >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > >>>> + else > >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; > >>>> + } else { > >>>> + frame.avi.colorspace = HDMI_COLORSPACE_RGB; > >>>> + } > >>>> + > >>>> drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, > >>>> crtc_state->limited_color_range ? > >>>> HDMI_QUANTIZATION_RANGE_LIMITED : > >>>> @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > >>>> struct intel_lspcon *lspcon = &intel_dig_port->lspcon; > >>>> struct drm_device *dev = intel_dig_port->base.base.dev; > >>>> struct drm_i915_private *dev_priv = to_i915(dev); > >>>> + struct drm_connector *connector = &dp->attached_connector->base; > >>>> > >>>> if (!HAS_LSPCON(dev_priv)) { > >>>> DRM_ERROR("LSPCON is not supported on this platform\n"); > >>>> @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > >>>> return false; > >>>> } > >>>> > >>>> + connector->ycbcr_420_allowed = true; > >>>> lspcon->active = true; > >>>> DRM_DEBUG_KMS("Success: LSPCON init\n"); > >>>> return true; > >>>> -- > >>>> 2.7.4
On Fri, 12 Oct 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Sat, Oct 13, 2018 at 12:02:25AM +0530, Sharma, Shashank wrote: >> >>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, >> >>>> + struct intel_crtc_state *crtc_state) >> >>>> +{ >> >>>> + const struct drm_display_info *info = &connector->display_info; >> >>>> + const struct drm_display_mode *adjusted_mode = >> >>>> + &crtc_state->base.adjusted_mode; >> >>>> + >> >>>> + if (drm_mode_is_420_only(info, adjusted_mode) && >> >>>> + connector->ycbcr_420_allowed) { >> >>>> + crtc_state->port_clock /= 2; >> >>> This looks bogus. We're talking about DP here so we should not be >> >>> frobbing port_clock. And since we output 4:4:4 from the port >> >>> anyway we don't even have to take 4:2:0 into consideration >> >>> when calculating port_clock. >> >> I agree on this. >> >>> Ah, this guy is called before we even calculate port_clock. >> >>> That explains why this didn't cause any problems. So this is >> >>> just dead code here. >> >>> >> >>> And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() >> >>> also dead code since the caller overwrites it there as well. >> >> I am not sure if that's the case for Native HDMI 2.0. >> >> In intel_hdmi_420_config we are updating the config->port_clock, which >> >> is the same thing being updated for 8/12/16 BPC deep color too, just >> >> after this function. >> >> I have tested the output with HDMI 2.0 analyzer, which reported right >> >> pixel clock (297Mhz). Now, as any of the port clock calculations do not >> >> get information >> >> about HDMI output type, there is no other way they will calculate the >> >> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is >> >> active. >> > It effectively does this: >> > >> > 1. port_clock = who knows >> > 2. port_clock /= 2; >> Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally >> doing this also: >> >> clock_8bpc /= 2; >> clock_10bpc /= 2; >> clock_12bpc /= 2; >> >> > 3. if (12bpc) >> > port_clock = clock_12bpc; >> > else if (10bpc) >> > port_clock = clock_10bpc; >> > else >> > port_clock = clock_8bpc; >> This means effectively the selected clock is /2, so the code is not dead >> (thankfully :-)) > > Only the port_clock/=2 part. I never claimed the rest was dead. The whole series pushed to dinq, dismissing this last part with Ville's approval, to get some closure here. Thanks for the patches and review. Further patches to clean up the existing and new dead code will be appreciated. BR, Jani.
Regards Shashank On 10/15/2018 6:42 PM, Jani Nikula wrote: > On Fri, 12 Oct 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> On Sat, Oct 13, 2018 at 12:02:25AM +0530, Sharma, Shashank wrote: >>>>>>> +void lspcon_ycbcr420_config(struct drm_connector *connector, >>>>>>> + struct intel_crtc_state *crtc_state) >>>>>>> +{ >>>>>>> + const struct drm_display_info *info = &connector->display_info; >>>>>>> + const struct drm_display_mode *adjusted_mode = >>>>>>> + &crtc_state->base.adjusted_mode; >>>>>>> + >>>>>>> + if (drm_mode_is_420_only(info, adjusted_mode) && >>>>>>> + connector->ycbcr_420_allowed) { >>>>>>> + crtc_state->port_clock /= 2; >>>>>> This looks bogus. We're talking about DP here so we should not be >>>>>> frobbing port_clock. And since we output 4:4:4 from the port >>>>>> anyway we don't even have to take 4:2:0 into consideration >>>>>> when calculating port_clock. >>>>> I agree on this. >>>>>> Ah, this guy is called before we even calculate port_clock. >>>>>> That explains why this didn't cause any problems. So this is >>>>>> just dead code here. >>>>>> >>>>>> And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config() >>>>>> also dead code since the caller overwrites it there as well. >>>>> I am not sure if that's the case for Native HDMI 2.0. >>>>> In intel_hdmi_420_config we are updating the config->port_clock, which >>>>> is the same thing being updated for 8/12/16 BPC deep color too, just >>>>> after this function. >>>>> I have tested the output with HDMI 2.0 analyzer, which reported right >>>>> pixel clock (297Mhz). Now, as any of the port clock calculations do not >>>>> get information >>>>> about HDMI output type, there is no other way they will calculate the >>>>> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is >>>>> active. >>>> It effectively does this: >>>> >>>> 1. port_clock = who knows >>>> 2. port_clock /= 2; >>> Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally >>> doing this also: >>> >>> clock_8bpc /= 2; >>> clock_10bpc /= 2; >>> clock_12bpc /= 2; >>> >>>> 3. if (12bpc) >>>> port_clock = clock_12bpc; >>>> else if (10bpc) >>>> port_clock = clock_10bpc; >>>> else >>>> port_clock = clock_8bpc; >>> This means effectively the selected clock is /2, so the code is not dead >>> (thankfully :-)) >> Only the port_clock/=2 part. I never claimed the rest was dead. > The whole series pushed to dinq, dismissing this last part with Ville's > approval, to get some closure here. Thanks for the patches and > review. Further patches to clean up the existing and new dead code will > be appreciated. Thanks Jani, I will have a look at the series post merge, and send any follow up / cleanup patches if required. - Shashank > BR, > Jani. > > >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2078541..1e13e51 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9229,6 +9229,8 @@ enum skl_power_gate { #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) #define TRANS_MSA_SYNC_CLK (1 << 0) +#define TRANS_MSA_SAMPLING_444 (2 << 1) +#define TRANS_MSA_CLRSP_YCBCR (2 << 3) #define TRANS_MSA_6_BPC (0 << 5) #define TRANS_MSA_8_BPC (1 << 5) #define TRANS_MSA_10_BPC (2 << 5) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index be21131..186111a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1784,6 +1784,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) break; } + /* + * As per DP 1.2 spec section 2.3.4.3 while sending + * YCBCR 444 signals we should program MSA MISC1/0 fields with + * colorspace information. The output colorspace encoding is BT601. + */ + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3317c6c..41abd03 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB; + pipe_config->lspcon_downsampling = false; + if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc, else output = INTEL_OUTPUT_FORMAT_YCBCR420; } else { + /* + * Currently there is no interface defined to + * check user preference between RGB/YCBCR444 + * or YCBCR420. So the only possible case for + * YCBCR444 usage is driving YCBCR420 output + * with LSPCON, when pipe is configured for + * YCBCR444 output and LSPCON takes care of + * downsampling it. + */ + pipe_config->lspcon_downsampling = true; output = INTEL_OUTPUT_FORMAT_YCBCR444; } } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c3f6330..1156d59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2074,6 +2074,7 @@ intel_dp_compute_config(struct intel_encoder *encoder, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode; struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base); enum port port = encoder->port; struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc); struct intel_connector *intel_connector = intel_dp->attached_connector; @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_pch_encoder = true; pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; + if (lspcon->active) + lspcon_ycbcr420_config(&intel_connector->base, pipe_config); + pipe_config->has_drrs = false; if (IS_G4X(dev_priv) || port == PORT_A) pipe_config->has_audio = false; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b8e5f95..95e252e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -908,6 +908,9 @@ struct intel_crtc_state { /* Output format RGB/YCBCR etc */ enum intel_output_format output_format; + + /* Output down scaling is done in LSPCON device */ + bool lspcon_downsampling; }; struct intel_crtc { @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, const struct drm_connector_state *conn_state); bool lspcon_infoframe_enabled(struct intel_encoder *encoder, const struct intel_crtc_state *pipe_config); +void lspcon_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *crtc_state); /* intel_pipe_crc.c */ #ifdef CONFIG_DEBUG_FS diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c index 829c40a..fff32b3 100644 --- a/drivers/gpu/drm/i915/intel_lspcon.c +++ b/drivers/gpu/drm/i915/intel_lspcon.c @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) return true; } +void lspcon_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *crtc_state) +{ + const struct drm_display_info *info = &connector->display_info; + const struct drm_display_mode *adjusted_mode = + &crtc_state->base.adjusted_mode; + + if (drm_mode_is_420_only(info, adjusted_mode) && + connector->ycbcr_420_allowed) { + crtc_state->port_clock /= 2; + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444; + crtc_state->lspcon_downsampling = true; + } +} + static bool lspcon_probe(struct intel_lspcon *lspcon) { int retry; @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder, return; } + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) { + if (crtc_state->lspcon_downsampling) + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; + else + frame.avi.colorspace = HDMI_COLORSPACE_YUV444; + } else { + frame.avi.colorspace = HDMI_COLORSPACE_RGB; + } + drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode, crtc_state->limited_color_range ? HDMI_QUANTIZATION_RANGE_LIMITED : @@ -517,6 +541,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) struct intel_lspcon *lspcon = &intel_dig_port->lspcon; struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_connector *connector = &dp->attached_connector->base; if (!HAS_LSPCON(dev_priv)) { DRM_ERROR("LSPCON is not supported on this platform\n"); @@ -541,6 +566,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) return false; } + connector->ycbcr_420_allowed = true; lspcon->active = true; DRM_DEBUG_KMS("Success: LSPCON init\n"); return true;