On Wed, 28 Aug 2019, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> wrote: >> >> > From: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> >> >> > >> >> > 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@xxxxxxxxx> >> >> > CC: Manasi Navare <manasi.d.navare@xxxxxxxxx> >> >> > --- >> >> > 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@xxxxxxxxx > > 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@xxxxxxxxxxxxxxxxxxxxx >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
