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

Thu Aug 29 09:09:21 UTC 2019

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.

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
>