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

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

[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.

Only for a DP sink without downstream ports we assume it is a native DP
sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.

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
precision.

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.

v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
    the detection function, so it only applies to native DP sinks.
    Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a297e1f..7ef52db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
 		int type = connector->base.connector_type;
 		int clamp_bpp = 24;
 
-		/* Fall back to 18 bpp when DP sink capability is unknown. */
+		/* On DisplayPort try harder to find sink bpc */
 		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
-		    type == DRM_MODE_CONNECTOR_eDP)
-			clamp_bpp = 18;
+		    type == DRM_MODE_CONNECTOR_eDP) {
+			int sink_bpc = intel_dp_sink_bpc(&connector->base);
+
+			if (sink_bpc) {
+				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
+				clamp_bpp = 3 * sink_bpc;
+			}
+		}
 
 		if (bpp > clamp_bpp) {
 			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f192f58..4dbb55b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
 		}
 	}
 }
+
+/* XXX Needs work for more than 1 downstream port */
+int intel_dp_sink_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 isn't any downstream port then this is a native DP sink.
+	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
+	 * sinks which don't provide bit depth via EDID.
+	 */
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return 6;
+
+	/* Basic type of downstream ports */
+	type = 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 (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 315c971..bdc977c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1306,6 +1306,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_sink_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

Re: [PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)

[Daniel Vetter]

On Thu, May 12, 2016 at 06:43:58PM +0200, Mario Kleiner wrote:
> 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.
> 
> Only for a DP sink without downstream ports we assume it is a native DP
> sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.
> 
> 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
> precision.
> 
> 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.
> 
> v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
>     the detection function, so it only applies to native DP sinks.
>     Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a297e1f..7ef52db 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>  		int type = connector->base.connector_type;
>  		int clamp_bpp = 24;
>  
> -		/* Fall back to 18 bpp when DP sink capability is unknown. */
> +		/* On DisplayPort try harder to find sink bpc */
>  		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
> -		    type == DRM_MODE_CONNECTOR_eDP)
> -			clamp_bpp = 18;
> +		    type == DRM_MODE_CONNECTOR_eDP) {
> +			int sink_bpc = intel_dp_sink_bpc(&connector->base);
> +
> +			if (sink_bpc) {
> +				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
> +				clamp_bpp = 3 * sink_bpc;
> +			}
> +		}
>  
>  		if (bpp > clamp_bpp) {
>  			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f192f58..4dbb55b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
>  		}
>  	}
>  }
> +
> +/* XXX Needs work for more than 1 downstream port */
> +int intel_dp_sink_bpc(struct drm_connector *connector)

I think these kind of functions are pretty much why we have a dp helepr.
Can you pls move it there (and add kerneldoc and all the usual
bits&pieces). There's also other patches in-flight to add more downstream
port handling ...
-Daniel

> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	uint8_t *dpcd = intel_dp->dpcd;
> +	uint8_t type;
> +	int bpc = 0;
> +
> +	/*
> +	 * If there isn't any downstream port then this is a native DP sink.
> +	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
> +	 * sinks which don't provide bit depth via EDID.
> +	 */
> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +		return 6;
> +
> +	/* Basic type of downstream ports */
> +	type = 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 (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 315c971..bdc977c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1306,6 +1306,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_sink_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
> 

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

Re: [PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)

[Daniel Vetter]

On Thu, May 12, 2016 at 06:43:58PM +0200, Mario Kleiner wrote:
> 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.
> 
> Only for a DP sink without downstream ports we assume it is a native DP
> sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.
> 
> 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
> precision.
> 
> 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.
> 
> v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
>     the detection function, so it only applies to native DP sinks.
>     Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a297e1f..7ef52db 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>  		int type = connector->base.connector_type;
>  		int clamp_bpp = 24;
>  
> -		/* Fall back to 18 bpp when DP sink capability is unknown. */
> +		/* On DisplayPort try harder to find sink bpc */
>  		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
> -		    type == DRM_MODE_CONNECTOR_eDP)
> -			clamp_bpp = 18;
> +		    type == DRM_MODE_CONNECTOR_eDP) {
> +			int sink_bpc = intel_dp_sink_bpc(&connector->base);
> +
> +			if (sink_bpc) {
> +				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
> +				clamp_bpp = 3 * sink_bpc;
> +			}
> +		}
>  
>  		if (bpp > clamp_bpp) {
>  			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f192f58..4dbb55b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
>  		}
>  	}
>  }
> +
> +/* XXX Needs work for more than 1 downstream port */
> +int intel_dp_sink_bpc(struct drm_connector *connector)

I think these kind of functions are pretty much why we have a dp helepr.
Can you pls move it there (and add kerneldoc and all the usual
bits&pieces). There's also other patches in-flight to add more downstream
port handling ...
-Daniel

> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	uint8_t *dpcd = intel_dp->dpcd;
> +	uint8_t type;
> +	int bpc = 0;
> +
> +	/*
> +	 * If there isn't any downstream port then this is a native DP sink.
> +	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
> +	 * sinks which don't provide bit depth via EDID.
> +	 */
> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +		return 6;
> +
> +	/* Basic type of downstream ports */
> +	type = 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 (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 315c971..bdc977c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1306,6 +1306,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_sink_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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)

