[Intel-gfx] [PATCH v5 3/9] drm/i915/dp: Replace intel_dp.dfp members with the new crtc_state sink_format

[Nautiyal, Ankit K]

On 11/15/2022 4:30 PM, Ville Syrjälä wrote:
> On Tue, Nov 15, 2022 at 12:23:53PM +0530, Nautiyal, Ankit K wrote:
>> On 11/11/2022 2:36 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 28, 2022 at 03:14:05PM +0530, Ankit Nautiyal wrote:
>>>> The decision to use DFP output format conversion capabilities should be
>>>> during compute_config phase.
>>>>
>>>> This patch uses the members of intel_dp->dfp to only store the
>>>> format conversion capabilities of the DP device and uses the crtc_state
>>>> sink_format member, to program the protocol-converter for
>>>> colorspace/format conversion.
>>>>
>>>> v2: Use sink_format to determine the color conversion config for the
>>>> pcon (Ville).
>>>>
>>>> v3: Fix typo: missing 'break' in switch case (lkp kernel test robot).
>>>>
>>>> v4: Add helper to check if DP supports YCBCR420.
>>>>
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dp.c | 122 ++++++++++++++++--------
>>>>    1 file changed, 84 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index 0e4f7b467970..95d0c653db7f 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -790,6 +790,7 @@ intel_dp_output_format(struct intel_connector *connector,
>>>>    		       enum intel_output_format sink_format)
>>>>    {
>>>>    	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>>>    
>>>>    	if (!connector->base.ycbcr_420_allowed ||
>>>>    	    sink_format != INTEL_OUTPUT_FORMAT_YCBCR420)
>>>> @@ -799,6 +800,10 @@ intel_dp_output_format(struct intel_connector *connector,
>>>>    	    intel_dp->dfp.ycbcr_444_to_420)
>>>>    		return INTEL_OUTPUT_FORMAT_RGB;
>>>>    
>>>> +	/* Prefer 4:2:0 passthrough over 4:4:4->4:2:0 conversion */
>>>> +	if (DISPLAY_VER(i915) >= 11 && intel_dp->dfp.ycbcr420_passthrough)
>>>> +		return INTEL_OUTPUT_FORMAT_YCBCR420;
>>>> +
>>>>    	if (intel_dp->dfp.ycbcr_444_to_420)
>>>>    		return INTEL_OUTPUT_FORMAT_YCBCR444;
>>>>    	else
>>> The else branch here is also 420, so the whole logic
>>> here doesn't seem to flow entirely sensibly.
>>>
>>> Thinking a bit more abstractly, could we restate
>>> this whole problem as something more like this?
>>>
>>> if (source_can_output(sink_format))
>>> 	return sink_format;
>>>
>>> if (source_can_output(RGB) &&
>>>       dfp_can_convert_from_rgb(sink_format))
>>> 	return RGB;
>>>
>>> if (source_can_output(YCBCR444) &&
>>>       dfp_can_convert_from_ycbcr444(sink_format))
>>> 	return YCBCR444;
>> This make sense and will also help to add 422 support easier.
>>
>> I am only seeing one problem: about how to have DSC considerations
>> during source_can_output( ).
>>
>> Gen 11+ should support YCBCR420. So for any ycbcr420_only mode we should
>> have sink_format, and output_format : YCbCr420.
>>
>> This works well for cases where DSC doesn't get in picture. When higher
>> modes like 8k60 ycbcr420_only are involved, we need to use DSC.
>>
>> Currently, our DSC1.1 does not support YCbCr420. The problem is that we
>> go for, dsc_compute_config at a later time.
>>
>> This issue might have been masked, due to the messy order of checks in
>> intel_dp_output_format.
>>
>> Specifically With HDMI2.1 PCONs supporting color conv, for such a case
>> we can have output_format as RGB, and sink_format as YCbCr420.
>>
>> The DSC compression with RGB and then configure PCON to Decompress,
>> conv. to YCbCr420.
>>
>> So can we put a check in source_can_output for Gen11+ where DSC might be
>> required, so that with source_can_output(YCBCR420) returns false in such
>> case, where DSC is the only way?
> I'm thinking it should work well enough without any extra checks
> since we'll always try RGB before YCbC4 4:2:0. I guess the only
> case where it could fail is if the sink only supports 4:2:0 for
> that particular mode. Then we'd also try direct YCbC4 4:2:0 output
> on icl+. Dunno if such sinks are still a thing when DSC is in the
> picture.

There indeed are some HDMI 8k Panels that have 8k at 60 in Ycbcr420 only mode.

These do not have DSC, so without DSC these can support 8k at 60 only in 
YCbCr420.

(We have a SONY XBR-Z8H in lab, and there are some in market from 
Samsung too, which support 8k at 60 in YCbcr420 only).

With PCON we can support these. As mentioned above, we need to send 
compressed stream in RGB to PCON.

PCON decompresses, converts RGB444 to Ycbcr420. The sink is sent 8k at 60 
Ycbcr420 uncompressed.

>
> Hmm. Do PCONs even support DSC + color conversion/etc. at the
> same time? They'd have to decompress and potentially recompress
> the data in that case.


Yes there are PCONs that support 3 modes of operations along with color 
conversion.

DSC1.2 pass-through: A DSC1.2 compressed just gets forwarded to DSC1.2 
supporting HDMI2.1sink.

DSC1.1->DSC1.2 : DSC1.1 compressed stream is decompressed and then 
re-compressed again with DSC1.2 and forward to DSC1.2 supporting HDMI2.1 
sink.

DSC1.x->uncompress: DSC1.x is decompressed and sent to HDMI sink that 
does not support DSC.

(the case mentioned above, uses this 3rd option)

>
> The problem with adding DSC checks into source_can_output() is
> that we'd need to express those in a way that is also usable in
> .mode_valid() since we'd want to use the same code there I think.
> Looks like we do have some DSC stuff in there already, but it
> doesn't seem to take that into account in the link bandwidth
> check. So to me it looks like the whole DSC support is incomplete
> anyway.

Hmm. We were not getting this earlier, due to the order in which 
YCbCr420 sink_format was chosen.

When sink format isYCbCr420, and DFP supports RGB444->YCBCR420, always 
go with the conv via PCON.

This seems crude though, because if source supports YCBCR20, it is 
natural to go with that first, and if not then try conv via PCON.

DSC consideration and the above case of 8k at 60 YCbcr420 makes this 
problematic.


Regards,

Ankit