Re: [Intel-gfx] [PATCH v10 5/7] drm/i915: Fill in native_420 field

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Tuesday, February 21, 2023 10:11 AM
> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH v10 5/7] drm/i915: Fill in native_420 field
> 
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Suraj Kandpal
> > > Sent: Wednesday, February 15, 2023 8:47 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH v10 5/7] drm/i915: Fill in native_420
> > > field
> >
> > Append "display"
> >
> 
> drm/i915/display ?

Yes

> > >
> > > Now that we have laid the groundwork for YUV420 Enablement we fill
> > > up
> > > native_420 field in vdsc_cfg and add appropriate checks wherever
> > required.
> > >
> > > ---v2
> > > -adding native_422 field as 0 [Vandita] -filling in
> > > second_line_bpg_offset, second_line_offset_adj and nsl_bpg_offset in
> > > vds_cfg when native_420 is true
> > >
> > > ---v3
> > > -adding display version check to solve igt issue
> > >
> > > --v7
> > > -remove is_pipe_dsc check as its always true for D14 [Jani]
> > >
> > > --v10
> > > -keep sink capability check [Jani]
> > > -move from !(x == y  || w == z) to x !=y && w != z [Jani]
> > >
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > > index 05e749861658..7065203460d3 100644
> > > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > > @@ -1534,8 +1534,6 @@ static int gen11_dsi_dsc_compute_config(struct
> > > intel_encoder *encoder,
> > >  	if (crtc_state->dsc.slice_count > 1)
> > >  		crtc_state->dsc.dsc_split = true;
> > >
> > > -	vdsc_cfg->convert_rgb = true;
> > > -
> > >  	/* FIXME: initialize from VBT */
> > >  	vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 1a397ab710dd..baa5af7d3bdc 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1466,9 +1466,10 @@ static int intel_dp_dsc_compute_params(struct
> > > intel_encoder *encoder,
> > >  	vdsc_cfg->dsc_version_minor =
> > >  		min(intel_dp_source_dsc_version_minor(intel_dp),
> > >  		    intel_dp_sink_dsc_version_minor(intel_dp));
> > > -
> > > -	vdsc_cfg->convert_rgb = intel_dp-
> > > >dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
> > > -		DP_DSC_RGB;
> > > +	if (vdsc_cfg->convert_rgb)
> > > +		vdsc_cfg->convert_rgb =
> > > +			intel_dp-
> > >dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP -  DP_DSC_SUPPORT] &
> > > +			DP_DSC_RGB;
> > >
> > >  	line_buf_depth = drm_dp_dsc_sink_line_buf_depth(intel_dp-
> > >dsc_dpcd);
> > >  	if (!line_buf_depth) {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > index ed16f63d6355..19f9fb53f139 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > @@ -460,14 +460,47 @@ int intel_dsc_compute_params(struct
> > >intel_crtc_state
> > > *pipe_config)
> > >  	vdsc_cfg->pic_width = pipe_config- hw.adjusted_mode.crtc_hdisplay;
> > >  	vdsc_cfg->slice_width = DIV_ROUND_UP(vdsc_cfg->pic_width,
> > >  					     pipe_config->dsc.slice_count);
> > > -
> > > -	/* Gen 11 does not support YCbCr */
> > > +	/*
> > > +	 * According to DSC 1.2 specs if colorspace is YCbCr then
> > > +convert_rgb
> > is 0
> > > +	 * else 1
> > > +	 */
> > > +	vdsc_cfg->convert_rgb = pipe_config->output_format !=
> > > INTEL_OUTPUT_FORMAT_YCBCR420 &&
> > > +				pipe_config->output_format !=
> > > INTEL_OUTPUT_FORMAT_YCBCR444;
> > > +
> > > +	if (pipe_config->output_format ==
> > INTEL_OUTPUT_FORMAT_YCBCR420)
> > > +		vdsc_cfg->native_420 = true;
> > > +	/* We do not support YcBCr422 as of now */
> > > +	vdsc_cfg->native_422 = false;
> > > +	/* Gen 11 does not support YCbCr422 */
> >
> > This comment can be merged with the one above.
> >
> 
> Can I remove "/* Gen 11 does not support YCbCr422 */ "
> And just keep "/* We do not support YcBCr422 as of now */" or Make it something
> like " Gen 11+ does not support YCbCr422 "

I think "/* We do not support YcBCr422 as of now */" looks better.

> > >  	vdsc_cfg->simple_422 = false;
> > >  	/* Gen 11 does not support VBR */
> > >  	vdsc_cfg->vbr_enable = false;
> > >
> > >  	/* Gen 11 only supports integral values of bpp */
> > >  	vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
> >
> > Leave a line gap here
> >
> > > +	/*
> > > +	 * According to DSC 1.2 specs if native_420 is set:
> >
> > It would be good to add the section name as well for ease of reference.
> >
> > > +	 * -We need to double the current bpp.
> > > +	 * -second_line_bpg_offset is 12 in general and equal to
> > > +2*(slice_height-1) if
> > > slice
> > > +	 * height < 8.
> > > +	 * -second_line_offset_adj is 512 as shown by emperical values to
> > > +yeild best
> > > chroma
> > > +	 * preservation in second line.
> > > +	 * -nsl_bpg_offset is calculated as
> > > +second_line_offset/slice_height
> > > +-1 then
> > > rounded
> > > +	 * up to 16 fractional bits, we left shift second line offset by
> > > +11 to preserve
> > > 11
> > > +	 * fractional bits.
> > > +	 */
> > > +	if (vdsc_cfg->native_420) {
> > > +		vdsc_cfg->bits_per_pixel <<= 1;
> >
> > Leave a line gap here
> >
> > > +		if (vdsc_cfg->slice_height >= 8)
> > > +			vdsc_cfg->second_line_bpg_offset = 12;
> > > +		else
> > > +			vdsc_cfg->second_line_bpg_offset =
> > > +				2 * (vdsc_cfg->slice_height - 1);
> >
> > Here as well
> >
> > > +		vdsc_cfg->second_line_offset_adj = 512;
> > > +		vdsc_cfg->nsl_bpg_offset = DIV_ROUND_UP(vdsc_cfg-
> > > >second_line_bpg_offset << 11,
> > > +							vdsc_cfg-
> > >slice_height - 1);
> >
> > The parameters we compute here are being programmed only for gen14. We
> > should limit the computation if they are going to be unused for prior
> > platforms.
> 
> How about we make native_420 field of vdsc_cfg true only if DISPLAY_VER() >= 14
> this should take care of not filling any extra fields or computations

Yeah we can even do that

Regards
Uma Shankar
 
> Regards,
> Suraj Kandpal
> >
> > > +	}
> > > +
> > >  	vdsc_cfg->bits_per_component = pipe_config->pipe_bpp / 3;
> > >
> > >  	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) { @@ -594,8 +627,13
> > @@
> > > static void intel_dsc_pps_configure(const struct intel_crtc_state
> > *crtc_state)
> > >  		DSC_VER_MIN_SHIFT |
> > >  		vdsc_cfg->bits_per_component << DSC_BPC_SHIFT |
> > >  		vdsc_cfg->line_buf_depth << DSC_LINE_BUF_DEPTH_SHIFT;
> > > -	if (vdsc_cfg->dsc_version_minor == 2)
> > > +	if (vdsc_cfg->dsc_version_minor == 2) {
> > >  		pps_val |= DSC_ALT_ICH_SEL;
> > > +		if (vdsc_cfg->native_420)
> > > +			pps_val |= DSC_NATIVE_420_ENABLE;
> > > +		if (vdsc_cfg->native_422)
> > > +			pps_val |= DSC_NATIVE_422_ENABLE;
> > > +	}
> > >  	if (vdsc_cfg->block_pred_enable)
> > >  		pps_val |= DSC_BLOCK_PREDICTION;
> > >  	if (vdsc_cfg->convert_rgb)
> > > @@ -906,6 +944,32 @@ static void intel_dsc_pps_configure(const
> > > struct intel_crtc_state *crtc_state)
> > >  				       pps_val);
> > >  	}
> > >
> > > +	if (DISPLAY_VER(dev_priv) >= 14) {
> > > +		/* Populate PICTURE_PARAMETER_SET_17 registers */
> > > +		pps_val = 0;
> > > +		pps_val |= DSC_SL_BPG_OFFSET(vdsc_cfg-
> > > >second_line_bpg_offset);
> > > +		drm_dbg_kms(&dev_priv->drm, "PPS17 = 0x%08x\n",
> > pps_val);
> > > +		intel_de_write(dev_priv,
> > > +			       MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe),
> > > +			       pps_val);
> > > +		if (crtc_state->dsc.dsc_split)
> > > +			intel_de_write(dev_priv,
> > > +
> > > MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe),
> > > +				       pps_val);
> > > +
> > > +		/* Populate PICTURE_PARAMETER_SET_18 registers */
> > > +		pps_val = 0;
> > > +		pps_val |= DSC_NSL_BPG_OFFSET(vdsc_cfg-
> > >nsl_bpg_offset) |
> > > +			   DSC_SL_OFFSET_ADJ(vdsc_cfg-
> > >second_line_offset_adj);
> > > +		drm_dbg_kms(&dev_priv->drm, "PPS18 = 0x%08x\n",
> > pps_val);
> > > +		intel_de_write(dev_priv,
> > > +			       MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe),
> > > +			       pps_val);
> > > +		if (crtc_state->dsc.dsc_split)
> > > +			intel_de_write(dev_priv,
> > > +
> > > MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe),
> > > +				       pps_val);
> > > +	}
> >
> > Leave a line gap.
> >
> > >  	/* Populate the RC_BUF_THRESH registers */
> > >  	memset(rc_buf_thresh_dword, 0, sizeof(rc_buf_thresh_dword));
> > >  	for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> > > --
> > > 2.25.1