Re: [PATCH v3 1/4] drm/i915/psr: Clean-up intel_enable_source_psr1()

[Jim Bride <jim.bride@linux.intel.com>]

On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25)
> >> On Tuesday, July 11, 2017 3:19:53 PM PDT Jim Bride wrote:
> >> > On SKL+ there is a bit in SRD_CTL that software is not supposed to
> >> > modify, but we currently clobber that bit when we enable PSR.  In
> >> > order to preserve the value of that bit, go ahead and read SRD_CTL 
> >> 
> >> And which bit is that?

Bit 29 (Context restore to PSR Active) in SRD_CTL.  I'll add it to the
commit message.  It's worth noting that the bit is not technically
reserved, but rather that SW is not allowed to change it.

> >
> > I think we would all be happier with keeping the explicit construction
> > (and a smaller patch) if we used
> >
> > 	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK;
> 
> Agreed. Avoid read-modify-write as much as possible.

I can do this if everyone thinks it's the thing to do, but it
does open us up to a similar class of bug (B-Spec restricting mods
to a bit / bit range after initial support for a platform was added)
in the future.  IMHO the code as written is safer.

Jim


> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx