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

[Daniel Vetter <daniel@ffwll.ch>]

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