EDID/DP color precision fixes on Intel hw for stable

EDID/DP color precision fixes on Intel hw for stable

[Mario Kleiner]

Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331

received a potential fix that was backported to stable. While that
patch itself is correct for treating DP video sinks with "unknown
color depth", it uncovered some lack in our general EDID 1.3
handling, and in how we treat DP->DVI/VGA, causing the fall back
of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
back. That leads to unhappy neuroscience/medical users of Intel gpus
which need their DP->DVI or DP->VGA display devices to operate at at
least 8 bpc without dithering.

The following three patches try to improve our EDID handling and
Intel DP to try harder to detect the proper bpc to avoid these
regressions for DP-DVI and DP-VGA. The third patch tries to fix
FDO bug 105331 without causing general unhappiness of other users.

thanks,
-mario

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

[PATCH 1/3] drm/edid: Set 8 bpc color depth for displays with "DFP 1.x compliant TMDS".

[Mario Kleiner]

According to E-EDID spec 1.3, table 3.9, a digital video sink with the
"DFP 1.x compliant TMDS" bit set is "signal compatible with VESA DFP 1.x
TMDS CRGB, 1 pixel / clock, up to 8 bits / color MSB aligned".

For such displays, the DFP spec 1.0, section 3.10 "EDID support" says:

"If the DFP monitor only supports EDID 1.X (1.1, 1.2, etc.)
 without extensions, the host will make the following assumptions:

 1. 24-bit MSB-aligned RGB TFT
 2. DE polarity is active high
 3. H and V syncs are active high
 4. Established CRT timings will be used
 5. Dithering will not be enabled on the host"

So if we don't know the bit depth of the display from additional
colorimetry info we should assume 8 bpc / 24 bpp with no dithering
by default.

This patch adds info->bpc = 8 assignement for that case.

Now the DVI 1.0 spec (section 2.2.11.2 "Monitor data format support")
mandates that "...If the monitor implements the EDID 2.0, 1.3 or newer
data structure the monitor may specify any data format that is definable
within the EDID data structure used. In all cases the monitor must support
the 24-bit MSB aligned RGB TFT data format as a minimum."

So any DVI display with EDID 1.3 should also have at least info->bpc = 8,
even if the "DFP 1.x compliant" bit isn't set, but in our EDID handling
we don't know if the EDID comes from a DVI display or something else. I
therefore don't know if it is safe to always assume 8 bpc for digital
inputs?

Most of my tested DVI sinks set the DFP bit, but one tested Dell panel
doesn't.

Lack of handling this correctly was exposed by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown")
which assumes that the sink capability is unknown if our edid handling
reports 0 bpc.

As we return bpc = 0 for such displays, that patch will cause a
degradation of output precison to 6 bpc for such displays on Intel hw
if they are connected via an active DP->dual-link DVI converter and
thereby treated as Displayport.

