[Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Dec 20 16:47:46 UTC 2019
On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote:
> On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 19, 2019 at 01:51:15PM -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.
> > >
> > > 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 | 143 +++++++++++++++++--
> > > 1 file changed, 131 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a3f9430493ae..00608d8cef50 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> > > new_crtc_state->uapi.mode_changed = false;
> > > new_crtc_state->update_pipe = true;
> > >
> > > - /*
> > > - * If we're not doing the full modeset we want to
> > > - * keep the current M/N values as they may be
> > > - * sufficiently different to the computed values
> > > - * to cause problems.
> > > - *
> > > - * FIXME: should really copy more fuzzy state here
> > > - */
> > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> > > }
> >
> > The check vs. copy should really be a separate patch. Otherwise we risk
> > having to revert the whole thing if something goes wrong.
> >
>
> Yes will sepaarte it out , also I think Jose's series might get merged before mine so..
>
>
> > >
> > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > +static void
> > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)
> >
> > I would remove "dp" from the names of all these functions. The fact
> > that we only enable port sync on DP outputs is just an implementation
> > detail we don't have/want to care about in higher level code like this.
> >
>
> Okay will rename
>
> > > +{
> > > + 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) {
> > > + if (is_trans_port_sync_mode(new_crtc_state)) {
> > > + new_crtc_state->uapi.mode_changed = true;
> > > + new_crtc_state->update_pipe = false;
> > > + }
> > > + }
> > > +}
> > > +
> > > +static void
> > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)
> > > +{
> > > + 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) {
> > > + if (!is_trans_port_sync_mode(new_crtc_state) ||
> > > + !needs_modeset(new_crtc_state))
> > > + continue;
> > > +
> > > + intel_dp_modeset_synced_crtcs(state);
> > > + }
> >
> > This is not sufficient for the pre-compute_config() check. The point is
> > that we don't yet necessarily have the synced crtcs in the atomic state
> > so we have to add them. Please have a look in my branch for what I mean
> > exactly.
> >
>
> But if I use old crtc state or new crtc state here, I do see that since we havent
> cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn,
> I do see both conns in trans port sync mode and i can set the mode changed to true for both.
>
> I dont understand why what you mean by we would beed to add them?
for_each_new_intel_crtc_in_state() only iterates the crtcs we've already
added to the state, not all crtcs. The whole point of the
pre-compute_config() check is that we add the ones that were previously
synced but weren't yet added to the state. This for the case where the
tile info has gone poof so the function that adds stuff based on the
tile info will have missed them.
>
> > For the check between the fastset check vs. copy this here should work.
> >
> > > +}
> > > +
> > > +static int
> > > +intel_dp_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_list_iter;
> >
> > 'conn_iter' is the customary name.
> >
>
> ok
>
> > > +
> > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > > + drm_for_each_connector_iter(connector, &conn_list_iter) {
> > > + struct drm_connector_state *conn_iter_state;
> >
> > 'conn_state'
> >
>
> ok
>
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + if (!(connector->has_tile &&
> > > + connector->tile_group->id == tile_grp_id))
> > > + continue;
> >
> > or maybe !has_tile || tile_grrp != tile_grp
> >
> > I find that form a bit easier on the eyes. But either will work.
> >
>
> Ok
>
> > > + conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > > + connector);
> > > + if (IS_ERR(conn_iter_state)) {
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > + return PTR_ERR(conn_iter_state);
> >
> > nit: could just do 'ret = PTR_ERR(); break;'
> > and 'int ret=0;' + 'return ret' at the fars ends of the function.
> >
>
> ok
>
> > > + }
> > > +
> > > + if (!conn_iter_state->crtc)
> > > + 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;
> >
> > Missing drm_atomic_add_affected_planes(). We also need that in the
> > pre-compute_config() check_synced_crtcs().
> >
>
> Manasi
>
>
> Oh we need to call this function along with mode_changed = true?
>
> > > + }
> > > + drm_connector_list_iter_end(&conn_list_iter);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +intel_dp_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, old_conn_state,
> > > + new_conn_state))
> > > + continue;
> > > +
> > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * intel_atomic_check - validate state object
> > > * @dev: drm device
> > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev,
> > > if (ret)
> > > goto fail;
> > >
> > > + ret = intel_dp_atomic_check_tiled_conns(state);
> > > + if (ret)
> > > + goto fail;
> > > +
> > > + intel_dp_atomic_check_synced_crtcs(state);
> > > +
> > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > new_crtc_state, i) {
> > > if (!needs_modeset(new_crtc_state)) {
> > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev,
> > > any_ms = true;
> > > }
> > >
> > > + /*
> > > + * 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.
> > > + */
> > > + intel_dp_atomic_check_synced_crtcs(state);
> > > +
> > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > + new_crtc_state, i) {
> > > + if (needs_modeset(new_crtc_state))
> > > + continue;
> > > +
> > > + /*
> > > + * If we're not doing the full modeset we want to
> > > + * keep the current M/N values as they may be
> > > + * sufficiently different to the computed values
> > > + * to cause problems.
> > > + *
> > > + * FIXME: should really copy more fuzzy state here
> > > + */
> > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >
> > Same comment I gave in Jose's patch: Pls keep this in a separate
> > function. My branch has an example.
> >
> > > + }
> > > +
> > > if (any_ms && !check_digital_port_conflicts(state)) {
> > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> > > ret = EINVAL;
> > > --
> > > 2.19.1
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list