[Mario Kleiner]

On 05/17/2016 08:51 AM, Daniel Vetter wrote:
> On Thu, May 12, 2016 at 06:43:58PM +0200, Mario Kleiner wrote:
>> 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.
>>
>> Only for a DP sink without downstream ports we assume it is a native DP
>> sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.
>>
>> 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
>> precision.
>>
>> 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.
>>
>> v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
>>      the detection function, so it only applies to native DP sinks.
>>      Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   3 files changed, 69 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a297e1f..7ef52db 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>>   		int type = connector->base.connector_type;
>>   		int clamp_bpp = 24;
>>
>> -		/* Fall back to 18 bpp when DP sink capability is unknown. */
>> +		/* On DisplayPort try harder to find sink bpc */
>>   		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
>> -		    type == DRM_MODE_CONNECTOR_eDP)
>> -			clamp_bpp = 18;
>> +		    type == DRM_MODE_CONNECTOR_eDP) {
>> +			int sink_bpc = intel_dp_sink_bpc(&connector->base);
>> +
>> +			if (sink_bpc) {
>> +				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
>> +				clamp_bpp = 3 * sink_bpc;
>> +			}
>> +		}
>>
>>   		if (bpp > clamp_bpp) {
>>   			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index f192f58..4dbb55b 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
>>   		}
>>   	}
>>   }
>> +
>> +/* XXX Needs work for more than 1 downstream port */
>> +int intel_dp_sink_bpc(struct drm_connector *connector)
>
> I think these kind of functions are pretty much why we have a dp helepr.
> Can you pls move it there (and add kerneldoc and all the usual
> bits&pieces). There's also other patches in-flight to add more downstream
> port handling ...
> -Daniel

I can factor out the parsing bit into a dp helper function. Getting the 
dpcd block seems to be driver specific, so a little bit of stub would be 
left in the kms driver.

However, is this then still fit/small/simple enough for backporting to 
stable kernels? Fixing the existing regression which reached stable 
kernels is important. In case not, we'd need some slight extra bits a la

1. A patch which reverts Jani's commit that caused the regression -> cc 
stable.

2. My patch with the panel quirk handling for re-fixing that FDO bug 
that originally was fixed by the reverted commit -> cc stable.

3. This stuff properly split up into dp-helper etc., not for stable.

1+2 would be certainly simple enough for stable.

-mario

>
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	uint8_t *dpcd = intel_dp->dpcd;
>> +	uint8_t type;
>> +	int bpc = 0;
>> +
>> +	/*
>> +	 * If there isn't any downstream port then this is a native DP sink.
>> +	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
>> +	 * sinks which don't provide bit depth via EDID.
>> +	 */
>> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>> +		return 6;
>> +
>> +	/* Basic type of downstream ports */
>> +	type = 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 (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 315c971..bdc977c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1306,6 +1306,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_sink_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
>>
>

Re: [PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)

[Mario Kleiner]

On 05/17/2016 08:51 AM, Daniel Vetter wrote:
> On Thu, May 12, 2016 at 06:43:58PM +0200, Mario Kleiner wrote:
>> 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.
>>
>> Only for a DP sink without downstream ports we assume it is a native DP
>> sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.
>>
>> 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
>> precision.
>>
>> 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.
>>
>> v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
>>      the detection function, so it only applies to native DP sinks.
>>      Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   3 files changed, 69 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a297e1f..7ef52db 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>>   		int type = connector->base.connector_type;
>>   		int clamp_bpp = 24;
>>
>> -		/* Fall back to 18 bpp when DP sink capability is unknown. */
>> +		/* On DisplayPort try harder to find sink bpc */
>>   		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
>> -		    type == DRM_MODE_CONNECTOR_eDP)
>> -			clamp_bpp = 18;
>> +		    type == DRM_MODE_CONNECTOR_eDP) {
>> +			int sink_bpc = intel_dp_sink_bpc(&connector->base);
>> +
>> +			if (sink_bpc) {
>> +				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
>> +				clamp_bpp = 3 * sink_bpc;
>> +			}
>> +		}
>>
>>   		if (bpp > clamp_bpp) {
>>   			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index f192f58..4dbb55b 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
>>   		}
>>   	}
>>   }
>> +
>> +/* XXX Needs work for more than 1 downstream port */
>> +int intel_dp_sink_bpc(struct drm_connector *connector)
>
> I think these kind of functions are pretty much why we have a dp helepr.
> Can you pls move it there (and add kerneldoc and all the usual
> bits&pieces). There's also other patches in-flight to add more downstream
> port handling ...
> -Daniel

I can factor out the parsing bit into a dp helper function. Getting the 
dpcd block seems to be driver specific, so a little bit of stub would be 
left in the kms driver.

However, is this then still fit/small/simple enough for backporting to 
stable kernels? Fixing the existing regression which reached stable 
kernels is important. In case not, we'd need some slight extra bits a la

