Re: [PATCH 1/2] drm/i915/display: Add mechanism to use sink model when applying quirk

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 20 Aug 2024, Jouni Högander <jouni.hogander@intel.com> wrote:
> Currently there is no way to apply quirk device only if certain panel
> model is installed. This patch implements such mechanism by adding
> sink_oui and sink_device_id field into intel_quirk and using them to
> figure out if applying quirk is needed.
>
> For all existing quirks sink_oui and sink_device_id are set as
> SINK_[OUI DEVICE_ID]_ANY to indicate quirk is not specific to any
> sink model.
>
> Existing intel_init_quirks is modified to ignore quirk if it has
> sink_oui set to something else than SINK_OUI_ANY.
>
> New intel_init_dpcd_quirks is added and called after drm_dp_read_desc
> with proper sink device identity read from dpcdc.
>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     |  5 ++
>  drivers/gpu/drm/i915/display/intel_quirks.c | 90 +++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_quirks.h |  3 +
>  3 files changed, 74 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6a0c7ae654f40..9d8bd41dacfe0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -84,6 +84,7 @@
>  #include "intel_pch_display.h"
>  #include "intel_pps.h"
>  #include "intel_psr.h"
> +#include "intel_quirks.h"
>  #include "intel_tc.h"
>  #include "intel_vdsc.h"
>  #include "intel_vrr.h"
> @@ -4053,6 +4054,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp, struct intel_connector *connector
>  
>  	drm_dp_read_desc(&intel_dp->aux, &intel_dp->desc,
>  			 drm_dp_is_branch(intel_dp->dpcd));
> +	intel_init_dpcd_quirks(&dev_priv->display, &intel_dp->desc.ident);

Please don't use &dev_priv->display inline in code. Instead, add a local
variable as the first thing:

	struct intel_display *display = to_intel_display(intel_dp);

