Re: [Intel-gfx] [PATCH v10 03/13] drm/i915/dp: Add Scaler constraint for YCbCr420 output

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 09, 2023 at 02:01:06PM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> Thanks for the comments and suggestions. Please find my response inline:
> 
> On 3/8/2023 8:56 PM, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2023 at 05:10:57PM +0200, Ville Syrjälä wrote:
> >> On Mon, Feb 27, 2023 at 09:33:14AM +0530, Ankit Nautiyal wrote:
> >>> For YCbCr420 output, scaler is required for downsampling.
> >>> Scaler can be used only when source size smaller than max_src_w and
> >>> max_src_h as defined by for the platform.
> >>> So go for native YCbCr420 only if there are no scaler constraints.
> >>>
> >>> v2: Corrected max-width based on Display Version.
> >>>
> >>> v3: Updated max-width as per latest Bspec change.
> >>>
> >>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/display/intel_dp.c | 41 ++++++++++++++++++++++---
> >>>   1 file changed, 37 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >>> index 1a30cc021b25..e95fc0f0d13a 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >>> @@ -804,11 +804,36 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static bool
> >>> +ycbcr420_scaler_constraints(struct drm_i915_private *i915,
> >>> +			    const struct drm_display_mode *mode)
> >>> +{
> >>> +	int max_src_w, max_src_h;
> >>> +
> >>> +	if (DISPLAY_VER(i915) < 11) {
> >>> +		max_src_w = 4096;
> >>> +		max_src_h = 4096;
> >>> +	} else if (DISPLAY_VER(i915) < 12) {
> >>> +		max_src_w = 5120;
> >>> +		max_src_h = 4096;
> >>> +	} else if (DISPLAY_VER(i915) < 14) {
> >>> +		max_src_w = 5120;
> >>> +		max_src_h = 8192;
> >>> +	} else {
> >>> +		max_src_w = 4096;
> >>> +		max_src_h = 8192;
> >>> +	}
> >>> +
> >>> +	return mode->hdisplay > max_src_w || mode->vdisplay > max_src_h;
> >>> +}
> >>> +
> >> I don't really like this. If we do something like this
> >> then it should be the scaler code that checks this stuff.
> 
> Makes sense, this does belong to the scaler file and scaler checks.
> 
> 
> >>
> >> However, after pondering about this more I'm actually
> >> leaning towards using 4:4:4 output whenever possible,
> >> only going for 4:2:0 if absolutely necessary. That
> >> avoids having to deal with all the annoying scaler/etc
> >> limitations.
> > In fact perhaps best to try RGB first (also avoids the whole
> > pipe CSC mess on glk), then YCbCr 4:4:4 (still avoids the
> > scaler mess), and finally accepting that we need to do
> > native YCbCr 4:2:0 output.
> 
> Ok so if I understand correctly, in intel_dp_output_format()
> 
> If sink_format is YCBCR420:
> 
> -first try with output_format as RGB and RGB->YCBCR420 conv via DFP (if 
> conv supported)
> 
> -Or else try with output_format as YCBCR444 and use YCBCR444->YCBCR420 
> conv via DFP (if conv supported)
> 
> -else try with output_format YCBCR420.

Yeah something along those lines. Maybe it can be expressed
in a pretty generic way even:

if (sink_format=RGB||dfp_can_convert_from_rgb(sink_format))
	return RGB;
if (sink_format=YCbCr444||dfp_can_convert_from_ycbcr444(sink_format))
	return YCbCr444;
return sink_format;

> 
> If there are indeed scaler constraints, those are to be taken care in 
> scaler check code.
> 
> Shall I drop the scaler constraint for now and have that as a separate 
> series?

Don't we already have sufficient checks in the scaler code?
Well, if not a separate series for that seems better.

-- 
Ville Syrjälä
Intel