As that patch was backported to stable we should include this one
also in stable to fix the regression in color depth for such panels.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_edid.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 414d7f6..ff28815 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3810,8 +3810,12 @@ static void drm_add_display_info(struct edid *edid,
 	if (edid->revision < 3)
 		return;
 
-	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
+	if (!(edid->input & DRM_EDID_INPUT_DIGITAL)) {
+		/* Analog sinks = infinite bpc, but driver decides */
+		DRM_DEBUG("%s: Assigning analog sink color depth as %d bpc.\n",
+			  connector->name, info->bpc);
 		return;
+	}
 
 	/* Get data from CEA blocks if present */
 	edid_ext = drm_find_cea_extension(edid);
@@ -3829,6 +3833,31 @@ static void drm_add_display_info(struct edid *edid,
 	/* HDMI deep color modes supported? Assign to info, if so */
 	drm_assign_hdmi_deep_color_info(edid, info, connector);
 
+	/*
+	 * Digital sink with "DFP 1.x compliant TMDS" according to EDID 1.3?
+	 *
+	 * For such displays, the DFP spec 1.0, section 3.10 "EDID support"
+	 * says:
+	 *
+	 * "If the DFP monitor only supports EDID 1.X (1.1, 1.2, etc.)
+	 * without extensions, the host will make the following assumptions:
+	 *
+	 * 1. 24-bit MSB-aligned RGB TFT
+	 * 2. DE polarity is active high
+	 * 3. H and V syncs are active high
+	 * 4. Established CRT timings will be used
+	 * 5. Dithering will not be enabled on the host"
+	 *
+	 * So we use 8 bpc in this case and "no dithering" will hopefully
+	 * follow.
+	 */
+	if ((info->bpc == 0) && (edid->revision < 4) &&
+	    (edid->input & DRM_EDID_DIGITAL_TYPE_DVI)) {
+		info->bpc = 8;
+		DRM_DEBUG("%s: Assigning DFP/DVI sink color depth as %d bpc.\n",
+			  connector->name, info->bpc);
+	}
+
 	/* Only defined for 1.4 with digital displays */
 	if (edid->revision < 4)
 		return;
-- 
2.7.0

[PATCH 2/3] drm/i915/dp: Try to find proper bpc for DP->legacy converters.

[Mario Kleiner]

This fixes a regression in output precision for DVI and VGA
video sinks connected to Intel hw via active DisplayPort->DVI/VGA
converters.

The regression was indirectly introduced by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown").

Our current drm edid 1.3 handling can't reliably assign a proper
minimum supported display depth of 8 bpc to all DVI sinks, as
mandated by DVI 1.0 spec, section 2.2.11.2 "Monitor data format
support", but returns 0 bpc = "Don't know" instead. For analog VGA
sinks it also returns 0 bpc, although those sinks themselves have
infinite color depth, only restricted by the DAC resolution of
the encoder.

If a VGA or dual-link DVI display is connected via DisplayPort
connector then due to above commit the driver would fall back to
only 6 bpc, which would cause degradation for DVI and VGA displays,
annoying in general, but especially harmful for application of display
devices used in neuroscience research and for medical diagnosic
which absolutely need native non-dithered 8 bpc at a minimum to
operate correctly.

For DP connectors with bpc == 0 according to EDID, fix this problem
by checking the dpcd data to find out if a DP->legacy converter
is connected. If the converter is DP->DVI/HDMI assume 8 bpc
depth. If the converter is DP->VGA assume at least 8 bpc, but
try to get a more accurate value (8, 10, 12 or 16 bpc) if the
converter exposes this info.

As the "fall back to 18 bpp" patch was backported to stable we should
include this one also into stable to fix the regression in color depth
for such panels.

Tested with MiniDP->DP adapter, MiniDP->HDMI adapter,
MiniDP->single-link DVI adapter, MiniDP->dual-link DVI active adapter,
and Apple MiniDP->VGA active adapter.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++++
 drivers/gpu/drm/i915/intel_dp.c      | 56 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e0d828..9903949 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12018,12 +12018,24 @@ connected_sink_compute_bpp(struct intel_connector *connector,
 	if (connector->base.display_info.bpc == 0) {
 		int type = connector->base.connector_type;
 		int clamp_bpp = 24;
+		int legacy_bpc = 0;
 
 		/* Fall back to 18 bpp when DP sink capability is unknown. */
 		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
 		    type == DRM_MODE_CONNECTOR_eDP)
 			clamp_bpp = 18;
 
+		/* On DP to legacy converter, try harder to find sink bpc */
+		if (type == DRM_MODE_CONNECTOR_DisplayPort) {
+			legacy_bpc = intel_dp_legacy_bpc(&connector->base);
+
+			if (legacy_bpc) {
+				DRM_DEBUG_KMS("DP to VGA/DVI/HDMI converter with bpc %d\n",
+					      legacy_bpc);
+				clamp_bpp = 3 * legacy_bpc;
+			}
+		}
+
 		if (bpp > clamp_bpp) {
 			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
 				      bpp, clamp_bpp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f069a82..963a8f8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6117,3 +6117,59 @@ void intel_dp_mst_resume(struct drm_device *dev)
 		}
 	}
 }
+
+/* XXX Needs work for more than 1 downstream port */
+int intel_dp_legacy_bpc(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	uint8_t *dpcd = intel_dp->dpcd;
+	uint8_t type;
+	int bpc = 0;
+
+	/* if there's no downstream port, then this can't be DP->legacy */
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return bpc;
+
+	/* Basic type of downstream ports */
+	type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+		DP_DWN_STRM_PORT_TYPE_MASK;
+
+	/*
+	 * Lacking other info, 8 bpc is a reasonable start for analog out.
+	 * E.g., Apple MiniDP->VGA adaptors don't provide more info than
+	 * that. Despite having DP_DPCD_REV == 0x11 their downstream_ports
+	 * descriptor is empty - all zeros.
+	 */
+	if (type == DP_DWN_STRM_PORT_TYPE_ANALOG)
+		bpc = 8;
+
+	if (intel_dp->dpcd[DP_DPCD_REV] < 0x11)
+		return bpc;
+
+	/* Rev 1.1+. More specific downstream port type available */
+	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
+
+	/* VGA, DVI and HDMI support at least 8 bpc */
+	if (type == DP_DS_PORT_TYPE_VGA || type == DP_DS_PORT_TYPE_DVI ||
+	    type == DP_DS_PORT_TYPE_HDMI)
+		bpc = 8;
+
+	/* As of DP interop v1.1a only VGA defines additional detail */
+	if (type != DP_DS_PORT_TYPE_VGA ||
+	    !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE))
+		return bpc;
+
+	/* VGA with DP_DETAILED_CAP_INFO_AVAILABLE provides bpc info */
+	switch (intel_dp->downstream_ports[2] & DP_DS_VGA_MAX_BPC_MASK) {
+	case DP_DS_VGA_8BPC:
+		return 8;
+	case DP_DS_VGA_10BPC:
+		return 10;
+	case DP_DS_VGA_12BPC:
+		return 12;
+	case DP_DS_VGA_16BPC:
+		return 16;
+	}
+
+	return bpc;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4c027d6..6692788 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1253,6 +1253,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector);
 void intel_dp_mst_suspend(struct drm_device *dev);
 void intel_dp_mst_resume(struct drm_device *dev);
