From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 30EC9C2D0C3 for ; Sat, 28 Dec 2019 00:23:36 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0B88920828 for ; Sat, 28 Dec 2019 00:23:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0B88920828 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 02C0E6E0F1; Sat, 28 Dec 2019 00:23:35 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2B3D06E0E0 for ; Sat, 28 Dec 2019 00:23:33 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Dec 2019 16:23:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,364,1571727600"; d="scan'208";a="243365221" Received: from mdroper-desk1.fm.intel.com (HELO mdroper-desk1.amr.corp.intel.com) ([10.1.27.64]) by fmsmga004.fm.intel.com with ESMTP; 27 Dec 2019 16:23:32 -0800 Date: Fri, 27 Dec 2019 16:23:32 -0800 From: Matt Roper To: Manasi Navare Message-ID: <20191228002332.GQ2877816@mdroper-desk1.amr.corp.intel.com> References: <20191223224429.5151-1-manasi.d.navare@intel.com> <20191227183652.GJ2877816@mdroper-desk1.amr.corp.intel.com> <20191227192750.GA26435@intel.com> <20191227200333.GM2877816@mdroper-desk1.amr.corp.intel.com> <20191227225128.GA27178@intel.com> <20191227231707.GN2877816@mdroper-desk1.amr.corp.intel.com> <20191227234915.GB27178@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191227234915.GB27178@intel.com> User-Agent: Mutt/1.12.1 (2019-06-15) Subject: Re: [Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, Dec 27, 2019 at 03:49:15PM -0800, Manasi Navare wrote: > On Fri, Dec 27, 2019 at 03:17:07PM -0800, Matt Roper wrote: > > On Fri, Dec 27, 2019 at 02:51:28PM -0800, Manasi Navare wrote: > > > On Fri, Dec 27, 2019 at 12:03:33PM -0800, Matt Roper wrote: > > > > On Fri, Dec 27, 2019 at 11:27:50AM -0800, Manasi Navare wrote: > > > > > On Fri, Dec 27, 2019 at 10:36:52AM -0800, Matt Roper wrote: > > > > > > On Mon, Dec 23, 2019 at 02:44:27PM -0800, Manasi Navare wrote: > > > > > > > In case of tiled displays, all the tiles are linke dto each o= ther > > > > > > > for transcoder port sync. So in intel_atomic_check() we need = to make > > > > > > > sure that we add all the tiles to the modeset and if one of t= he > > > > > > > tiles needs a full modeset then mark all other tiles for a fu= ll modeset. > > > > > > > = > > > > > > > We also need to force modeset for all synced crtcs after fast= set check. > > > > > > > = > > > > > > > v5: > > > > > > > * Rebase > > > > > > = > > > > > > I sent a reply to your v4 of this right at the same time you se= nt out > > > > > > v5, but I'm not sure if my reply went through since it doesn't = show up > > > > > > in patchwork. I've included the same feedback I gave on v4 bel= ow in > > > > > > case it got lost, plus a few more comments. > > > > > > = > > > > > > > v4: > > > > > > > * Fix logic for modeset_synced_crtcs (Ville) > > > > > > > v3: > > > > > > > * Add tile checks only for Gen >11 > > > > > > > v2: > > > > > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > > > > > * Use intel_connector_needs_modeset() (Ville) > > > > > > > * Add modeset_synced_crtcs (Ville) > > > > > > > * Make sure synced crtcs are forced full modeset > > > > > > > after fastset check (Ville) > > > > > > > = > > > > > > > Suggested-by: Ville Syrj=E4l=E4 > > > > > > > Cc: Ville Syrj=E4l=E4 > > > > > > > Cc: Jos=E9 Roberto de Souza > > > > > > > Cc: Matt Roper > > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > > > > > Signed-off-by: Manasi Navare > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 123 +++++++++= ++++++++++ > > > > > > > 1 file changed, 123 insertions(+) > > > > > > > = > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/d= rivers/gpu/drm/i915/display/intel_display.c > > > > > > > index 94fc4b5bacc0..45a699bac34a 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > @@ -14304,6 +14304,118 @@ static bool intel_cpu_transcoder_ne= eds_modeset(struct intel_atomic_state *state, > > > > > > > return false; > > > > > > > } > > > > > > > = > > > > > > > +static void > > > > > > > +intel_modeset_synced_crtcs(struct intel_atomic_state *state, > > > > > > > + u8 transcoders) > > > > > > > +{ > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > + struct intel_crtc *crtc; > > > > > > > + int i; > > > > > > > + > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > + new_crtc_state, i) { > > > > > > = > > > > > > Are we guaranteed to have the other CRTC in the state at this p= oint? If > > > > > > the tile information was gone at the beginning of the transacti= on, then > > > > > > it wouldn't have known to bring in the other CRTC. So if a mod= eset was > > > > > > only submitted on one of the two synchronized CRTCs then I don'= t see > > > > > > where the other one would be added to the transaction before th= is point? > > > > > > > > > > > = > > > > > Yes basically at this point the new crtc state is not cleared so = we use the = > > > > > previously synced crtcs through new crtc state master slave assig= nments from previous > > > > > modeset. > > > > = > > > > I'm not sure we're talking about the same thing here. Initially an > > > > atomic state (transaction) is empty, and all mode objects have a > > > > currently-committed state, but no new state associated with this > > > > specific transaction. If userspace requests a modeset on CRTC-A (e= .g., > > > > master), that will add CRTC-A to the drm_atomic_state, but won't > > > > automatically add CRTC-B (slave). CRTC B's committed state does in= deed > > > > indicate that it's part of a port-sync pair, but it won't get itera= ted > > > > over in the loop above because it still hasn't been added with an > > > > explicit drm_atomic_get_crtc_state(). So we'll never set mode_chan= ged > > > > on CRTC-B as far as I can see because nothing ever brought it into = the > > > > transaction (the only place where we bring in new CRTCs in this pat= ch is > > > > when handling the tiles). > > > > > > > = > > > Ah okay I understand your concern now. So if you see the first call > > > intel_atomic_check_tiled_conns(), that one loops through all the > > > connectors not ust the ones in the state and adds all connectors with > > > tiles to the modeset. If we unplug a conn an connect a new monitor > > > that is untiled, the conn state goes from conn to unconn and that > > > should triggera full modeset even though the tile info does not exist. > > = > > Right, intel_atomic_check_tiled_conns loops through all connectors, so > > it can pick stuff up that isn't already part of our atomic transaction. > > However it won't know it needs to pick up the partner connector in cases > > where we've already lost the tile information for the connector that was > > re-plugged. > > = > > For example, let's say we have connector-A/crtc-A (tile grp 1) and > > connector-B/crtc-B (tile grp 1). The current state for both indicates > > they're synced and the tile information for both is still present. > > = > > - Now we unplug connector-B and plug in a new monitor that's either > > untiled or part of a different tile group. The committed state still > > shows both as being synchronized, but the tile information for > > connector-B has now vanished. > > = > > - Userspace reacts to the hotplug event and performs a modeset on > > connector-B. > > * the initial atomic state contains only crtc-B / connector-B since > > userspace didn't see a need to update A. > > * intel_atomic_check_tiled_conns loops over connectors that are > > already in the state, so it only processes connector-B. It eith= er > > sees !connector->has_tile and never calls intel_modeset_all_tiles > > _or_ it sees a completely different tile group ID that won't > > match against connector-A. > > = > > So just looking at tiles isn't enough to bring in the missing partner in > > cases where the tile information has vanished before our commit begins; > > i.e., that's why we need to also look at the current port sync status to > > find old partners that we're still sync'd against and bring them into > > the transaction as well. > > > = > Yes this above example makes complete sense. I think now the DRM patches = make sure that > if we lose one of the tiles through hotplug it also fallsback the mode on= remaining conn > so conn A here to a lower non tiled mode so I see that it also gets added= to the state > with mode changed and the other disable due to conn B unplug goes through= disable sequence > and disables all associated master/slaves as well. I'm not sure what the drm fbcon hotplug policy is without looking, but that's only only one potential client sending us atomic transactions in reaction to hotplugs. If you have a userspace DRM master running, then nothing will happen automatically in the kernel/drm except what userspace tells us to do. Some compositors might recognize that you did something to one tile and add both tiles to the next transaction they submit, but other compositors may not be so smart. > = > But from what you are saying is it would be safe to check if any of the c= onns in state that needs a modeset is involved in port sync > and then loop through all drm connectors and force modeset on all other = conns in the same port sync? > So kind of like the intel_modeset_synced_crtcs() right after modeset_all_= tiles() ? > = > But may be for now, add a FIXME for this case and land this patch to make= sure that it fixes > the current hotplug/unplug issues? Yeah, it sounds like what you have here does fix the most immediate bugs, even if there are some corner cases that still aren't covered. So I'm okay landing what we have with a FIXME so that we unblock some usage while we keep working on the fixes for other corner cases. Matt > = > Regards > Manasi > = > > > = > > > Now this modeset_synced_crtcs() is called after new master slave > > > assignments are done and the only purpose of this call is to make sure > > > that we dont override the full modeset with fastset and that if one > > > either master or slave needs full modeset other one also shd be forced > > > modeset. > > = > > Ah, so if modeset_synced_crtcs() is only intended to prevent problematic > > demotions to fastset, then it seems like the loop just above your call > > that Jose just added for the MST stuff is already intended for that > > purpose. Maybe we can build your intel_atomic_check_synced_crtcs() into > > that existing loop rather than handling port sync as an independent > > loop? I'm fine with doing that as part of a follow-up patch though so > > we don't delay getting these fixes landed. > > = > > But that means we still need some processing of synced CRTC's up at the > > top of the function around the same place (and with the same general > > approach) as where you handle the tiled connectors. I.e., something > > that will notice a pre-existing port sync on the old state, then loop > > over all crtcs (not just the ones already in the state) to bring in the > > old partner. > > = > > > = > > > This patch as of now just fixes the current hotplug issues, more > > > enhancements to cover other corner cases with get_Connector overriding > > > the tile info etc will be taken care of later as per discussions with > > > Ville > > = > > Okay, if this is a known shortcoming that's still planned for the > > future, then I guess we can just add a FIXME comment or something for > > now so it's clear why some cases still aren't expected to work properly. > > = > > > = > > > > > = > > > > > = > > > > > > > + if (transcoders & BIT(new_crtc_state->cpu_transcoder)) { > > > > > > > + new_crtc_state->uapi.mode_changed =3D true; > > > > > > > + new_crtc_state->update_pipe =3D false; > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > +static void > > > > > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *s= tate) > > > > > > > +{ > > > > > > > + struct drm_i915_private *dev_priv =3D to_i915(state->base.d= ev); > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > + struct intel_crtc *crtc; > > > > > > > + int i; > > > > > > > + > > > > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > > > > + return; > > > > > > > + > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > + new_crtc_state, i) { > > > > > > > + if (is_trans_port_sync_master(new_crtc_state) && > > > > > > > + needs_modeset(new_crtc_state)) { > > > > > > > + intel_modeset_synced_crtcs(state, > > > > > > > + new_crtc_state->sync_mode_slaves_mask); > > > > > > > + } else if (is_trans_port_sync_slave(new_crtc_state) && > > > > > > > + needs_modeset(new_crtc_state)) { > > > > > > > + intel_modeset_synced_crtcs(state, > > > > > > > + BIT(new_crtc_state->master_transcoder)); > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > +static int > > > > > > > +intel_modeset_all_tiles(struct intel_atomic_state *state, in= t tile_grp_id) > > > > > > > +{ > > > > > > > + struct drm_i915_private *dev_priv =3D to_i915(state->base.d= ev); > > > > > > > + struct drm_connector *connector; > > > > > > > + struct drm_connector_list_iter conn_iter; > > > > > > > + int ret =3D 0; > > > > > > > + > > > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > > > > + drm_for_each_connector_iter(connector, &conn_iter) { > > > > > > > + struct drm_connector_state *conn_state; > > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > > + > > > > > > > + if (!connector->has_tile || > > > > > > > + connector->tile_group->id !=3D tile_grp_id) > > > > > > > + continue; > > > > > > > + conn_state =3D drm_atomic_get_connector_state(&state->base, > > > > > > > + connector); > > > > > > > + if (IS_ERR(conn_state)) { > > > > > > > + ret =3D PTR_ERR(conn_state); > > > > > > > + break; > > > > > > > + } > > > > > > > + > > > > > > > + if (!conn_state->crtc) > > > > > > > + continue; > > > > > > > + > > > > > > > + crtc_state =3D drm_atomic_get_crtc_state(&state->base, > > > > > > > + conn_state->crtc); > > > > > > > + if (IS_ERR(crtc_state)) { > > > > > > > + ret =3D PTR_ERR(conn_state); > > > > > > > + break; > > > > > > > + } > > > > > > > + crtc_state->mode_changed =3D true; > > > > > > > + ret =3D drm_atomic_add_affected_connectors(&state->base, > > > > > > > + conn_state->crtc); > > > > > > > + if (ret) > > > > > > > + break; > > > > > > > + } > > > > > > > + drm_connector_list_iter_end(&conn_iter); > > > > > > > + > > > > > > > + return ret; > > > > > > > +} > > > > > > = > > > > > > There isn't really anything i915-specific in this function and = it feels > > > > > > like something other drivers may need as well if they treat til= ed > > > > > > monitors in a similar manner. We may want to consider pulling = this out > > > > > > to a DRM core helper, although we can do that in a future patch. > > > > > > = > > > > > > With a bit of extra work, the function below could also potenti= ally be > > > > > > moved to the core too. Do you know if there's other hardware w= ith port > > > > > > sync capabilities that could benefit from these? > > > > > > > > > > > = > > > > > Yes definitely can eventually make this a drm helper even though = the whole master slave > > > > > thing is i915 specific for now > > > > > = > > > > > > > + > > > > > > > +static int > > > > > > > +intel_atomic_check_tiled_conns(struct intel_atomic_state *st= ate) > > > > > > > +{ > > > > > > > + struct drm_i915_private *dev_priv =3D to_i915(state->base.d= ev); > > > > > > > + struct drm_connector *connector; > > > > > > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > > > > > > + int i, ret; > > > > > > > + > > > > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > > > > + return 0; > > > > > > > + > > > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modese= t */ > > > > > > > + for_each_oldnew_connector_in_state(&state->base, connector, > > > > > > > + old_conn_state, new_conn_state, i) { > > > > > > > + if (!connector->has_tile) > > > > > > > + continue; > > > > > > > + if (!intel_connector_needs_modeset(state, connector)) > > > > > > > + continue; > > > > > > > + > > > > > > > + ret =3D intel_modeset_all_tiles(state, connector->tile_gro= up->id); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > /** > > > > > > > * intel_atomic_check - validate state object > > > > > > > * @dev: drm device > > > > > > > @@ -14331,6 +14443,10 @@ static int intel_atomic_check(struct= drm_device *dev, > > > > > > > if (ret) > > > > > > > goto fail; > > > > > > > = > > > > > > > + ret =3D intel_atomic_check_tiled_conns(state); > > > > > > > + if (ret) > > > > > > > + goto fail; > > > > > > > + > > > > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_s= tate, > > > > > > > new_crtc_state, i) { > > > > > > > if (!needs_modeset(new_crtc_state)) { > > > > > > > @@ -14378,6 +14494,13 @@ static int intel_atomic_check(struct= drm_device *dev, > > > > > > > } > > > > > > > } > > > > > > > = > > > > > > > + /* > > > > > > > + * In case of port synced crtcs, if one of the synced crtcs > > > > > > > + * needs a full modeset, all other synced crtcs should be > > > > > > > + * forced a full modeset. > > > > > > = > > > > > > Since it's somewhat non-intuitive, I'd add a little bit of extra > > > > > > explanation for why intel_atomic_check_tiled_conns above didn't= already > > > > > > take care of this. I.e., if you've plugged in a different moni= tor, the > > > > > > tile information may have already vanished from drm_connector b= y the > > > > > > time we start this atomic transaction, so we still need to deal= with > > > > > > connectors that used to be tiled (and thus were port-synced) bu= t no > > > > > > longer are. > > > > > > > > > > > = > > > > > Yes will add this in the comments > > > > > = > > > > > > = > > > > > > > + */ > > > > > > > + intel_atomic_check_synced_crtcs(state); > > > > > > = > > > > > > Although I can't think of a reason why it would cause a problem= in this > > > > > > case, we do seem to be violating the directions in the big kern= eldoc > > > > > > warning attached to drm_atomic_helper_check_modeset(). I.e. if= we set > > > > > > mode_changed in our own check functions, then we're supposed to= re-call > > > > > > drm_atomic_helper_check_modeset() to make sure everything is pr= operly > > > > > > handled. If we're not going to follow those directions, we sho= uld > > > > > > probably be clear why we don't think it's necessary. > > > > > > = > > > > > > Actually, I wonder if some of this tiling handling should event= ually > > > > > > migrate into that core helper function in the future... > > > > > > > > > > > = > > > > > We are setting the mode hanged to true directly in our function s= o yes it makes > > > > > sense to eventually do that as part of the core helper function b= ut i still > > > > > dont see the need for us to call drm_atomic_helper_check_modeset(= ) again > > > > > since our function is essentially checking modeset based on the t= iled conditions > > > > = > > > > I'd still like to see either a commit message note or a code comment > > > > justifying this. The instructions say we need to re-invoke that > > > > function in cases like what we're doing here, so if we're intention= ally > > > > not following those directions we should explain why for future > > > > reference. It's also possible that function may expand its role in= the > > > > future and start doing something new that we really do need to worry > > > > about, so it would be good to have the history and justification fo= r not > > > > calling it documented. > > > > > > > = > > > Yes adding a bigger comment explaining this is a good idea. > > > = > > > With these changes do you think this is a r-b from you? > > = > > Yeah, if we add a FIXME for the cases that we're still missing (i.e., > > when tile information is gone and we never pull in the old sync > > partner), you can consider this > > = > > Reviewed-by: Matt Roper > > = > > since even if it's not 100% complete it fixes some cases and shouldn't > > cause new regressions, so it makes sense to land it early and then > > follow up with the rest of the fixes when they're ready. > > = > > = > > Matt > > = > > > = > > > Manasi = > > > > = > > > > Matt > > > > = > > > > > = > > > > > Manasi > > > > > = > > > > > > = > > > > > > Matt > > > > > > = > > > > > > > + > > > > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_s= tate, > > > > > > > new_crtc_state, i) { > > > > > > > if (needs_modeset(new_crtc_state)) { > > > > > > > -- = > > > > > > > 2.19.1 > > > > > > > = > > > > > > = > > > > > > -- = > > > > > > Matt Roper > > > > > > Graphics Software Engineer > > > > > > VTT-OSGC Platform Enablement > > > > > > Intel Corporation > > > > > > (916) 356-2795 > > > > = > > > > -- = > > > > Matt Roper > > > > Graphics Software Engineer > > > > VTT-OSGC Platform Enablement > > > > Intel Corporation > > > > (916) 356-2795 > > = > > -- = > > Matt Roper > > Graphics Software Engineer > > VTT-OSGC Platform Enablement > > Intel Corporation > > (916) 356-2795 -- = Matt Roper Graphics Software Engineer VTT-OSGC Platform Enablement Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx