[Intel-gfx] [PATCH 1/2] drm/dp: start a DPCD based DP sink/branch device quirk database

[Jani Nikula]

On Thu, 11 May 2017, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Thu, May 11, 2017 at 12:57:20PM +0300, 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 OUI (with the option of
>> expanding to device identification string and hardware/firmware
>> revisions later). 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 M and N attributes properly. Naturally, the workaround of reducing
>> main link attributes for all devices ended up in regressions for other
>> devices. So here we are.
>> 
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>> Cc: Clint Taylor <clinton.a.taylor at intel.com>
>> Cc: Adam Jackson <ajax at redhat.com>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>
> A few small bikesheds below. Ack in principle.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_dp_helper.c | 61 +++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h     |  7 +++++
>>  2 files changed, 68 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 3e5f52110ea1..58b2ced33151 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -1208,3 +1208,64 @@ int drm_dp_stop_crc(struct drm_dp_aux *aux)
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_dp_stop_crc);
>> +
>> +/*
>> + * 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.
>> + */
>
> I'd stash this in the kerneldoc for drm_dp_get_quirks().
>
>> +
>> +/*
>> + * FIXME: Add support for device identification and hardware/firmware revision.
>> + */
>> +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, DP_DPCD_QUIRK_LIMITED_M_N },
>> +};
>> +
>> +#undef OUI
>> +
>> +/**
>> + * drm_dp_get_quirks() - get quirks for the sink/branch device
>> + * @oui: pointer to len bytes of DPCD at 0x400 (sink) or 0x500 (branch)
>> + * @len: 3-12 bytes of DPCD data
>> + * @is_branch: true for branch devices, false for sink devices
>> + *
>> + * Get a bit mask of DPCD quirks for the sink/branch device. 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.
>> + */
>> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch)
>
> Should we either base this on drm_dp_aux (so that this function would
> query for quirks), or on struct drm_dp_link (which is kinda something
> tegra and msm started to use to cache some sink properties)?
>
> This is with an eye towards handling your FIXME, and maybe even converging
> the dp helpers a bit more again.
>
> Personally (aka my own bikeshed right now) I'm leaning towards struct
> drm_dp_link and extending that.

Except we don't use drm_dp_link in i915, and I don't see starting to use
that would help us in any way. What we do with the source/sink/link
properties is much more complicated.

As to drm_dp_aux, I think that's the wrong level of abstraction, and I'd
much prefer the drivers would be in control of the DPCD access.

I guess we could move struct intel_dp_desc and intel_dp_read_desc() to
drm helpers, and store the quirks in struct intel_dp_desc upon reading.

>
>> +{
>> +	const struct dpcd_quirk *quirk;
>> +	u32 quirks = 0;
>> +	int i;
>> +
>> +	if (WARN_ON_ONCE(len < 3))
>> +		return 0;
>> +
>> +	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, oui, 3) != 0)
>> +			continue;
>> +
>> +		quirks |= quirk->quirks;
>> +	}
>> +
>> +	return quirks;
>> +}
>> +EXPORT_SYMBOL(drm_dp_get_quirks);
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index f7007e544f29..8abe9897fe59 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -1079,4 +1079,11 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>>  int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc);
>>  int drm_dp_stop_crc(struct drm_dp_aux *aux);
>>  
>> +/* Display Port sink and branch device quirks. */
>> +
>> +/* The sink is limited to small link M and N values. */
>> +#define DP_DPCD_QUIRK_LIMITED_M_N			BIT(0)
>
> Bikeshed: I much prefer enums (with bitfields) than defines for internal
> stuff, allows them to be nicely kernel-doc documented and linked. Which I
> think would be good to do here, to spec out very precisely what exactly
> the hack is that needs to be applied. LIMITED_M_N doesn't tell you
> anything without looking at the i915 code.

Personally I don't think enums should be used for defining bits, because
they are not enumerations. The bits are usually combined to come up with
values that are not part of the enum.

If we added the quirks to struct intel_dp_desc (and moved to drm
helpers) we could use enums for defining the bit shifts and add a
function to query for a specific quirk, say, drm_dp_has_quirk(struct
drm_dp_desc *desc, enum drm_dpcd_quirk quirk).


BR,
Jani.

>
>> +
>> +u32 drm_dp_get_quirks(const u8 *oui, unsigned int len, bool is_branch);
>> +
>>  #endif /* _DRM_DP_HELPER_H_ */
>> -- 
>> 2.11.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center