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@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html