Re: [Intel-gfx] [RFC-v19 02/13] drm/i915/pxp: set KCR reg init during the boot time

From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Vivi\, Rodrigo" <rodrigo.vivi@intel.com>,
	"joonas.lahtinen\@linux.intel.com"
	<joonas.lahtinen@linux.intel.com>, "Huang\,
	Sean Z" <sean.z.huang@intel.com>,
	"Intel-gfx\@lists.freedesktop.org"
	<Intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [RFC-v19 02/13] drm/i915/pxp: set KCR reg init during the boot time
Date: Tue, 12 Jan 2021 17:36:02 +0200	[thread overview]
Message-ID: <871requpzx.fsf@intel.com> (raw)
In-Reply-To: <dc0506d6ba1f28d6ceda1fc2763f010213cc0031.camel@intel.com>

On Tue, 12 Jan 2021, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
> On Mon, 2021-01-11 at 21:38 +0000, Huang, Sean Z wrote:
>> Hello,
>> 
>> I see, based on Joonas and Rodrigo's feedback.
>> 
>> I made the modification as below, I will still keep the macro in this
>> .c instead of i915_reg.h, and this change will be reflected in rev20.
>> 
>> /* KCR register definitions */
>>  #define KCR_INIT            _MMIO(0x320f0)
>> -#define KCR_INIT_MASK_SHIFT (16)

Useless parenthesis.

>> +
>>  /* Setting KCR Init bit is required after system boot */
>> -#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) <<
>> KCR_INIT_MASK_SHIFT))
>> +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) << 16))

BIT(14) << 16 is actually BIT(14+16), or BIT(30). The above is
pointless.

Also, you'll end up with problems by using BIT() instead of REG_BIT()
defined in i915_reg.h due to BIT() using unsigned long.

Also, read the big style comment near the top of i915_reg.h.

BR,
Jani.

>
> This is not what I asked actually.
>
> I asked to get the BIT(14) to be defined separated, shift defined as
> well... and the | and actuall shift operations to be performed in the
> code and not in the defines
>
>> 
>> Best regards,
>> Sean
>> 
>> -----Original Message-----
>> From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Sent: Friday, January 8, 2021 3:31 AM
>> To: Huang, Sean Z <sean.z.huang@intel.com>; 
>> Intel-gfx@lists.freedesktop.org; Vivi, Rodrigo <
>> rodrigo.vivi@intel.com>
>> Subject: Re: [Intel-gfx] [RFC-v19 02/13] drm/i915/pxp: set KCR reg
>> init during the boot time
>> 
>> Quoting Vivi, Rodrigo (2021-01-07 17:31:36)
>> > On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote:
>> > > Set the KCR init during the boot time, which is required by
>> > > hardware, to allow us doing further protection operation such as
>> > > sending commands to GPU or TEE.
>> > > 
>> > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++
>> > >  1 file changed, 8 insertions(+)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> > > b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> > > index 9bc3c7e30654..f566a4fda044 100644
>> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> > > @@ -6,6 +6,12 @@
>> > >  #include "intel_pxp.h"
>> > >  #include "intel_pxp_context.h"
>> > > 
>> > > +/* KCR register definitions */
>> > 
>> > please define this in i915_reg.h
>> 
>> Generally the trend on the GT side is to contain in a .c file if
>> there are no shared users like these. So they should be at this spot,
>> yet the rest of the review comments apply.
>> 
>> The spurious comments should be dropped and like Rodrigo pointed out,
>> we should be using the appropriate macros for a masked writes, not
>> baking in the #define.
>> 
>> Regards, Joonas
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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