[PATCH] drm/i915/dp: Try to find proper bpc for DP->legacy converters. (v2)
Mario Kleiner
mario.kleiner.de at gmail.com
Wed May 18 13:52:04 UTC 2016
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
>>
>
More information about the dri-devel
mailing list