[Intel-gfx] [PATCH] drm/i915: Don't clear all watermarks when updating.

[Bob Paauwe]

On Thu, 16 Jul 2015 13:30:11 +0100
Damien Lespiau <damien.lespiau at intel.com> wrote:

> On Wed, Jul 08, 2015 at 09:05:53AM -0700, Bob Paauwe wrote:
> > Clearing the watermarks for all pipes/planes when updating the
> > watermarks for a single CRTC change seems like the wrong thing to
> > do here. As is, this code will update any pipe/plane watermarks
> > that need updating and leave the remaining set to zero.  Later, the
> > watermark checks in check_wm_state() will flag these zero'd out pipe/plane
> > watermarks and throw errors.
> > 
> > By not clearing all pipe/plane watermark values, only those that
> > require changes are changed and the remaining stay unchanged.
> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe at intel.com>
> 
> Hi Bob,
> 
> The spirit of this patch looks good to me (thanks for looking into
> this!), but we I think we still need to clear the data for the pipe
> we're are updating as it will contain stale values from the previous
> skl_update_wm().
> 
> For instance we don't reset ->dirty[pipe] to false after the register
> write and were relying on that memset. 2 options I can see:
> 
>   - selectively zero all the fields for that pipe (not helped by the
>     fact we have arrays indexed by the pipe (array of structure Vs
>     structure of arrays))
>   - make sure we don't rely on any field from the previous call (eg.
>     update ->dirty in skl_write_wm_values())
> 
> I have a slight preference for the first solution as it seems a bit more
> fool proof, but don't mind either way.

Thanks Damien,

I'll look into implementing your first suggestion and see how it goes.

I wasn't sure if anything needed to be cleared out or not. Based on
tracing the code, it looked like the pipe (or pipes) that needed
updating would get updated properly in the structure. And this did
eliminate the DRM ERRORs so it seemed like it might be a valid
solution. But I didn't look at the dirty flag to see if something might
be wrong there.

> 
> Thanks,
>