diff mbox series

[20/20] drm/i915/dp: Disable DFP RGB->YCbCr conversion for now

Message ID 20211015133921.4609-21-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Fix up DP DFP 4:2:0 handling more | expand

Commit Message

Ville Syrjala Oct. 15, 2021, 1:39 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We lack sufficient state tracking to figure out whether
we want the DFP to perform the RGB->YCbCr conversion for us
or not. So currently we are blindly just enabling that all the
time when supported by the DFP. That is nonsense. So until
we imporve our state tracking for this just disable the feature.

Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Nautiyal, Ankit K Oct. 27, 2021, 7:27 a.m. UTC | #1 
On 10/15/2021 7:09 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We lack sufficient state tracking to figure out whether
> we want the DFP to perform the RGB->YCbCr conversion for us
> or not. So currently we are blindly just enabling that all the
> time when supported by the DFP. That is nonsense. So until
> we imporve our state tracking for this just disable the feature.
>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 29b12456c461..3e2a29b589a9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1107,6 +1107,7 @@ static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>   static bool intel_dp_is_ycbcr420(struct intel_dp *intel_dp,
>   				 const struct intel_crtc_state *crtc_state)
>   {
> +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
>   	return crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>   		(crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
>   		 intel_dp->dfp.ycbcr_444_to_420);
> @@ -2456,6 +2457,7 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
>   			    "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n",
>   			    enabledisable(intel_dp->dfp.ycbcr_444_to_420));
>   
> +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
>   	tmp = intel_dp->dfp.rgb_to_ycbcr ?
>   		DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
>   
> @@ -4261,6 +4263,14 @@ intel_dp_update_420(struct intel_dp *intel_dp)
>   	rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
>   								 intel_dp->downstream_ports,
>   								 DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
> +	/*
> +	 * FIXME need to actually track whether we're really
> +	 * going to be doing the RGB->YCbCr connversion or not.
> +	 * We can't tell by simply looking at intel_dp->dfp.rgb_to_ycbcr.
> +	 * Readout is going to annoying due to having to read that
> +	 * state from external hardware that may vanish at any time :(
> +	 */


Hmm right. Do you have any suggestion what should be the better place 
for defining the policy for using DFPs CSC or use our own HW, in case of 
YCbCr 420 output?

Also, IIUC, this might need consideration for higher modes with DSC1.1 
H/w that do not support compression for YCbCr420.

In that case we might need to either use RGB compressed output and ask 
for DFPs CSC for conversion to 444 and then down sampling to 420.

Regards,

Ankit

