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 at gmail.com> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> >> Cc: stable at 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 >> >