On Thu, Nov 10, 2016 at 07:59:05AM +0100, Maarten Lankhorst wrote:
> Op 10-11-16 om 01:52 schreef Matt Roper:
> > On Tue, Nov 08, 2016 at 01:55:35PM +0100, Maarten Lankhorst wrote:
> >> This member is only used in skl_update_crtcs now. It's easy to remove it
> >> by keeping track of which ddb entries in an array, and update them after
> > I'm having trouble parsing this line...not sure if you have an extra
> > word or are missing a word. But I think what you meant is that you're
> > snapshotting the DDB values at the beginning of this function so that
> > you'll have a copy of what the 'old' values that were already in the
> > hardware , then you update that snapshot as you write the DDB for pipe
> > to the hardware?
> >
> >> we update the crtc. This removes the last bits of SKL-style watermarks
> >> kept outside of crtc_state.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
> >> drivers/gpu/drm/i915/intel_drv.h | 11 +++--------
> >> drivers/gpu/drm/i915/intel_pm.c | 25 +++++++------------------
> >> 3 files changed, 22 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 69f9addb29b3..e59adb03933e 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> unsigned int updated = 0;
> >> bool progress;
> >> enum pipe pipe;
> >> + int i;
> >> +
> >> + const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
> > Do we want this to be const?
> What is intended here is that the struct skl_ddb_entry is const. The pointers can be reassigned,
> and point to either before state or after state, but the values are unmodified. :)
Ah, okay, that makes sense.
> >> +
> >> + for_each_crtc_in_state(state, crtc, old_crtc_state, i)
> >> + /* ignore allocations for crtc's that have been turned off. */
> >> + if (crtc->state->active)
> >> + entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
> >>
> >> /*
> >> * Whenever the number of active pipes changes, we need to make sure we
> >> @@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> * cause pipe underruns and other bad stuff.
> >> */
> >> do {
> >> - int i;
> >> progress = false;
> >>
> >> for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> >> @@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
> >> cstate = to_intel_crtc_state(crtc->state);
> >> pipe = intel_crtc->pipe;
> >>
> >> - if (updated & cmask || !crtc->state->active)
> >> + if (updated & cmask || !cstate->base.active)
> > This change seems unrelated/unnecessary? cstate was just set a couple
> > lines above, so this is effectively just replacing crtc->state with
> > to_intel_crtc_state(crtc->state)->base.
>
> I'm planning to replace all iterations over state with a new macro that has old and new state,
> in which case dereferencing crtc->state directly becomes unneeded and dangerous if we ever
> implement queue depth > 1.
>
> I also plan to add some new macros that can pull the new obj->state from drm_atomic_state, or
> (with locking verification) from the current state. This should kill all direct obj->state
> dereferences.
>
> It's the same conversion we did with dev -> dev_priv cleanups, I try to sneak those conversions
> in where it makes sense. :)
Okay, fair enough. Just wanted to make sure it wasn't something you'd
mis-squashed into this patch without realizing it.
If you update the confusing wording in the commit message that I pointed
out above, you can consider this
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> ~Maarten
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