> +	rgb_to_ycbcr = false;
>   
>   	if (DISPLAY_VER(i915) >= 11) {
>   		/* Let PCON convert from RGB->YCbCr if possible */
Ville Syrjala Oct. 27, 2021, 8:54 a.m. UTC | #2 
On Wed, Oct 27, 2021 at 12:57:37PM +0530, Nautiyal, Ankit K wrote:
> 
> On 10/15/2021 7:09 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We lack sufficient state tracking to figure out whether
> > we want the DFP to perform the RGB->YCbCr conversion for us
> > or not. So currently we are blindly just enabling that all the
> > time when supported by the DFP. That is nonsense. So until
> > we imporve our state tracking for this just disable the feature.
> >
> > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 29b12456c461..3e2a29b589a9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1107,6 +1107,7 @@ static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
> >   static bool intel_dp_is_ycbcr420(struct intel_dp *intel_dp,
> >   				 const struct intel_crtc_state *crtc_state)
> >   {
> > +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
> >   	return crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> >   		(crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
> >   		 intel_dp->dfp.ycbcr_444_to_420);
> > @@ -2456,6 +2457,7 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
> >   			    "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n",
> >   			    enabledisable(intel_dp->dfp.ycbcr_444_to_420));
> >   
> > +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
> >   	tmp = intel_dp->dfp.rgb_to_ycbcr ?
> >   		DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
> >   
> > @@ -4261,6 +4263,14 @@ intel_dp_update_420(struct intel_dp *intel_dp)
> >   	rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
> >   								 intel_dp->downstream_ports,
> >   								 DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
> > +	/*
> > +	 * FIXME need to actually track whether we're really
> > +	 * going to be doing the RGB->YCbCr connversion or not.
> > +	 * We can't tell by simply looking at intel_dp->dfp.rgb_to_ycbcr.
> > +	 * Readout is going to annoying due to having to read that
> > +	 * state from external hardware that may vanish at any time :(
> > +	 */
> 
> 
> Hmm right. Do you have any suggestion what should be the better place 
> for defining the policy for using DFPs CSC or use our own HW, in case of 
> YCbCr 420 output?

One idea that came to mind was just adding some kind of
sink_format thing into the crtc_state, and computing that appropriately
during .compute_config(). But as mentioned the readout part is going to
be annoying. Maybe we just won't have readout for it.
Nautiyal, Ankit K Dec. 10, 2021, 6:04 a.m. UTC | #3 
On 10/27/2021 2:24 PM, Ville Syrjälä wrote:
> On Wed, Oct 27, 2021 at 12:57:37PM +0530, Nautiyal, Ankit K wrote:
>> On 10/15/2021 7:09 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> We lack sufficient state tracking to figure out whether
>>> we want the DFP to perform the RGB->YCbCr conversion for us
>>> or not. So currently we are blindly just enabling that all the
>>> time when supported by the DFP. That is nonsense. So until
>>> we imporve our state tracking for this just disable the feature.
>>>
>>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 29b12456c461..3e2a29b589a9 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -1107,6 +1107,7 @@ static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>>>    static bool intel_dp_is_ycbcr420(struct intel_dp *intel_dp,
>>>    				 const struct intel_crtc_state *crtc_state)
>>>    {
>>> +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
>>>    	return crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>>>    		(crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
>>>    		 intel_dp->dfp.ycbcr_444_to_420);
>>> @@ -2456,6 +2457,7 @@ void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
>>>    			    "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n",
>>>    			    enabledisable(intel_dp->dfp.ycbcr_444_to_420));
>>>    
>>> +	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
>>>    	tmp = intel_dp->dfp.rgb_to_ycbcr ?
>>>    		DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
>>>    
>>> @@ -4261,6 +4263,14 @@ intel_dp_update_420(struct intel_dp *intel_dp)
>>>    	rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
>>>    								 intel_dp->downstream_ports,
>>>    								 DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
>>> +	/*
>>> +	 * FIXME need to actually track whether we're really
>>> +	 * going to be doing the RGB->YCbCr connversion or not.
>>> +	 * We can't tell by simply looking at intel_dp->dfp.rgb_to_ycbcr.
>>> +	 * Readout is going to annoying due to having to read that
>>> +	 * state from external hardware that may vanish at any time :(
>>> +	 */
>>
>> Hmm right. Do you have any suggestion what should be the better place
>> for defining the policy for using DFPs CSC or use our own HW, in case of
>> YCbCr 420 output?
> One idea that came to mind was just adding some kind of
> sink_format thing into the crtc_state, and computing that appropriately
> during .compute_config(). But as mentioned the readout part is going to
> be annoying. Maybe we just won't have readout for it.

Agreed.

As mentioned in the FIX ME, lets have dfp->rgb_to_ycbcr only to capture 
that the dfp supports conversion.

and dfp->ycbcr_444_420 to capture that it supports 444_420.

Whether we need to program the DPCD to avail these support can be 
captured in another member to crtc_state->dp_dfp_444_420, and 
crtc_state->dp_dfp_rgb_ycbcr based on crtc_state->sink_format.

We can fill these members, during compute_config along with dsc 
considerations.

These can then be used in dp_configure_protocol_converter().

Regards,

Ankit
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 29b12456c461..3e2a29b589a9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1107,6 +1107,7 @@  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
 static bool intel_dp_is_ycbcr420(struct intel_dp *intel_dp,
 				 const struct intel_crtc_state *crtc_state)
 {
+	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
 	return crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
 		(crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444 &&
 		 intel_dp->dfp.ycbcr_444_to_420);
@@ -2456,6 +2457,7 @@  void intel_dp_configure_protocol_converter(struct intel_dp *intel_dp,
 			    "Failed to %s protocol converter YCbCr 4:2:0 conversion mode\n",
 			    enabledisable(intel_dp->dfp.ycbcr_444_to_420));
 
+	/* FIXME see intel_dp_update_420() regarding rgb_to_ycbcr */
 	tmp = intel_dp->dfp.rgb_to_ycbcr ?
 		DP_CONVERSION_BT709_RGB_YCBCR_ENABLE : 0;
 
@@ -4261,6 +4263,14 @@  intel_dp_update_420(struct intel_dp *intel_dp)
 	rgb_to_ycbcr = drm_dp_downstream_rgb_to_ycbcr_conversion(intel_dp->dpcd,
 								 intel_dp->downstream_ports,
 								 DP_DS_HDMI_BT709_RGB_YCBCR_CONV);
+	/*
+	 * FIXME need to actually track whether we're really
+	 * going to be doing the RGB->YCbCr connversion or not.
+	 * We can't tell by simply looking at intel_dp->dfp.rgb_to_ycbcr.
+	 * Readout is going to annoying due to having to read that
+	 * state from external hardware that may vanish at any time :(
+	 */
+	rgb_to_ycbcr = false;
 
 	if (DISPLAY_VER(i915) >= 11) {
 		/* Let PCON convert from RGB->YCbCr if possible */