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@xxxxxxxxxxxxxxx> wrote: > > > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote: > > >> On Wed, 12 Jul 2017, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
