Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Adam Jackson]

On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
> This panel (manufacturer is SDC, product ID is 0x4141)
> used manufacturer defined DPCD register to control brightness
> that not defined in eDP spec so far. This change follow panel
> vendor's instruction to support brightness adjustment.

I'm sure this works, but this smells a little funny to me.

> +	/* Samsung eDP panel */
> +	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },

It feels a bit like a layering violation to identify eDP behavior
changes based on EDID. But I'm not sure there's any obvious way to
identify this device by its DPCD. The Sink OUI (from the linked
bugzilla) seems to be 0011F8, which doesn't match up to anything in my
oui.txt...

> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid *edid)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(edid_get_quirks);

If we're going to export this it should probably get a drm_ prefix.

> +#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
> +#define DPCD_EDP_CONTENT_LUMINANCE		0x346
> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
> +#define DPCD_EDP_BRIGHTNESS_NITS		0x354
> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
> +
> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)

This also seems a bit weird, the 0x300-0x3FF registers belong to the
_source_ DP device. But then later...

> +	/* write source OUI */
> +	write_val[0] = 0x00;
> +	write_val[1] = 0xaa;
> +	write_val[2] = 0x01;

Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it
makes sense that you're writing to registers whose behavior the source
defines. But this does raise the question: is this just a convention
between Intel and this particular panel? Would we expect this to work
with other similar panels? Is there any way to know to expect this
convention from DPCD instead?

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for drm/i915: customize DPCD brightness control for specific panel

[Patchwork]

== Series Details ==

Series: drm/i915: customize DPCD brightness control for specific panel
URL   : https://patchwork.freedesktop.org/series/67595/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
84d67190854b drm/i915: customize DPCD brightness control for specific panel
-:88: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#88: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:39:
+	uint8_t read_val[2] = { 0x0 };

-:93: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#93: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:44:
+		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
+			DPCD_EDP_BRIGHTNESS_NITS);

-:106: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#106: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:57:
+	uint8_t new_vals[4];

-:121: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#121: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:72:
+static void intel_dp_aux_enable_customize_backlight(const struct intel_crtc_state *crtc_state,
+					  const struct drm_connector_state *conn_state)

-:125: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#125: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:76:
+	uint8_t read_val[4], i;

-:126: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#126: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:77:
+	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00, 0x00};

-:128: WARNING:LONG_LINE: line over 100 characters
#128: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:79:
+	if (drm_dp_dpcd_write(&intel_dp->aux, DPCD_EDP_PANEL_LUMINANCE_OVERRIDE, write_val, sizeof(write_val)) < 0)

-:133: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#133: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:84:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_BRIGHTNESS_OPTIMIZATION);

-:143: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#143: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:94:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);

-:147: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#147: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:98:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);

-:151: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#151: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:102:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);

-:153: WARNING:LONG_LINE: line over 100 characters
#153: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:104:
+	if (drm_dp_dpcd_read(&intel_dp->aux, DPCD_EDP_CONTENT_LUMINANCE, &read_val, sizeof(read_val)) < 0)

-:155: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#155: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:106:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_CONTENT_LUMINANCE);

-:158: WARNING:LONG_LINE: line over 100 characters
#158: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:109:
+	if (drm_dp_dpcd_write(&intel_dp->aux, DPCD_EDP_CONTENT_LUMINANCE, read_val, sizeof(read_val)) < 0)

-:160: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#160: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:111:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_CONTENT_LUMINANCE);

-:164: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#164: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:115:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);

-:182: WARNING:LONG_LINE: line over 100 characters
#182: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:133:
+static void intel_dp_aux_disable_customize_backlight(const struct drm_connector_state *old_conn_state)

-:186: WARNING:RETURN_VOID: void function return statements are not generally useful
#186: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:137:
+	return;
+}

-:196: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#196: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:340:
+static int intel_dp_aux_setup_customize_backlight(struct intel_connector *connector,
+					enum pipe pipe)

total: 0 errors, 5 warnings, 14 checks, 211 lines checked

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

✓ Fi.CI.BAT: success for drm/i915: customize DPCD brightness control for specific panel

[Patchwork]

== Series Details ==

Series: drm/i915: customize DPCD brightness control for specific panel
URL   : https://patchwork.freedesktop.org/series/67595/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7011 -> Patchwork_14670
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/index.html

Known issues
------------

  Here are the changes found in Patchwork_14670 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_basic@create-fd-close:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/fi-icl-u3/igt@gem_basic@create-fd-close.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/fi-icl-u3/igt@gem_basic@create-fd-close.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u3:          [PASS][3] -> [INCOMPLETE][4] ([fdo#107713] / [fdo#109100])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/fi-icl-u3/igt@gem_ctx_create@basic-files.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/fi-icl-u3/igt@gem_ctx_create@basic-files.html

  * igt@kms_chamelium@dp-edid-read:
    - fi-kbl-7500u:       [PASS][5] -> [WARN][6] ([fdo#109483])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/fi-kbl-7500u/igt@kms_chamelium@dp-edid-read.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][7] -> [FAIL][8] ([fdo#111045] / [fdo#111096])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096


Participating hosts (52 -> 42)
------------------------------

  Missing    (10): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-kbl-guc fi-gdg-551 fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7011 -> Patchwork_14670

  CI-20190529: 20190529
  CI_DRM_7011: 7d13733c2a87909eef8bc945ecbf4d47e0b67133 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5212: d17a484b3c22706b2b004ef1577f367d79235e43 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14670: 84d67190854beb3f0c5d0809b82646bc3250cac3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

84d67190854b drm/i915: customize DPCD brightness control for specific panel

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Fri, 04 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
> On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
>> This panel (manufacturer is SDC, product ID is 0x4141)
>> used manufacturer defined DPCD register to control brightness
>> that not defined in eDP spec so far. This change follow panel
>> vendor's instruction to support brightness adjustment.
>
> I'm sure this works, but this smells a little funny to me.

That was kindly put. ;)

>> +	/* Samsung eDP panel */
>> +	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },
>
> It feels a bit like a layering violation to identify eDP behavior
> changes based on EDID. But I'm not sure there's any obvious way to
> identify this device by its DPCD. The Sink OUI (from the linked
> bugzilla) seems to be 0011F8, which doesn't match up to anything in my
> oui.txt...

We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case
there's only the OUI, and the device id etc. are all zeros. Otherwise I
think that would be the natural thing to do, and all this could be
better hidden away in i915.

>
>> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid *edid)
>>  
>>  	return 0;
>>  }
>> +EXPORT_SYMBOL(edid_get_quirks);
>
> If we're going to export this it should probably get a drm_ prefix.
>
>> +#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
>> +#define DPCD_EDP_CONTENT_LUMINANCE		0x346
>> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
>> +#define DPCD_EDP_BRIGHTNESS_NITS		0x354
>> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
>> +
>> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
>
> This also seems a bit weird, the 0x300-0x3FF registers belong to the
> _source_ DP device. But then later...
>
>> +	/* write source OUI */
>> +	write_val[0] = 0x00;
>> +	write_val[1] = 0xaa;
>> +	write_val[2] = 0x01;
>
> Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it
> makes sense that you're writing to registers whose behavior the source
> defines. But this does raise the question: is this just a convention
> between Intel and this particular panel? Would we expect this to work
> with other similar panels? Is there any way to know to expect this
> convention from DPCD instead?

For one thing, it's not standard. I honestly don't know, but I'd assume
you wouldn't find behaviour with Intel OUI in non-Intel designs... and a
quirk of some sort seems like the only way to make this work.

I suppose we could start off with a DPCD quirk that only looks at the
sink OUI, and then, if needed, limit by DMI matching or by checking for
some DPCD registers (what, I am not sure, perhaps write the source OUI
and see how it behaves).

That would avoid the mildly annoying change in the EDID quirk interface
and how it's being used.

Thoughts?


BR,
Jani.





-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lee Shawn C]

This panel (manufacturer is SDC, product ID is 0x4141)
used manufacturer defined DPCD register to control brightness
that not defined in eDP spec so far. This change follow panel
vendor's instruction to support brightness adjustment.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/drm_edid.c                    |   6 +-
 drivers/gpu/drm/i915/display/intel_dp.c       |   7 +
 .../drm/i915/display/intel_dp_aux_backlight.c | 143 +++++++++++++++++-
 include/drm/drm_edid.h                        |   3 +
 4 files changed, 153 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3c9703b08491..5aee0ebc200e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -211,6 +211,9 @@ static const struct edid_quirk {
 
 	/* OSVR HDK and HDK2 VR Headsets */
 	{ "SVR", 0x1019, EDID_QUIRK_NON_DESKTOP },
+
+	/* Samsung eDP panel */
+	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },
 };
 
 /*
@@ -1938,7 +1941,7 @@ static bool edid_vendor(const struct edid *edid, const char *vendor)
  *
  * This tells subsequent routines what fixes they need to apply.
  */
-static u32 edid_get_quirks(const struct edid *edid)
+u32 edid_get_quirks(const struct edid *edid)
 {
 	const struct edid_quirk *quirk;
 	int i;
@@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid *edid)
 
 	return 0;
 }
+EXPORT_SYMBOL(edid_get_quirks);
 
 #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
 #define MODE_REFRESH_DIFF(c,t) (abs((c) - (t)))
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 2b1e71f992b0..89193bd2d8ea 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7097,6 +7097,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 	intel_connector->edid = edid;
 
+	if (edid_get_quirks(intel_connector->edid) ==
+	    EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL) {
+		i915_modparams.enable_dpcd_backlight = true;
+		i915_modparams.fastboot = false;
+		DRM_DEBUG_KMS("Using specific DPCD to control brightness\n");
+	}
+
 	fixed_mode = intel_panel_edid_fixed_mode(intel_connector);
 	if (fixed_mode)
 		downclock_mode = intel_dp_drrs_init(intel_connector, fixed_mode);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 020422da2ae2..7d9a2249cfb1 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -25,6 +25,117 @@
 #include "intel_display_types.h"
 #include "intel_dp_aux_backlight.h"
 
+#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
+#define DPCD_EDP_CONTENT_LUMINANCE		0x346
+#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
+#define DPCD_EDP_BRIGHTNESS_NITS		0x354
+#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
+
+#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
+
+static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector *connector)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	uint8_t read_val[2] = { 0x0 };
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DPCD_EDP_BRIGHTNESS_NITS,
+			     &read_val, sizeof(read_val)) < 0) {
+		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
+			DPCD_EDP_BRIGHTNESS_NITS);
+		return 0;
+	}
+
+	return (read_val[1] << 8 | read_val[0]);
+}
+
+static void
+intel_dp_aux_set_customize_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	struct intel_panel *panel = &connector->panel;
+	uint8_t new_vals[4];
+
+	if (level > panel->backlight.max)
+		level = panel->backlight.max;
+
+	new_vals[0] = level & 0xFF;
+	new_vals[1] = (level & 0xFF00) >> 8;
+	new_vals[2] = 0;
+	new_vals[3] = 0;
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DPCD_EDP_BRIGHTNESS_NITS, new_vals, 4) < 0)
+		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
+}
+
+static void intel_dp_aux_enable_customize_backlight(const struct intel_crtc_state *crtc_state,
+					  const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	uint8_t read_val[4], i;
+	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00, 0x00};
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DPCD_EDP_PANEL_LUMINANCE_OVERRIDE, write_val, sizeof(write_val)) < 0)
+		DRM_DEBUG_KMS("Failed to write panel luminance.\n");
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DPCD_EDP_BRIGHTNESS_OPTIMIZATION, 0x01) < 0)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_BRIGHTNESS_OPTIMIZATION);
+	/* write source OUI */
+	write_val[0] = 0x00;
+	write_val[1] = 0xaa;
+	write_val[2] = 0x01;
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) < 0)
+		DRM_DEBUG_KMS("Failed to write OUI\n");
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DPCD_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DPCD_EDP_GETSET_CTRL_PARAMS, 0) != 1)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DPCD_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DPCD_EDP_CONTENT_LUMINANCE, &read_val, sizeof(read_val)) < 0)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_CONTENT_LUMINANCE);
+
+	memset(read_val, 0x0, 4);
+	if (drm_dp_dpcd_write(&intel_dp->aux, DPCD_EDP_CONTENT_LUMINANCE, read_val, sizeof(read_val)) < 0)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DPCD_EDP_CONTENT_LUMINANCE);
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DPCD_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DPCD_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0)
+		DRM_DEBUG_KMS("Failed to read OUI\n");
+
+	DRM_DEBUG_KMS("got OUI %3ph\n", read_val);
+
+	for (i = 0; i < 6; i++) {
+		intel_dp_aux_set_customize_backlight(conn_state, connector->panel.backlight.level);
+
+		msleep(60);
+		if (intel_dp_aux_get_customize_backlight(connector))
+			return;
+	}
+
+	DRM_DEBUG_KMS("Restore brightness may have problem.\n");
+}
+
+static void intel_dp_aux_disable_customize_backlight(const struct drm_connector_state *old_conn_state)
+{
+	// do nothing
+	return;
+}
+
 static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	u8 reg_val = 0;
@@ -225,6 +336,19 @@ static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old
 	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false);
 }
 
