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

Thu Aug 29 18:36:46 UTC 2019

On Thu, Aug 29, 2019 at 02:36:18PM +0300, Jani Nikula wrote:
> On Thu, 29 Aug 2019, "Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
> > Hi Jani, Manasi,
> >
> > Thanks for the comments and suggestions. Please find my response inline.
> >
> > On 8/29/2019 12:14 PM, Jani Nikula wrote:
> >> On Wed, 28 Aug 2019, Manasi Navare <manasi.d.navare at intel.com> wrote:
> >>> Thanks Jani for your feedback, please see my comments inline
> >>>
> >>> On Wed, Aug 28, 2019 at 10:46:44AM +0300, Jani Nikula wrote:
> >>>> 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.
> >>>
> >>> Hmm I think even with separate panels to work without artifacts we would need some kind of
> >>> synchronization. But yes I agree that it might just be working well and we cant assume
> >>> that they are failing.
> >>>
> >>> So for now the EDID quirk sounds like the best way to fix this FDO
> >>>
> >>>>
> >>>>>> 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.
> >>>
> >>> Yes so in that case the quirk would be to set the has_tile to false so that
> >>> the driver will actually see it as non tiled and scale it to the entire display
> >>>
> >>>>
> >>>> 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.
> >>>
> >>> I agree completely. Ankit could you test it on the 5K screen what happens if
> >>> you set the has_tile to false and allow it to stretch out in untiled fashion?
> >>> If that works we can add that to the quirk.
> >>
> >> I'm probably missing something here.
> >>
> >> Ankit lists the modes for DP-2 in [1], and among them is
> >> 2560x2880. How's that different from using, say, 3840x2160?
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> [1] http://mid.mail-archive.com/54c6c2c1-d95e-3bb4-50dd-1efff6bed7dd@intel.com
> >>
> >>
> >
> > The issue is seen only when the mode 2560x2880 is set for both of the 
> > connectors. So if we see any other combination, say 2560x2880 on DP-1, 
> > 3840x2160 on DP-2, one of the mode will cover the entire screen and 
> > there is no corruption observed. This is true for all combinations other 
> > than the (2560x2880,2560x2880) combination.
> 
> Right, so my point was, if 2560x2880 is usable when used on one
> connector only, it's not really proper to filter that out from the
> modes.

Hi Jani,

So the has_tile or the TILE property is set per connector irrespective of the mode.
But the way userspace handles it IMO is that if TILE prop is set and the mode matches
the tile_h_size and tile_v_size only then it will do two atomic modesets on two connectors
Otherwise it will just be one connector modeset.

So for eg: 2560 x 2880 would be the size of each tile so for that it is doing modesets
on two tiles and thats where we see the corruption issue for the complete total fb
of 5120 x 2880.
But for 3840 x 2160, that modeset just happens on a single connector and we dont see
any issue.

So i think what the quirk shd do is for this specific panel, we would not be able to
display tiled mode together of 5120 x 2880 correctly due to corruption, so the resolution
of 2560 x 2880 should not appear in the modelist for that connector or we just set has_tile
to false so userspace will not try to do the tiled display magic and do a modeset on
single connector.

Ankit, could you please email me the dmesg logs in:
1. Case where you dont apply this patch, what happens with 2 connectors in 5K tiled mode
2. You force has_tile to false in the code to see the behaviour and logs in non tiled case
3. Prune the 2560 x 2880 mode and collect logs.

I think looking at these logs will give us a clear picture and we can finalize the proper fix

Regards
Manasi

> 
> BR,
> Jani.
> 
> 
> >
> > I am not sure but it seems like, the monitor when receives the 2560x2880 
> > modes on both connectors, at that time the dual-dp comes to play and the 
> > corruption occurs. (I had tried to set the mode using the Ubuntu Display 
> > settings.)
> >
> > I had tried with Dell UP2715K monitor, I can try to get other tiled 5k 
> > monitors and check the issue without X-server on.
> >
> > If its Panel specific issue, its better to add quirk as suggested.
> >
> > Thanks & Regards,
> > Ankit
> >
> >>
> >>
> >>>
> >>> Manasi
> >>>
> >>>>
> >>>>
> >>>> 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
> >>
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center