[Intel-gfx] [PATCH 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
Tue Dec 17 19:04:53 UTC 2019
On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Dec 11, 2019 at 01:14:23PM -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.
> > > > > > >
> > > > > > > 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>
> > > > > > > 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 | 78 ++++++++++++++++++++
> > > > > > > 1 file changed, 78 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 803993a01ca7..7263eaa66cda 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > +static int
> > > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > > > > > > + struct intel_atomic_state *state, int tile_grp_id)
> > > > > > > +{
> > > > > > > + struct drm_connector *conn_iter;
> > > > > > > + struct drm_connector_list_iter conn_list_iter;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > > > > > > + struct drm_connector_state *conn_iter_state;
> > > > > > > +
> > > > > > > + if (!conn_iter->has_tile)
> > > > > > > + continue;
> > > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > > > > > + conn_iter);
> > > > > > > + if (IS_ERR(conn_iter_state)) {
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > + }
> > > > > > > +
> > > > > > > + if (!conn_iter_state->crtc)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + if (conn_iter->tile_group->id != tile_grp_id)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > > > > > > + if (IS_ERR(crtc_state)) {
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > + return PTR_ERR(conn_iter_state);
> > > > > > > + }
> > > > > > > + crtc_state->mode_changed = true;
> > > > > > > + }
> > > > > > > + drm_connector_list_iter_end(&conn_list_iter);
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int
> > > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > > > > > > + struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > + struct drm_connector *connector;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > + struct drm_connector_state *connector_state;
> > > > > > > + int i, ret, tile_grp_id = 0;
> > > > > > > +
> > > > > > > + if (INTEL_GEN(dev_priv) < 11)
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > > > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > > > > > > + if (!connector->has_tile)
> > > > > > > + continue;
> > > > > > > + if (connector_state->crtc &&
> > > > > > > + tile_grp_id != connector->tile_group->id) {
> > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > > > > > > + connector_state->crtc);
> > > > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + tile_grp_id = connector->tile_group->id;
> > > > > > > + } else
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > >
> > > > > > BTW after some more pondering I don't think this alone is sufficient.
> > > > > > The tile information may have already disppeared so I believe we also
> > > > > > need to make sure we mark all currently synced crtcs as needing a
> > > > > > modeset if any of them need a modeset. And I guess that's pretty much
> > > > > > the same function we'll need to handle fastset correctly.
> > > > > >
> > > > >
> > > > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
> > > > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
> > > > > to true for all synced crtcs if one of them needs modeset?
> > > >
> > > > I think it should look something like:
> > > >
> > > > modeset_tiled_things();
> > > > modeset_synced_things();
> > >
> > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > > from within modeset_pipe_config just before compute_config() call.
> > >
> > > >
> > > > for_each() {
> > >
> > > This is the for_each_crtc loop in intel_atomic_check() right?
> > >
> > > > modeset_pipes_config();
> > >
> > > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > >
> > > > if (can_fastset()) {
> > > > modeset=false;
> > > > fastset=true;
> > > > }
> > > > }
> > > >
> > > > modeset_synced_things();
> > > >
> > > > for_each() {
> > > > if (!modeset && fastset)
> > > > copy_state();
> > > > }
> > > We already do this in the code right?
> > >
> > > Manasi
> > >
> > > >
> > > > >
> > > > > And why would the tile information be disappeared?
> > > >
> > > > It'll get updated whenever someone does a getconnector() or whatever.
> > > >
> > > > Example:
> > > > 1. sync pipe A and B, pipe A is master
> > > > 2. swap pipe B display for something else
> >
> > If we disconnect and connect other display for pipe B, port sync mode is off and
> > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
>
> Port sync will stay enabled until we do a modeset to disable it. In this
> example there is never any modeset on pipe B until we add soemthing to
> force one in step 4.
seems to be working with the current intel_dp_add_modeset_tiles() since the moment
we unplug, it goes through the disable sequence where all the associated tiles get disabled
and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates
a mode change since it fallsback to a lower non tiled mode.
But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is
still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs
to the modeset?
So the order of calls can still be :
intel_atomic_check() {
intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs}
intel_modeset_pipe_Config()
icl_add_sync_crtcs
compute_config
fastsetcheck()
modeset_synced_things (here it looks at the new master slave assignments)
Does this look correct, I want send this patch out today
Manasi
>
> >
> > so why we wold need to add both pipes to modeset in this case at all
> >
> > Manasi
> >
> > > > 3. getconnector() -> tile info goes poof
> > > > 4. do something on pipe A that needs a modeset
> > > > no tile info so we miss that pipe B also needs a modeset
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
More information about the Intel-gfx
mailing list