+int intel_dp_legacy_bpc(struct drm_connector *connector);
 int intel_dp_max_link_rate(struct intel_dp *intel_dp);
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate);
 void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
-- 
2.7.0

[PATCH 3/3] drm/edid: Add 6 bpc quirk for display AEO model 0.

[Mario Kleiner]

Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
reports that the "AEO model 0" display is driven with 8 bpc
without dithering by default, which looks bad because that
panel is apparently a 6 bpc panel.

A fix for this was made by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown").

However, that commit, while correct in itself, as a side effect
triggers new regressions in precision for DisplayPort->DVI and
DP->VGA displays. Patches are out to fix those regressions, but
they will revert video output for the AEO model 0 panel to 8 bpc
without dithering.

The EDID 1.3 of that panel, as decoded from the xrandr output
attached to that bugzilla bug report, is somewhat faulty, and beyond
other problems also sets the "DFP 1.x compliant TMDS" bit, which
according to DFP spec means to drive the panel with 8 bpc and
no dithering in absence of other colorimetry information.

Try to make the original bug reporter happy despite the
faulty EDID by adding a quirk to mark that panel as 6 bpc,
so 6 bpc output with dithering creates a nice picture.

As the original patch was backported to stable and the
fixes for regressions caused by that patch are aimed
at stable, this patch should also go to stable to
make everybody happy again.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_edid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ff28815..7d10537 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -74,6 +74,8 @@
 #define EDID_QUIRK_FORCE_8BPC			(1 << 8)
 /* Force 12bpc */
 #define EDID_QUIRK_FORCE_12BPC			(1 << 9)
