diff mbox series

[10/11] drm/i915/dp_mst: Make HBLANK expansion quirk work for logical ports

Message ID 20240320201152.3487892-11-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/dp: Few MTL/DSC and a UHBR monitor fix | expand

Commit Message

Imre Deak March 20, 2024, 8:11 p.m. UTC 
The DPCD OUI of the logical port on a Dell UHBR monitor - on which the
AUX device is used to enable DSC - is all 0. To detect if the HBLANK
expansion quirk is required for this monitor use the OUI of the port's
parent instead.

Since in the above case the DPCD of both the logical port and the parent
port reports being a sink device (vs. branch device) type, read the
proper sink/branch OUI based on the DPCD device type.

This is required by a follow-up patch enabling the quirk for the above
Dell monitor.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Nautiyal, Ankit K March 27, 2024, 8:10 a.m. UTC | #1 
On 3/21/2024 1:41 AM, Imre Deak wrote:
> The DPCD OUI of the logical port on a Dell UHBR monitor - on which the
> AUX device is used to enable DSC - is all 0. To detect if the HBLANK
> expansion quirk is required for this monitor use the OUI of the port's
> parent instead.
>
> Since in the above case the DPCD of both the logical port and the parent
> port reports being a sink device (vs. branch device) type, read the
> proper sink/branch OUI based on the DPCD device type.
>
> This is required by a follow-up patch enabling the quirk for the above
> Dell monitor.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 516b00f791420..76a8fb21b8e52 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1512,23 +1512,33 @@ intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
>   static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *connector)
>   {
>   	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +	struct drm_dp_aux *aux = connector->dp.dsc_decompression_aux;
>   	struct drm_dp_desc desc;
>   	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>   
> -	if (!connector->dp.dsc_decompression_aux)
> +	if (!aux)
>   		return false;
>   
> -	if (drm_dp_read_desc(connector->dp.dsc_decompression_aux,
> -			     &desc, true) < 0)
> +	/*
> +	 * A logical port's OUI (at least for affected sinks) is all 0, so
> +	 * instead of that the parent port's OUI is used for identification.
> +	 */
> +	if (drm_dp_mst_port_is_logical(connector->port)) {
> +		aux = drm_dp_mst_aux_for_parent(connector->port);
> +		if (!aux)
> +			aux = &connector->mst_port->aux;


As I understand, we are setting connector->mst_port as intel_dp, in the 
caller intel_dp_add_mst_connector so its unlikely that aux can be NULL, 
but do you see if we need to check for aux?

Regards,

Ankit

> +	}
> +
> +	if (drm_dp_read_dpcd_caps(aux, dpcd) < 0)
> +		return false;
> +
> +	if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(dpcd)) < 0)
>   		return false;
>   
>   	if (!drm_dp_has_quirk(&desc,
>   			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
>   		return false;
>   
> -	if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd) < 0)
> -		return false;
> -
>   	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
>   		return false;
>
Imre Deak March 27, 2024, 2:19 p.m. UTC | #2 
On Wed, Mar 27, 2024 at 01:40:58PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/21/2024 1:41 AM, Imre Deak wrote:
> > The DPCD OUI of the logical port on a Dell UHBR monitor - on which the
> > AUX device is used to enable DSC - is all 0. To detect if the HBLANK
> > expansion quirk is required for this monitor use the OUI of the port's
> > parent instead.
> > 
> > Since in the above case the DPCD of both the logical port and the parent
> > port reports being a sink device (vs. branch device) type, read the
> > proper sink/branch OUI based on the DPCD device type.
> > 
> > This is required by a follow-up patch enabling the quirk for the above
> > Dell monitor.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++------
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 516b00f791420..76a8fb21b8e52 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -1512,23 +1512,33 @@ intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
> >   static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *connector)
> >   {
> >   	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > +	struct drm_dp_aux *aux = connector->dp.dsc_decompression_aux;
> >   	struct drm_dp_desc desc;
> >   	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> > -	if (!connector->dp.dsc_decompression_aux)
> > +	if (!aux)
> >   		return false;
> > -	if (drm_dp_read_desc(connector->dp.dsc_decompression_aux,
> > -			     &desc, true) < 0)
> > +	/*
> > +	 * A logical port's OUI (at least for affected sinks) is all 0, so
> > +	 * instead of that the parent port's OUI is used for identification.
> > +	 */
> > +	if (drm_dp_mst_port_is_logical(connector->port)) {
> > +		aux = drm_dp_mst_aux_for_parent(connector->port);
> > +		if (!aux)
> > +			aux = &connector->mst_port->aux;
> 
> As I understand, we are setting connector->mst_port as intel_dp, in the
> caller intel_dp_add_mst_connector so its unlikely that aux can be NULL, but
> do you see if we need to check for aux?

Yes, intel_connector::mst_port (always) points to the intel_dp of the
MST root port, and aux will be always initialized for all the registered
DP encoders/intel_dps; so mst_port->aux will always point to a
valid/non-NULL AUX device. (In any case above we take the address of
intel_dp::aux, which can't be NULL.)

> Regards,
> 
> Ankit
> 
> > +	}
> > +
> > +	if (drm_dp_read_dpcd_caps(aux, dpcd) < 0)
> > +		return false;
> > +
> > +	if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(dpcd)) < 0)
> >   		return false;
> >   	if (!drm_dp_has_quirk(&desc,
> >   			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
> >   		return false;
> > -	if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd) < 0)
> > -		return false;
> > -
> >   	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> >   		return false;
Nautiyal, Ankit K March 28, 2024, 3:30 a.m. UTC | #3 
On 3/27/2024 7:49 PM, Imre Deak wrote:
> On Wed, Mar 27, 2024 at 01:40:58PM +0530, Nautiyal, Ankit K wrote:
>> On 3/21/2024 1:41 AM, Imre Deak wrote:
>>> The DPCD OUI of the logical port on a Dell UHBR monitor - on which the
>>> AUX device is used to enable DSC - is all 0. To detect if the HBLANK
>>> expansion quirk is required for this monitor use the OUI of the port's
>>> parent instead.
>>>
>>> Since in the above case the DPCD of both the logical port and the parent
>>> port reports being a sink device (vs. branch device) type, read the
>>> proper sink/branch OUI based on the DPCD device type.
>>>
>>> This is required by a follow-up patch enabling the quirk for the above
>>> Dell monitor.
>>>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++------
>>>    1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> index 516b00f791420..76a8fb21b8e52 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
>>> @@ -1512,23 +1512,33 @@ intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
>>>    static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *connector)
>>>    {
>>>    	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>> +	struct drm_dp_aux *aux = connector->dp.dsc_decompression_aux;
>>>    	struct drm_dp_desc desc;
>>>    	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>> -	if (!connector->dp.dsc_decompression_aux)
>>> +	if (!aux)
>>>    		return false;
>>> -	if (drm_dp_read_desc(connector->dp.dsc_decompression_aux,
>>> -			     &desc, true) < 0)
>>> +	/*
>>> +	 * A logical port's OUI (at least for affected sinks) is all 0, so
>>> +	 * instead of that the parent port's OUI is used for identification.
>>> +	 */
>>> +	if (drm_dp_mst_port_is_logical(connector->port)) {
>>> +		aux = drm_dp_mst_aux_for_parent(connector->port);
>>> +		if (!aux)
>>> +			aux = &connector->mst_port->aux;
>> As I understand, we are setting connector->mst_port as intel_dp, in the
>> caller intel_dp_add_mst_connector so its unlikely that aux can be NULL, but
>> do you see if we need to check for aux?
> Yes, intel_connector::mst_port (always) points to the intel_dp of the
> MST root port, and aux will be always initialized for all the registered
> DP encoders/intel_dps; so mst_port->aux will always point to a
> valid/non-NULL AUX device. (In any case above we take the address of
> intel_dp::aux, which can't be NULL.)

Agreed. The change LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


>
>> Regards,
>>
>> Ankit
>>
>>> +	}
>>> +
>>> +	if (drm_dp_read_dpcd_caps(aux, dpcd) < 0)
>>> +		return false;
>>> +
>>> +	if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(dpcd)) < 0)
>>>    		return false;
>>>    	if (!drm_dp_has_quirk(&desc,
>>>    			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
>>>    		return false;
>>> -	if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd) < 0)
>>> -		return false;
>>> -
>>>    	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
>>>    		return false;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 516b00f791420..76a8fb21b8e52 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -1512,23 +1512,33 @@  intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
 static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *connector)
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
+	struct drm_dp_aux *aux = connector->dp.dsc_decompression_aux;
 	struct drm_dp_desc desc;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 
-	if (!connector->dp.dsc_decompression_aux)
+	if (!aux)
 		return false;
 
-	if (drm_dp_read_desc(connector->dp.dsc_decompression_aux,
-			     &desc, true) < 0)
+	/*
+	 * A logical port's OUI (at least for affected sinks) is all 0, so
+	 * instead of that the parent port's OUI is used for identification.
+	 */
+	if (drm_dp_mst_port_is_logical(connector->port)) {
+		aux = drm_dp_mst_aux_for_parent(connector->port);
+		if (!aux)
+			aux = &connector->mst_port->aux;
+	}
+
+	if (drm_dp_read_dpcd_caps(aux, dpcd) < 0)
+		return false;
+
+	if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(dpcd)) < 0)
 		return false;
 
 	if (!drm_dp_has_quirk(&desc,
 			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
 		return false;
 
-	if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd) < 0)
-		return false;
-
 	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
 		return false;