>  
>  	/*
>  	 * Read the eDP display control registers.
> @@ -4152,6 +4154,7 @@ void intel_dp_update_sink_caps(struct intel_dp *intel_dp)
>  static bool
>  intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  {
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);

Ditto.

>  	int ret;
>  
>  	if (intel_dp_init_lttpr_and_dprx_caps(intel_dp) < 0)
> @@ -4165,6 +4168,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  		drm_dp_read_desc(&intel_dp->aux, &intel_dp->desc,
>  				 drm_dp_is_branch(intel_dp->dpcd));
>  
> +		intel_init_dpcd_quirks(&i915->display, &intel_dp->desc.ident);
> +
>  		intel_dp_update_sink_caps(intel_dp);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.c b/drivers/gpu/drm/i915/display/intel_quirks.c
> index 14d5fefc9c5b2..d9045b317cd16 100644
> --- a/drivers/gpu/drm/i915/display/intel_quirks.c
> +++ b/drivers/gpu/drm/i915/display/intel_quirks.c
> @@ -69,9 +69,18 @@ struct intel_quirk {
>  	int device;
>  	int subsystem_vendor;
>  	int subsystem_device;
> +	u8 sink_oui[3];
> +	u8 sink_device_id[6];
>  	void (*hook)(struct intel_display *display);
>  };
>  
> +#define SINK_OUI(first, second, third) { (first), (second), (third) }
> +#define SINK_DEVICE_ID(first, second, third, fourth, fifth, sixth) \
> +	{ (first), (second), (third), (fourth), (fifth), (sixth) }
> +
> +#define SINK_OUI_ANY		SINK_OUI(0, 0, 0)
> +#define SINK_DEVICE_ID_ANY	SINK_DEVICE_ID(0, 0, 0, 0, 0, 0)
> +
>  /* For systems that don't have a meaningful PCI subdevice/subvendor ID */
>  struct intel_dmi_quirk {
>  	void (*hook)(struct intel_display *display);
> @@ -140,77 +149,82 @@ static const struct intel_dmi_quirk intel_dmi_quirks[] = {
>  
>  static struct intel_quirk intel_quirks[] = {
>  	/* Lenovo U160 cannot use SSC on LVDS */
> -	{ 0x0046, 0x17aa, 0x3920, quirk_ssc_force_disable },
> +	{ 0x0046, 0x17aa, 0x3920, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_ssc_force_disable },
>  
>  	/* Sony Vaio Y cannot use SSC on LVDS */
> -	{ 0x0046, 0x104d, 0x9076, quirk_ssc_force_disable },
> +	{ 0x0046, 0x104d, 0x9076, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_ssc_force_disable },
>  
>  	/* Acer Aspire 5734Z must invert backlight brightness */
> -	{ 0x2a42, 0x1025, 0x0459, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x0459, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer/eMachines G725 */
> -	{ 0x2a42, 0x1025, 0x0210, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x0210, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer/eMachines e725 */
> -	{ 0x2a42, 0x1025, 0x0212, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x0212, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer/Packard Bell NCL20 */
> -	{ 0x2a42, 0x1025, 0x034b, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x034b, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer Aspire 4736Z */
> -	{ 0x2a42, 0x1025, 0x0260, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x0260, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer Aspire 5336 */
> -	{ 0x2a42, 0x1025, 0x048a, quirk_invert_brightness },
> +	{ 0x2a42, 0x1025, 0x048a, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  
>  	/* Acer C720 and C720P Chromebooks (Celeron 2955U) have backlights */
> -	{ 0x0a06, 0x1025, 0x0a11, quirk_backlight_present },
> +	{ 0x0a06, 0x1025, 0x0a11, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Acer C720 Chromebook (Core i3 4005U) */
> -	{ 0x0a16, 0x1025, 0x0a11, quirk_backlight_present },
> +	{ 0x0a16, 0x1025, 0x0a11, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Apple Macbook 2,1 (Core 2 T7400) */
> -	{ 0x27a2, 0x8086, 0x7270, quirk_backlight_present },
> +	{ 0x27a2, 0x8086, 0x7270, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Apple Macbook 4,1 */
> -	{ 0x2a02, 0x106b, 0x00a1, quirk_backlight_present },
> +	{ 0x2a02, 0x106b, 0x00a1, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Toshiba CB35 Chromebook (Celeron 2955U) */
> -	{ 0x0a06, 0x1179, 0x0a88, quirk_backlight_present },
> +	{ 0x0a06, 0x1179, 0x0a88, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* HP Chromebook 14 (Celeron 2955U) */
> -	{ 0x0a06, 0x103c, 0x21ed, quirk_backlight_present },
> +	{ 0x0a06, 0x103c, 0x21ed, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Dell Chromebook 11 */
> -	{ 0x0a06, 0x1028, 0x0a35, quirk_backlight_present },
> +	{ 0x0a06, 0x1028, 0x0a35, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Dell Chromebook 11 (2015 version) */
> -	{ 0x0a16, 0x1028, 0x0a35, quirk_backlight_present },
> +	{ 0x0a16, 0x1028, 0x0a35, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_backlight_present },
>  
>  	/* Toshiba Satellite P50-C-18C */
> -	{ 0x191B, 0x1179, 0xF840, quirk_increase_t12_delay },
> +	{ 0x191B, 0x1179, 0xF840, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_t12_delay },
>  
>  	/* GeminiLake NUC */
> -	{ 0x3185, 0x8086, 0x2072, quirk_increase_ddi_disabled_time },
> -	{ 0x3184, 0x8086, 0x2072, quirk_increase_ddi_disabled_time },
> +	{ 0x3185, 0x8086, 0x2072, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
> +	{ 0x3184, 0x8086, 0x2072, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
>  	/* ASRock ITX*/
> -	{ 0x3185, 0x1849, 0x2212, quirk_increase_ddi_disabled_time },
> -	{ 0x3184, 0x1849, 0x2212, quirk_increase_ddi_disabled_time },
> +	{ 0x3185, 0x1849, 0x2212, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
> +	{ 0x3184, 0x1849, 0x2212, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
>  	/* ECS Liva Q2 */
> -	{ 0x3185, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time },
> -	{ 0x3184, 0x1019, 0xa94d, quirk_increase_ddi_disabled_time },
> +	{ 0x3185, 0x1019, 0xa94d, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
> +	{ 0x3184, 0x1019, 0xa94d, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_increase_ddi_disabled_time },
>  	/* HP Notebook - 14-r206nv */
> -	{ 0x0f31, 0x103c, 0x220f, quirk_invert_brightness },
> +	{ 0x0f31, 0x103c, 0x220f, SINK_OUI_ANY, SINK_DEVICE_ID_ANY, quirk_invert_brightness },
>  };

Mmh. Since SINK_OUI_ANY and SINK_DEVICE_ID_ANY are both all zeros, it
would be nice to not have to initialize them.

One approach is to first convert the whole thing to designated
initializers as a separate patch. Maybe the device, subsystem_vendor and
subsystem_device could be initialized with a macro that contains the
designated initializer to not be too verbose.

>  
>  void intel_init_quirks(struct intel_display *display)
>  {
>  	struct pci_dev *d = to_pci_dev(display->drm->dev);
> +	u8 any_sink_oui[] = SINK_OUI_ANY;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(intel_quirks); i++) {
>  		struct intel_quirk *q = &intel_quirks[i];
>  
> +		if (memcmp(q->sink_oui, any_sink_oui,
> +			   sizeof(any_sink_oui)) != 0)
> +			continue;
> +
>  		if (d->device == q->device &&
>  		    (d->subsystem_vendor == q->subsystem_vendor ||
>  		     q->subsystem_vendor == PCI_ANY_ID) &&
> @@ -224,6 +238,34 @@ void intel_init_quirks(struct intel_display *display)
>  	}
>  }
>  
> +void intel_init_dpcd_quirks(struct intel_display *display,
> +			    struct drm_dp_dpcd_ident *ident)

Can be const.

> +{
> +	struct pci_dev *d = to_pci_dev(display->drm->dev);
> +	u8 any_sink_oui[] = SINK_OUI_ANY;
> +	u8 any_sink_device_id[] = SINK_DEVICE_ID_ANY;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(intel_quirks); i++) {
> +		struct intel_quirk *q = &intel_quirks[i];
> +
> +		if (!memcmp(q->sink_oui, any_sink_oui, sizeof(any_sink_oui)))

Maybe we could just check if it's zero?

> +			continue;
> +
> +		if (d->device == q->device &&
> +		    (d->subsystem_vendor == q->subsystem_vendor ||
> +		     q->subsystem_vendor == PCI_ANY_ID) &&
> +		    (d->subsystem_device == q->subsystem_device ||
> +		     q->subsystem_device == PCI_ANY_ID) &&
> +		    !memcmp(q->sink_oui, ident->oui, sizeof(ident->oui)) &&
> +		    (!memcmp(q->sink_device_id, ident->device_id,
> +			    sizeof(ident->device_id)) ||
> +		     !memcmp(q->sink_device_id, any_sink_device_id,
> +			    sizeof(any_sink_device_id))))

Ditto.

> +			q->hook(display);

It's a bit odd that a display specific quirk affects
everything. Shouldn't the quirk be applied only for connectors that have
the affected display?

> +	}
> +}
> +
>  bool intel_has_quirk(struct intel_display *display, enum intel_quirk_id quirk)
>  {
>  	return display->quirks.mask & BIT(quirk);
> diff --git a/drivers/gpu/drm/i915/display/intel_quirks.h b/drivers/gpu/drm/i915/display/intel_quirks.h
> index 151c8f4ae5760..2d664af7e89f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_quirks.h
> +++ b/drivers/gpu/drm/i915/display/intel_quirks.h
> @@ -9,6 +9,7 @@
>  #include <linux/types.h>
>  
>  struct intel_display;
> +struct drm_dp_dpcd_ident;
>  
>  enum intel_quirk_id {
>  	QUIRK_BACKLIGHT_PRESENT,
> @@ -20,6 +21,8 @@ enum intel_quirk_id {
>  };
>  
>  void intel_init_quirks(struct intel_display *display);
> +void intel_init_dpcd_quirks(struct intel_display *display,
> +			    struct drm_dp_dpcd_ident *ident);
>  bool intel_has_quirk(struct intel_display *display, enum intel_quirk_id quirk);
>  
>  #endif /* __INTEL_QUIRKS_H__ */

-- 
Jani Nikula, Intel