+/* Force 6bpc */
+#define EDID_QUIRK_FORCE_6BPC			(1 << 10)
 
 struct detailed_mode_closure {
 	struct drm_connector *connector;
@@ -100,6 +102,9 @@ static struct edid_quirk {
 	/* Unknown Acer */
 	{ "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
 
+	/* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
+	{ "AEO", 0, EDID_QUIRK_FORCE_6BPC },
+
 	/* Belinea 10 15 55 */
 	{ "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
 	{ "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
@@ -3950,6 +3955,9 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 
 	drm_add_display_info(edid, &connector->display_info, connector);
 
+	if (quirks & EDID_QUIRK_FORCE_6BPC)
+		connector->display_info.bpc = 6;
+
 	if (quirks & EDID_QUIRK_FORCE_8BPC)
 		connector->display_info.bpc = 8;
 
-- 
2.7.0

Re: EDID/DP color precision fixes on Intel hw for stable

[Mario Kleiner]

Ping? Could somebody give this a review? Would be good to get the 
associated regressions in DVI/VGA output precision fixed in intel
hw, also for stable kernels which regressed.

Resending the three patches...

thanks,
-mario

On 03/28/2016 01:52 AM, Mario Kleiner wrote:
> Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
>
> received a potential fix that was backported to stable. While that
> patch itself is correct for treating DP video sinks with "unknown
> color depth", it uncovered some lack in our general EDID 1.3
> handling, and in how we treat DP->DVI/VGA, causing the fall back
> of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
> back. That leads to unhappy neuroscience/medical users of Intel gpus
> which need their DP->DVI or DP->VGA display devices to operate at at
> least 8 bpc without dithering.
>
> The following three patches try to improve our EDID handling and
> Intel DP to try harder to detect the proper bpc to avoid these
> regressions for DP-DVI and DP-VGA. The third patch tries to fix
> FDO bug 105331 without causing general unhappiness of other users.
>
> thanks,
> -mario
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Fwd: [PATCH 1/3] drm/edid: Set 8 bpc color depth for displays with "DFP 1.x compliant TMDS".

[Mario Kleiner]

-------- Forwarded Message --------
Subject: [PATCH 1/3] drm/edid: Set 8 bpc color depth for displays with 
"DFP 1.x compliant TMDS".
Date: Mon, 28 Mar 2016 01:52:45 +0200
From: Mario Kleiner <mario.kleiner.de@gmail.com>
To: dri-devel@lists.freedesktop.org
CC: mario.kleiner.de@gmail.com, Jani Nikula <jani.nikula@intel.com>, 
Ville Syrjälä <ville.syrjala@linux.intel.com>, stable@vger.kernel.org

According to E-EDID spec 1.3, table 3.9, a digital video sink with the
"DFP 1.x compliant TMDS" bit set is "signal compatible with VESA DFP 1.x
TMDS CRGB, 1 pixel / clock, up to 8 bits / color MSB aligned".

For such displays, the DFP spec 1.0, section 3.10 "EDID support" says:

"If the DFP monitor only supports EDID 1.X (1.1, 1.2, etc.)
  without extensions, the host will make the following assumptions:

  1. 24-bit MSB-aligned RGB TFT
  2. DE polarity is active high
  3. H and V syncs are active high
  4. Established CRT timings will be used
  5. Dithering will not be enabled on the host"

So if we don't know the bit depth of the display from additional
colorimetry info we should assume 8 bpc / 24 bpp with no dithering
by default.

This patch adds info->bpc = 8 assignement for that case.

Now the DVI 1.0 spec (section 2.2.11.2 "Monitor data format support")
mandates that "...If the monitor implements the EDID 2.0, 1.3 or newer
data structure the monitor may specify any data format that is definable
within the EDID data structure used. In all cases the monitor must support
the 24-bit MSB aligned RGB TFT data format as a minimum."

So any DVI display with EDID 1.3 should also have at least info->bpc = 8,
even if the "DFP 1.x compliant" bit isn't set, but in our EDID handling
we don't know if the EDID comes from a DVI display or something else. I
therefore don't know if it is safe to always assume 8 bpc for digital
inputs?

Most of my tested DVI sinks set the DFP bit, but one tested Dell panel
doesn't.

Lack of handling this correctly was exposed by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown")
which assumes that the sink capability is unknown if our edid handling
reports 0 bpc.

As we return bpc = 0 for such displays, that patch will cause a
degradation of output precison to 6 bpc for such displays on Intel hw
if they are connected via an active DP->dual-link DVI converter and
thereby treated as Displayport.

As that patch was backported to stable we should include this one
also in stable to fix the regression in color depth for such panels.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
  drivers/gpu/drm/drm_edid.c | 31 ++++++++++++++++++++++++++++++-
  1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 414d7f6..ff28815 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3810,8 +3810,12 @@ static void drm_add_display_info(struct edid *edid,
  	if (edid->revision < 3)
  		return;

-	if (!(edid->input & DRM_EDID_INPUT_DIGITAL))
+	if (!(edid->input & DRM_EDID_INPUT_DIGITAL)) {
+		/* Analog sinks = infinite bpc, but driver decides */
+		DRM_DEBUG("%s: Assigning analog sink color depth as %d bpc.\n",
+			  connector->name, info->bpc);
  		return;
+	}

  	/* Get data from CEA blocks if present */
  	edid_ext = drm_find_cea_extension(edid);
@@ -3829,6 +3833,31 @@ static void drm_add_display_info(struct edid *edid,
  	/* HDMI deep color modes supported? Assign to info, if so */
  	drm_assign_hdmi_deep_color_info(edid, info, connector);

+	/*
+	 * Digital sink with "DFP 1.x compliant TMDS" according to EDID 1.3?
+	 *
+	 * For such displays, the DFP spec 1.0, section 3.10 "EDID support"
+	 * says:
+	 *
+	 * "If the DFP monitor only supports EDID 1.X (1.1, 1.2, etc.)
+	 * without extensions, the host will make the following assumptions:
+	 *
+	 * 1. 24-bit MSB-aligned RGB TFT
+	 * 2. DE polarity is active high
+	 * 3. H and V syncs are active high
+	 * 4. Established CRT timings will be used
+	 * 5. Dithering will not be enabled on the host"
+	 *
+	 * So we use 8 bpc in this case and "no dithering" will hopefully
+	 * follow.
+	 */
+	if ((info->bpc == 0) && (edid->revision < 4) &&
+	    (edid->input & DRM_EDID_DIGITAL_TYPE_DVI)) {
+		info->bpc = 8;
+		DRM_DEBUG("%s: Assigning DFP/DVI sink color depth as %d bpc.\n",
+			  connector->name, info->bpc);
+	}
+
  	/* Only defined for 1.4 with digital displays */
  	if (edid->revision < 4)
  		return;
-- 
2.7.0



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

Fwd: [PATCH 2/3] drm/i915/dp: Try to find proper bpc for DP->legacy converters.

[Mario Kleiner]

-------- Forwarded Message --------
Subject: [PATCH 2/3] drm/i915/dp: Try to find proper bpc for DP->legacy 
converters.
Date: Mon, 28 Mar 2016 01:52:46 +0200
From: Mario Kleiner <mario.kleiner.de@gmail.com>
To: dri-devel@lists.freedesktop.org
CC: mario.kleiner.de@gmail.com, Jani Nikula <jani.nikula@intel.com>, 
Ville Syrjälä <ville.syrjala@linux.intel.com>, Daniel Vetter 
<daniel.vetter@ffwll.ch>, stable@vger.kernel.org

This fixes a regression in output precision for DVI and VGA
video sinks connected to Intel hw via active DisplayPort->DVI/VGA
converters.

The regression was indirectly introduced by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown").

Our current drm edid 1.3 handling can't reliably assign a proper
minimum supported display depth of 8 bpc to all DVI sinks, as
mandated by DVI 1.0 spec, section 2.2.11.2 "Monitor data format
support", but returns 0 bpc = "Don't know" instead. For analog VGA
sinks it also returns 0 bpc, although those sinks themselves have
infinite color depth, only restricted by the DAC resolution of
the encoder.

If a VGA or dual-link DVI display is connected via DisplayPort
connector then due to above commit the driver would fall back to
only 6 bpc, which would cause degradation for DVI and VGA displays,
annoying in general, but especially harmful for application of display
devices used in neuroscience research and for medical diagnosic
which absolutely need native non-dithered 8 bpc at a minimum to
operate correctly.

For DP connectors with bpc == 0 according to EDID, fix this problem
by checking the dpcd data to find out if a DP->legacy converter
is connected. If the converter is DP->DVI/HDMI assume 8 bpc
depth. If the converter is DP->VGA assume at least 8 bpc, but
try to get a more accurate value (8, 10, 12 or 16 bpc) if the
converter exposes this info.

As the "fall back to 18 bpp" patch was backported to stable we should
include this one also into stable to fix the regression in color depth
for such panels.

Tested with MiniDP->DP adapter, MiniDP->HDMI adapter,
MiniDP->single-link DVI adapter, MiniDP->dual-link DVI active adapter,
and Apple MiniDP->VGA active adapter.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++
  drivers/gpu/drm/i915/intel_dp.c      | 56 
++++++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h     |  1 +
  3 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 6e0d828..9903949 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12018,12 +12018,24 @@ connected_sink_compute_bpp(struct 
intel_connector *connector,
  	if (connector->base.display_info.bpc == 0) {
  		int type = connector->base.connector_type;
  		int clamp_bpp = 24;
+		int legacy_bpc = 0;

  		/* Fall back to 18 bpp when DP sink capability is unknown. */
  		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
  		    type == DRM_MODE_CONNECTOR_eDP)
  			clamp_bpp = 18;

+		/* On DP to legacy converter, try harder to find sink bpc */
+		if (type == DRM_MODE_CONNECTOR_DisplayPort) {
+			legacy_bpc = intel_dp_legacy_bpc(&connector->base);
+
+			if (legacy_bpc) {
+				DRM_DEBUG_KMS("DP to VGA/DVI/HDMI converter with bpc %d\n",
+					      legacy_bpc);
+				clamp_bpp = 3 * legacy_bpc;
+			}
+		}
+
  		if (bpp > clamp_bpp) {
  			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
  				      bpp, clamp_bpp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c 
b/drivers/gpu/drm/i915/intel_dp.c
index f069a82..963a8f8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6117,3 +6117,59 @@ void intel_dp_mst_resume(struct drm_device *dev)
  		}
  	}
  }
+
+/* XXX Needs work for more than 1 downstream port */
+int intel_dp_legacy_bpc(struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	uint8_t *dpcd = intel_dp->dpcd;
+	uint8_t type;
+	int bpc = 0;
+
+	/* if there's no downstream port, then this can't be DP->legacy */
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return bpc;
+
+	/* Basic type of downstream ports */
+	type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+		DP_DWN_STRM_PORT_TYPE_MASK;
+
+	/*
+	 * Lacking other info, 8 bpc is a reasonable start for analog out.
+	 * E.g., Apple MiniDP->VGA adaptors don't provide more info than
+	 * that. Despite having DP_DPCD_REV == 0x11 their downstream_ports
+	 * descriptor is empty - all zeros.
+	 */
+	if (type == DP_DWN_STRM_PORT_TYPE_ANALOG)
+		bpc = 8;
+
+	if (intel_dp->dpcd[DP_DPCD_REV] < 0x11)
+		return bpc;
+
+	/* Rev 1.1+. More specific downstream port type available */
+	type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
+
+	/* VGA, DVI and HDMI support at least 8 bpc */
+	if (type == DP_DS_PORT_TYPE_VGA || type == DP_DS_PORT_TYPE_DVI ||
+	    type == DP_DS_PORT_TYPE_HDMI)
+		bpc = 8;
+
+	/* As of DP interop v1.1a only VGA defines additional detail */
+	if (type != DP_DS_PORT_TYPE_VGA ||
+	    !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE))
+		return bpc;
+
+	/* VGA with DP_DETAILED_CAP_INFO_AVAILABLE provides bpc info */
+	switch (intel_dp->downstream_ports[2] & DP_DS_VGA_MAX_BPC_MASK) {
+	case DP_DS_VGA_8BPC:
+		return 8;
+	case DP_DS_VGA_10BPC:
+		return 10;
+	case DP_DS_VGA_12BPC:
+		return 12;
+	case DP_DS_VGA_16BPC:
+		return 16;
+	}
+
+	return bpc;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h 
b/drivers/gpu/drm/i915/intel_drv.h
index 4c027d6..6692788 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1253,6 +1253,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
  void intel_dp_add_properties(struct intel_dp *intel_dp, struct 
drm_connector *connector);
  void intel_dp_mst_suspend(struct drm_device *dev);
  void intel_dp_mst_resume(struct drm_device *dev);
+int intel_dp_legacy_bpc(struct drm_connector *connector);
  int intel_dp_max_link_rate(struct intel_dp *intel_dp);
  int intel_dp_rate_select(struct intel_dp *intel_dp, int rate);
  void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
-- 
2.7.0



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

Fwd: [PATCH 3/3] drm/edid: Add 6 bpc quirk for display AEO model 0.

[Mario Kleiner]

-------- Forwarded Message --------
Subject: [PATCH 3/3] drm/edid: Add 6 bpc quirk for display AEO model 0.
Date: Mon, 28 Mar 2016 01:52:47 +0200
From: Mario Kleiner <mario.kleiner.de@gmail.com>
To: dri-devel@lists.freedesktop.org
CC: mario.kleiner.de@gmail.com, Jani Nikula <jani.nikula@intel.com>, 
Ville Syrjälä <ville.syrjala@linux.intel.com>, Daniel Vetter 
<daniel.vetter@ffwll.ch>, stable@vger.kernel.org

Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
reports that the "AEO model 0" display is driven with 8 bpc
without dithering by default, which looks bad because that
panel is apparently a 6 bpc panel.

A fix for this was made by commit 013dd9e03872
("drm/i915/dp: fall back to 18 bpp when sink capability is unknown").

However, that commit, while correct in itself, as a side effect
triggers new regressions in precision for DisplayPort->DVI and
DP->VGA displays. Patches are out to fix those regressions, but
they will revert video output for the AEO model 0 panel to 8 bpc
without dithering.

The EDID 1.3 of that panel, as decoded from the xrandr output
attached to that bugzilla bug report, is somewhat faulty, and beyond
other problems also sets the "DFP 1.x compliant TMDS" bit, which
according to DFP spec means to drive the panel with 8 bpc and
no dithering in absence of other colorimetry information.

Try to make the original bug reporter happy despite the
faulty EDID by adding a quirk to mark that panel as 6 bpc,
so 6 bpc output with dithering creates a nice picture.

As the original patch was backported to stable and the
fixes for regressions caused by that patch are aimed
at stable, this patch should also go to stable to
make everybody happy again.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
  drivers/gpu/drm/drm_edid.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ff28815..7d10537 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -74,6 +74,8 @@
  #define EDID_QUIRK_FORCE_8BPC			(1 << 8)
  /* Force 12bpc */
  #define EDID_QUIRK_FORCE_12BPC			(1 << 9)
+/* Force 6bpc */
+#define EDID_QUIRK_FORCE_6BPC			(1 << 10)

  struct detailed_mode_closure {
  	struct drm_connector *connector;
@@ -100,6 +102,9 @@ static struct edid_quirk {
  	/* Unknown Acer */
  	{ "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },

+	/* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
+	{ "AEO", 0, EDID_QUIRK_FORCE_6BPC },
+
  	/* Belinea 10 15 55 */
  	{ "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
  	{ "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
@@ -3950,6 +3955,9 @@ int drm_add_edid_modes(struct drm_connector 
*connector, struct edid *edid)

  	drm_add_display_info(edid, &connector->display_info, connector);

+	if (quirks & EDID_QUIRK_FORCE_6BPC)
+		connector->display_info.bpc = 6;
+
  	if (quirks & EDID_QUIRK_FORCE_8BPC)
  		connector->display_info.bpc = 8;

-- 
2.7.0



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

Re: EDID/DP color precision fixes on Intel hw for stable

[Ville Syrjälä]

On Mon, Mar 28, 2016 at 01:52:44AM +0200, Mario Kleiner wrote:
> Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
> 
> received a potential fix that was backported to stable. While that
> patch itself is correct for treating DP video sinks with "unknown
> color depth", it uncovered some lack in our general EDID 1.3
> handling, and in how we treat DP->DVI/VGA, causing the fall back
> of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
> back. That leads to unhappy neuroscience/medical users of Intel gpus
> which need their DP->DVI or DP->VGA display devices to operate at at
> least 8 bpc without dithering.
> 
> The following three patches try to improve our EDID handling and
> Intel DP to try harder to detect the proper bpc to avoid these
> regressions for DP-DVI and DP-VGA. The third patch tries to fix
> FDO bug 105331 without causing general unhappiness of other users.

It would seem simpler to me to move the 18bpp fallback into intel_dp.c
and only do it for native DP sinks/downstream ports. That way we should
avoid the need for any EDID quirks IIUC.

The downstream port caps we should still check I suppose. Later
versions of the spec extend the information for pretty much all port
types. I started to write something similar [1] a while back, and by the
looks of things I was probably basing that on the DP 1.2 spec since 1.3
has even more stuff there. Anyways we should put that logic into the
DP helper so that other drivers migth use it as well.

[1] git://github.com/vsyrjala/linux.git dp_downstream_ports

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: EDID/DP color precision fixes on Intel hw for stable

[Mario Kleiner]

On 05/06/2016 08:27 PM, Ville Syrjälä wrote:
> On Mon, Mar 28, 2016 at 01:52:44AM +0200, Mario Kleiner wrote:
>> Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
>>
>> received a potential fix that was backported to stable. While that
>> patch itself is correct for treating DP video sinks with "unknown
>> color depth", it uncovered some lack in our general EDID 1.3
>> handling, and in how we treat DP->DVI/VGA, causing the fall back
>> of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
>> back. That leads to unhappy neuroscience/medical users of Intel gpus
>> which need their DP->DVI or DP->VGA display devices to operate at at
>> least 8 bpc without dithering.
>>
>> The following three patches try to improve our EDID handling and
>> Intel DP to try harder to detect the proper bpc to avoid these
>> regressions for DP-DVI and DP-VGA. The third patch tries to fix
>> FDO bug 105331 without causing general unhappiness of other users.
>

Thanks for the feedback.

> It would seem simpler to me to move the 18bpp fallback into intel_dp.c
> and only do it for native DP sinks/downstream ports. That way we should
> avoid the need for any EDID quirks IIUC.
>

I think that specific EDID quirk in patch 3/3 for that FDO bug we'd 
always need, because that specific panels EDID reports 8 bpc capability 
by setting the "DFP 1.x compliant TMDS" bit in its EDID 1.3, but 
according to the FDO bug it needs to be driven with 6 bpc + dithering 
for good results.

Do you agree with patch 1/3? That would avoid kms drivers needing to 
work out DFP compliant displays.I think we could probably make the 
assumption that anything that has EDID 1.3 is 8 bpc capable? DVI spec 
seems to require that for anything DVI, and  i'd assume any VGA DAC 
manufactured in the last 20 years would have at least 8 bpc?

Wrt. 18 bpp fallback you mean putting it into intel_dp_legacy_bpc() from 
patch 2/3 or similar and checking that the sink is really not an active 
DVI or VGA converter?

I tried to keep these patches relatively simple/conservative to allow 
safe backporting to stable kernels that are affected by the regression.

> The downstream port caps we should still check I suppose. Later
> versions of the spec extend the information for pretty much all port
> types. I started to write something similar [1] a while back, and by the
> looks of things I was probably basing that on the DP 1.2 spec since 1.3
> has even more stuff there. Anyways we should put that logic into the
> DP helper so that other drivers migth use it as well.
>
> [1] git://github.com/vsyrjala/linux.git dp_downstream_ports
>

Have to look at that. I don't have official access to the latest specs, 
just to whatever i could find floating in the internet.

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

Re: EDID/DP color precision fixes on Intel hw for stable

[Ville Syrjälä]

On Fri, May 06, 2016 at 10:03:06PM +0200, Mario Kleiner wrote:
> On 05/06/2016 08:27 PM, Ville Syrjälä wrote:
> > On Mon, Mar 28, 2016 at 01:52:44AM +0200, Mario Kleiner wrote:
> >> Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
> >>
> >> received a potential fix that was backported to stable. While that
> >> patch itself is correct for treating DP video sinks with "unknown
> >> color depth", it uncovered some lack in our general EDID 1.3
> >> handling, and in how we treat DP->DVI/VGA, causing the fall back
> >> of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
> >> back. That leads to unhappy neuroscience/medical users of Intel gpus
> >> which need their DP->DVI or DP->VGA display devices to operate at at
> >> least 8 bpc without dithering.
> >>
> >> The following three patches try to improve our EDID handling and
> >> Intel DP to try harder to detect the proper bpc to avoid these
> >> regressions for DP-DVI and DP-VGA. The third patch tries to fix
> >> FDO bug 105331 without causing general unhappiness of other users.
> >
> 
> Thanks for the feedback.
> 
> > It would seem simpler to me to move the 18bpp fallback into intel_dp.c
> > and only do it for native DP sinks/downstream ports. That way we should
> > avoid the need for any EDID quirks IIUC.
> >
> 
> I think that specific EDID quirk in patch 3/3 for that FDO bug we'd 
> always need, because that specific panels EDID reports 8 bpc capability 
> by setting the "DFP 1.x compliant TMDS" bit in its EDID 1.3, but 
> according to the FDO bug it needs to be driven with 6 bpc + dithering 
> for good results.

If we just do the fallback for DP, then I don;t think we need any
changes to the EDID parser, and hence no quirk either.

> 
> Do you agree with patch 1/3? That would avoid kms drivers needing to 
> work out DFP compliant displays.I think we could probably make the 
> assumption that anything that has EDID 1.3 is 8 bpc capable? DVI spec 
> seems to require that for anything DVI, and  i'd assume any VGA DAC 
> manufactured in the last 20 years would have at least 8 bpc?
> 
> Wrt. 18 bpp fallback you mean putting it into intel_dp_legacy_bpc() from 
> patch 2/3 or similar and checking that the sink is really not an active 
> DVI or VGA converter?

Yeah, I'd just check the downstream port type, and do the 18bpp fallback
only for native DP if the sink didn't give us a bpc. For everything else
8bpc seems like a reasonable default (unless the port caps say otherwise,
of course).

> 
> I tried to keep these patches relatively simple/conservative to allow 
> safe backporting to stable kernels that are affected by the regression.
> 
> > The downstream port caps we should still check I suppose. Later
> > versions of the spec extend the information for pretty much all port
> > types. I started to write something similar [1] a while back, and by the
> > looks of things I was probably basing that on the DP 1.2 spec since 1.3
> > has even more stuff there. Anyways we should put that logic into the
> > DP helper so that other drivers migth use it as well.
> >
> > [1] git://github.com/vsyrjala/linux.git dp_downstream_ports
> >
> 
> Have to look at that. I don't have official access to the latest specs, 
> just to whatever i could find floating in the internet.
> 
> thanks,
> -mario

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: EDID/DP color precision fixes on Intel hw for stable

[Mario Kleiner]

On 05/07/2016 08:15 PM, Ville Syrjälä wrote:
> On Fri, May 06, 2016 at 10:03:06PM +0200, Mario Kleiner wrote:
>> On 05/06/2016 08:27 PM, Ville Syrjälä wrote:
>>> On Mon, Mar 28, 2016 at 01:52:44AM +0200, Mario Kleiner wrote:
>>>> Bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=105331
>>>>
>>>> received a potential fix that was backported to stable. While that
>>>> patch itself is correct for treating DP video sinks with "unknown
>>>> color depth", it uncovered some lack in our general EDID 1.3
>>>> handling, and in how we treat DP->DVI/VGA, causing the fall back
>>>> of Intel DP to 6 bpc / 18 bpp in cases where it shouldn't fall
>>>> back. That leads to unhappy neuroscience/medical users of Intel gpus
>>>> which need their DP->DVI or DP->VGA display devices to operate at at
>>>> least 8 bpc without dithering.
>>>>
>>>> The following three patches try to improve our EDID handling and
>>>> Intel DP to try harder to detect the proper bpc to avoid these
>>>> regressions for DP-DVI and DP-VGA. The third patch tries to fix
>>>> FDO bug 105331 without causing general unhappiness of other users.
>>>
>>
>> Thanks for the feedback.
>>
>>> It would seem simpler to me to move the 18bpp fallback into intel_dp.c
>>> and only do it for native DP sinks/downstream ports. That way we should
>>> avoid the need for any EDID quirks IIUC.
>>>
>>
>> I think that specific EDID quirk in patch 3/3 for that FDO bug we'd
>> always need, because that specific panels EDID reports 8 bpc capability
>> by setting the "DFP 1.x compliant TMDS" bit in its EDID 1.3, but
>> according to the FDO bug it needs to be driven with 6 bpc + dithering
>> for good results.
>
> If we just do the fallback for DP, then I don;t think we need any
> changes to the EDID parser, and hence no quirk either.
>
>>

You are probably right. Rereading the bug report it seems to be a native 
DP panel without EDID color info, so the 6 bpc fallback only for native 
DP sinks should handle that.

I just sent out a new patch, also for stable, which folds the 6 bpc 
fallback into my DP bpc detection code.

I wonder if it would still make sense to apply the DFP 1.x bit handling 
in the shared EDID parser, just so we take full advantage of what info a 
EDID 1.3 provides? Although then we'd need a quirk for that panel again, 
as it does have a somewhat broken EDID.

thanks,
-mario

>> Do you agree with patch 1/3? That would avoid kms drivers needing to
>> work out DFP compliant displays.I think we could probably make the
>> assumption that anything that has EDID 1.3 is 8 bpc capable? DVI spec
>> seems to require that for anything DVI, and  i'd assume any VGA DAC
>> manufactured in the last 20 years would have at least 8 bpc?
>>
>> Wrt. 18 bpp fallback you mean putting it into intel_dp_legacy_bpc() from
>> patch 2/3 or similar and checking that the sink is really not an active
>> DVI or VGA converter?
>
> Yeah, I'd just check the downstream port type, and do the 18bpp fallback
> only for native DP if the sink didn't give us a bpc. For everything else
> 8bpc seems like a reasonable default (unless the port caps say otherwise,
> of course).
>
>>
>> I tried to keep these patches relatively simple/conservative to allow
>> safe backporting to stable kernels that are affected by the regression.
>>
>>> The downstream port caps we should still check I suppose. Later
>>> versions of the spec extend the information for pretty much all port
>>> types. I started to write something similar [1] a while back, and by the
>>> looks of things I was probably basing that on the DP 1.2 spec since 1.3
>>> has even more stuff there. Anyways we should put that logic into the
>>> DP helper so that other drivers migth use it as well.
>>>
>>> [1] git://github.com/vsyrjala/linux.git dp_downstream_ports
>>>
>>
>> Have to look at that. I don't have official access to the latest specs,
>> just to whatever i could find floating in the internet.
>>
>> thanks,
>> -mario
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel