Re: [PATCH v6 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format

["Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel