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

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

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;