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

[Daniel Vetter]

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.

> +{
> +	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.

> +
> +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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch