Em Seg, 2016-02-15 às 15:31 +0100, Maarten Lankhorst escreveu:
> Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> > > Factor out intel_fbc_supports_rotation
> > ^ not anymore
> >
> >
> > > and use it in
> > > pre_plane_update as well. This leaves intel_crtc->atomic
> > > empty, so remove it too.
> > >
> > > Changes since v1:
> > > - Add a intel_fbc_supports_rotation helper.
> > Changes since v2:
> > - No more need for rotation special-casing.
> >
> > (I suppose you also have to edit the commit title to be v3)
> >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++---------
> > > ----
> > > ----------
> > > drivers/gpu/drm/i915/intel_drv.h | 15 ----------
> > > 2 files changed, 20 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 54be8a255f1f..00cb261c6787 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > > {
> > > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> > > > base.crtc);
> > > struct drm_atomic_state *old_state = old_crtc_state-
> > > > base.state;
> > > - struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> > > struct intel_crtc_state *pipe_config =
> > > to_intel_crtc_state(crtc->base.state);
> > > struct drm_device *dev = crtc->base.dev;
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct drm_plane *primary = crtc->base.primary;
> > > struct drm_plane_state *old_pri_state =
> > > drm_atomic_get_existing_plane_state(old_state,
> > > primary);
> > > @@ -4798,22 +4796,19 @@ static void
> > > intel_post_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > > if (pipe_config->wm_changed && pipe_config->base.active)
> > > intel_update_watermarks(&crtc->base);
> > >
> > > - if (atomic->update_fbc)
> > > - intel_fbc_post_update(crtc);
> > > -
> > > if (old_pri_state) {
> > For a code reader that is not very familiar with all the atomic
> > history
> > and its details, it's not trivial to conclude that "if
> > (drm_atomic_get_existing_plane_state(primary))", then we're
> > updating
> > the primary plane on this atomic commit. And before this patch,
> > it's
> > much much easier to conclude that update_fbc will be true when an
> > atomic update is touching the primary plane because that's
> > explicitly
> > stated by the cod.
> >
> > So although "let's kill this redundant struct" sounds good, it
> > seems to
> > me that code clarity is going away with this patch, so I wonder if
> > the
> > benefits of the patch outweigh the downsides. Isn't there something
> > else we could do about this, such as some renaming, or adding some
> > function aliases or just extra commenting?
> >
> > > struct intel_plane_state *primary_state =
> > > to_intel_plane_state(primary->state);
> > > struct intel_plane_state *old_primary_state =
> > > to_intel_plane_state(old_pri_state);
> > >
> > > + intel_fbc_post_update(crtc);
> > > +
> > > if (primary_state->visible &&
> > > (needs_modeset(&pipe_config->base) ||
> > > !old_primary_state->visible))
> > > intel_post_enable_primary(&crtc->base);
> > > }
> > > -
> > > - memset(atomic, 0, sizeof(*atomic));
> > > }
> > >
> > > static void intel_pre_plane_update(struct intel_crtc_state
> > > *old_crtc_state)
> > > @@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> > > > base.crtc);
> > > struct drm_device *dev = crtc->base.dev;
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > - struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> > > struct intel_crtc_state *pipe_config =
> > > to_intel_crtc_state(crtc->base.state);
> > > struct drm_atomic_state *old_state = old_crtc_state-
> > > > base.state;
> > > @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
> > > intel_crtc_state *old_crtc_state)
> > > drm_atomic_get_existing_plane_state(old_state,
> > > primary);
> > > bool modeset = needs_modeset(&pipe_config->base);
> > >
> > > - if (atomic->update_fbc)
> > > - intel_fbc_pre_update(crtc);
> > > -
> > > if (old_pri_state) {
> > > struct intel_plane_state *primary_state =
> > > to_intel_plane_state(primary->state);
> > > struct intel_plane_state *old_primary_state =
> > > to_intel_plane_state(old_pri_state);
> > > + bool turn_off = old_primary_state->visible &&
> > > + (modeset || !primary_state->visible);
> > Not really related to the patch, but ok to keep since it's
> > trivial...
> >
> > > +
> > > + intel_fbc_pre_update(crtc);
> > >
> > > - if (old_primary_state->visible &&
> > > - (modeset || !primary_state->visible))
> > > + if (turn_off)
> > > intel_pre_disable_primary(&crtc->base);
> > > }
> > >
> > > @@ -11822,27 +11816,17 @@ int
> > > intel_plane_atomic_calc_changes(struct
> > > drm_crtc_state *crtc_state,
> > > if (visible || was_visible)
> > > pipe_config->fb_bits |= to_intel_plane(plane)-
> > > > frontbuffer_bit;
> > >
> > > - switch (plane->type) {
> > > - case DRM_PLANE_TYPE_PRIMARY:
> > > - intel_crtc->atomic.update_fbc = true;
> > > -
> > > - break;
> > > - case DRM_PLANE_TYPE_CURSOR:
> > > - break;
> > > - case DRM_PLANE_TYPE_OVERLAY:
> > > - /*
> > > - * WaCxSRDisabledForSpriteScaling:ivb
> > > - *
> > > - * cstate->update_wm was already set above, so
> > > this
> > > flag will
> > > - * take effect when we commit and program
> > > watermarks.
> > > - */
> > > - if (IS_IVYBRIDGE(dev) &&
> > > - needs_scaling(to_intel_plane_state(plane_sta
> > > te))
> > > &&
> > > - !needs_scaling(old_plane_state))
> > > - pipe_config->disable_lp_wm = true;
> > > + /*
> > > + * WaCxSRDisabledForSpriteScaling:ivb
> > > + *
> > > + * cstate->update_wm was already set above, so this flag
> > > will
> > > + * take effect when we commit and program watermarks.
> > > + */
> > > + if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
> > > IS_IVYBRIDGE(dev) &&
> > > + needs_scaling(to_intel_plane_state(plane_state)) &&
> > > + !needs_scaling(old_plane_state))
> > > + pipe_config->disable_lp_wm = true;
> > >
> > > - break;
> > > - }
> > > return 0;
> > > }
> > >
> > > @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > > struct intel_crtc_state *pipe_config =
> > > to_intel_crtc_state(crtc_state);
> > >
> > > - memset(&to_intel_crtc(crtc)->atomic, 0,
> > > - sizeof(struct intel_crtc_atomic_commit));
> > > -
> > > /* Catch I915_MODE_FLAG_INHERITED */
> > > if (crtc_state->mode.private_flags != crtc-
> > > >state-
> > > > mode.private_flags)
> > > crtc_state->mode_changed = true;
> > > @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > > if (!modeset)
> > > intel_pre_plane_update(to_intel_crtc_sta
> > > te(c
> > > rtc_state));
> > >
> > > - if (crtc->state->active && intel_crtc-
> > > > atomic.update_fbc)
> > > + if ((modeset || update_pipe) && crtc->state-
> > > >active)
> > Same here: not easy for me to verify things are the same now. Even
> > worse: the check for "is this atomic commit touching the primary
> > plane?" is now written in a completely different way than the one
> > introduced above. Maybe there's something we could do to make the
> > code
> > easier to read.
> No, this is called now every time when the crtc has a modeset or
> update_pipe, even with no primary plane.
> This is because atomic may set a mode without having a primary plane,
> and in that case fbc_enable may never be called.
> Seemed like a bug in the original code..
I suppose that proves my point that this code with all those similar-
but-different boolean checks is really hard to read :)
FBC can only happen when there's a primary plane, so AFAIU even if we
don't call intel_fbc_enable here because the primary is disable, we'll
end up calling it later when we enable the primary plane.
Anyway, a change in behavior like this (which was supposed to be a bug
fix) should be on its own separate patch, not hidden inside a refactor.
>
> ~Maarten
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