+static int intel_dp_aux_setup_customize_backlight(struct intel_connector *connector,
+					enum pipe pipe)
+{
+	struct intel_panel *panel = &connector->panel;
+
+	panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL;
+	panel->backlight.min = 0;
+	panel->backlight.level = panel->backlight.get(connector);
+	panel->backlight.enabled = panel->backlight.level != 0;
+
+	return 0;
+}
+
 static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
 					enum pipe pipe)
 {
@@ -274,11 +398,20 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 	if (!intel_dp_aux_display_control_capable(intel_connector))
 		return -ENODEV;
 
-	panel->backlight.setup = intel_dp_aux_setup_backlight;
-	panel->backlight.enable = intel_dp_aux_enable_backlight;
-	panel->backlight.disable = intel_dp_aux_disable_backlight;
-	panel->backlight.set = intel_dp_aux_set_backlight;
-	panel->backlight.get = intel_dp_aux_get_backlight;
+	if (edid_get_quirks(intel_connector->edid) ==
+	    EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL) {
+		panel->backlight.setup   = intel_dp_aux_setup_customize_backlight;
+		panel->backlight.enable  = intel_dp_aux_enable_customize_backlight;
+		panel->backlight.disable = intel_dp_aux_disable_customize_backlight;
+		panel->backlight.set = intel_dp_aux_set_customize_backlight;
+		panel->backlight.get = intel_dp_aux_get_customize_backlight;
+	} else {
+		panel->backlight.setup   = intel_dp_aux_setup_backlight;
+		panel->backlight.enable  = intel_dp_aux_enable_backlight;
+		panel->backlight.disable = intel_dp_aux_disable_backlight;
+		panel->backlight.set = intel_dp_aux_set_backlight;
+		panel->backlight.get = intel_dp_aux_get_backlight;
+	}
 
 	return 0;
 }
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b9719418c3d2..b73bc3ee071e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -282,6 +282,8 @@ struct detailed_timing {
 
 #define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
 
+#define EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL	(1 << 13)
+
 struct edid {
 	u8 header[8];
 	/* Vendor & product info */
@@ -500,4 +502,5 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
+u32 edid_get_quirks(const struct edid *edid);
 #endif /* __DRM_EDID_H__ */
-- 
2.17.1

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

✗ Fi.CI.IGT: failure for drm/i915: customize DPCD brightness control for specific panel

[Patchwork]

== Series Details ==

Series: drm/i915: customize DPCD brightness control for specific panel
URL   : https://patchwork.freedesktop.org/series/67595/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7011_full -> Patchwork_14670_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14670_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14670_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_14670_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_ctx_isolation@vcs0-dirty-switch:
    - shard-apl:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl2/igt@gem_ctx_isolation@vcs0-dirty-switch.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl2/igt@gem_ctx_isolation@vcs0-dirty-switch.html

  * igt@gem_ctx_isolation@vcs0-s3:
    - shard-kbl:          [PASS][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-kbl1/igt@gem_ctx_isolation@vcs0-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-kbl7/igt@gem_ctx_isolation@vcs0-s3.html

  * igt@gem_ctx_isolation@vecs0-dirty-switch:
    - shard-skl:          [PASS][5] -> [FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl8/igt@gem_ctx_isolation@vecs0-dirty-switch.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl1/igt@gem_ctx_isolation@vecs0-dirty-switch.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_ctx_isolation@bcs0-s3:
    - {shard-tglb}:       NOTRUN -> [INCOMPLETE][7]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb7/igt@gem_ctx_isolation@bcs0-s3.html

  * igt@gem_exec_nop@basic-parallel:
    - {shard-tglb}:       [PASS][8] -> [INCOMPLETE][9] +2 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-tglb7/igt@gem_exec_nop@basic-parallel.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb8/igt@gem_exec_nop@basic-parallel.html

  
Known issues
------------

  Here are the changes found in Patchwork_14670_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@reset-stress:
    - shard-snb:          [PASS][10] -> [FAIL][11] ([fdo#109661])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-snb2/igt@gem_eio@reset-stress.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-snb2/igt@gem_eio@reset-stress.html

  * igt@gem_exec_schedule@reorder-wide-bsd:
    - shard-iclb:         [PASS][12] -> [SKIP][13] ([fdo#111325]) +7 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb8/igt@gem_exec_schedule@reorder-wide-bsd.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb1/igt@gem_exec_schedule@reorder-wide-bsd.html

  * igt@gem_softpin@noreloc-interruptible:
    - shard-apl:          [PASS][14] -> [INCOMPLETE][15] ([fdo#103927]) +1 similar issue
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl3/igt@gem_softpin@noreloc-interruptible.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl1/igt@gem_softpin@noreloc-interruptible.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup:
    - shard-snb:          [PASS][16] -> [DMESG-WARN][17] ([fdo#111870])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-snb2/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [PASS][18] -> [FAIL][19] ([fdo#105363])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl9/igt@kms_flip@flip-vs-expired-vblank.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl8/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbc-1p-shrfb-fliptrack:
    - shard-snb:          [PASS][20] -> [SKIP][21] ([fdo#109271]) +1 similar issue
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-snb1/igt@kms_frontbuffer_tracking@fbc-1p-shrfb-fliptrack.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-snb6/igt@kms_frontbuffer_tracking@fbc-1p-shrfb-fliptrack.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-iclb:         [PASS][22] -> [INCOMPLETE][23] ([fdo#106978] / [fdo#107713])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-pwrite.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-iclb:         [PASS][24] -> [FAIL][25] ([fdo#103167]) +3 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-skl:          [PASS][26] -> [INCOMPLETE][27] ([fdo#104108] / [fdo#106978])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl2/igt@kms_frontbuffer_tracking@psr-suspend.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl5/igt@kms_frontbuffer_tracking@psr-suspend.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [PASS][28] -> [SKIP][29] ([fdo#109642] / [fdo#111068])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb6/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_plane_move:
    - shard-iclb:         [PASS][30] -> [SKIP][31] ([fdo#109441]) +2 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb2/igt@kms_psr@psr2_cursor_plane_move.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb4/igt@kms_psr@psr2_cursor_plane_move.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][32] -> [FAIL][33] ([fdo#99912])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl4/igt@kms_setmode@basic.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl2/igt@kms_setmode@basic.html
    - shard-skl:          [PASS][34] -> [FAIL][35] ([fdo#99912])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl9/igt@kms_setmode@basic.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl4/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-apl:          [PASS][36] -> [DMESG-WARN][37] ([fdo#108566]) +3 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl3/igt@kms_vblank@pipe-b-ts-continuation-suspend.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl1/igt@kms_vblank@pipe-b-ts-continuation-suspend.html

  * igt@perf@polling:
    - shard-skl:          [PASS][38] -> [FAIL][39] ([fdo#110728])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl5/igt@perf@polling.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl6/igt@perf@polling.html

  * igt@perf_pmu@rc6:
    - shard-iclb:         [PASS][40] -> [INCOMPLETE][41] ([fdo#107713])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb4/igt@perf_pmu@rc6.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb1/igt@perf_pmu@rc6.html

  * igt@prime_busy@after-bsd2:
    - shard-iclb:         [PASS][42] -> [SKIP][43] ([fdo#109276]) +17 similar issues
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb4/igt@prime_busy@after-bsd2.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb8/igt@prime_busy@after-bsd2.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs0-reset:
    - shard-skl:          [FAIL][44] -> [PASS][45] +2 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl5/igt@gem_ctx_isolation@vcs0-reset.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl6/igt@gem_ctx_isolation@vcs0-reset.html

  * igt@gem_ctx_isolation@vecs0-none:
    - shard-apl:          [FAIL][46] -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl4/igt@gem_ctx_isolation@vecs0-none.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl3/igt@gem_ctx_isolation@vecs0-none.html

  * igt@gem_ctx_isolation@vecs0-reset:
    - shard-glk:          [FAIL][48] -> [PASS][49] +2 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-glk4/igt@gem_ctx_isolation@vecs0-reset.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-glk3/igt@gem_ctx_isolation@vecs0-reset.html

  * igt@gem_ctx_shared@exec-single-timeline-bsd:
    - shard-iclb:         [SKIP][50] ([fdo#110841]) -> [PASS][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb7/igt@gem_ctx_shared@exec-single-timeline-bsd.html

  * igt@gem_eio@in-flight-contexts-immediate:
    - shard-snb:          [FAIL][52] -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-snb7/igt@gem_eio@in-flight-contexts-immediate.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-snb4/igt@gem_eio@in-flight-contexts-immediate.html

  * igt@gem_exec_balancer@busy:
    - shard-apl:          [INCOMPLETE][54] ([fdo#103927]) -> [PASS][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl6/igt@gem_exec_balancer@busy.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl6/igt@gem_exec_balancer@busy.html

  * igt@gem_exec_schedule@independent-bsd:
    - shard-iclb:         [SKIP][56] ([fdo#111325]) -> [PASS][57] +1 similar issue
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb1/igt@gem_exec_schedule@independent-bsd.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb7/igt@gem_exec_schedule@independent-bsd.html

  * igt@gem_exec_schedule@preempt-queue-bsd1:
    - shard-iclb:         [SKIP][58] ([fdo#109276]) -> [PASS][59] +26 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb6/igt@gem_exec_schedule@preempt-queue-bsd1.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb2/igt@gem_exec_schedule@preempt-queue-bsd1.html

  * igt@gem_userptr_blits@dmabuf-unsync:
    - shard-snb:          [DMESG-WARN][60] ([fdo#111870]) -> [PASS][61]
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-snb2/igt@gem_userptr_blits@dmabuf-unsync.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-snb4/igt@gem_userptr_blits@dmabuf-unsync.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [DMESG-WARN][62] ([fdo#108566]) -> [PASS][63] +4 similar issues
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-apl6/igt@gem_workarounds@suspend-resume-context.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-apl1/igt@gem_workarounds@suspend-resume-context.html

  * igt@i915_suspend@debugfs-reader:
    - {shard-tglb}:       [INCOMPLETE][64] ([fdo#111832] / [fdo#111867]) -> [PASS][65]
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-tglb2/igt@i915_suspend@debugfs-reader.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb5/igt@i915_suspend@debugfs-reader.html

  * igt@i915_suspend@sysfs-reader:
    - {shard-tglb}:       [INCOMPLETE][66] ([fdo#111832]) -> [PASS][67] +1 similar issue
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-tglb4/igt@i915_suspend@sysfs-reader.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb5/igt@i915_suspend@sysfs-reader.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][68] ([fdo#105363]) -> [PASS][69]
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl10/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-cpu:
    - {shard-tglb}:       [FAIL][70] ([fdo#103167]) -> [PASS][71] +8 similar issues
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-tglb2/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-cpu.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb8/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-cpu.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
    - shard-iclb:         [FAIL][72] ([fdo#103167]) -> [PASS][73] +4 similar issues
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html

  * igt@kms_plane@plane-panning-bottom-right-pipe-b-planes:
    - {shard-tglb}:       [INCOMPLETE][74] -> [PASS][75]
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-tglb2/igt@kms_plane@plane-panning-bottom-right-pipe-b-planes.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-tglb7/igt@kms_plane@plane-panning-bottom-right-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][76] ([fdo#108145]) -> [PASS][77]
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-skl9/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][78] ([fdo#109441]) -> [PASS][79] +1 similar issue
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb3/igt@kms_psr@psr2_suspend.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb2/igt@kms_psr@psr2_suspend.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [FAIL][80] ([fdo#111329]) -> [SKIP][81] ([fdo#109276])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb6/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_mocs_settings@mocs-isolation-bsd2:
    - shard-iclb:         [SKIP][82] ([fdo#109276]) -> [FAIL][83] ([fdo#111330])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7011/shard-iclb5/igt@gem_mocs_settings@mocs-isolation-bsd2.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/shard-iclb4/igt@gem_mocs_settings@mocs-isolation-bsd2.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108972]: https://bugs.freedesktop.org/show_bug.cgi?id=108972
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#111832]: https://bugs.freedesktop.org/show_bug.cgi?id=111832
  [fdo#111850]: https://bugs.freedesktop.org/show_bug.cgi?id=111850
  [fdo#111867]: https://bugs.freedesktop.org/show_bug.cgi?id=111867
  [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (11 -> 11)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7011 -> Patchwork_14670

  CI-20190529: 20190529
  CI_DRM_7011: 7d13733c2a87909eef8bc945ecbf4d47e0b67133 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5212: d17a484b3c22706b2b004ef1577f367d79235e43 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14670: 84d67190854beb3f0c5d0809b82646bc3250cac3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14670/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lee, Shawn C]

On Fri, 04 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>On Fri, 04 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>> On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
>>> This panel (manufacturer is SDC, product ID is 0x4141) used 
>>> manufacturer defined DPCD register to control brightness that not 
>>> defined in eDP spec so far. This change follow panel vendor's 
>>> instruction to support brightness adjustment.
>>
>> I'm sure this works, but this smells a little funny to me.
>
>That was kindly put. ;)
>
>>> +	/* Samsung eDP panel */
>>> +	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },
>>
>> It feels a bit like a layering violation to identify eDP behavior 
>> changes based on EDID. But I'm not sure there's any obvious way to 
>> identify this device by its DPCD. The Sink OUI (from the linked
>> bugzilla) seems to be 0011F8, which doesn't match up to anything in my 
>> oui.txt...
>
>We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case 
>there's only the OUI, and the device id etc. are all zeros. Otherwise I
>think that would be the natural thing to do, and all this could be
>better hidden away in i915.
>

Below is what we dumped from this panel. Only sink OUI (ba-41-59) in it
and nothing else. 
00000400  ba 41 59 00 00 00 00 00  00 00 00 00 00 00 00 00  |.AY.............|
00000410  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................

That's why the patch to identify EDID's manufacturer and product ID
to make sure this method applied on specific panel.

>>
>>> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid 
>>> *edid)
>>>  
>>>  	return 0;
>>>  }
>>> +EXPORT_SYMBOL(edid_get_quirks);
>>
>> If we're going to export this it should probably get a drm_ prefix.

Yes! It will be better to have drm_ prefix for export funciton.

>>
>>> +#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
>>> +#define DPCD_EDP_CONTENT_LUMINANCE		0x346
>>> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
>>> +#define DPCD_EDP_BRIGHTNESS_NITS		0x354
>>> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
>>> +
>>> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
>>
>> This also seems a bit weird, the 0x300-0x3FF registers belong to the 
>> _source_ DP device. But then later...
>>
>>> +	/* write source OUI */
>>> +	write_val[0] = 0x00;
>>> +	write_val[1] = 0xaa;
>>> +	write_val[2] = 0x01;
>>
>> Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it 
>> makes sense that you're writing to registers whose behavior the source 
>> defines. But this does raise the question: is this just a convention 
>> between Intel and this particular panel? Would we expect this to work 
>> with other similar panels? Is there any way to know to expect this 
>> convention from DPCD instead?

TCON would reply on source OUI to configure its capability. And these
DPCD registers were defined by vendor and Intel. This change should works
with similar panels (with same TCON). Seems there is another issue so
vendor decide to use non standard way to setup brightness.

>For one thing, it's not standard. I honestly don't know, but I'd assume
>you wouldn't find behaviour with Intel OUI in non-Intel designs... and a
>quirk of some sort seems like the only way to make this work.
>
>I suppose we could start off with a DPCD quirk that only looks at the
>sink OUI, and then, if needed, limit by DMI matching or by checking for
>some DPCD registers (what, I am not sure, perhaps write the source OUI
>and see how it behaves).
>
>That would avoid the mildly annoying change in the EDID quirk interface
>and how it's being used.
>
>Thoughts?
>
>
>BR,
>Jani.
>

To be honest. Panel vendor did not provide enough sink info in DPCD.
That's why hard to recognize it and we have to confirm EDID instead of DPCD.

Do you mean just confirm sink OUI only from DPCD quirk? I'm afraid it
may impact the other panels with the same TCON. Any suggestion?

>
>--
>Jani Nikula, Intel Open Source Graphics Center
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Mon, 07 Oct 2019, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote:
> On Fri, 04 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>>On Fri, 04 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>>> On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
>>>> This panel (manufacturer is SDC, product ID is 0x4141) used 
>>>> manufacturer defined DPCD register to control brightness that not 
>>>> defined in eDP spec so far. This change follow panel vendor's 
>>>> instruction to support brightness adjustment.
>>>
>>> I'm sure this works, but this smells a little funny to me.
>>
>>That was kindly put. ;)
>>
>>>> +	/* Samsung eDP panel */
>>>> +	{ "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },
>>>
>>> It feels a bit like a layering violation to identify eDP behavior 
>>> changes based on EDID. But I'm not sure there's any obvious way to 
>>> identify this device by its DPCD. The Sink OUI (from the linked
>>> bugzilla) seems to be 0011F8, which doesn't match up to anything in my 
>>> oui.txt...
>>
>>We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case 
>>there's only the OUI, and the device id etc. are all zeros. Otherwise I
>>think that would be the natural thing to do, and all this could be
>>better hidden away in i915.
>>
>
> Below is what we dumped from this panel. Only sink OUI (ba-41-59) in it
> and nothing else. 
> 00000400  ba 41 59 00 00 00 00 00  00 00 00 00 00 00 00 00  |.AY.............|
> 00000410  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................
>
> That's why the patch to identify EDID's manufacturer and product ID
> to make sure this method applied on specific panel.
>
>>>
>>>> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid 
>>>> *edid)
>>>>  
>>>>  	return 0;
>>>>  }
>>>> +EXPORT_SYMBOL(edid_get_quirks);
>>>
>>> If we're going to export this it should probably get a drm_ prefix.
>
> Yes! It will be better to have drm_ prefix for export funciton.
>
>>>
>>>> +#define DPCD_EDP_GETSET_CTRL_PARAMS		0x344
>>>> +#define DPCD_EDP_CONTENT_LUMINANCE		0x346
>>>> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE	0x34a
>>>> +#define DPCD_EDP_BRIGHTNESS_NITS		0x354
>>>> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION	0x358
>>>> +
>>>> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
>>>
>>> This also seems a bit weird, the 0x300-0x3FF registers belong to the 
>>> _source_ DP device. But then later...
>>>
>>>> +	/* write source OUI */
>>>> +	write_val[0] = 0x00;
>>>> +	write_val[1] = 0xaa;
>>>> +	write_val[2] = 0x01;
>>>
>>> Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it 
>>> makes sense that you're writing to registers whose behavior the source 
>>> defines. But this does raise the question: is this just a convention 
>>> between Intel and this particular panel? Would we expect this to work 
>>> with other similar panels? Is there any way to know to expect this 
>>> convention from DPCD instead?
>
> TCON would reply on source OUI to configure its capability. And these
> DPCD registers were defined by vendor and Intel. This change should works
> with similar panels (with same TCON). Seems there is another issue so
> vendor decide to use non standard way to setup brightness.
>
>>For one thing, it's not standard. I honestly don't know, but I'd assume
>>you wouldn't find behaviour with Intel OUI in non-Intel designs... and a
>>quirk of some sort seems like the only way to make this work.
>>
>>I suppose we could start off with a DPCD quirk that only looks at the
>>sink OUI, and then, if needed, limit by DMI matching or by checking for
>>some DPCD registers (what, I am not sure, perhaps write the source OUI
>>and see how it behaves).
>>
>>That would avoid the mildly annoying change in the EDID quirk interface
>>and how it's being used.
>>
>>Thoughts?
>>
>>
>>BR,
>>Jani.
>>
>
> To be honest. Panel vendor did not provide enough sink info in DPCD.
> That's why hard to recognize it and we have to confirm EDID instead of DPCD.
>
> Do you mean just confirm sink OUI only from DPCD quirk? I'm afraid it
> may impact the other panels with the same TCON. Any suggestion?

The problem with the EDID quirks is that exposing the quirks sticks out
like a sore thumb. Thus far all of it has been contained in drm_edid.c
and they affect how the EDID gets parsed, for all drivers. Obviously
this could be changed, but is it the right thing to do?

What I suggested was, check the OUI only, and if it matches, do
more. Perhaps there's something in the 0x300 range of DPCD offsets that
you can read? Or perhaps you need to write the source OUI first, and
then do that.

BR,
Jani.


>
>>
>>--
>>Jani Nikula, Intel Open Source Graphics Center
>>

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Adam Jackson]

On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:

> The problem with the EDID quirks is that exposing the quirks sticks out
> like a sore thumb. Thus far all of it has been contained in drm_edid.c
> and they affect how the EDID gets parsed, for all drivers. Obviously
> this could be changed, but is it the right thing to do?
> 
> What I suggested was, check the OUI only, and if it matches, do
> more. Perhaps there's something in the 0x300 range of DPCD offsets that
> you can read? Or perhaps you need to write the source OUI first, and
> then do that.

My issue isn't really with identifying the panel from EDID rather than
DPCD, whichever identifier is most specific is probably the best thing
to use. It's more that this quirk is identified in common code but only
applied in one driver. If this panel were ever to be attached to some
other source, they might well want to apply the same kind of fix. My
(admittedly naïve) reading of the OUI handshake process is that when
the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
"I'm about to issue commands that conform to _this_ vendor's own
conventions". If that convention communicates information that is
entirely contained within AUXCH transactions (and doesn't, for example,
require looking at some other strapping pin or external device) then in
principle it doesn't matter if the source device "matches" that OUI; it
would be legal for an AMD GPU to write the same sequence and expect the
same reaction, should that panel be attached to an AMD GPU.

So, it would be nice to know exactly what that protocol is meant to do,
if it applies only to this specific panel or anything else with the
same TCON, how one would identify such TCONs in the wild other than
EDID, if it relies on an external PWM or something, etc. And it might
make sense for now to make this a (shudder) driver-specific EDID quirk
rather than match by DPCD, at least until we know if the panel is ever
seen attached to other source devices and if the OUI convention is
self-contained.

- ajax

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

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
> On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
>
>> The problem with the EDID quirks is that exposing the quirks sticks out
>> like a sore thumb. Thus far all of it has been contained in drm_edid.c
>> and they affect how the EDID gets parsed, for all drivers. Obviously
>> this could be changed, but is it the right thing to do?
>> 
>> What I suggested was, check the OUI only, and if it matches, do
>> more. Perhaps there's something in the 0x300 range of DPCD offsets that
>> you can read? Or perhaps you need to write the source OUI first, and
>> then do that.
>
> My issue isn't really with identifying the panel from EDID rather than
> DPCD, whichever identifier is most specific is probably the best thing
> to use. It's more that this quirk is identified in common code but only
> applied in one driver. If this panel were ever to be attached to some
> other source, they might well want to apply the same kind of fix. My
> (admittedly naïve) reading of the OUI handshake process is that when
> the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
> "I'm about to issue commands that conform to _this_ vendor's own
> conventions". If that convention communicates information that is
> entirely contained within AUXCH transactions (and doesn't, for example,
> require looking at some other strapping pin or external device) then in
> principle it doesn't matter if the source device "matches" that OUI; it
> would be legal for an AMD GPU to write the same sequence and expect the
> same reaction, should that panel be attached to an AMD GPU.
>
> So, it would be nice to know exactly what that protocol is meant to do,
> if it applies only to this specific panel or anything else with the
> same TCON, how one would identify such TCONs in the wild other than
> EDID, if it relies on an external PWM or something, etc. And it might
> make sense for now to make this a (shudder) driver-specific EDID quirk
> rather than match by DPCD, at least until we know if the panel is ever
> seen attached to other source devices and if the OUI convention is
> self-contained.

Thanks for clarifying. Pretty much agreed, unfortunately also on the
"would be nice to know more" part...

If this were to be an EDID quirk after all, I wonder if it would be
better to store the parsed quirks to, say, struct drm_display_info, and
have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().

This would also allow us to not return quirks from
drm_add_display_info(), which would arguably clean up the interface.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915: customize DPCD brightness control for specific panel (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: customize DPCD brightness control for specific panel (rev2)
URL   : https://patchwork.freedesktop.org/series/67595/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ebafaef17574 drm/i915: customize DPCD brightness control for specific panel
-:35: WARNING:LONG_LINE: line over 100 characters
#35: FILE: drivers/gpu/drm/drm_dp_helper.c:1272:
+	{ OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) },

-:52: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 32)
#52: FILE: drivers/gpu/drm/drm_dp_helper.c:1306:
+		if (quirk->quirks == DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL &&
[...]
+				continue;

-:72: WARNING:LONG_LINE: line over 100 characters
#72: FILE: drivers/gpu/drm/drm_dp_helper.c:1344:
+		ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0, tcon_cap, sizeof(tcon_cap));

-:90: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#90: FILE: drivers/gpu/drm/i915/display/intel_dp.c:7095:
+	if (drm_dp_has_quirk(&intel_dp->desc,
+	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {

-:112: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#112: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:33:
+	uint8_t read_val[2] = { 0x0 };

-:117: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#117: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:38:
+		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
+			DP_EDP_BRIGHTNESS_NITS);

-:130: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#130: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:51:
+	uint8_t new_vals[4];

-:145: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#145: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:66:
+static void intel_dp_aux_enable_customize_backlight(const struct intel_crtc_state *crtc_state,
+					  const struct drm_connector_state *conn_state)

-:149: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#149: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:70:
+	uint8_t read_val[4], i;

-:150: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u8' over 'uint8_t'
#150: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:71:
+	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00, 0x00};

-:152: WARNING:LONG_LINE: line over 100 characters
#152: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:73:
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE, write_val, sizeof(write_val)) < 0)

-:157: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#157: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:78:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_BRIGHTNESS_OPTIMIZATION);

-:167: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#167: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:88:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);

-:171: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#171: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:92:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);

-:175: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#175: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:96:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);

-:177: WARNING:LONG_LINE: line over 100 characters
#177: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:98:
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, &read_val, sizeof(read_val)) < 0)

-:179: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#179: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:100:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_CONTENT_LUMINANCE);

-:182: WARNING:LONG_LINE: line over 100 characters
#182: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:103:
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, read_val, sizeof(read_val)) < 0)

-:184: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#184: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:105:
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_CONTENT_LUMINANCE);

-:188: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#188: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:109:
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);

-:206: WARNING:LONG_LINE: line over 100 characters
#206: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:127:
+static void intel_dp_aux_disable_customize_backlight(const struct drm_connector_state *old_conn_state)

-:210: WARNING:RETURN_VOID: void function return statements are not generally useful
#210: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:131:
+	return;
+}

-:220: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#220: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:334:
+static int intel_dp_aux_setup_customize_backlight(struct intel_connector *connector,
+					enum pipe pipe)

-:253: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#253: FILE: drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c:397:
+	if (drm_dp_has_quirk(&intel_dp->desc,
+	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {

total: 0 errors, 8 warnings, 16 checks, 254 lines checked

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

✗ Fi.CI.BAT: failure for drm/i915: customize DPCD brightness control for specific panel (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: customize DPCD brightness control for specific panel (rev2)
URL   : https://patchwork.freedesktop.org/series/67595/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7040 -> Patchwork_14720
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14720 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14720, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_14720:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_coherency:
    - fi-icl-u3:          [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7040/fi-icl-u3/igt@i915_selftest@live_coherency.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/fi-icl-u3/igt@i915_selftest@live_coherency.html

  
Known issues
------------

  Here are the changes found in Patchwork_14720 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@flink-lifetime:
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4] ([fdo#107724]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7040/fi-icl-u3/igt@gem_flink_basic@flink-lifetime.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/fi-icl-u3/igt@gem_flink_basic@flink-lifetime.html

  
#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u4}:        [FAIL][5] ([fdo#103167]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7040/fi-icl-u4/igt@kms_frontbuffer_tracking@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/fi-icl-u4/igt@kms_frontbuffer_tracking@basic.html
    - fi-hsw-peppy:       [DMESG-WARN][7] ([fdo#102614]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7040/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  * igt@prime_busy@basic-wait-after-default:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10] +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7040/fi-icl-u3/igt@prime_busy@basic-wait-after-default.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/fi-icl-u3/igt@prime_busy@basic-wait-after-default.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111049]: https://bugs.freedesktop.org/show_bug.cgi?id=111049
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111600]: https://bugs.freedesktop.org/show_bug.cgi?id=111600


Participating hosts (52 -> 43)
------------------------------

  Missing    (9): fi-ilk-m540 fi-tgl-u fi-bsw-n3050 fi-byt-j1900 fi-hsw-4200u fi-byt-squawks fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7040 -> Patchwork_14720

  CI-20190529: 20190529
  CI_DRM_7040: 932ef79204b22399bbf7e9d9db5cb813b5ca01b7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5218: 869ed1ee0b71ce17f0a864512488f8b1a6cb8545 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14720: ebafaef1757413d5d7b4519356807cbbcaa8590a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ebafaef17574 drm/i915: customize DPCD brightness control for specific panel

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14720/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: customize DPCD brightness control for specific panel

[Lee Shawn C]

This panel (manufacturer is SDC, product ID is 0x4141)
used manufacturer defined DPCD register to control brightness
that not defined in eDP spec so far. This change follow panel
vendor's instruction to support brightness adjustment.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883

V2: To check sink OUI instead of EDID quirk.
    According to TCON's capability to decide to enable this
    method for brightness control.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Cooper Chiou <cooper.chiou@intel.com>

Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c               |  18 ++-
 drivers/gpu/drm/i915/display/intel_dp.c       |   7 +
 .../drm/i915/display/intel_dp_aux_backlight.c | 138 +++++++++++++++++-
 include/drm/drm_dp_helper.h                   |  20 +++
 4 files changed, 176 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f373798d82f6..02f79587c337 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1268,6 +1268,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
 	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_NO_PSR) },
 	/* CH7511 seems to leave SINK_COUNT zeroed */
 	{ OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'), false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) },
+	/* Samsung eDP panel */
+	{ OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) },
 };
 
 #undef OUI
@@ -1281,7 +1283,7 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
  * 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)
+drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch, u8 *tcon_cap)
 {
 	const struct dpcd_quirk *quirk;
 	u32 quirks = 0;
@@ -1301,6 +1303,11 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch)
 		    memcmp(quirk->device_id, ident->device_id, sizeof(ident->device_id)) != 0)
 			continue;
 
+		if (quirk->quirks == DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL &&
+		    (!(tcon_cap[1] & DP_BRIGHTNESS_CONTROL_NITS) ||
+		     !(tcon_cap[2] & DP_BRIGHTNESS_CONTROL_BY_AUX)))
+				continue;
+
 		quirks |= quirk->quirks;
 	}
 
@@ -1327,12 +1334,19 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct drm_dp_desc *desc,
 	struct drm_dp_dpcd_ident *ident = &desc->ident;
 	unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI;
 	int ret, dev_id_len;
+	u8 tcon_cap[4] = {0};
 
 	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
 	if (ret < 0)
 		return ret;
 
-	desc->quirks = drm_dp_get_quirks(ident, is_branch);
+	if (offset == DP_SINK_OUI) {
+		ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0, tcon_cap, sizeof(tcon_cap));
+		if (ret < 0)
+			return ret;
+	}
+
+	desc->quirks = drm_dp_get_quirks(ident, is_branch, tcon_cap);
 
 	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 2b1e71f992b0..766b4b252147 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7082,6 +7082,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		goto out_vdd_off;
 	}
 
+	if (drm_dp_has_quirk(&intel_dp->desc,
+	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
+		i915_modparams.enable_dpcd_backlight = true;
+		i915_modparams.fastboot = false;
+		DRM_DEBUG_KMS("Using specific DPCD to control brightness\n");
+	}
+
 	mutex_lock(&dev->mode_config.mutex);
 	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 020422da2ae2..09480e772bd4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -25,6 +25,111 @@
 #include "intel_display_types.h"
 #include "intel_dp_aux_backlight.h"
 
+#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
+
+static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector *connector)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	uint8_t read_val[2] = { 0x0 };
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
+			     &read_val, sizeof(read_val)) < 0) {
+		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
+			DP_EDP_BRIGHTNESS_NITS);
+		return 0;
+	}
+
+	return (read_val[1] << 8 | read_val[0]);
+}
+
+static void
+intel_dp_aux_set_customize_backlight(const struct drm_connector_state *conn_state, u32 level)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	struct intel_panel *panel = &connector->panel;
+	uint8_t new_vals[4];
+
+	if (level > panel->backlight.max)
+		level = panel->backlight.max;
+
+	new_vals[0] = level & 0xFF;
+	new_vals[1] = (level & 0xFF00) >> 8;
+	new_vals[2] = 0;
+	new_vals[3] = 0;
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS, new_vals, 4) < 0)
+		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
+}
+
+static void intel_dp_aux_enable_customize_backlight(const struct intel_crtc_state *crtc_state,
+					  const struct drm_connector_state *conn_state)
+{
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	uint8_t read_val[4], i;
+	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00, 0x00};
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE, write_val, sizeof(write_val)) < 0)
+		DRM_DEBUG_KMS("Failed to write panel luminance.\n");
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BRIGHTNESS_OPTIMIZATION, 0x01) < 0)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_BRIGHTNESS_OPTIMIZATION);
+	/* write source OUI */
+	write_val[0] = 0x00;
+	write_val[1] = 0xaa;
+	write_val[2] = 0x01;
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) < 0)
+		DRM_DEBUG_KMS("Failed to write OUI\n");
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, 0) != 1)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, &read_val, sizeof(read_val)) < 0)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_CONTENT_LUMINANCE);
+
+	memset(read_val, 0x0, 4);
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE, read_val, sizeof(read_val)) < 0)
+		DRM_DEBUG_KMS("Failed to write %x\n",
+			DP_EDP_CONTENT_LUMINANCE);
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, read_val) != 1)
+		DRM_DEBUG_KMS("Failed to read %x\n",
+			DP_EDP_GETSET_CTRL_PARAMS);
+
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0)
+		DRM_DEBUG_KMS("Failed to read OUI\n");
+
+	DRM_DEBUG_KMS("got OUI %3ph\n", read_val);
+
+	for (i = 0; i < 6; i++) {
+		intel_dp_aux_set_customize_backlight(conn_state, connector->panel.backlight.level);
+
+		msleep(60);
+		if (intel_dp_aux_get_customize_backlight(connector))
+			return;
+	}
+
+	DRM_DEBUG_KMS("Restore brightness may have problem.\n");
+}
+
+static void intel_dp_aux_disable_customize_backlight(const struct drm_connector_state *old_conn_state)
+{
+	// do nothing
+	return;
+}
+
 static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	u8 reg_val = 0;
@@ -225,6 +330,19 @@ static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old
 	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false);
 }
 
+static int intel_dp_aux_setup_customize_backlight(struct intel_connector *connector,
+					enum pipe pipe)
+{
+	struct intel_panel *panel = &connector->panel;
+
+	panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL;
+	panel->backlight.min = 0;
+	panel->backlight.level = panel->backlight.get(connector);
+	panel->backlight.enabled = panel->backlight.level != 0;
+
+	return 0;
+}
+
 static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
 					enum pipe pipe)
 {
@@ -265,6 +383,7 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 {
 	struct intel_panel *panel = &intel_connector->panel;
 	struct drm_i915_private *dev_priv = to_i915(intel_connector->base.dev);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_connector->encoder->base);
 
 	if (i915_modparams.enable_dpcd_backlight == 0 ||
 	    (i915_modparams.enable_dpcd_backlight == -1 &&
@@ -274,11 +393,20 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 	if (!intel_dp_aux_display_control_capable(intel_connector))
 		return -ENODEV;
 
-	panel->backlight.setup = intel_dp_aux_setup_backlight;
-	panel->backlight.enable = intel_dp_aux_enable_backlight;
-	panel->backlight.disable = intel_dp_aux_disable_backlight;
-	panel->backlight.set = intel_dp_aux_set_backlight;
-	panel->backlight.get = intel_dp_aux_get_backlight;
+	if (drm_dp_has_quirk(&intel_dp->desc,
+	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
+		panel->backlight.setup   = intel_dp_aux_setup_customize_backlight;
+		panel->backlight.enable  = intel_dp_aux_enable_customize_backlight;
+		panel->backlight.disable = intel_dp_aux_disable_customize_backlight;
+		panel->backlight.set = intel_dp_aux_set_customize_backlight;
+		panel->backlight.get = intel_dp_aux_get_customize_backlight;
+	} else {
+		panel->backlight.setup   = intel_dp_aux_setup_backlight;
+		panel->backlight.enable  = intel_dp_aux_enable_backlight;
+		panel->backlight.disable = intel_dp_aux_disable_backlight;
+		panel->backlight.set = intel_dp_aux_set_backlight;
+		panel->backlight.get = intel_dp_aux_get_backlight;
+	}
 
 	return 0;
 }
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index ed1a985745ba..581d1ba8435d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -740,6 +740,20 @@
 /* up to ID_SLOT_63 at 0x2ff */
 
 #define DP_SOURCE_OUI			    0x300
+#define DP_EDP_TCON_CAPABILITY_BYTE0	    0x340
+
+#define DP_EDP_TCON_CAPABILITY_BYTE1	    0x341
+# define DP_BRIGHTNESS_CONTROL_NITS	    0x10
+
+#define DP_EDP_TCON_CAPABILITY_BYTE2	    0x342
+# define DP_BRIGHTNESS_CONTROL_BY_AUX	    0x01
+
+#define DP_EDP_GETSET_CTRL_PARAMS	    0x344
+#define DP_EDP_CONTENT_LUMINANCE	    0x346
+#define DP_EDP_PANEL_LUMINANCE_OVERRIDE     0x34A
+#define DP_EDP_BRIGHTNESS_NITS		    0x354
+#define DP_EDP_BRIGHTNESS_OPTIMIZATION	    0x358
+
 #define DP_SINK_OUI			    0x400
 #define DP_BRANCH_OUI			    0x500
 #define DP_BRANCH_ID                        0x503
@@ -1475,6 +1489,12 @@ enum drm_dp_quirk {
 	 * The driver should ignore SINK_COUNT during detection.
 	 */
 	DP_DPCD_QUIRK_NO_SINK_COUNT,
+	/**
+	 * @DP_DPCD_QUIRK_NON_STANDARD_BRIGHTNESS_CONTROL:
+	 *
+	 * The device used specific DPCD register to control brightness.
+	 */
+	DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL,
 };
 
 /**
-- 
2.17.1

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

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

I'm about to post some more review comments for the v2 version of this, but
some comments down below...

On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
> > 
> > > The problem with the EDID quirks is that exposing the quirks sticks out
> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
> > > and they affect how the EDID gets parsed, for all drivers. Obviously
> > > this could be changed, but is it the right thing to do?
> > > 
> > > What I suggested was, check the OUI only, and if it matches, do
> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
> > > you can read? Or perhaps you need to write the source OUI first, and
> > > then do that.
> > 
> > My issue isn't really with identifying the panel from EDID rather than
> > DPCD, whichever identifier is most specific is probably the best thing
> > to use. It's more that this quirk is identified in common code but only
> > applied in one driver. If this panel were ever to be attached to some
> > other source, they might well want to apply the same kind of fix. My
> > (admittedly naïve) reading of the OUI handshake process is that when
> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
> > "I'm about to issue commands that conform to _this_ vendor's own
> > conventions". If that convention communicates information that is
> > entirely contained within AUXCH transactions (and doesn't, for example,
> > require looking at some other strapping pin or external device) then in
> > principle it doesn't matter if the source device "matches" that OUI; it
> > would be legal for an AMD GPU to write the same sequence and expect the
> > same reaction, should that panel be attached to an AMD GPU.
> > 
> > So, it would be nice to know exactly what that protocol is meant to do,
> > if it applies only to this specific panel or anything else with the
> > same TCON, how one would identify such TCONs in the wild other than
> > EDID, if it relies on an external PWM or something, etc. And it might
> > make sense for now to make this a (shudder) driver-specific EDID quirk
> > rather than match by DPCD, at least until we know if the panel is ever
> > seen attached to other source devices and if the OUI convention is
> > self-contained.
> 
> Thanks for clarifying. Pretty much agreed, unfortunately also on the
> "would be nice to know more" part...
> 
> If this were to be an EDID quirk after all, I wonder if it would be
> better to store the parsed quirks to, say, struct drm_display_info, and
> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
> 
> This would also allow us to not return quirks from
> drm_add_display_info(), which would arguably clean up the interface.

Did anyone check if this is specified in the vbios? There appears to be a
field defined for this right...

enum intel_backlight_type {
	INTEL_BACKLIGHT_PMIC,
	INTEL_BACKLIGHT_LPSS,
	INTEL_BACKLIGHT_DISPLAY_DDI,
	INTEL_BACKLIGHT_DSI_DCS,
	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
};

> 
> BR,
> Jani.
> 
> 
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

I'm about to post some more review comments for the v2 version of this, but
some comments down below...

On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
> > 
> > > The problem with the EDID quirks is that exposing the quirks sticks out
> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
> > > and they affect how the EDID gets parsed, for all drivers. Obviously
> > > this could be changed, but is it the right thing to do?
> > > 
> > > What I suggested was, check the OUI only, and if it matches, do
> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
> > > you can read? Or perhaps you need to write the source OUI first, and
> > > then do that.
> > 
> > My issue isn't really with identifying the panel from EDID rather than
> > DPCD, whichever identifier is most specific is probably the best thing
> > to use. It's more that this quirk is identified in common code but only
> > applied in one driver. If this panel were ever to be attached to some
> > other source, they might well want to apply the same kind of fix. My
> > (admittedly naïve) reading of the OUI handshake process is that when
> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
> > "I'm about to issue commands that conform to _this_ vendor's own
> > conventions". If that convention communicates information that is
> > entirely contained within AUXCH transactions (and doesn't, for example,
> > require looking at some other strapping pin or external device) then in
> > principle it doesn't matter if the source device "matches" that OUI; it
> > would be legal for an AMD GPU to write the same sequence and expect the
> > same reaction, should that panel be attached to an AMD GPU.
> > 
> > So, it would be nice to know exactly what that protocol is meant to do,
> > if it applies only to this specific panel or anything else with the
> > same TCON, how one would identify such TCONs in the wild other than
> > EDID, if it relies on an external PWM or something, etc. And it might
> > make sense for now to make this a (shudder) driver-specific EDID quirk
> > rather than match by DPCD, at least until we know if the panel is ever
> > seen attached to other source devices and if the OUI convention is
> > self-contained.
> 
> Thanks for clarifying. Pretty much agreed, unfortunately also on the
> "would be nice to know more" part...
> 
> If this were to be an EDID quirk after all, I wonder if it would be
> better to store the parsed quirks to, say, struct drm_display_info, and
> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
> 
> This would also allow us to not return quirks from
> drm_add_display_info(), which would arguably clean up the interface.

Did anyone check if this is specified in the vbios? There appears to be a
field defined for this right...

enum intel_backlight_type {
	INTEL_BACKLIGHT_PMIC,
	INTEL_BACKLIGHT_LPSS,
	INTEL_BACKLIGHT_DISPLAY_DDI,
	INTEL_BACKLIGHT_DSI_DCS,
	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
};

> 
> BR,
> Jani.
> 
> 
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

I'm about to post some more review comments for the v2 version of this, but
some comments down below...

On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
> > 
> > > The problem with the EDID quirks is that exposing the quirks sticks out
> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
> > > and they affect how the EDID gets parsed, for all drivers. Obviously
> > > this could be changed, but is it the right thing to do?
> > > 
> > > What I suggested was, check the OUI only, and if it matches, do
> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
> > > you can read? Or perhaps you need to write the source OUI first, and
> > > then do that.
> > 
> > My issue isn't really with identifying the panel from EDID rather than
> > DPCD, whichever identifier is most specific is probably the best thing
> > to use. It's more that this quirk is identified in common code but only
> > applied in one driver. If this panel were ever to be attached to some
> > other source, they might well want to apply the same kind of fix. My
> > (admittedly naïve) reading of the OUI handshake process is that when
> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
> > "I'm about to issue commands that conform to _this_ vendor's own
> > conventions". If that convention communicates information that is
> > entirely contained within AUXCH transactions (and doesn't, for example,
> > require looking at some other strapping pin or external device) then in
> > principle it doesn't matter if the source device "matches" that OUI; it
> > would be legal for an AMD GPU to write the same sequence and expect the
> > same reaction, should that panel be attached to an AMD GPU.
> > 
> > So, it would be nice to know exactly what that protocol is meant to do,
> > if it applies only to this specific panel or anything else with the
> > same TCON, how one would identify such TCONs in the wild other than
> > EDID, if it relies on an external PWM or something, etc. And it might
> > make sense for now to make this a (shudder) driver-specific EDID quirk
> > rather than match by DPCD, at least until we know if the panel is ever
> > seen attached to other source devices and if the OUI convention is
> > self-contained.
> 
> Thanks for clarifying. Pretty much agreed, unfortunately also on the
> "would be nice to know more" part...
> 
> If this were to be an EDID quirk after all, I wonder if it would be
> better to store the parsed quirks to, say, struct drm_display_info, and
> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
> 
> This would also allow us to not return quirks from
> drm_add_display_info(), which would arguably clean up the interface.

Did anyone check if this is specified in the vbios? There appears to be a
field defined for this right...

enum intel_backlight_type {
	INTEL_BACKLIGHT_PMIC,
	INTEL_BACKLIGHT_LPSS,
	INTEL_BACKLIGHT_DISPLAY_DDI,
	INTEL_BACKLIGHT_DSI_DCS,
	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
};

> 
> BR,
> Jani.
> 
> 
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH v2] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

Hey! Saw your comment on the RHBZ regarding this, figured I'd take a look
since I've got some DPCD backlight related fixes for one of your other laptops
on the list as well.

On Thu, 2019-10-10 at 00:13 +0800, Lee Shawn C wrote:
> This panel (manufacturer is SDC, product ID is 0x4141)
> used manufacturer defined DPCD register to control brightness
> that not defined in eDP spec so far. This change follow panel
> vendor's instruction to support brightness adjustment.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883
> 
> V2: To check sink OUI instead of EDID quirk.
>     According to TCON's capability to decide to enable this
>     method for brightness control.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> 
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c               |  18 ++-
>  drivers/gpu/drm/i915/display/intel_dp.c       |   7 +
>  .../drm/i915/display/intel_dp_aux_backlight.c | 138 +++++++++++++++++-
>  include/drm/drm_dp_helper.h                   |  20 +++
>  4 files changed, 176 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index f373798d82f6..02f79587c337 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1268,6 +1268,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_NO_PSR) },
>  	/* CH7511 seems to leave SINK_COUNT zeroed */
>  	{ OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'),
> false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) },
> +	/* Samsung eDP panel */
> +	{ OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) },

We definitely need some more info on how this is supposed to work, since it's
not in the eDP spec and it would be hard for anyone to fix or move around code
on this upstream if upstream doesn't really know how it works.

Additionally, we really should take care to label these macros and quirks
correctly. CUSTOMIZE_BRIGHTNESS_CONTROL is pretty vague, and doesn't really
make it clear that these are non-eDP compliant displays with proprietary
backlight controls, not part of the VESA spec. If there's a name for the
backlight protocol that Samsung uses on these panels we should name it after
that, or just name it after samsung (something like
DP_DPCD_QUIRK_INTEL_BRIGHTNESS_CONTROL).

>  };
>  
>  #undef OUI
> @@ -1281,7 +1283,7 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>   * 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)
> +drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch, u8
> *tcon_cap)
>  {
>  	const struct dpcd_quirk *quirk;
>  	u32 quirks = 0;
> @@ -1301,6 +1303,11 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident
> *ident, bool is_branch)
>  		    memcmp(quirk->device_id, ident->device_id, sizeof(ident-
> >device_id)) != 0)
>  			continue;
>  
> +		if (quirk->quirks ==
> DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL &&
> +		    (!(tcon_cap[1] & DP_BRIGHTNESS_CONTROL_NITS) ||
> +		     !(tcon_cap[2] & DP_BRIGHTNESS_CONTROL_BY_AUX)))
> +				continue;
> +
>  		quirks |= quirk->quirks;
>  	}
>  
> @@ -1327,12 +1334,19 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct
> drm_dp_desc *desc,
>  	struct drm_dp_dpcd_ident *ident = &desc->ident;
>  	unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI;
>  	int ret, dev_id_len;
> +	u8 tcon_cap[4] = {0};
>  
>  	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
>  	if (ret < 0)
>  		return ret;
>  
> -	desc->quirks = drm_dp_get_quirks(ident, is_branch);
> +	if (offset == DP_SINK_OUI) {
> +		ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0,
> tcon_cap, sizeof(tcon_cap));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	desc->quirks = drm_dp_get_quirks(ident, is_branch, tcon_cap);
>  
>  	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 2b1e71f992b0..766b4b252147 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7082,6 +7082,13 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
>  		goto out_vdd_off;
>  	}
>  
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		i915_modparams.enable_dpcd_backlight = true;
> +		i915_modparams.fastboot = false;
> +		DRM_DEBUG_KMS("Using specific DPCD to control brightness\n");
> +	}
> +
mhhhh, changing kernel module parameters like this in code is probably not a
great idea. I agree with Jani as well that for something like this, we should 
I would take a look at how I implemented the quirk to work around the X1
Extreme 2nd Generation's broken vbios (which forgets to specify that the
built-in display uses standard DPCD backlight controls):

https://patchwork.freedesktop.org/patch/342166/?series=69914&rev=1

Also, we should probably just change the default for enable_dpcd_backlight to
-1 anyway, and do so in a separate patch, since Intel sounds OK with doing
that from the past discussions I've had with them. I've actually got a patch
on the mailing list that's awaiting review which does this as well:

https://patchwork.freedesktop.org/patch/342165/?series=69914&rev=1

If intel's OK with it, we probably could push this along with your patch if
yours gets reviewed before my series

>  	mutex_lock(&dev->mode_config.mutex);
>  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>  	if (edid) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 020422da2ae2..09480e772bd4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -25,6 +25,111 @@
>  #include "intel_display_types.h"
>  #include "intel_dp_aux_backlight.h"
>  
> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
> +
> +static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector
> *connector)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[2] = { 0x0 };
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> +			     &read_val, sizeof(read_val)) < 0) {
> +		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
> +			DP_EDP_BRIGHTNESS_NITS);
> +		return 0;
> +	}
> +
> +	return (read_val[1] << 8 | read_val[0]);
> +}
> +
> +static void
> +intel_dp_aux_set_customize_backlight(const struct drm_connector_state
> *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	struct intel_panel *panel = &connector->panel;
> +	uint8_t new_vals[4];
> +
> +	if (level > panel->backlight.max)
> +		level = panel->backlight.max;
> +
> +	new_vals[0] = level & 0xFF;
> +	new_vals[1] = (level & 0xFF00) >> 8;
> +	new_vals[2] = 0;
> +	new_vals[3] = 0;
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> new_vals, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
> +}
> +
> +static void intel_dp_aux_enable_customize_backlight(const struct
> intel_crtc_state *crtc_state,
> +					  const struct drm_connector_state
> *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[4], i;
> +	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00,
> 0x00};
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE,
> write_val, sizeof(write_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write panel luminance.\n");
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BRIGHTNESS_OPTIMIZATION,
> 0x01) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_BRIGHTNESS_OPTIMIZATION);

