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

From: Jani Nikula <jani.nikula@intel.com>
To: Jim Bride <jim.bride@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Wayne Boyer <wayne.boyer@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v3 1/4] drm/i915/psr: Clean-up intel_enable_source_psr1()
Date: Fri, 04 Aug 2017 10:29:33 +0300	[thread overview]
Message-ID: <8760e3hjtu.fsf@nikula.org> (raw)
In-Reply-To: <20170803214825.GA30928@shiv>

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.

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.

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