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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@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. :)
>> +
>> +	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. :)

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx