Re: [PATCH 3/4] drm/dp: start a DPCD based DP sink/branch device quirk database

From: Clint Taylor <clinton.a.taylor@intel.com>
To: Jani Nikula <jani.nikula@intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: harry.wentland@amd.com, ajax@redhat.com, dhinakaran.pandiyan@intel.com
Subject: Re: [PATCH 3/4] drm/dp: start a DPCD based DP sink/branch device quirk database
Date: Tue, 30 May 2017 08:24:32 -0700	[thread overview]
Message-ID: <6ce91657-4f4f-ad44-0e55-44adda6291ba@intel.com> (raw)
In-Reply-To: <87fufnly6n.fsf@intel.com>


On 05/29/2017 04:06 AM, Jani Nikula wrote:
> On Thu, 18 May 2017, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>> On 05/18/2017 04:10 AM, Jani Nikula wrote:
>>> Face the fact, there are Display Port sink and branch devices out there
>>> in the wild that don't follow the Display Port specifications, or they
>>> have bugs, or just otherwise require special treatment. Start a common
>>> quirk database the drivers can query based on the DP device
>>> identification. At least for now, we leave the workarounds for the
>>> drivers to implement as they see fit.
>>>
>>> For starters, add a branch device that can't handle full 24-bit main
>>> link Mdiv and Ndiv main link attributes properly. Naturally, the
>>> workaround of reducing main link attributes for all devices ended up in
>>> regressions for other devices. So here we are.
>>>
>>> v2: Rebase on DRM DP desc read helpers
>>>
>>> v3: Fix the OUI memcmp blunder (Clint)
>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> I pushed the series to drm-intel topic/dp-quirks branch based on
> v4.12-rc3, with the goal of merging this to v4.12. Thanks for the review
> and testing so far; would you mind giving that branch a go too, to
> ensure I didn't screw anything up while applying?
Topic branch appears to work as expected. Quirk is applied only to OUI 
0x0022B9 and the n and m values are reduced.

Topic branch is:
Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

-Clint


> BR,
> Jani.
>
>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>> Cc: Adam Jackson <ajax@redhat.com>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++++++++++++++++++--
>>>    include/drm/drm_dp_helper.h     | 32 +++++++++++++++++++++++++
>>>    2 files changed, 82 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>>> index 52e0ca9a5bb1..213fb837e1c4 100644
>>> --- a/drivers/gpu/drm/drm_dp_helper.c
>>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>>> @@ -1209,6 +1209,51 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>>>    }
>>>    EXPORT_SYMBOL(drm_dp_stop_crc);
>>>    
>>> +struct dpcd_quirk {
>>> +	u8 oui[3];
>>> +	bool is_branch;
>>> +	u32 quirks;
>>> +};
>>> +
>>> +#define OUI(first, second, third) { (first), (second), (third) }
>>> +
>>> +static const struct dpcd_quirk dpcd_quirk_list[] = {
>>> +	/* Analogix 7737 needs reduced M and N at HBR2 link rates */
>>> +	{ OUI(0x00, 0x22, 0xb9), true, BIT(DP_DPCD_QUIRK_LIMITED_M_N) },
>>> +};
>>> +
>>> +#undef OUI
>>> +
>>> +/*
>>> + * Get a bit mask of DPCD quirks for the sink/branch device identified by
>>> + * ident. The quirk data is shared but it's up to the drivers to act on the
>>> + * data.
>>> + *
>>> + * For now, only the OUI (first three bytes) is used, but this may be extended
>>> + * to device identification string and hardware/firmware revisions later.
>>> + */
>>> +static u32
>>> +drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch)
>>> +{
>>> +	const struct dpcd_quirk *quirk;
>>> +	u32 quirks = 0;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(dpcd_quirk_list); i++) {
>>> +		quirk = &dpcd_quirk_list[i];
>>> +
>>> +		if (quirk->is_branch != is_branch)
>>> +			continue;
>>> +
>>> +		if (memcmp(quirk->oui, ident->oui, sizeof(ident->oui)) != 0)
>>> +			continue;
>>> +
>>> +		quirks |= quirk->quirks;
>>> +	}
>>> +
>>> +	return quirks;
>>> +}
>>> +
>>>    /**
>>>     * drm_dp_read_desc - read sink/branch descriptor from DPCD
>>>     * @aux: DisplayPort AUX channel
>>> @@ -1231,14 +1276,17 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
>>>    	if (ret < 0)
>>>    		return ret;
>>>    
>>> +	desc->quirks = drm_dp_get_quirks(ident, is_branch);
>>> +
>>>    	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
>>>    
>>> -	DRM_DEBUG_KMS("DP %s: OUI %*phD dev-ID %*pE HW-rev %d.%d SW-rev %d.%d\n",
>>> +	DRM_DEBUG_KMS("DP %s: OUI %*phD dev-ID %*pE HW-rev %d.%d SW-rev %d.%d quirks 0x%04x\n",
>>>    		      is_branch ? "branch" : "sink",
>>>    		      (int)sizeof(ident->oui), ident->oui,
>>>    		      dev_id_len, ident->device_id,
>>>    		      ident->hw_rev >> 4, ident->hw_rev & 0xf,
>>> -		      ident->sw_major_rev, ident->sw_minor_rev);
>>> +		      ident->sw_major_rev, ident->sw_minor_rev,
>>> +		      desc->quirks);
>>>    
>>>    	return 0;
>>>    }
>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>>> index aee5b96b51d7..717cb8496725 100644
>>> --- a/include/drm/drm_dp_helper.h
>>> +++ b/include/drm/drm_dp_helper.h
>>> @@ -1090,12 +1090,44 @@ struct drm_dp_dpcd_ident {
>>>    /**
>>>     * struct drm_dp_desc - DP branch/sink device descriptor
>>>     * @ident: DP device identification from DPCD 0x400 (sink) or 0x500 (branch).
>>> + * @quirks: Quirks; use drm_dp_has_quirk() to query for the quirks.
>>>     */
>>>    struct drm_dp_desc {
>>>    	struct drm_dp_dpcd_ident ident;
>>> +	u32 quirks;
>>>    };
>>>    
>>>    int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
>>>    		     bool is_branch);
>>>    
>>> +/**
>>> + * enum drm_dp_quirk - Display Port sink/branch device specific quirks
>>> + *
>>> + * Display Port sink and branch devices in the wild have a variety of bugs, try
>>> + * to collect them here. The quirks are shared, but it's up to the drivers to
>>> + * implement workarounds for them.
>>> + */
>>> +enum drm_dp_quirk {
>>> +	/**
>>> +	 * @DP_DPCD_QUIRK_LIMITED_M_N:
>>> +	 *
>>> +	 * The device requires main link attributes Mdiv and Ndiv to be limited
>>> +	 * to 16 bits.
>>> +	 */
>>> +	DP_DPCD_QUIRK_LIMITED_M_N,
>>> +};
>>> +
>>> +/**
>>> + * drm_dp_has_quirk() - does the DP device have a specific quirk
>>> + * @desc: Device decriptor filled by drm_dp_read_desc()
>>> + * @quirk: Quirk to query for
>>> + *
>>> + * Return true if DP device identified by @desc has @quirk.
>>> + */
>>> +static inline bool
>>> +drm_dp_has_quirk(const struct drm_dp_desc *desc, enum drm_dp_quirk quirk)
>>> +{
>>> +	return desc->quirks & BIT(quirk);
>>> +}
>>> +
>>>    #endif /* _DRM_DP_HELPER_H_ */

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx