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

Thu Aug 29 06:44:50 UTC 2019

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

>
> 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