> +	/* write source OUI */
> +	write_val[0] = 0x00;
> +	write_val[1] = 0xaa;
> +	write_val[2] = 0x01;

This made me do one big double take for a moment, but I believe I understand
what it does now that I've read through the rest of this thread. We should
definitely leave a comment here though.

> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) <
> 0)
> +		DRM_DEBUG_KMS("Failed to write OUI\n");
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, 0)
> != 1)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> &read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	memset(read_val, 0x0, 4);
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to read OUI\n");
> +
> +	DRM_DEBUG_KMS("got OUI %3ph\n", read_val);
> +
> +	for (i = 0; i < 6; i++) {
> +		intel_dp_aux_set_customize_backlight(conn_state, connector-
> >panel.backlight.level);
> +
> +		msleep(60);
> +		if (intel_dp_aux_get_customize_backlight(connector))
> +			return;
> +	}
> +
> +	DRM_DEBUG_KMS("Restore brightness may have problem.\n");
Why not just "Failed to restore brightness\n"

> +}
> +
> +static void intel_dp_aux_disable_customize_backlight(const struct
> drm_connector_state *old_conn_state)
> +{
> +	// do nothing
> +	return;
> +}
> +
>  static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool
> enable)
>  {
>  	u8 reg_val = 0;
> @@ -225,6 +330,19 @@ static void intel_dp_aux_disable_backlight(const struct
> drm_connector_state *old
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state-
> >best_encoder), false);
>  }
>  
> +static int intel_dp_aux_setup_customize_backlight(struct intel_connector
> *connector,
> +					enum pipe pipe)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL;
> +	panel->backlight.min = 0;
> +	panel->backlight.level = panel->backlight.get(connector);