1. A patch which reverts Jani's commit that caused the regression -> cc 
stable.

2. My patch with the panel quirk handling for re-fixing that FDO bug 
that originally was fixed by the reverted commit -> cc stable.

3. This stuff properly split up into dp-helper etc., not for stable.

1+2 would be certainly simple enough for stable.

-mario

>
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	uint8_t *dpcd = intel_dp->dpcd;
>> +	uint8_t type;
>> +	int bpc = 0;
>> +
>> +	/*
>> +	 * If there isn't any downstream port then this is a native DP sink.
>> +	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
>> +	 * sinks which don't provide bit depth via EDID.
>> +	 */
>> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>> +		return 6;
>> +
>> +	/* Basic type of downstream ports */
>> +	type = 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 (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 315c971..bdc977c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1306,6 +1306,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_sink_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

Re: [PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)

[Jani Nikula]

On Wed, 18 May 2016, Mario Kleiner <mario.kleiner.de@gmail.com> wrote:
> On 05/17/2016 08:51 AM, Daniel Vetter wrote:
>> On Thu, May 12, 2016 at 06:43:58PM +0200, Mario Kleiner wrote:
>>> 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.
>>>
>>> Only for a DP sink without downstream ports we assume it is a native DP
>>> sink and apply the 6 bpc / 18 bpp fallback as required by the DP spec.
>>>
>>> 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
>>> precision.
>>>
>>> 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.
>>>
>>> v2: Take Ville's feedback into account: Fold the 18 bpp fallback into
>>>      the detection function, so it only applies to native DP sinks.
>>>      Rename intel_dp_legacy_bpc() to intel_dp_sink_bpc().
>>>
>>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.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      | 59 ++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>   3 files changed, 69 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index a297e1f..7ef52db 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -12072,10 +12072,16 @@ connected_sink_compute_bpp(struct intel_connector *connector,
>>>   		int type = connector->base.connector_type;
>>>   		int clamp_bpp = 24;
>>>
>>> -		/* Fall back to 18 bpp when DP sink capability is unknown. */
>>> +		/* On DisplayPort try harder to find sink bpc */
>>>   		if (type == DRM_MODE_CONNECTOR_DisplayPort ||
>>> -		    type == DRM_MODE_CONNECTOR_eDP)
>>> -			clamp_bpp = 18;
>>> +		    type == DRM_MODE_CONNECTOR_eDP) {
>>> +			int sink_bpc = intel_dp_sink_bpc(&connector->base);
>>> +
>>> +			if (sink_bpc) {
>>> +				DRM_DEBUG_KMS("DP sink with bpc %d\n", sink_bpc);
>>> +				clamp_bpp = 3 * sink_bpc;
>>> +			}
>>> +		}
>>>
>>>   		if (bpp > clamp_bpp) {
>>>   			DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of %d\n",
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index f192f58..4dbb55b 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -6046,3 +6046,62 @@ void intel_dp_mst_resume(struct drm_device *dev)
>>>   		}
>>>   	}
>>>   }
>>> +
>>> +/* XXX Needs work for more than 1 downstream port */
>>> +int intel_dp_sink_bpc(struct drm_connector *connector)
>>
>> I think these kind of functions are pretty much why we have a dp helepr.
>> Can you pls move it there (and add kerneldoc and all the usual
>> bits&pieces). There's also other patches in-flight to add more downstream
>> port handling ...
>> -Daniel
>
> I can factor out the parsing bit into a dp helper function. Getting the 
> dpcd block seems to be driver specific, so a little bit of stub would be 
> left in the kms driver.
>
> However, is this then still fit/small/simple enough for backporting to 
> stable kernels? Fixing the existing regression which reached stable 
> kernels is important. In case not, we'd need some slight extra bits a la

Even if they're small and simple enough, they'll most likely conflict
with something and the backports end up in tears. So ack on the revert
first, fix later approach.

BR,
Jani.

>
> 1. A patch which reverts Jani's commit that caused the regression -> cc 
> stable.
>
> 2. My patch with the panel quirk handling for re-fixing that FDO bug 
> that originally was fixed by the reverted commit -> cc stable.
>
> 3. This stuff properly split up into dp-helper etc., not for stable.
>
> 1+2 would be certainly simple enough for stable.
>
> -mario
>
>>
>>> +{
>>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> +	uint8_t *dpcd = intel_dp->dpcd;
>>> +	uint8_t type;
>>> +	int bpc = 0;
>>> +
>>> +	/*
>>> +	 * If there isn't any downstream port then this is a native DP sink.
>>> +	 * The standard requires to fall back to 6 bpc / 18 bpp for native DP
>>> +	 * sinks which don't provide bit depth via EDID.
>>> +	 */
>>> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>> +		return 6;
>>> +
>>> +	/* Basic type of downstream ports */
>>> +	type = 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 (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 315c971..bdc977c 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1306,6 +1306,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_sink_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

-- 
Jani Nikula, Intel Open Source Technology Center