| Message ID | 1515145535-11228-8-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: > 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) > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: 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_dp.c | 10 ++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ > 5 files changed, 49 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 966e4df..45ee264 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. > + */ > + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) > + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; This fails to state that we're indicating BT.601 encoding. I think we should spell that out explicitly. > I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 35c5299..3bf82ea 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1613,6 +1613,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; > @@ -1642,6 +1643,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; I think I'd like to see all compute_config hooks explicitly set the outout_format (to RGB usually obviously). > + lspcon->output_format = CRTC_OUTPUT_YCBCR420; You should not modify any non-atomic state like that in compute_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 2de6b41..5edba06 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > return; > } > > + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) > + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) > + 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 +538,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 +563,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 1/17/2018 11:57 PM, Ville Syrjälä wrote: > On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: >> 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) >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: 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_dp.c | 10 ++++++++++ >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ >> 5 files changed, 49 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 966e4df..45ee264 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. >> + */ >> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) >> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > This fails to state that we're indicating BT.601 encoding. I think we > should spell that out explicitly. Agree, I will add this in comments. >> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 35c5299..3bf82ea 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1613,6 +1613,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; >> @@ -1642,6 +1643,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; > I think I'd like to see all compute_config hooks explicitly set the > outout_format (to RGB usually obviously). That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need not to do this, as reset of the pipe_config will automatically make it RGB. >> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; > You should not modify any non-atomic state like that in compute_config. Please help me to understand this better, Can you elaborate a bit more on: - Why is this a non-atomic state ? - What is the right place we should modify it ? - Shashank >> + } >> + } >> + >> 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 2de6b41..5edba06 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >> return; >> } >> >> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) >> + 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 +538,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 +563,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
Op 18-01-18 om 07:27 schreef Sharma, Shashank: > Regards > > Shashank > > > On 1/17/2018 11:57 PM, Ville Syrjälä wrote: >> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: >>> 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) >>> >>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>> Cc: 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_dp.c | 10 ++++++++++ >>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ >>> 5 files changed, 49 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>> index 966e4df..45ee264 100644 >>> --- a/drivers/gpu/drm/i915/i915_reg.h >>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. >>> + */ >>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) >>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >> This fails to state that we're indicating BT.601 encoding. I think we >> should spell that out explicitly. > Agree, I will add this in comments. >>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >>> } >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> index 35c5299..3bf82ea 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -1613,6 +1613,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; >>> @@ -1642,6 +1643,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; >> I think I'd like to see all compute_config hooks explicitly set the >> outout_format (to RGB usually obviously). > That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need > not to do this, as reset of the pipe_config will automatically make it RGB. >>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; >> You should not modify any non-atomic state like that in compute_config. > Please help me to understand this better, Can you elaborate a bit more on: > - Why is this a non-atomic state ? Because lspcon->output_format is modified even if a commit is TEST_ONLY.. This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :) > - What is the right place we should modify it ? intel_digital_connector_state probably. ~Maarten > > - Shashank >>> + } >>> + } >>> + >>> 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 2de6b41..5edba06 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>> return; >>> } >>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) >>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) >>> + 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 +538,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 +563,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 Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 1/17/2018 11:57 PM, Ville Syrjälä wrote: > > On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: > >> 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) > >> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >> Cc: 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_dp.c | 10 ++++++++++ > >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ > >> 5 files changed, 49 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 966e4df..45ee264 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. > >> + */ > >> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) > >> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > > This fails to state that we're indicating BT.601 encoding. I think we > > should spell that out explicitly. > Agree, I will add this in comments. > >> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index 35c5299..3bf82ea 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -1613,6 +1613,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; > >> @@ -1642,6 +1643,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; > > I think I'd like to see all compute_config hooks explicitly set the > > outout_format (to RGB usually obviously). > That's the one I was talking about in previous patch. If we keep > output_format_RGB = 0, we need > not to do this, as reset of the pipe_config will automatically make it RGB. IMO either we have INVALID=0 so that we use it to catch readout fails, or we have no INVALID. Other options make little sense to me. > >> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; > > You should not modify any non-atomic state like that in compute_config. > Please help me to understand this better, Can you elaborate a bit more on: > - Why is this a non-atomic state ? > - What is the right place we should modify it ? I don't think you need it at all. Just consult the crtc state when needed. > > - Shashank > >> + } > >> + } > >> + > >> 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 2de6b41..5edba06 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > >> return; > >> } > >> > >> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) > >> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > >> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) > >> + 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 +538,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 +563,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 1/18/2018 7:33 PM, Ville Syrjälä wrote: > On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 1/17/2018 11:57 PM, Ville Syrjälä wrote: >>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: >>>> 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) >>>> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>> Cc: 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_dp.c | 10 ++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ >>>> 5 files changed, 49 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 966e4df..45ee264 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. >>>> + */ >>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >>> This fails to state that we're indicating BT.601 encoding. I think we >>> should spell that out explicitly. >> Agree, I will add this in comments. >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index 35c5299..3bf82ea 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -1613,6 +1613,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; >>>> @@ -1642,6 +1643,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; >>> I think I'd like to see all compute_config hooks explicitly set the >>> outout_format (to RGB usually obviously). >> That's the one I was talking about in previous patch. If we keep >> output_format_RGB = 0, we need >> not to do this, as reset of the pipe_config will automatically make it RGB. > IMO either we have INVALID=0 so that we use it to catch readout > fails, or we have no INVALID. Other options make little sense to me. Its a minor thing, and doesn't really matter. I can change this. >>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; >>> You should not modify any non-atomic state like that in compute_config. >> Please help me to understand this better, Can you elaborate a bit more on: >> - Why is this a non-atomic state ? >> - What is the right place we should modify it ? > I don't think you need it at all. Just consult the crtc state when > needed. I dont think CRTC state can cover all the cases, for example, what would be do when we need YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444 for LSPCON output 420, we can't handle this. This is equivalent to your previous suggestion of adding 'scaling_reqd', but I added in LPSCON (instead of CRTC state) as this information is required only in case of LSPCON. In all native cases, scaling would be always required for 4:2:0 outputs. - Shashank >> - Shashank >>>> + } >>>> + } >>>> + >>>> 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 2de6b41..5edba06 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>>> return; >>>> } >>>> >>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) >>>> + 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 +538,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 +563,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 Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 1/18/2018 7:33 PM, Ville Syrjälä wrote: > > On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> > >> On 1/17/2018 11:57 PM, Ville Syrjälä wrote: > >>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: > >>>> 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) > >>>> > >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >>>> Cc: 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_dp.c | 10 ++++++++++ > >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ > >>>> 5 files changed, 49 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >>>> index 966e4df..45ee264 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_reg.h > >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h > >>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. > >>>> + */ > >>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) > >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > >>> This fails to state that we're indicating BT.601 encoding. I think we > >>> should spell that out explicitly. > >> Agree, I will add this in comments. > >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > >>>> } > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >>>> index 35c5299..3bf82ea 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_dp.c > >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c > >>>> @@ -1613,6 +1613,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; > >>>> @@ -1642,6 +1643,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; > >>> I think I'd like to see all compute_config hooks explicitly set the > >>> outout_format (to RGB usually obviously). > >> That's the one I was talking about in previous patch. If we keep > >> output_format_RGB = 0, we need > >> not to do this, as reset of the pipe_config will automatically make it RGB. > > IMO either we have INVALID=0 so that we use it to catch readout > > fails, or we have no INVALID. Other options make little sense to me. > Its a minor thing, and doesn't really matter. I can change this. > >>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; > >>> You should not modify any non-atomic state like that in compute_config. > >> Please help me to understand this better, Can you elaborate a bit more on: > >> - Why is this a non-atomic state ? > >> - What is the right place we should modify it ? > > I don't think you need it at all. Just consult the crtc state when > > needed. > I dont think CRTC state can cover all the cases, for example, what would > be do when we need > YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444 > for LSPCON output > 420, we can't handle this. Like I said earlier, just add another thing to the state to indicate what LSPCON should do with the 444 data. Either pass it through or downsample it. > This is equivalent to your previous > suggestion of adding 'scaling_reqd', > but I added in LPSCON (instead of CRTC state) as this information is > required only in case of LSPCON. But we have no atomic lspcon state. And probably not worth adding one since there won't be much there. So just adding what's needed into the crtc state seems like the best approach. > In all native cases, scaling would be always required for 4:2:0 outputs. > > - Shashank > >> - Shashank > >>>> + } > >>>> + } > >>>> + > >>>> 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 2de6b41..5edba06 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_drv.h > >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h > >>>> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, > >>>> return; > >>>> } > >>>> > >>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) > >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; > >>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) > >>>> + 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 +538,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 +563,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 1/18/2018 9:05 PM, Ville Syrjälä wrote: > On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 1/18/2018 7:33 PM, Ville Syrjälä wrote: >>> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote: >>>> Regards >>>> >>>> Shashank >>>> >>>> >>>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote: >>>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: >>>>>> 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) >>>>>> >>>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>>>> Cc: 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_dp.c | 10 ++++++++++ >>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ >>>>>> 5 files changed, 49 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>>>> index 966e4df..45ee264 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. >>>>>> + */ >>>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) >>>>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >>>>> This fails to state that we're indicating BT.601 encoding. I think we >>>>> should spell that out explicitly. >>>> Agree, I will add this in comments. >>>>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >>>>>> } >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>>>> index 35c5299..3bf82ea 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>>>> @@ -1613,6 +1613,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; >>>>>> @@ -1642,6 +1643,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; >>>>> I think I'd like to see all compute_config hooks explicitly set the >>>>> outout_format (to RGB usually obviously). >>>> That's the one I was talking about in previous patch. If we keep >>>> output_format_RGB = 0, we need >>>> not to do this, as reset of the pipe_config will automatically make it RGB. >>> IMO either we have INVALID=0 so that we use it to catch readout >>> fails, or we have no INVALID. Other options make little sense to me. >> Its a minor thing, and doesn't really matter. I can change this. >>>>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; >>>>> You should not modify any non-atomic state like that in compute_config. >>>> Please help me to understand this better, Can you elaborate a bit more on: >>>> - Why is this a non-atomic state ? >>>> - What is the right place we should modify it ? >>> I don't think you need it at all. Just consult the crtc state when >>> needed. >> I dont think CRTC state can cover all the cases, for example, what would >> be do when we need >> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444 >> for LSPCON output >> 420, we can't handle this. > Like I said earlier, just add another thing to the state to indicate what > LSPCON should do with the 444 data. Either pass it through or downsample > it. > >> This is equivalent to your previous >> suggestion of adding 'scaling_reqd', >> but I added in LPSCON (instead of CRTC state) as this information is >> required only in case of LSPCON. > But we have no atomic lspcon state. And probably not worth adding one > since there won't be much there. So just adding what's needed into the > crtc state seems like the best approach. Ok, So I guess the suggestion to move this bool into CRTC state is to maintain the atomic flow. Sure, in that case I will add a new bool in crtc_state called "output_scaling_reqd" or something similar. - Shashank >> In all native cases, scaling would be always required for 4:2:0 outputs. >> >> - Shashank >>>> - Shashank >>>>>> + } >>>>>> + } >>>>>> + >>>>>> 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 2de6b41..5edba06 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>>> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>>>>> return; >>>>>> } >>>>>> >>>>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) >>>>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >>>>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) >>>>>> + 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 +538,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 +563,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 1/18/2018 3:00 PM, Maarten Lankhorst wrote: > Op 18-01-18 om 07:27 schreef Sharma, Shashank: >> Regards >> >> Shashank >> >> >> On 1/17/2018 11:57 PM, Ville Syrjälä wrote: >>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote: >>>> 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) >>>> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>> Cc: 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_dp.c | 10 ++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>> drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ >>>> 5 files changed, 49 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >>>> index 966e4df..45ee264 100644 >>>> --- a/drivers/gpu/drm/i915/i915_reg.h >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h >>>> @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. >>>> + */ >>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) >>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; >>> This fails to state that we're indicating BT.601 encoding. I think we >>> should spell that out explicitly. >> Agree, I will add this in comments. >>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); >>>> } >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index 35c5299..3bf82ea 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -1613,6 +1613,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; >>>> @@ -1642,6 +1643,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; >>> I think I'd like to see all compute_config hooks explicitly set the >>> outout_format (to RGB usually obviously). >> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need >> not to do this, as reset of the pipe_config will automatically make it RGB. >>>> + lspcon->output_format = CRTC_OUTPUT_YCBCR420; >>> You should not modify any non-atomic state like that in compute_config. >> Please help me to understand this better, Can you elaborate a bit more on: >> - Why is this a non-atomic state ? > Because lspcon->output_format is modified even if a commit is TEST_ONLY.. > > This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :) >> - What is the right place we should modify it ? > intel_digital_connector_state probably. > > ~Maarten Thanks for the explanation Maarten, this was very helpful :-). - Shashank >> - Shashank >>>> + } >>>> + } >>>> + >>>> 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 2de6b41..5edba06 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, >>>> return; >>>> } >>>> + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) >>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; >>>> + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) >>>> + 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 +538,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 +563,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 966e4df..45ee264 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8547,6 +8547,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 7b89f2a..7616f6f 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. + */ + 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_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 35c5299..3bf82ea 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1613,6 +1613,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; @@ -1642,6 +1643,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; + lspcon->output_format = CRTC_OUTPUT_YCBCR420; + } + } + 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 2de6b41..5edba06 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2047,6 +2047,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..cb88138 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,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder, return; } + if (lspcon->output_format == CRTC_OUTPUT_YCBCR420) + frame.avi.colorspace = HDMI_COLORSPACE_YUV420; + else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444) + 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 +538,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 +563,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;
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) Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: 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_dp.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++ 5 files changed, 49 insertions(+)