I can't really test this locally since I don't have this laptop, did you make
sure this part actually returns the currently set brightness? I know on the X1
Carbon Extreme 2nd Gen (which uses standard eDP compliant DPCD backlight
controls) the initial backlight value is reported as 0 when it's actually max,
and we have to check the control register to guess what the actual brightness
is:

https://patchwork.freedesktop.org/patch/342160/?series=69914&rev=1

> +	panel->backlight.enabled = panel->backlight.level != 0;
> +
> +	return 0;
> +}
> +
>  static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>  					enum pipe pipe)
>  {
> @@ -265,6 +383,7 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  	struct drm_i915_private *dev_priv = to_i915(intel_connector-
> >base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_connector->encoder-
> >base);
>  
>  	if (i915_modparams.enable_dpcd_backlight == 0 ||
>  	    (i915_modparams.enable_dpcd_backlight == -1 &&
> @@ -274,11 +393,20 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;
>  
> -	panel->backlight.setup = intel_dp_aux_setup_backlight;
> -	panel->backlight.enable = intel_dp_aux_enable_backlight;
> -	panel->backlight.disable = intel_dp_aux_disable_backlight;
> -	panel->backlight.set = intel_dp_aux_set_backlight;
> -	panel->backlight.get = intel_dp_aux_get_backlight;
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		panel->backlight.setup   =
> intel_dp_aux_setup_customize_backlight;
> +		panel->backlight.enable  =
> intel_dp_aux_enable_customize_backlight;
> +		panel->backlight.disable =
> intel_dp_aux_disable_customize_backlight;
> +		panel->backlight.set = intel_dp_aux_set_customize_backlight;
> +		panel->backlight.get = intel_dp_aux_get_customize_backlight;
> +	} else {
> +		panel->backlight.setup   = intel_dp_aux_setup_backlight;
> +		panel->backlight.enable  = intel_dp_aux_enable_backlight;
> +		panel->backlight.disable = intel_dp_aux_disable_backlight;
> +		panel->backlight.set = intel_dp_aux_set_backlight;
> +		panel->backlight.get = intel_dp_aux_get_backlight;
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index ed1a985745ba..581d1ba8435d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -740,6 +740,20 @@
>  /* up to ID_SLOT_63 at 0x2ff */
>  
>  #define DP_SOURCE_OUI			    0x300
> +#define DP_EDP_TCON_CAPABILITY_BYTE0	    0x340
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE1	    0x341
> +# define DP_BRIGHTNESS_CONTROL_NITS	    0x10
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE2	    0x342
> +# define DP_BRIGHTNESS_CONTROL_BY_AUX	    0x01
> +
> +#define DP_EDP_GETSET_CTRL_PARAMS	    0x344
> +#define DP_EDP_CONTENT_LUMINANCE	    0x346
> +#define DP_EDP_PANEL_LUMINANCE_OVERRIDE     0x34A
> +#define DP_EDP_BRIGHTNESS_NITS		    0x354
> +#define DP_EDP_BRIGHTNESS_OPTIMIZATION	    0x358
> +

These definitely need to be renamed as well. A prefix like DP_EDP_INTEL_BL or
something named after the name of this protocol would work. I'd also leave a
comment explaining what these are for

>  #define DP_SINK_OUI			    0x400
>  #define DP_BRANCH_OUI			    0x500
>  #define DP_BRANCH_ID                        0x503
> @@ -1475,6 +1489,12 @@ enum drm_dp_quirk {
>  	 * The driver should ignore SINK_COUNT during detection.
>  	 */
>  	DP_DPCD_QUIRK_NO_SINK_COUNT,
> +	/**
> +	 * @DP_DPCD_QUIRK_NON_STANDARD_BRIGHTNESS_CONTROL:
> +	 *
> +	 * The device used specific DPCD register to control brightness.
> +	 */
> +	DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL,
This should be renamed as well, and should have a comment explaining that this
is a non-standard form of brightness control over DPCD
>  };
>  
>  /**
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH v2] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

Hey! Saw your comment on the RHBZ regarding this, figured I'd take a look
since I've got some DPCD backlight related fixes for one of your other laptops
on the list as well.

On Thu, 2019-10-10 at 00:13 +0800, Lee Shawn C wrote:
> This panel (manufacturer is SDC, product ID is 0x4141)
> used manufacturer defined DPCD register to control brightness
> that not defined in eDP spec so far. This change follow panel
> vendor's instruction to support brightness adjustment.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883
> 
> V2: To check sink OUI instead of EDID quirk.
>     According to TCON's capability to decide to enable this
>     method for brightness control.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> 
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c               |  18 ++-
>  drivers/gpu/drm/i915/display/intel_dp.c       |   7 +
>  .../drm/i915/display/intel_dp_aux_backlight.c | 138 +++++++++++++++++-
>  include/drm/drm_dp_helper.h                   |  20 +++
>  4 files changed, 176 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index f373798d82f6..02f79587c337 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1268,6 +1268,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_NO_PSR) },
>  	/* CH7511 seems to leave SINK_COUNT zeroed */
>  	{ OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'),
> false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) },
> +	/* Samsung eDP panel */
> +	{ OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) },

We definitely need some more info on how this is supposed to work, since it's
not in the eDP spec and it would be hard for anyone to fix or move around code
on this upstream if upstream doesn't really know how it works.

Additionally, we really should take care to label these macros and quirks
correctly. CUSTOMIZE_BRIGHTNESS_CONTROL is pretty vague, and doesn't really
make it clear that these are non-eDP compliant displays with proprietary
backlight controls, not part of the VESA spec. If there's a name for the
backlight protocol that Samsung uses on these panels we should name it after
that, or just name it after samsung (something like
DP_DPCD_QUIRK_INTEL_BRIGHTNESS_CONTROL).

>  };
>  
>  #undef OUI
> @@ -1281,7 +1283,7 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>   * 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)
> +drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch, u8
> *tcon_cap)
>  {
>  	const struct dpcd_quirk *quirk;
>  	u32 quirks = 0;
> @@ -1301,6 +1303,11 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident
> *ident, bool is_branch)
>  		    memcmp(quirk->device_id, ident->device_id, sizeof(ident-
> >device_id)) != 0)
>  			continue;
>  
> +		if (quirk->quirks ==
> DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL &&
> +		    (!(tcon_cap[1] & DP_BRIGHTNESS_CONTROL_NITS) ||
> +		     !(tcon_cap[2] & DP_BRIGHTNESS_CONTROL_BY_AUX)))
> +				continue;
> +
>  		quirks |= quirk->quirks;
>  	}
>  
> @@ -1327,12 +1334,19 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct
> drm_dp_desc *desc,
>  	struct drm_dp_dpcd_ident *ident = &desc->ident;
>  	unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI;
>  	int ret, dev_id_len;
> +	u8 tcon_cap[4] = {0};
>  
>  	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
>  	if (ret < 0)
>  		return ret;
>  
> -	desc->quirks = drm_dp_get_quirks(ident, is_branch);
> +	if (offset == DP_SINK_OUI) {
> +		ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0,
> tcon_cap, sizeof(tcon_cap));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	desc->quirks = drm_dp_get_quirks(ident, is_branch, tcon_cap);
>  
>  	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 2b1e71f992b0..766b4b252147 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7082,6 +7082,13 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
>  		goto out_vdd_off;
>  	}
>  
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		i915_modparams.enable_dpcd_backlight = true;
> +		i915_modparams.fastboot = false;
> +		DRM_DEBUG_KMS("Using specific DPCD to control brightness\n");
> +	}
> +
mhhhh, changing kernel module parameters like this in code is probably not a
great idea. I agree with Jani as well that for something like this, we should 
I would take a look at how I implemented the quirk to work around the X1
Extreme 2nd Generation's broken vbios (which forgets to specify that the
built-in display uses standard DPCD backlight controls):

