[Intel-gfx] [PATCH] drm/i915: Prune 2560x2880 mode for 5K tiled dual DP monitors

[Jani Nikula]

On Tue, 27 Aug 2019, Manasi Navare <manasi.d.navare at intel.com> wrote:
> On Tue, Aug 27, 2019 at 01:34:15PM +0300, Jani Nikula wrote:
>> On Tue, 27 Aug 2019, "Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
>> > From: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>> >
>> > Currently, the transcoder port sync feature is not available, due to
>> > which the 5K-tiled dual DP monitors experience corruption when
>> > 2560x2880 mode is applied for both of the tiled DP connectors.
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97244
>> >
>> > There is a patch series to enable transcode port sync feature for
>> > tiled display for ICL+, which is under review:
>> > https://patchwork.kernel.org/project/intel-gfx/list/?series=137339
>> >
>> > For the older platforms, we need to remove the 2560x2880 mode to avoid
>> > a possibility of userspace choosing 2560x2880 mode for both tiled
>> > displays, resulting in corruption.
>> >
>> > This patch prunes 2560x2880 mode for one of the tiled DP connector.
>> > Since both the tiled DP connectors have different tile_h_loc and
>> > tile_v_loc, the tiled connector with tile_h_loc and tile_v_loc as '0',
>> > is chosen, for which the given resolution is removed.
>> >
>> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>> > CC: Manasi Navare <manasi.d.navare at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 5c45a3b..aa43a3b 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -564,6 +564,17 @@ intel_dp_mode_valid(struct drm_connector *connector,
>> >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> >  		return MODE_H_ILLEGAL;
>> >  
>> > +	/*
>> > +	 * For 5K tiled dual DP monitors, dual-DP sync is not yet supported.
>> > +	 * This results in display sync issues, when both tiled connectors run
>> > +	 * on 2560x2880 resolution. Therefore prune the 2560x2880 mode on one
>> > +	 * of the tiled connector, to avoid such a case.
>> > +	 */
>> > +	if (connector->has_tile &&
>> > +	    (connector->tile_h_loc == 0 && connector->tile_v_loc == 0) &&
>> > +	    (mode->hdisplay == 2560 && mode->vdisplay == 2880))
>> > +		return MODE_PANEL;
>> > +
>> 
>> This assumes all tiled cases with specific resolutions fail. You don't
>> know that. You only know this fails on a specific display. Instead of
>> coming up with various rules on tiles and resolutions that match the
>> display (but might *also* match any number of *other* displays!), you
>> need to actually identify and match that specific display instead.
>>
>
> Actually without the transcoder port sync feature, we do not expect
> any tiled display over two separate ports to work correctly, so if it
> is two connectors in state with tile props set then we should reject
> the tiled mode on both those connectors since that might cause the
> artifacts without proper sync between two ports which is supported
> only on ICL+

Consider a multi-screen display with independent panels mounted
together, and EDIDs set up to describe the physical tiling
layout. Should we reject them all because the cases you know about fail?

You know about the issues with the specific 5k displays precisely
because they fail. You never hear about the ones that work. Ever. Until
they stop working, that is.

>> There are two ways to add display specific quirks: based on EDID
>> (edid_quirk_list in drm_edid.c) and based on DPCD (dpcd_quirk_list in
>> drm_dp_helper.c). You identify the display, and then prune the modes
>> that require port sync to work, for *that* display.
>
> We have seen this issue on multiple 5K tiled displays IMH, so just
> adding a quirk for specific monitors will not suffice.

Adding one quirk per failing display quite obviously will suffice.

> But we would need to make sure that the mode gets rejected only if
> there are multiple SST connectors with tile prop or
> connector->has_tile set because MST tiled displays still work
> correctly.
>
> Ville, you had played a little bit with this 5K display I believe, do
> you think pruning the tiled mode if there are tiled SST connectors and
> platform < ICL is a good solution?

Come to think of it, can you use the tiled mode *untiled* on one port,
and have it strech the entire display? There are plenty of other modes
you can use like this. I don't think we should reject that use case
either.

I'll repeat, you have issues with a very specific case. You need to have
*very* specific rules to filter them out in order to not inadvertently
filter out valid use cases. Remember, if there's just *one* valid use
case that you end up rejecting here, it's a regression and you need to
revert and get back to the drawing board.

---

Finally, and perhaps most importantly, there are people on the bug that
are going to be rather underwhelmed that after three years they get a
patch that simply rejects the very mode that was the reason for buying
the display they have. Insult to injury, the real fix is for a platform
that didn't exist when they bought the displays.


BR,
Jani.



>
> Regards
> Manasi
>> 
>> Blanket filters like this are a no-go.
>> 
>> BR,
>> Jani.
>> 
>> 
>> >  	return MODE_OK;
>> >  }
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center