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

[Ville Syrjälä]

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.

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.

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.

-- 
Ville Syrjälä
Intel