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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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.

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?

Regards,

Ankit




>
>>>   static enum intel_output_format
>>>   intel_dp_output_format(struct intel_connector *connector,
>>> +		       const struct drm_display_mode *mode,
>>>   		       enum intel_output_format sink_format)
>>>   {
>>>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> +	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>>   
>>>   	if (!connector->base.ycbcr_420_allowed ||
>>>   	    sink_format != INTEL_OUTPUT_FORMAT_YCBCR420)
>>> @@ -820,8 +845,15 @@ intel_dp_output_format(struct intel_connector *connector,
>>>   
>>>   	if (intel_dp->dfp.ycbcr_444_to_420)
>>>   		return INTEL_OUTPUT_FORMAT_YCBCR444;
>>> -	else
>>> +
>>> +	/*
>>> +	 * For YCbCr420 output, scaler is required for downsampling
>>> +	 * So go for native YCbCr420 only if there are no scaler constraints.
>>> +	 */
>>> +	if (!ycbcr420_scaler_constraints(i915, mode))
>>>   		return INTEL_OUTPUT_FORMAT_YCBCR420;
>>> +
>>> +	return INTEL_OUTPUT_FORMAT_RGB;
>>>   }
>>>   
>>>   int intel_dp_min_bpp(enum intel_output_format output_format)
>>> @@ -857,7 +889,7 @@ intel_dp_mode_min_output_bpp(struct intel_connector *connector,
>>>   	else
>>>   		sink_format = INTEL_OUTPUT_FORMAT_RGB;
>>>   
>>> -	output_format = intel_dp_output_format(connector, sink_format);
>>> +	output_format = intel_dp_output_format(connector, mode, sink_format);
>>>   
>>>   	return intel_dp_output_bpp(output_format, intel_dp_min_bpp(output_format));
>>>   }
>>> @@ -2052,7 +2084,8 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>>>   		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
>>>   	}
>>>   
>>> -	crtc_state->output_format = intel_dp_output_format(connector, crtc_state->sink_format);
>>> +	crtc_state->output_format = intel_dp_output_format(connector, adjusted_mode,
>>> +							   crtc_state->sink_format);
>>>   
>>>   	ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>>>   					   respect_downstream_limits);
>>> @@ -2063,7 +2096,7 @@ intel_dp_compute_output_format(struct intel_encoder *encoder,
>>>   			return ret;
>>>   
>>>   		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>>> -		crtc_state->output_format = intel_dp_output_format(connector,
>>> +		crtc_state->output_format = intel_dp_output_format(connector, adjusted_mode,
>>>   								   crtc_state->sink_format);
>>>   		ret = intel_dp_compute_link_config(encoder, crtc_state, conn_state,
>>>   						   respect_downstream_limits);
>>> -- 
>>> 2.25.1
>> -- 
>> Ville Syrjälä
>> Intel