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

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

On Mon, Aug 07, 2017 at 08:55:00AM -0700, Jim Bride wrote:
> 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.


Ok, folks think brevity wins out here, so I'm going to go ahead and
spin a different, stand-alone  patch following Chris' suggestion.
Please disregard this one.

Jim


> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx