| Message ID | 20190508081757.28042-4-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Support for DP YCbCr4:2:0 outputs | expand |
On Wed, May 08, 2019 at 11:17:54AM +0300, Gwan-gyeong Mun wrote: > Function intel_pixel_encoding_setup_vsc handles vsc header and data block > setup for pixel encoding / colorimetry format. > > Setup VSC header and data block in function intel_pixel_encoding_setup_vsc > for pixel encoding / colorimetry format as per dp 1.4a spec, > section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section 2.2.5.7.5, > table 2-120:VSC SDP Payload for DB16 through DB18. > > v2: > Minor style fix. [Maarten] > Refer to commit ids instead of patchwork. [Maarten] > > v6: Rebase > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 1 + > drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > 3 files changed, 76 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index cd5277d98b03..2f1688ea5a2c 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder, > > intel_edp_backlight_on(crtc_state, conn_state); > intel_psr_enable(intel_dp, crtc_state); > + intel_dp_ycbcr_420_enable(intel_dp, crtc_state); I wonder if this is a bit too late. But we do it for PSR here, so I guess we should think about this for both cases. We should actually add full readout + state checker for the VSC SDP for both cases as well. But that can be done later. > intel_edp_drrs_enable(intel_dp, crtc_state); > > if (crtc_state->has_audio) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 06a3417a88d1..74aad8830a80 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4394,6 +4394,79 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp, > return 0; > } > > +static void > +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct dp_vsc_sdp vsc_sdp; > + > + if (!intel_dp->attached_connector->base.ycbcr_420_allowed) > + return; > + > + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */ > + memset(&vsc_sdp, 0, sizeof(vsc_sdp)); Can be replaced with '= {}' in the declaration. > + vsc_sdp.sdp_header.HB0 = 0; > + vsc_sdp.sdp_header.HB1 = 0x7; > + > + /* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/ > + * Colorimetry Format indication. A DP Source device is allowed > + * to indicate the pixel encoding/colorimetry format to the DP Sink > + * device with VSC SDP only when the DP Sink device supports it > + * (i.e., VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit in the register > + * DPRX_FEATURE_ENUMERATION_LIST (DPCD Address 02210h, bit 3) is set to 1) > + */ Are we checking that bit somewhere? I suppose the sink might a bit nuts if it declares 420_only modes without that set, but maybe it should be checked anyway. Also non-standard comment format all over. Should be /* * blah */ > + vsc_sdp.sdp_header.HB2 = 0x5; > + > + /* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/ > + * Colorimetry Format indication (HB2 = 05h). > + */ > + vsc_sdp.sdp_header.HB3 = 0x13; > + /* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 = 1h > + * DB16[3:0] DP 1.4a spec, Table 2-120 > + */ > + > + /* Commit id (25edf91501b8 "drm/i915: prepare csc unit for YCBCR420 output") > + * uses the BT.709 color space to perform RGB->YCBCR conversion. > + */ I don't think referring to specific commit here is particularly helpful. The situation will change anyway at some point. > + vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/ > + vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */ > + > + /* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and Y Only, > + * the following Component Bit Depth values are defined: > + * 001b = 8bpc. > + * 010b = 10bpc. > + * 011b = 12bpc. > + * 100b = 16bpc. > + */ > + vsc_sdp.DB17 = 0x1; Don't we need to base this on pipe_bpp? And I'm thinking we want the CEA bit set here as well. > + > + /* > + * Content Type (Bits 2:0) > + * 000b = Not defined. > + * 001b = Graphics. > + * 010b = Photo. > + * 011b = Video. > + * 100b = Game > + * All other values are RESERVED. > + * Note: See CTA-861-G for the definition and expected > + * processing by a stream sink for the above contect types. > + */ > + vsc_sdp.DB18 = 0; Hmm. I guess we could add the content type prop for DP too. But that's something for the future. All the magic numbers should be defined somewhere in the core dp header. But that could be done as a followup. The PSR code probably needs changing too in that case. > + > + intel_dig_port->write_infoframe(&intel_dig_port->base, > + crtc_state, DP_SDP_VSC, &vsc_sdp, sizeof(vsc_sdp)); > +} > + > +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420) > + return; > + > + intel_pixel_encoding_setup_vsc(intel_dp, crtc_state); > +} > + > static u8 intel_dp_autotest_link_training(struct intel_dp *intel_dp) > { > int status = 0; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 247893ed1543..5d1845526cf8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1576,6 +1576,8 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); > void intel_dp_set_m_n(const struct intel_crtc_state *crtc_state, > enum link_m_n_set m_n); > +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state); > int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); > bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, > struct dpll *best_clock); > -- > 2.21.0
On Wed, 2019-05-08 at 20:56 +0300, Ville Syrjälä wrote: > On Wed, May 08, 2019 at 11:17:54AM +0300, Gwan-gyeong Mun wrote: > > Function intel_pixel_encoding_setup_vsc handles vsc header and data > > block > > setup for pixel encoding / colorimetry format. > > > > Setup VSC header and data block in function > > intel_pixel_encoding_setup_vsc > > for pixel encoding / colorimetry format as per dp 1.4a spec, > > section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section > > 2.2.5.7.5, > > table 2-120:VSC SDP Payload for DB16 through DB18. > > > > v2: > > Minor style fix. [Maarten] > > Refer to commit ids instead of patchwork. [Maarten] > > > > v6: Rebase > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 1 + > > drivers/gpu/drm/i915/intel_dp.c | 73 > > ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > 3 files changed, 76 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index cd5277d98b03..2f1688ea5a2c 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct > > intel_encoder *encoder, > > > > intel_edp_backlight_on(crtc_state, conn_state); > > intel_psr_enable(intel_dp, crtc_state); > > + intel_dp_ycbcr_420_enable(intel_dp, crtc_state); > > I wonder if this is a bit too late. But we do it for PSR here, so I > guess we should think about this for both cases. We should actually > add full readout + state checker for the VSC SDP for both cases as > well. But that can be done later. > I'll check it again. > > intel_edp_drrs_enable(intel_dp, crtc_state); > > > > if (crtc_state->has_audio) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 06a3417a88d1..74aad8830a80 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4394,6 +4394,79 @@ u8 intel_dp_dsc_get_slice_count(struct > > intel_dp *intel_dp, > > return 0; > > } > > > > +static void > > +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp, > > + const struct intel_crtc_state > > *crtc_state) > > +{ > > + struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > + struct dp_vsc_sdp vsc_sdp; > > + > > + if (!intel_dp->attached_connector->base.ycbcr_420_allowed) > > + return; > > + > > + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 > > */ > > + memset(&vsc_sdp, 0, sizeof(vsc_sdp)); > > Can be replaced with '= {}' in the declaration. > Yes, I'll use a structure initializer instead of memset(). > > + vsc_sdp.sdp_header.HB0 = 0; > > + vsc_sdp.sdp_header.HB1 = 0x7; > > + > > + /* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/ > > + * Colorimetry Format indication. A DP Source device is allowed > > + * to indicate the pixel encoding/colorimetry format to the DP > > Sink > > + * device with VSC SDP only when the DP Sink device supports it > > + * (i.e., VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit in > > the register > > + * DPRX_FEATURE_ENUMERATION_LIST (DPCD Address 02210h, bit 3) > > is set to 1) > > + */ > > Are we checking that bit somewhere? I suppose the sink might a bit > nuts > if it declares 420_only modes without that set, but maybe it should > be > checked anyway. > > Also non-standard comment format all over. Should be > /* > * blah > */ > I'll add a checking of VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit with intel_dp_get_colorimetry_status() on intel_dp_ycbcr420_config(). And I will fix non-standard comment format. > > + vsc_sdp.sdp_header.HB2 = 0x5; > > + > > + /* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/ > > + * Colorimetry Format indication (HB2 = 05h). > > + */ > > + vsc_sdp.sdp_header.HB3 = 0x13; > > + /* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 = > > 1h > > + * DB16[3:0] DP 1.4a spec, Table 2-120 > > + */ > > + > > + /* Commit id (25edf91501b8 "drm/i915: prepare csc unit for > > YCBCR420 output") > > + * uses the BT.709 color space to perform RGB->YCBCR > > conversion. > > + */ > > I don't think referring to specific commit here is particularly > helpful. > The situation will change anyway at some point. > Yes, I'll remove a referring to specific commit. > > + vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/ > > + vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */ > > + > > + /* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and > > Y Only, > > + * the following Component Bit Depth values are defined: > > + * 001b = 8bpc. > > + * 010b = 10bpc. > > + * 011b = 12bpc. > > + * 100b = 16bpc. > > + */ > > + vsc_sdp.DB17 = 0x1; > > Don't we need to base this on pipe_bpp? > > And I'm thinking we want the CEA bit set here as well. > I'll move a setting of dynamic range bit and a setting of bpc which is based on pipe_bpp from a "drm/i915/dp: Change a link bandwidth computation for DP" commit. > > + > > + /* > > + * Content Type (Bits 2:0) > > + * 000b = Not defined. > > + * 001b = Graphics. > > + * 010b = Photo. > > + * 011b = Video. > > + * 100b = Game > > + * All other values are RESERVED. > > + * Note: See CTA-861-G for the definition and expected > > + * processing by a stream sink for the above contect types. > > + */ > > + vsc_sdp.DB18 = 0; > > Hmm. I guess we could add the content type prop for DP too. > But that's something for the future. > Yes. Yes. After this step, I would like to handle it. > All the magic numbers should be defined somewhere in the core dp > header. > But that could be done as a followup. The PSR code probably needs > changing too in that case. > Sure, After this step, I also would like to refactor these magic numbers. > > + > > + intel_dig_port->write_infoframe(&intel_dig_port->base, > > + crtc_state, DP_SDP_VSC, &vsc_sdp, > > sizeof(vsc_sdp)); > > +} > > + > > +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, > > + const struct intel_crtc_state > > *crtc_state) > > +{ > > + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420) > > + return; > > + > > + intel_pixel_encoding_setup_vsc(intel_dp, crtc_state); > > +} > > + > > static u8 intel_dp_autotest_link_training(struct intel_dp > > *intel_dp) > > { > > int status = 0; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 247893ed1543..5d1845526cf8 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1576,6 +1576,8 @@ void intel_dp_get_m_n(struct intel_crtc > > *crtc, > > struct intel_crtc_state *pipe_config); > > void intel_dp_set_m_n(const struct intel_crtc_state *crtc_state, > > enum link_m_n_set m_n); > > +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, > > + const struct intel_crtc_state > > *crtc_state); > > int intel_dotclock_calculate(int link_freq, const struct > > intel_link_m_n *m_n); > > bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, > > struct dpll *best_clock); > > -- > > 2.21.0
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index cd5277d98b03..2f1688ea5a2c 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder, intel_edp_backlight_on(crtc_state, conn_state); intel_psr_enable(intel_dp, crtc_state); + intel_dp_ycbcr_420_enable(intel_dp, crtc_state); intel_edp_drrs_enable(intel_dp, crtc_state); if (crtc_state->has_audio) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 06a3417a88d1..74aad8830a80 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4394,6 +4394,79 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp, return 0; } +static void +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct dp_vsc_sdp vsc_sdp; + + if (!intel_dp->attached_connector->base.ycbcr_420_allowed) + return; + + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */ + memset(&vsc_sdp, 0, sizeof(vsc_sdp)); + vsc_sdp.sdp_header.HB0 = 0; + vsc_sdp.sdp_header.HB1 = 0x7; + + /* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/ + * Colorimetry Format indication. A DP Source device is allowed + * to indicate the pixel encoding/colorimetry format to the DP Sink + * device with VSC SDP only when the DP Sink device supports it + * (i.e., VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit in the register + * DPRX_FEATURE_ENUMERATION_LIST (DPCD Address 02210h, bit 3) is set to 1) + */ + vsc_sdp.sdp_header.HB2 = 0x5; + + /* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/ + * Colorimetry Format indication (HB2 = 05h). + */ + vsc_sdp.sdp_header.HB3 = 0x13; + /* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 = 1h + * DB16[3:0] DP 1.4a spec, Table 2-120 + */ + + /* Commit id (25edf91501b8 "drm/i915: prepare csc unit for YCBCR420 output") + * uses the BT.709 color space to perform RGB->YCBCR conversion. + */ + vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/ + vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */ + + /* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and Y Only, + * the following Component Bit Depth values are defined: + * 001b = 8bpc. + * 010b = 10bpc. + * 011b = 12bpc. + * 100b = 16bpc. + */ + vsc_sdp.DB17 = 0x1; + + /* + * Content Type (Bits 2:0) + * 000b = Not defined. + * 001b = Graphics. + * 010b = Photo. + * 011b = Video. + * 100b = Game + * All other values are RESERVED. + * Note: See CTA-861-G for the definition and expected + * processing by a stream sink for the above contect types. + */ + vsc_sdp.DB18 = 0; + + intel_dig_port->write_infoframe(&intel_dig_port->base, + crtc_state, DP_SDP_VSC, &vsc_sdp, sizeof(vsc_sdp)); +} + +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state) +{ + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420) + return; + + intel_pixel_encoding_setup_vsc(intel_dp, crtc_state); +} + static u8 intel_dp_autotest_link_training(struct intel_dp *intel_dp) { int status = 0; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 247893ed1543..5d1845526cf8 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1576,6 +1576,8 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config); void intel_dp_set_m_n(const struct intel_crtc_state *crtc_state, enum link_m_n_set m_n); +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state); int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, struct dpll *best_clock);
Function intel_pixel_encoding_setup_vsc handles vsc header and data block setup for pixel encoding / colorimetry format. Setup VSC header and data block in function intel_pixel_encoding_setup_vsc for pixel encoding / colorimetry format as per dp 1.4a spec, section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section 2.2.5.7.5, table 2-120:VSC SDP Payload for DB16 through DB18. v2: Minor style fix. [Maarten] Refer to commit ids instead of patchwork. [Maarten] v6: Rebase Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_dp.c | 73 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 76 insertions(+)