[Intel-gfx] [PATCH v5 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
Manasi Navare
manasi.d.navare at intel.com
Fri Dec 27 22:51:28 UTC 2019
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 other
> > > > 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 the
> > > > tiles needs a full modeset then mark all other tiles for a full modeset.
> > > >
> > > > We also need to force modeset for all synced crtcs after fastset check.
> > > >
> > > > v5:
> > > > * Rebase
> > >
> > > I sent a reply to your v4 of this right at the same time you sent 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 below 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älä <ville.syrjala at linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> > > > ---
> > > > 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/drivers/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_needs_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 point? If
> > > the tile information was gone at the beginning of the transaction, then
> > > it wouldn't have known to bring in the other CRTC. So if a modeset 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 this 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 assignments 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 indeed
> indicate that it's part of a port-sync pair, but it won't get iterated
> 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_changed
> 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 patch 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.
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.
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
> >
> >
> > > > + if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
> > > > + new_crtc_state->uapi.mode_changed = true;
> > > > + new_crtc_state->update_pipe = false;
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +static void
> > > > +intel_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > + 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, int tile_grp_id)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > + struct drm_connector *connector;
> > > > + struct drm_connector_list_iter conn_iter;
> > > > + int ret = 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 != tile_grp_id)
> > > > + continue;
> > > > + conn_state = drm_atomic_get_connector_state(&state->base,
> > > > + connector);
> > > > + if (IS_ERR(conn_state)) {
> > > > + ret = PTR_ERR(conn_state);
> > > > + break;
> > > > + }
> > > > +
> > > > + if (!conn_state->crtc)
> > > > + continue;
> > > > +
> > > > + crtc_state = drm_atomic_get_crtc_state(&state->base,
> > > > + conn_state->crtc);
> > > > + if (IS_ERR(crtc_state)) {
> > > > + ret = PTR_ERR(conn_state);
> > > > + break;
> > > > + }
> > > > + crtc_state->mode_changed = true;
> > > > + ret = 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 tiled
> > > 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 potentially be
> > > moved to the core too. Do you know if there's other hardware with 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 *state)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > + 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 modeset */
> > > > + 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 = intel_modeset_all_tiles(state, connector->tile_group->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 = intel_atomic_check_tiled_conns(state);
> > > > + if (ret)
> > > > + goto fail;
> > > > +
> > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > 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 monitor, the
> > > tile information may have already vanished from drm_connector by 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) but 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 kerneldoc
> > > 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 properly
> > > handled. If we're not going to follow those directions, we should
> > > probably be clear why we don't think it's necessary.
> > >
> > > Actually, I wonder if some of this tiling handling should eventually
> > > migrate into that core helper function in the future...
> > >
> >
> > We are setting the mode hanged to true directly in our function so yes it makes
> > sense to eventually do that as part of the core helper function but 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 tiled 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 intentionally
> 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 for 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?
Manasi
>
> Matt
>
> >
> > Manasi
> >
> > >
> > > Matt
> > >
> > > > +
> > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > 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
More information about the Intel-gfx
mailing list