| Message ID | 1517304963-27932-8-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: > From: "Sharma, Shashank" <shashank.sharma@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. > > 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 | 11 ++++++++++- > drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > drivers/gpu/drm/i915/intel_lspcon.c | 30 ++++++++++++++++++++++++++++++ > 6 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 64933fd..e383b05 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8721,6 +8721,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 510bb77..db7d940 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1499,6 +1499,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 == CRTC_OUTPUT_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 4a40de9..b26f4a9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > pipe_config->gamma_mode = > I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; > > + pipe_config->external_scaling = false; I really don't like the name of that. Makes me think we're actually scaling the entire output or something. 'external_ycbcr_420_downsample' or something like that would be better. > if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > enum intel_output_format output_format = CRTC_OUTPUT_RGB; > @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > output_format = CRTC_OUTPUT_YCBCR420; > } else { > output_format = CRTC_OUTPUT_YCBCR444; > - } > > + /* > + * As there is no interface defined (yet) > + * to get the user's preference for output > + * format, YCBCR444 output format is only > + * possible with LSPCON YCBCR420 output, > + * which uses LSPCON's (external )scaler. > + */ > + pipe_config->external_scaling = true; > + } > } > > pipe_config->output_format = output_format; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a2e8879..be19579 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1640,6 +1640,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; > @@ -1669,6 +1670,15 @@ intel_dp_compute_config(struct intel_encoder *encoder, > if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) > pipe_config->has_pch_encoder = true; > > + if (lspcon->active) { > + struct drm_connector *connector = &intel_connector->base; > + > + if (lspcon_ycbcr420_config(connector, pipe_config)) { > + pipe_config->output_format = CRTC_OUTPUT_YCBCR444; > + pipe_config->external_scaling = true; Why split the work between two functions like this? I think it would be cleaner to either inline all the code here, or move it all into lspcon_ycbcr420_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 efaa331..f23129b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -882,6 +882,9 @@ struct intel_crtc_state { > > /* Output format RGB/YCBCR etc */ > enum intel_output_format output_format; > + > + /* Output up/down scaling is done in external device */ > + bool external_scaling; > }; > > struct intel_crtc { > @@ -2139,6 +2142,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > const struct drm_connector_state *conn_state); > bool lspcon_infoframe_enabled(struct drm_encoder *encoder, > const struct intel_crtc_state *pipe_config); > +bool lspcon_ycbcr420_config(struct drm_connector *connector, > + struct intel_crtc_state *config); > > /* intel_pipe_crc.c */ > int intel_pipe_crc_create(struct drm_minor *minor); > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > index 066ea91..8d6a9b2 100644 > --- a/drivers/gpu/drm/i915/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) > return true; > } > > +bool lspcon_ycbcr420_config(struct drm_connector *connector, > + struct intel_crtc_state *config) crtc_state > +{ > + struct drm_display_info *info = &connector->display_info; const > + struct drm_display_mode *mode = &config->base.adjusted_mode; const ... adjusted_mode; > + > + if (drm_mode_is_420_only(info, mode)) { > + if (!connector->ycbcr_420_allowed) { > + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > + return false; How can we even get here? > + } > + > + config->port_clock /= 2; > + return true; > + } > + > + return false; > +} > + > static bool lspcon_probe(struct intel_lspcon *lspcon) > { > int retry; > @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > return; > } > > + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) { > + if (crtc_state->external_scaling) > + 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 : > @@ -512,6 +540,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"); > @@ -536,6 +565,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 2/2/2018 12:39 AM, Ville Syrjälä wrote: > On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: >> From: "Sharma, Shashank" <shashank.sharma@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. >> >> 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 | 11 ++++++++++- >> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ >> drivers/gpu/drm/i915/intel_lspcon.c | 30 ++++++++++++++++++++++++++++++ >> 6 files changed, 64 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 64933fd..e383b05 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -8721,6 +8721,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 510bb77..db7d940 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1499,6 +1499,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 == CRTC_OUTPUT_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 4a40de9..b26f4a9 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >> pipe_config->gamma_mode = >> I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; >> >> + pipe_config->external_scaling = false; > I really don't like the name of that. Makes me think we're actually > scaling the entire output or something. 'external_ycbcr_420_downsample' > or something like that would be better. ok, let me rename this to something in those lines. >> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { >> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); >> enum intel_output_format output_format = CRTC_OUTPUT_RGB; >> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >> output_format = CRTC_OUTPUT_YCBCR420; >> } else { >> output_format = CRTC_OUTPUT_YCBCR444; >> - } >> >> + /* >> + * As there is no interface defined (yet) >> + * to get the user's preference for output >> + * format, YCBCR444 output format is only >> + * possible with LSPCON YCBCR420 output, >> + * which uses LSPCON's (external )scaler. >> + */ >> + pipe_config->external_scaling = true; >> + } >> } >> >> pipe_config->output_format = output_format; >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index a2e8879..be19579 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1640,6 +1640,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; >> @@ -1669,6 +1670,15 @@ intel_dp_compute_config(struct intel_encoder *encoder, >> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) >> pipe_config->has_pch_encoder = true; >> >> + if (lspcon->active) { >> + struct drm_connector *connector = &intel_connector->base; >> + >> + if (lspcon_ycbcr420_config(connector, pipe_config)) { >> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444; >> + pipe_config->external_scaling = true; > Why split the work between two functions like this? I think it would be > cleaner to either inline all the code here, or move it all into > lspcon_ycbcr420_config(). Sure, makes sense. >> + } >> + } >> + >> 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 efaa331..f23129b 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -882,6 +882,9 @@ struct intel_crtc_state { >> >> /* Output format RGB/YCBCR etc */ >> enum intel_output_format output_format; >> + >> + /* Output up/down scaling is done in external device */ >> + bool external_scaling; >> }; >> >> struct intel_crtc { >> @@ -2139,6 +2142,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >> const struct drm_connector_state *conn_state); >> bool lspcon_infoframe_enabled(struct drm_encoder *encoder, >> const struct intel_crtc_state *pipe_config); >> +bool lspcon_ycbcr420_config(struct drm_connector *connector, >> + struct intel_crtc_state *config); >> >> /* intel_pipe_crc.c */ >> int intel_pipe_crc_create(struct drm_minor *minor); >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >> index 066ea91..8d6a9b2 100644 >> --- a/drivers/gpu/drm/i915/intel_lspcon.c >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) >> return true; >> } >> >> +bool lspcon_ycbcr420_config(struct drm_connector *connector, >> + struct intel_crtc_state *config) > crtc_state ok > >> +{ >> + struct drm_display_info *info = &connector->display_info; > const ok >> + struct drm_display_mode *mode = &config->base.adjusted_mode; > const ... adjusted_mode; ok >> + >> + if (drm_mode_is_420_only(info, mode)) { >> + if (!connector->ycbcr_420_allowed) { >> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); >> + return false; > How can we even get here? This code flow is coming from CRTC/encoder side, whereas 420 capability is set in connector_init, this is the first time we are checking connector's capabilities. So yes, its possible to reach here. - Shashank >> + } >> + >> + config->port_clock /= 2; >> + return true; >> + } >> + >> + return false; >> +} >> + >> static bool lspcon_probe(struct intel_lspcon *lspcon) >> { >> int retry; >> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >> return; >> } >> >> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) { >> + if (crtc_state->external_scaling) >> + 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 : >> @@ -512,6 +540,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"); >> @@ -536,6 +565,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, Feb 02, 2018 at 11:44:01AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 2/2/2018 12:39 AM, Ville Syrjälä wrote: > > On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: > >> From: "Sharma, Shashank" <shashank.sharma@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. > >> > >> 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 | 11 ++++++++++- > >> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++ > >> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > >> drivers/gpu/drm/i915/intel_lspcon.c | 30 ++++++++++++++++++++++++++++++ > >> 6 files changed, 64 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 64933fd..e383b05 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -8721,6 +8721,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 510bb77..db7d940 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -1499,6 +1499,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 == CRTC_OUTPUT_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 4a40de9..b26f4a9 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > >> pipe_config->gamma_mode = > >> I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; > >> > >> + pipe_config->external_scaling = false; > > I really don't like the name of that. Makes me think we're actually > > scaling the entire output or something. 'external_ycbcr_420_downsample' > > or something like that would be better. > ok, let me rename this to something in those lines. > >> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > >> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > >> enum intel_output_format output_format = CRTC_OUTPUT_RGB; > >> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > >> output_format = CRTC_OUTPUT_YCBCR420; > >> } else { > >> output_format = CRTC_OUTPUT_YCBCR444; > >> - } > >> > >> + /* > >> + * As there is no interface defined (yet) > >> + * to get the user's preference for output > >> + * format, YCBCR444 output format is only > >> + * possible with LSPCON YCBCR420 output, > >> + * which uses LSPCON's (external )scaler. > >> + */ > >> + pipe_config->external_scaling = true; > >> + } > >> } > >> > >> pipe_config->output_format = output_format; > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index a2e8879..be19579 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -1640,6 +1640,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; > >> @@ -1669,6 +1670,15 @@ intel_dp_compute_config(struct intel_encoder *encoder, > >> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) > >> pipe_config->has_pch_encoder = true; > >> > >> + if (lspcon->active) { > >> + struct drm_connector *connector = &intel_connector->base; > >> + > >> + if (lspcon_ycbcr420_config(connector, pipe_config)) { > >> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444; > >> + pipe_config->external_scaling = true; > > Why split the work between two functions like this? I think it would be > > cleaner to either inline all the code here, or move it all into > > lspcon_ycbcr420_config(). > Sure, makes sense. > >> + } > >> + } > >> + > >> 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 efaa331..f23129b 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -882,6 +882,9 @@ struct intel_crtc_state { > >> > >> /* Output format RGB/YCBCR etc */ > >> enum intel_output_format output_format; > >> + > >> + /* Output up/down scaling is done in external device */ > >> + bool external_scaling; > >> }; > >> > >> struct intel_crtc { > >> @@ -2139,6 +2142,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > >> const struct drm_connector_state *conn_state); > >> bool lspcon_infoframe_enabled(struct drm_encoder *encoder, > >> const struct intel_crtc_state *pipe_config); > >> +bool lspcon_ycbcr420_config(struct drm_connector *connector, > >> + struct intel_crtc_state *config); > >> > >> /* intel_pipe_crc.c */ > >> int intel_pipe_crc_create(struct drm_minor *minor); > >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > >> index 066ea91..8d6a9b2 100644 > >> --- a/drivers/gpu/drm/i915/intel_lspcon.c > >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c > >> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) > >> return true; > >> } > >> > >> +bool lspcon_ycbcr420_config(struct drm_connector *connector, > >> + struct intel_crtc_state *config) > > crtc_state > ok > > > >> +{ > >> + struct drm_display_info *info = &connector->display_info; > > const > ok > >> + struct drm_display_mode *mode = &config->base.adjusted_mode; > > const ... adjusted_mode; > ok > >> + > >> + if (drm_mode_is_420_only(info, mode)) { > >> + if (!connector->ycbcr_420_allowed) { > >> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > >> + return false; > > How can we even get here? > This code flow is coming from CRTC/encoder side, whereas 420 capability > is set in connector_init, this is the first time we are checking > connector's capabilities. So yes, its possible to reach here. Oh right, we don't yet use the .mode_valid() stuff for modesets. One thing that bothers me is that the code here is structured slightly differently than the native HDMI case. Would be nice if we could make the two codepaths at least follow the same general flow. Oh and the DRM_ERROR() has to go since it's possibly user triggerable. Looks like one snuck into the native HDMI code as well. That too should be killed off. > - Shashank > >> + } > >> + > >> + config->port_clock /= 2; > >> + return true; > >> + } > >> + > >> + return false; > >> +} > >> + > >> static bool lspcon_probe(struct intel_lspcon *lspcon) > >> { > >> int retry; > >> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > >> return; > >> } > >> > >> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) { > >> + if (crtc_state->external_scaling) > >> + 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 : > >> @@ -512,6 +540,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"); > >> @@ -536,6 +565,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 2/2/2018 7:22 PM, Ville Syrjälä wrote: > On Fri, Feb 02, 2018 at 11:44:01AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 2/2/2018 12:39 AM, Ville Syrjälä wrote: >>> On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: >>>> From: "Sharma, Shashank" <shashank.sharma@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. >>>> >>>> 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 | 11 ++++++++++- >>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ >>>> drivers/gpu/drm/i915/intel_lspcon.c | 30 ++++++++++++++++++++++++++++++ >>>> 6 files changed, 64 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 64933fd..e383b05 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -8721,6 +8721,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 510bb77..db7d940 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -1499,6 +1499,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 == CRTC_OUTPUT_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 4a40de9..b26f4a9 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >>>> pipe_config->gamma_mode = >>>> I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; >>>> >>>> + pipe_config->external_scaling = false; >>> I really don't like the name of that. Makes me think we're actually >>> scaling the entire output or something. 'external_ycbcr_420_downsample' >>> or something like that would be better. >> ok, let me rename this to something in those lines. >>>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { >>>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); >>>> enum intel_output_format output_format = CRTC_OUTPUT_RGB; >>>> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, >>>> output_format = CRTC_OUTPUT_YCBCR420; >>>> } else { >>>> output_format = CRTC_OUTPUT_YCBCR444; >>>> - } >>>> >>>> + /* >>>> + * As there is no interface defined (yet) >>>> + * to get the user's preference for output >>>> + * format, YCBCR444 output format is only >>>> + * possible with LSPCON YCBCR420 output, >>>> + * which uses LSPCON's (external )scaler. >>>> + */ >>>> + pipe_config->external_scaling = true; >>>> + } >>>> } >>>> >>>> pipe_config->output_format = output_format; >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index a2e8879..be19579 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -1640,6 +1640,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; >>>> @@ -1669,6 +1670,15 @@ intel_dp_compute_config(struct intel_encoder *encoder, >>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) >>>> pipe_config->has_pch_encoder = true; >>>> >>>> + if (lspcon->active) { >>>> + struct drm_connector *connector = &intel_connector->base; >>>> + >>>> + if (lspcon_ycbcr420_config(connector, pipe_config)) { >>>> + pipe_config->output_format = CRTC_OUTPUT_YCBCR444; >>>> + pipe_config->external_scaling = true; >>> Why split the work between two functions like this? I think it would be >>> cleaner to either inline all the code here, or move it all into >>> lspcon_ycbcr420_config(). >> Sure, makes sense. >>>> + } >>>> + } >>>> + >>>> 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 efaa331..f23129b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -882,6 +882,9 @@ struct intel_crtc_state { >>>> >>>> /* Output format RGB/YCBCR etc */ >>>> enum intel_output_format output_format; >>>> + >>>> + /* Output up/down scaling is done in external device */ >>>> + bool external_scaling; >>>> }; >>>> >>>> struct intel_crtc { >>>> @@ -2139,6 +2142,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>>> const struct drm_connector_state *conn_state); >>>> bool lspcon_infoframe_enabled(struct drm_encoder *encoder, >>>> const struct intel_crtc_state *pipe_config); >>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector, >>>> + struct intel_crtc_state *config); >>>> >>>> /* intel_pipe_crc.c */ >>>> int intel_pipe_crc_create(struct drm_minor *minor); >>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >>>> index 066ea91..8d6a9b2 100644 >>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c >>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) >>>> return true; >>>> } >>>> >>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector, >>>> + struct intel_crtc_state *config) >>> crtc_state >> ok >>>> +{ >>>> + struct drm_display_info *info = &connector->display_info; >>> const >> ok >>>> + struct drm_display_mode *mode = &config->base.adjusted_mode; >>> const ... adjusted_mode; >> ok >>>> + >>>> + if (drm_mode_is_420_only(info, mode)) { >>>> + if (!connector->ycbcr_420_allowed) { >>>> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); >>>> + return false; >>> How can we even get here? >> This code flow is coming from CRTC/encoder side, whereas 420 capability >> is set in connector_init, this is the first time we are checking >> connector's capabilities. So yes, its possible to reach here. > Oh right, we don't yet use the .mode_valid() stuff for modesets. > > One thing that bothers me is that the code here is structured slightly > differently than the native HDMI case. Would be nice if we could make > the two codepaths at least follow the same general flow. Very valid point, but the difference mostly is due to LSPCON still running like a DP encoder, whereas native HDMI case is in the HDMI side. > > Oh and the DRM_ERROR() has to go since it's possibly user triggerable. > Looks like one snuck into the native HDMI code as well. That too should > be killed off. Agree, will prune it ! - Shashank > >> - Shashank >>>> + } >>>> + >>>> + config->port_clock /= 2; >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>>> + >>>> static bool lspcon_probe(struct intel_lspcon *lspcon) >>>> { >>>> int retry; >>>> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>>> return; >>>> } >>>> >>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) { >>>> + if (crtc_state->external_scaling) >>>> + 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 : >>>> @@ -512,6 +540,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"); >>>> @@ -536,6 +565,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
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 64933fd..e383b05 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8721,6 +8721,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 510bb77..db7d940 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1499,6 +1499,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 == CRTC_OUTPUT_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 4a40de9..b26f4a9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, pipe_config->gamma_mode = I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; + pipe_config->external_scaling = false; if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); enum intel_output_format output_format = CRTC_OUTPUT_RGB; @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, output_format = CRTC_OUTPUT_YCBCR420; } else { output_format = CRTC_OUTPUT_YCBCR444; - } + /* + * As there is no interface defined (yet) + * to get the user's preference for output + * format, YCBCR444 output format is only + * possible with LSPCON YCBCR420 output, + * which uses LSPCON's (external )scaler. + */ + pipe_config->external_scaling = true; + } } pipe_config->output_format = output_format; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a2e8879..be19579 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1640,6 +1640,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; @@ -1669,6 +1670,15 @@ intel_dp_compute_config(struct intel_encoder *encoder, if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A) pipe_config->has_pch_encoder = true; + if (lspcon->active) { + struct drm_connector *connector = &intel_connector->base; + + if (lspcon_ycbcr420_config(connector, pipe_config)) { + pipe_config->output_format = CRTC_OUTPUT_YCBCR444; + pipe_config->external_scaling = true; + } + } + 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 efaa331..f23129b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -882,6 +882,9 @@ struct intel_crtc_state { /* Output format RGB/YCBCR etc */ enum intel_output_format output_format; + + /* Output up/down scaling is done in external device */ + bool external_scaling; }; struct intel_crtc { @@ -2139,6 +2142,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, const struct drm_connector_state *conn_state); bool lspcon_infoframe_enabled(struct drm_encoder *encoder, const struct intel_crtc_state *pipe_config); +bool lspcon_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *config); /* intel_pipe_crc.c */ int intel_pipe_crc_create(struct drm_minor *minor); diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c index 066ea91..8d6a9b2 100644 --- a/drivers/gpu/drm/i915/intel_lspcon.c +++ b/drivers/gpu/drm/i915/intel_lspcon.c @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon) return true; } +bool lspcon_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *config) +{ + struct drm_display_info *info = &connector->display_info; + struct drm_display_mode *mode = &config->base.adjusted_mode; + + if (drm_mode_is_420_only(info, mode)) { + if (!connector->ycbcr_420_allowed) { + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); + return false; + } + + config->port_clock /= 2; + return true; + } + + return false; +} + static bool lspcon_probe(struct intel_lspcon *lspcon) { int retry; @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, return; } + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) { + if (crtc_state->external_scaling) + 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 : @@ -512,6 +540,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"); @@ -536,6 +565,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;