https://patchwork.freedesktop.org/patch/342166/?series=69914&rev=1

Also, we should probably just change the default for enable_dpcd_backlight to
-1 anyway, and do so in a separate patch, since Intel sounds OK with doing
that from the past discussions I've had with them. I've actually got a patch
on the mailing list that's awaiting review which does this as well:

https://patchwork.freedesktop.org/patch/342165/?series=69914&rev=1

If intel's OK with it, we probably could push this along with your patch if
yours gets reviewed before my series

>  	mutex_lock(&dev->mode_config.mutex);
>  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>  	if (edid) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 020422da2ae2..09480e772bd4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -25,6 +25,111 @@
>  #include "intel_display_types.h"
>  #include "intel_dp_aux_backlight.h"
>  
> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
> +
> +static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector
> *connector)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[2] = { 0x0 };
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> +			     &read_val, sizeof(read_val)) < 0) {
> +		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
> +			DP_EDP_BRIGHTNESS_NITS);
> +		return 0;
> +	}
> +
> +	return (read_val[1] << 8 | read_val[0]);
> +}
> +
> +static void
> +intel_dp_aux_set_customize_backlight(const struct drm_connector_state
> *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	struct intel_panel *panel = &connector->panel;
> +	uint8_t new_vals[4];
> +
> +	if (level > panel->backlight.max)
> +		level = panel->backlight.max;
> +
> +	new_vals[0] = level & 0xFF;
> +	new_vals[1] = (level & 0xFF00) >> 8;
> +	new_vals[2] = 0;
> +	new_vals[3] = 0;
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> new_vals, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
> +}
> +
> +static void intel_dp_aux_enable_customize_backlight(const struct
> intel_crtc_state *crtc_state,
> +					  const struct drm_connector_state
> *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[4], i;
> +	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00,
> 0x00};
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE,
> write_val, sizeof(write_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write panel luminance.\n");
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BRIGHTNESS_OPTIMIZATION,
> 0x01) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_BRIGHTNESS_OPTIMIZATION);

