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. :)
>> +
>> + 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
