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

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

On Fri, Aug 04, 2017 at 10:29:33AM +0300, Jani Nikula wrote:
> On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote:
> > 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.
> 
> Chris' suggestion preserves the restricted bits that must remain the
> same, while initializing everything else. Instead of only changing the
> bits we must change, only preserve the bits we must not change. Sorry if
> I wasn't clear with the "as much as possible" part there.

I think I followed you.  What I was trying to highlight is that the
patch as written doesn't touch anything other than what we explicitly
need to initialize.  While Chris' suggestion is much more terse, it
leaves us open to another bit being flagged out as 'software
shouldn't change' and we'd have a similar bug again.  The patch as
written doesn't expose us to that situation.  I'm happy to go with
Chris' suggestion if everyone still thinks it's the right thing, but
I wanted to highlight that it's not entirely equivalent to what was
in the original patch and in my opinion it's less safe than the
original patch.

> Preserving the restricted bits is a functional change, and the subject
> of this patch does not reflect that. When I look at the logs, I pretty
> much expect clean up commits to be non-functional. There are some areas
> where I'd look the other way, but PSR is something where we must
> carefully split up the patches and write the commit messages diligently,
> because I know we will be spending time debugging this code and reading
> the logs.

I will remove the word 'clean-up' and reword the subject, independent
of what we decide relative to the two approaches described above.
The body of the commit message (IMHO) does a good job (and I'll add
the specific bit in SRD_CTL to the body also) of describing the
functional changes that the patch makes.

Jim

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