> +	/* write source OUI */
> +	write_val[0] = 0x00;
> +	write_val[1] = 0xaa;
> +	write_val[2] = 0x01;

This made me do one big double take for a moment, but I believe I understand
what it does now that I've read through the rest of this thread. We should
definitely leave a comment here though.

> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) <
> 0)
> +		DRM_DEBUG_KMS("Failed to write OUI\n");
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, 0)
> != 1)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> &read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	memset(read_val, 0x0, 4);
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to read OUI\n");
> +
> +	DRM_DEBUG_KMS("got OUI %3ph\n", read_val);
> +
> +	for (i = 0; i < 6; i++) {
> +		intel_dp_aux_set_customize_backlight(conn_state, connector-
> >panel.backlight.level);
> +
> +		msleep(60);
> +		if (intel_dp_aux_get_customize_backlight(connector))
> +			return;
> +	}
> +
> +	DRM_DEBUG_KMS("Restore brightness may have problem.\n");
Why not just "Failed to restore brightness\n"

> +}
> +
> +static void intel_dp_aux_disable_customize_backlight(const struct
> drm_connector_state *old_conn_state)
> +{
> +	// do nothing
> +	return;
> +}
> +
>  static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool
> enable)
>  {
>  	u8 reg_val = 0;
> @@ -225,6 +330,19 @@ static void intel_dp_aux_disable_backlight(const struct
> drm_connector_state *old
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state-
> >best_encoder), false);
>  }
>  
> +static int intel_dp_aux_setup_customize_backlight(struct intel_connector
> *connector,
> +					enum pipe pipe)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL;
> +	panel->backlight.min = 0;
> +	panel->backlight.level = panel->backlight.get(connector);

I can't really test this locally since I don't have this laptop, did you make
sure this part actually returns the currently set brightness? I know on the X1
Carbon Extreme 2nd Gen (which uses standard eDP compliant DPCD backlight
controls) the initial backlight value is reported as 0 when it's actually max,
and we have to check the control register to guess what the actual brightness
is:

https://patchwork.freedesktop.org/patch/342160/?series=69914&rev=1

> +	panel->backlight.enabled = panel->backlight.level != 0;
> +
> +	return 0;
> +}
> +
>  static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>  					enum pipe pipe)
>  {
> @@ -265,6 +383,7 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  	struct drm_i915_private *dev_priv = to_i915(intel_connector-
> >base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_connector->encoder-
> >base);
>  
>  	if (i915_modparams.enable_dpcd_backlight == 0 ||
>  	    (i915_modparams.enable_dpcd_backlight == -1 &&
> @@ -274,11 +393,20 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;
>  
> -	panel->backlight.setup = intel_dp_aux_setup_backlight;
> -	panel->backlight.enable = intel_dp_aux_enable_backlight;
> -	panel->backlight.disable = intel_dp_aux_disable_backlight;
> -	panel->backlight.set = intel_dp_aux_set_backlight;
> -	panel->backlight.get = intel_dp_aux_get_backlight;
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		panel->backlight.setup   =
> intel_dp_aux_setup_customize_backlight;
> +		panel->backlight.enable  =
> intel_dp_aux_enable_customize_backlight;
> +		panel->backlight.disable =
> intel_dp_aux_disable_customize_backlight;
> +		panel->backlight.set = intel_dp_aux_set_customize_backlight;
> +		panel->backlight.get = intel_dp_aux_get_customize_backlight;
> +	} else {
> +		panel->backlight.setup   = intel_dp_aux_setup_backlight;
> +		panel->backlight.enable  = intel_dp_aux_enable_backlight;
> +		panel->backlight.disable = intel_dp_aux_disable_backlight;
> +		panel->backlight.set = intel_dp_aux_set_backlight;
> +		panel->backlight.get = intel_dp_aux_get_backlight;
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index ed1a985745ba..581d1ba8435d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -740,6 +740,20 @@
>  /* up to ID_SLOT_63 at 0x2ff */
>  
>  #define DP_SOURCE_OUI			    0x300
> +#define DP_EDP_TCON_CAPABILITY_BYTE0	    0x340
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE1	    0x341
> +# define DP_BRIGHTNESS_CONTROL_NITS	    0x10
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE2	    0x342
> +# define DP_BRIGHTNESS_CONTROL_BY_AUX	    0x01
> +
> +#define DP_EDP_GETSET_CTRL_PARAMS	    0x344
> +#define DP_EDP_CONTENT_LUMINANCE	    0x346
> +#define DP_EDP_PANEL_LUMINANCE_OVERRIDE     0x34A
> +#define DP_EDP_BRIGHTNESS_NITS		    0x354
> +#define DP_EDP_BRIGHTNESS_OPTIMIZATION	    0x358
> +

These definitely need to be renamed as well. A prefix like DP_EDP_INTEL_BL or
something named after the name of this protocol would work. I'd also leave a
comment explaining what these are for

>  #define DP_SINK_OUI			    0x400
>  #define DP_BRANCH_OUI			    0x500
>  #define DP_BRANCH_ID                        0x503
> @@ -1475,6 +1489,12 @@ enum drm_dp_quirk {
>  	 * The driver should ignore SINK_COUNT during detection.
>  	 */
>  	DP_DPCD_QUIRK_NO_SINK_COUNT,
> +	/**
> +	 * @DP_DPCD_QUIRK_NON_STANDARD_BRIGHTNESS_CONTROL:
> +	 *
> +	 * The device used specific DPCD register to control brightness.
> +	 */
> +	DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL,
This should be renamed as well, and should have a comment explaining that this
is a non-standard form of brightness control over DPCD
>  };
>  
>  /**
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2] drm/i915: customize DPCD brightness control for specific panel

[Lyude Paul]

Hey! Saw your comment on the RHBZ regarding this, figured I'd take a look
since I've got some DPCD backlight related fixes for one of your other laptops
on the list as well.

On Thu, 2019-10-10 at 00:13 +0800, Lee Shawn C wrote:
> This panel (manufacturer is SDC, product ID is 0x4141)
> used manufacturer defined DPCD register to control brightness
> that not defined in eDP spec so far. This change follow panel
> vendor's instruction to support brightness adjustment.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97883
> 
> V2: To check sink OUI instead of EDID quirk.
>     According to TCON's capability to decide to enable this
>     method for brightness control.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> 
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c               |  18 ++-
>  drivers/gpu/drm/i915/display/intel_dp.c       |   7 +
>  .../drm/i915/display/intel_dp_aux_backlight.c | 138 +++++++++++++++++-
>  include/drm/drm_dp_helper.h                   |  20 +++
>  4 files changed, 176 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index f373798d82f6..02f79587c337 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1268,6 +1268,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_NO_PSR) },
>  	/* CH7511 seems to leave SINK_COUNT zeroed */
>  	{ OUI(0x00, 0x00, 0x00), DEVICE_ID('C', 'H', '7', '5', '1', '1'),
> false, BIT(DP_DPCD_QUIRK_NO_SINK_COUNT) },
> +	/* Samsung eDP panel */
> +	{ OUI(0xba, 0x41, 0x59), DEVICE_ID_ANY, false,
> BIT(DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL) },

We definitely need some more info on how this is supposed to work, since it's
not in the eDP spec and it would be hard for anyone to fix or move around code
on this upstream if upstream doesn't really know how it works.

Additionally, we really should take care to label these macros and quirks
correctly. CUSTOMIZE_BRIGHTNESS_CONTROL is pretty vague, and doesn't really
make it clear that these are non-eDP compliant displays with proprietary
backlight controls, not part of the VESA spec. If there's a name for the
backlight protocol that Samsung uses on these panels we should name it after
that, or just name it after samsung (something like
DP_DPCD_QUIRK_INTEL_BRIGHTNESS_CONTROL).

>  };
>  
>  #undef OUI
> @@ -1281,7 +1283,7 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>   * 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)
> +drm_dp_get_quirks(const struct drm_dp_dpcd_ident *ident, bool is_branch, u8
> *tcon_cap)
>  {
>  	const struct dpcd_quirk *quirk;
>  	u32 quirks = 0;
> @@ -1301,6 +1303,11 @@ drm_dp_get_quirks(const struct drm_dp_dpcd_ident
> *ident, bool is_branch)
>  		    memcmp(quirk->device_id, ident->device_id, sizeof(ident-
> >device_id)) != 0)
>  			continue;
>  
> +		if (quirk->quirks ==
> DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL &&
> +		    (!(tcon_cap[1] & DP_BRIGHTNESS_CONTROL_NITS) ||
> +		     !(tcon_cap[2] & DP_BRIGHTNESS_CONTROL_BY_AUX)))
> +				continue;
> +
>  		quirks |= quirk->quirks;
>  	}
>  
> @@ -1327,12 +1334,19 @@ int drm_dp_read_desc(struct drm_dp_aux *aux, struct
> drm_dp_desc *desc,
>  	struct drm_dp_dpcd_ident *ident = &desc->ident;
>  	unsigned int offset = is_branch ? DP_BRANCH_OUI : DP_SINK_OUI;
>  	int ret, dev_id_len;
> +	u8 tcon_cap[4] = {0};
>  
>  	ret = drm_dp_dpcd_read(aux, offset, ident, sizeof(*ident));
>  	if (ret < 0)
>  		return ret;
>  
> -	desc->quirks = drm_dp_get_quirks(ident, is_branch);
> +	if (offset == DP_SINK_OUI) {
> +		ret = drm_dp_dpcd_read(aux, DP_EDP_TCON_CAPABILITY_BYTE0,
> tcon_cap, sizeof(tcon_cap));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	desc->quirks = drm_dp_get_quirks(ident, is_branch, tcon_cap);
>  
>  	dev_id_len = strnlen(ident->device_id, sizeof(ident->device_id));
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 2b1e71f992b0..766b4b252147 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7082,6 +7082,13 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
>  		goto out_vdd_off;
>  	}
>  
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		i915_modparams.enable_dpcd_backlight = true;
> +		i915_modparams.fastboot = false;
> +		DRM_DEBUG_KMS("Using specific DPCD to control brightness\n");
> +	}
> +
mhhhh, changing kernel module parameters like this in code is probably not a
great idea. I agree with Jani as well that for something like this, we should 
I would take a look at how I implemented the quirk to work around the X1
Extreme 2nd Generation's broken vbios (which forgets to specify that the
built-in display uses standard DPCD backlight controls):

https://patchwork.freedesktop.org/patch/342166/?series=69914&rev=1

Also, we should probably just change the default for enable_dpcd_backlight to
-1 anyway, and do so in a separate patch, since Intel sounds OK with doing
that from the past discussions I've had with them. I've actually got a patch
on the mailing list that's awaiting review which does this as well:

https://patchwork.freedesktop.org/patch/342165/?series=69914&rev=1

If intel's OK with it, we probably could push this along with your patch if
yours gets reviewed before my series

>  	mutex_lock(&dev->mode_config.mutex);
>  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>  	if (edid) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 020422da2ae2..09480e772bd4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -25,6 +25,111 @@
>  #include "intel_display_types.h"
>  #include "intel_dp_aux_backlight.h"
>  
> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL	(512)
> +
> +static uint32_t intel_dp_aux_get_customize_backlight(struct intel_connector
> *connector)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[2] = { 0x0 };
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> +			     &read_val, sizeof(read_val)) < 0) {
> +		DRM_DEBUG_KMS("Failed to read DPCD register %x\n",
> +			DP_EDP_BRIGHTNESS_NITS);
> +		return 0;
> +	}
> +
> +	return (read_val[1] << 8 | read_val[0]);
> +}
> +
> +static void
> +intel_dp_aux_set_customize_backlight(const struct drm_connector_state
> *conn_state, u32 level)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	struct intel_panel *panel = &connector->panel;
> +	uint8_t new_vals[4];
> +
> +	if (level > panel->backlight.max)
> +		level = panel->backlight.max;
> +
> +	new_vals[0] = level & 0xFF;
> +	new_vals[1] = (level & 0xFF00) >> 8;
> +	new_vals[2] = 0;
> +	new_vals[3] = 0;
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_BRIGHTNESS_NITS,
> new_vals, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
> +}
> +
> +static void intel_dp_aux_enable_customize_backlight(const struct
> intel_crtc_state *crtc_state,
> +					  const struct drm_connector_state
> *conn_state)
> +{
> +	struct intel_connector *connector = to_intel_connector(conn_state-
> >connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder-
> >base);
> +	uint8_t read_val[4], i;
> +	uint8_t write_val[8] = {0x00, 0x00, 0xF0, 0x01, 0x90, 0x01, 0x00,
> 0x00};
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_PANEL_LUMINANCE_OVERRIDE,
> write_val, sizeof(write_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write panel luminance.\n");
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BRIGHTNESS_OPTIMIZATION,
> 0x01) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_BRIGHTNESS_OPTIMIZATION);

> +	/* write source OUI */
> +	write_val[0] = 0x00;
> +	write_val[1] = 0xaa;
> +	write_val[2] = 0x01;

This made me do one big double take for a moment, but I believe I understand
what it does now that I've read through the rest of this thread. We should
definitely leave a comment here though.

> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, write_val, 3) <
> 0)
> +		DRM_DEBUG_KMS("Failed to write OUI\n");
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS, 0)
> != 1)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> &read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	memset(read_val, 0x0, 4);
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_CONTENT_LUMINANCE,
> read_val, sizeof(read_val)) < 0)
> +		DRM_DEBUG_KMS("Failed to write %x\n",
> +			DP_EDP_CONTENT_LUMINANCE);
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GETSET_CTRL_PARAMS,
> read_val) != 1)
> +		DRM_DEBUG_KMS("Failed to read %x\n",
> +			DP_EDP_GETSET_CTRL_PARAMS);
> +
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SOURCE_OUI, read_val, 4) < 0)
> +		DRM_DEBUG_KMS("Failed to read OUI\n");
> +
> +	DRM_DEBUG_KMS("got OUI %3ph\n", read_val);
> +
> +	for (i = 0; i < 6; i++) {
> +		intel_dp_aux_set_customize_backlight(conn_state, connector-
> >panel.backlight.level);
> +
> +		msleep(60);
> +		if (intel_dp_aux_get_customize_backlight(connector))
> +			return;
> +	}
> +
> +	DRM_DEBUG_KMS("Restore brightness may have problem.\n");
Why not just "Failed to restore brightness\n"

> +}
> +
> +static void intel_dp_aux_disable_customize_backlight(const struct
> drm_connector_state *old_conn_state)
> +{
> +	// do nothing
> +	return;
> +}
> +
>  static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool
> enable)
>  {
>  	u8 reg_val = 0;
> @@ -225,6 +330,19 @@ static void intel_dp_aux_disable_backlight(const struct
> drm_connector_state *old
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state-
> >best_encoder), false);
>  }
>  
> +static int intel_dp_aux_setup_customize_backlight(struct intel_connector
> *connector,
> +					enum pipe pipe)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	panel->backlight.max = EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL;
> +	panel->backlight.min = 0;
> +	panel->backlight.level = panel->backlight.get(connector);

I can't really test this locally since I don't have this laptop, did you make
sure this part actually returns the currently set brightness? I know on the X1
Carbon Extreme 2nd Gen (which uses standard eDP compliant DPCD backlight
controls) the initial backlight value is reported as 0 when it's actually max,
and we have to check the control register to guess what the actual brightness
is:

https://patchwork.freedesktop.org/patch/342160/?series=69914&rev=1

