[Intel-gfx] [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.

[Matt Roper]

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 at linux.intel.com>
> >> ---
> >>  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 at intel.com>

> 
> ~Maarten
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795