> +	panel->backlight.enabled = panel->backlight.level != 0;
> +
> +	return 0;
> +}
> +
>  static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>  					enum pipe pipe)
>  {
> @@ -265,6 +383,7 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  	struct drm_i915_private *dev_priv = to_i915(intel_connector-
> >base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_connector->encoder-
> >base);
>  
>  	if (i915_modparams.enable_dpcd_backlight == 0 ||
>  	    (i915_modparams.enable_dpcd_backlight == -1 &&
> @@ -274,11 +393,20 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;
>  
> -	panel->backlight.setup = intel_dp_aux_setup_backlight;
> -	panel->backlight.enable = intel_dp_aux_enable_backlight;
> -	panel->backlight.disable = intel_dp_aux_disable_backlight;
> -	panel->backlight.set = intel_dp_aux_set_backlight;
> -	panel->backlight.get = intel_dp_aux_get_backlight;
> +	if (drm_dp_has_quirk(&intel_dp->desc,
> +	    DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL)) {
> +		panel->backlight.setup   =
> intel_dp_aux_setup_customize_backlight;
> +		panel->backlight.enable  =
> intel_dp_aux_enable_customize_backlight;
> +		panel->backlight.disable =
> intel_dp_aux_disable_customize_backlight;
> +		panel->backlight.set = intel_dp_aux_set_customize_backlight;
> +		panel->backlight.get = intel_dp_aux_get_customize_backlight;
> +	} else {
> +		panel->backlight.setup   = intel_dp_aux_setup_backlight;
> +		panel->backlight.enable  = intel_dp_aux_enable_backlight;
> +		panel->backlight.disable = intel_dp_aux_disable_backlight;
> +		panel->backlight.set = intel_dp_aux_set_backlight;
> +		panel->backlight.get = intel_dp_aux_get_backlight;
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index ed1a985745ba..581d1ba8435d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -740,6 +740,20 @@
>  /* up to ID_SLOT_63 at 0x2ff */
>  
>  #define DP_SOURCE_OUI			    0x300
> +#define DP_EDP_TCON_CAPABILITY_BYTE0	    0x340
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE1	    0x341
> +# define DP_BRIGHTNESS_CONTROL_NITS	    0x10
> +
> +#define DP_EDP_TCON_CAPABILITY_BYTE2	    0x342
> +# define DP_BRIGHTNESS_CONTROL_BY_AUX	    0x01
> +
> +#define DP_EDP_GETSET_CTRL_PARAMS	    0x344
> +#define DP_EDP_CONTENT_LUMINANCE	    0x346
> +#define DP_EDP_PANEL_LUMINANCE_OVERRIDE     0x34A
> +#define DP_EDP_BRIGHTNESS_NITS		    0x354
> +#define DP_EDP_BRIGHTNESS_OPTIMIZATION	    0x358
> +

These definitely need to be renamed as well. A prefix like DP_EDP_INTEL_BL or
something named after the name of this protocol would work. I'd also leave a
comment explaining what these are for

>  #define DP_SINK_OUI			    0x400
>  #define DP_BRANCH_OUI			    0x500
>  #define DP_BRANCH_ID                        0x503
> @@ -1475,6 +1489,12 @@ enum drm_dp_quirk {
>  	 * The driver should ignore SINK_COUNT during detection.
>  	 */
>  	DP_DPCD_QUIRK_NO_SINK_COUNT,
> +	/**
> +	 * @DP_DPCD_QUIRK_NON_STANDARD_BRIGHTNESS_CONTROL:
> +	 *
> +	 * The device used specific DPCD register to control brightness.
> +	 */
> +	DP_DPCD_QUIRK_CUSTOMIZE_BRIGHTNESS_CONTROL,
This should be renamed as well, and should have a comment explaining that this
is a non-standard form of brightness control over DPCD
>  };
>  
>  /**
-- 
Cheers,
	Lyude Paul

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

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Tue, 26 Nov 2019, Lyude Paul <lyude@redhat.com> wrote:
> I'm about to post some more review comments for the v2 version of this, but
> some comments down below...
>
> On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
>> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
>> > 
>> > > The problem with the EDID quirks is that exposing the quirks sticks out
>> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
>> > > and they affect how the EDID gets parsed, for all drivers. Obviously
>> > > this could be changed, but is it the right thing to do?
>> > > 
>> > > What I suggested was, check the OUI only, and if it matches, do
>> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
>> > > you can read? Or perhaps you need to write the source OUI first, and
>> > > then do that.
>> > 
>> > My issue isn't really with identifying the panel from EDID rather than
>> > DPCD, whichever identifier is most specific is probably the best thing
>> > to use. It's more that this quirk is identified in common code but only
>> > applied in one driver. If this panel were ever to be attached to some
>> > other source, they might well want to apply the same kind of fix. My
>> > (admittedly naïve) reading of the OUI handshake process is that when
>> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
>> > "I'm about to issue commands that conform to _this_ vendor's own
>> > conventions". If that convention communicates information that is
>> > entirely contained within AUXCH transactions (and doesn't, for example,
>> > require looking at some other strapping pin or external device) then in
>> > principle it doesn't matter if the source device "matches" that OUI; it
>> > would be legal for an AMD GPU to write the same sequence and expect the
>> > same reaction, should that panel be attached to an AMD GPU.
>> > 
>> > So, it would be nice to know exactly what that protocol is meant to do,
>> > if it applies only to this specific panel or anything else with the
>> > same TCON, how one would identify such TCONs in the wild other than
>> > EDID, if it relies on an external PWM or something, etc. And it might
>> > make sense for now to make this a (shudder) driver-specific EDID quirk
>> > rather than match by DPCD, at least until we know if the panel is ever
>> > seen attached to other source devices and if the OUI convention is
>> > self-contained.
>> 
>> Thanks for clarifying. Pretty much agreed, unfortunately also on the
>> "would be nice to know more" part...
>> 
>> If this were to be an EDID quirk after all, I wonder if it would be
>> better to store the parsed quirks to, say, struct drm_display_info, and
>> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
>> 
>> This would also allow us to not return quirks from
>> drm_add_display_info(), which would arguably clean up the interface.
>
> Did anyone check if this is specified in the vbios? There appears to be a
> field defined for this right...
>
> enum intel_backlight_type {
> 	INTEL_BACKLIGHT_PMIC,
> 	INTEL_BACKLIGHT_LPSS,
> 	INTEL_BACKLIGHT_DISPLAY_DDI,
> 	INTEL_BACKLIGHT_DSI_DCS,
> 	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
> 	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
> };

Would just need /sys/kernel/debug/dri/0/i915_vbt on the affected machine
to check.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Tue, 26 Nov 2019, Lyude Paul <lyude@redhat.com> wrote:
> I'm about to post some more review comments for the v2 version of this, but
> some comments down below...
>
> On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
>> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
>> > 
>> > > The problem with the EDID quirks is that exposing the quirks sticks out
>> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
>> > > and they affect how the EDID gets parsed, for all drivers. Obviously
>> > > this could be changed, but is it the right thing to do?
>> > > 
>> > > What I suggested was, check the OUI only, and if it matches, do
>> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
>> > > you can read? Or perhaps you need to write the source OUI first, and
>> > > then do that.
>> > 
>> > My issue isn't really with identifying the panel from EDID rather than
>> > DPCD, whichever identifier is most specific is probably the best thing
>> > to use. It's more that this quirk is identified in common code but only
>> > applied in one driver. If this panel were ever to be attached to some
>> > other source, they might well want to apply the same kind of fix. My
>> > (admittedly naïve) reading of the OUI handshake process is that when
>> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
>> > "I'm about to issue commands that conform to _this_ vendor's own
>> > conventions". If that convention communicates information that is
>> > entirely contained within AUXCH transactions (and doesn't, for example,
>> > require looking at some other strapping pin or external device) then in
>> > principle it doesn't matter if the source device "matches" that OUI; it
>> > would be legal for an AMD GPU to write the same sequence and expect the
>> > same reaction, should that panel be attached to an AMD GPU.
>> > 
>> > So, it would be nice to know exactly what that protocol is meant to do,
>> > if it applies only to this specific panel or anything else with the
>> > same TCON, how one would identify such TCONs in the wild other than
>> > EDID, if it relies on an external PWM or something, etc. And it might
>> > make sense for now to make this a (shudder) driver-specific EDID quirk
>> > rather than match by DPCD, at least until we know if the panel is ever
>> > seen attached to other source devices and if the OUI convention is
>> > self-contained.
>> 
>> Thanks for clarifying. Pretty much agreed, unfortunately also on the
>> "would be nice to know more" part...
>> 
>> If this were to be an EDID quirk after all, I wonder if it would be
>> better to store the parsed quirks to, say, struct drm_display_info, and
>> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
>> 
>> This would also allow us to not return quirks from
>> drm_add_display_info(), which would arguably clean up the interface.
>
> Did anyone check if this is specified in the vbios? There appears to be a
> field defined for this right...
>
> enum intel_backlight_type {
> 	INTEL_BACKLIGHT_PMIC,
> 	INTEL_BACKLIGHT_LPSS,
> 	INTEL_BACKLIGHT_DISPLAY_DDI,
> 	INTEL_BACKLIGHT_DSI_DCS,
> 	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
> 	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
> };

Would just need /sys/kernel/debug/dri/0/i915_vbt on the affected machine
to check.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Jani Nikula]

On Tue, 26 Nov 2019, Lyude Paul <lyude@redhat.com> wrote:
> I'm about to post some more review comments for the v2 version of this, but
> some comments down below...
>
> On Tue, 2019-10-08 at 12:19 +0300, Jani Nikula wrote:
>> On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>> > On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
>> > 
>> > > The problem with the EDID quirks is that exposing the quirks sticks out
>> > > like a sore thumb. Thus far all of it has been contained in drm_edid.c
>> > > and they affect how the EDID gets parsed, for all drivers. Obviously
>> > > this could be changed, but is it the right thing to do?
>> > > 
>> > > What I suggested was, check the OUI only, and if it matches, do
>> > > more. Perhaps there's something in the 0x300 range of DPCD offsets that
>> > > you can read? Or perhaps you need to write the source OUI first, and
>> > > then do that.
>> > 
>> > My issue isn't really with identifying the panel from EDID rather than
>> > DPCD, whichever identifier is most specific is probably the best thing
>> > to use. It's more that this quirk is identified in common code but only
>> > applied in one driver. If this panel were ever to be attached to some
>> > other source, they might well want to apply the same kind of fix. My
>> > (admittedly naïve) reading of the OUI handshake process is that when
>> > the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
>> > "I'm about to issue commands that conform to _this_ vendor's own
>> > conventions". If that convention communicates information that is
>> > entirely contained within AUXCH transactions (and doesn't, for example,
>> > require looking at some other strapping pin or external device) then in
>> > principle it doesn't matter if the source device "matches" that OUI; it
>> > would be legal for an AMD GPU to write the same sequence and expect the
>> > same reaction, should that panel be attached to an AMD GPU.
>> > 
>> > So, it would be nice to know exactly what that protocol is meant to do,
>> > if it applies only to this specific panel or anything else with the
>> > same TCON, how one would identify such TCONs in the wild other than
>> > EDID, if it relies on an external PWM or something, etc. And it might
>> > make sense for now to make this a (shudder) driver-specific EDID quirk
>> > rather than match by DPCD, at least until we know if the panel is ever
>> > seen attached to other source devices and if the OUI convention is
>> > self-contained.
>> 
>> Thanks for clarifying. Pretty much agreed, unfortunately also on the
>> "would be nice to know more" part...
>> 
>> If this were to be an EDID quirk after all, I wonder if it would be
>> better to store the parsed quirks to, say, struct drm_display_info, and
>> have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
>> 
>> This would also allow us to not return quirks from
>> drm_add_display_info(), which would arguably clean up the interface.
>
> Did anyone check if this is specified in the vbios? There appears to be a
> field defined for this right...
>
> enum intel_backlight_type {
> 	INTEL_BACKLIGHT_PMIC,
> 	INTEL_BACKLIGHT_LPSS,
> 	INTEL_BACKLIGHT_DISPLAY_DDI,
> 	INTEL_BACKLIGHT_DSI_DCS,
> 	INTEL_BACKLIGHT_PANEL_DRIVER_INTERFACE, /* <- ... over here */
> 	INTEL_BACKLIGHT_VESA_EDP_AUX_INTERFACE,
> };

Would just need /sys/kernel/debug/dri/0/i915_vbt on the affected machine
to check.

BR,
Jani.

>
>> 
>> BR,
>> Jani.
>> 
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lee, Shawn C]

[-- Attachment #1.1: Type: text/plain, Size: 3021 bytes --]

On Tue, 08 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>On Mon, 07 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>> On Mon, 2019-10-07 at 12:08 +0300, Jani Nikula wrote:
>>
>>> The problem with the EDID quirks is that exposing the quirks sticks out
>>> like a sore thumb. Thus far all of it has been contained in drm_edid.c
>>> and they affect how the EDID gets parsed, for all drivers. Obviously
>>> this could be changed, but is it the right thing to do?
>>>
>>> What I suggested was, check the OUI only, and if it matches, do
>>> more. Perhaps there's something in the 0x300 range of DPCD offsets that
>>> you can read? Or perhaps you need to write the source OUI first, and
>>> then do that.
>>
>> My issue isn't really with identifying the panel from EDID rather than
>> DPCD, whichever identifier is most specific is probably the best thing
>> to use. It's more that this quirk is identified in common code but only
>> applied in one driver. If this panel were ever to be attached to some
>> other source, they might well want to apply the same kind of fix. My
>> (admittedly naïve) reading of the OUI handshake process is that when
>> the source device writes an OUI to DP_SOURCE_OUI it is telling the sink
>> "I'm about to issue commands that conform to _this_ vendor's own
>> conventions". If that convention communicates information that is
>> entirely contained within AUXCH transactions (and doesn't, for example,
>> require looking at some other strapping pin or external device) then in
>> principle it doesn't matter if the source device "matches" that OUI; it
>> would be legal for an AMD GPU to write the same sequence and expect the
>> same reaction, should that panel be attached to an AMD GPU.
>>
>> So, it would be nice to know exactly what that protocol is meant to do,
>> if it applies only to this specific panel or anything else with the
>> same TCON, how one would identify such TCONs in the wild other than
>> EDID, if it relies on an external PWM or something, etc. And it might
>> make sense for now to make this a (shudder) driver-specific EDID quirk
>> rather than match by DPCD, at least until we know if the panel is ever
>> seen attached to other source devices and if the OUI convention is
>> self-contained.
>
>Thanks for clarifying. Pretty much agreed, unfortunately also on the
>"would be nice to know more" part...
>
>If this were to be an EDID quirk after all, I wonder if it would be
>better to store the parsed quirks to, say, struct drm_display_info, and
>have a drm_connector_has_quirk() function similar to drm_dp_has_quirk().
>
>This would also allow us to not return quirks from
>drm_add_display_info(), which would arguably clean up the interface.
>
>BR,
>Jani.

Thanks for advice! I've already update patch V2. Driver will check sink OUI
and confirm TCON's capability to decide to enable this method or not.
It depends on TCON's feature description and does not export EDID quirk.

Best regards,
Shawn


[-- Attachment #1.2: Type: text/html, Size: 7255 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: customize DPCD brightness control for specific panel

[Lee, Shawn C]

[-- Attachment #1.1: Type: text/plain, Size: 5178 bytes --]


On Mon, 07 Oct 2019, "Jani Nikula" <jani.nikula@intel.com> wrote:
>On Mon, 07 Oct 2019, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote:
>> On Fri, 04 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>>>On Fri, 04 Oct 2019, Adam Jackson <ajax@redhat.com> wrote:
>>>> On Sat, 2019-10-05 at 05:58 +0800, Lee Shawn C wrote:
>>>>> This panel (manufacturer is SDC, product ID is 0x4141) used
>>>>> manufacturer defined DPCD register to control brightness that not
>>>>> defined in eDP spec so far. This change follow panel vendor's
>>>>> instruction to support brightness adjustment.
>>>>
>>>> I'm sure this works, but this smells a little funny to me.
>>>
>>>That was kindly put. ;)
>>>
>>>>> + /* Samsung eDP panel */
>>>>> + { "SDC", 0x4141, EDID_QUIRK_NON_STD_BRIGHTNESS_CONTROL },
>>>>
>>>> It feels a bit like a layering violation to identify eDP behavior
>>>> changes based on EDID. But I'm not sure there's any obvious way to
>>>> identify this device by its DPCD. The Sink OUI (from the linked
>>>> bugzilla) seems to be 0011F8, which doesn't match up to anything in my
>>>> oui.txt...
>>>
>>>We have the DPCD quirk stuff in drm_dp_helper.c, but IIUC in this case
>>>there's only the OUI, and the device id etc. are all zeros. Otherwise I
>>>think that would be the natural thing to do, and all this could be
>>>better hidden away in i915.
>>>
>>
>> Below is what we dumped from this panel. Only sink OUI (ba-41-59) in it
>> and nothing else.
>> 00000400  ba 41 59 00 00 00 00 00  00 00 00 00 00 00 00 00  |.AY.............|
>> 00000410  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................
>>
>> That's why the patch to identify EDID's manufacturer and product ID
>> to make sure this method applied on specific panel.
>>
>>>>
>>>>> @@ -1953,6 +1956,7 @@ static u32 edid_get_quirks(const struct edid
>>>>> *edid)
>>>>>
>>>>>    return 0;
>>>>>  }
>>>>> +EXPORT_SYMBOL(edid_get_quirks);
>>>>
>>>> If we're going to export this it should probably get a drm_ prefix.
>>
>> Yes! It will be better to have drm_ prefix for export funciton.
>>
>>>>
>>>>> +#define DPCD_EDP_GETSET_CTRL_PARAMS                         0x344
>>>>> +#define DPCD_EDP_CONTENT_LUMINANCE                        0x346
>>>>> +#define DPCD_EDP_PANEL_LUMINANCE_OVERRIDE         0x34a
>>>>> +#define DPCD_EDP_BRIGHTNESS_NITS                   0x354
>>>>> +#define DPCD_EDP_BRIGHTNESS_OPTIMIZATION               0x358
>>>>> +
>>>>> +#define EDP_CUSTOMIZE_MAX_BRIGHTNESS_LEVEL         (512)
>>>>
>>>> This also seems a bit weird, the 0x300-0x3FF registers belong to the
>>>> _source_ DP device. But then later...
>>>>
>>>>> + /* write source OUI */
>>>>> + write_val[0] = 0x00;
>>>>> + write_val[1] = 0xaa;
>>>>> + write_val[2] = 0x01;
>>>>
>>>> Oh hey, you're writing (an) Intel OUI to the Source OUI, so now it
>>>> makes sense that you're writing to registers whose behavior the source
>>>> defines. But this does raise the question: is this just a convention
>>>> between Intel and this particular panel? Would we expect this to work
>>>> with other similar panels? Is there any way to know to expect this
>>>> convention from DPCD instead?
>>
>> TCON would reply on source OUI to configure its capability. And these
>> DPCD registers were defined by vendor and Intel. This change should works
>> with similar panels (with same TCON). Seems there is another issue so
>> vendor decide to use non standard way to setup brightness.
>>
>>>For one thing, it's not standard. I honestly don't know, but I'd assume
>>>you wouldn't find behaviour with Intel OUI in non-Intel designs... and a
>>>quirk of some sort seems like the only way to make this work.
>>>
>>>I suppose we could start off with a DPCD quirk that only looks at the
>>>sink OUI, and then, if needed, limit by DMI matching or by checking for
>>>some DPCD registers (what, I am not sure, perhaps write the source OUI
>>>and see how it behaves).
>>>
>>>That would avoid the mildly annoying change in the EDID quirk interface
>>>and how it's being used.
>>>
>>>Thoughts?
>>>
>>>
>>>BR,
>>>Jani.
>>>
>>
>> To be honest. Panel vendor did not provide enough sink info in DPCD.
>> That's why hard to recognize it and we have to confirm EDID instead of DPCD.
>>
>> Do you mean just confirm sink OUI only from DPCD quirk? I'm afraid it
>> may impact the other panels with the same TCON. Any suggestion?
>
>The problem with the EDID quirks is that exposing the quirks sticks out
>like a sore thumb. Thus far all of it has been contained in drm_edid.c
>and they affect how the EDID gets parsed, for all drivers. Obviously
>this could be changed, but is it the right thing to do?
>
>What I suggested was, check the OUI only, and if it matches, do
>more. Perhaps there's something in the 0x300 range of DPCD offsets that
>you can read? Or perhaps you need to write the source OUI first, and
>then do that.
>
>BR,
>Jani.
>

These bytes are RO. Seems we can used it to identify this panel
as well. I will use DPCD quirk and renew this patch again.

>
>>
>>>
>>>--
>>>Jani Nikula, Intel Open Source Graphics Center
>>>


[-- Attachment #1.2: Type: text/html, Size: 13577 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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