| Message ID | 20210106231223.8323-3-sean.z.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Intel PXP component - Mesa single session | expand |
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 > +#define KCR_INIT _MMIO(0x320f0) > +#define KCR_INIT_MASK_SHIFT (16) mask or shift? > +/* Setting KCR Init bit is required after system boot */ > +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) << > KCR_INIT_MASK_SHIFT)) Also, use only bit definitions here and leave the mask and/or shift only when calling the write function. > > + > void intel_pxp_init(struct intel_pxp *pxp) > { > struct intel_gt *gt = container_of(pxp, struct intel_gt, > pxp); > @@ -15,6 +21,8 @@ void intel_pxp_init(struct intel_pxp *pxp) > > intel_pxp_ctx_init(&pxp->ctx); > > + intel_uncore_write(gt->uncore, KCR_INIT, > KCR_INIT_ALLOW_DISPLAY_ME_WRITES); > + > drm_info(>->i915->drm, "Protected Xe Path (PXP) protected > content support initialized\n"); > } >
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
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) + /* 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)) 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
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) > + > /* 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)) 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
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
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 */ +#define KCR_INIT _MMIO(0x320f0) +#define KCR_INIT_MASK_SHIFT (16) +/* Setting KCR Init bit is required after system boot */ +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES (BIT(14) | (BIT(14) << KCR_INIT_MASK_SHIFT)) + void intel_pxp_init(struct intel_pxp *pxp) { struct intel_gt *gt = container_of(pxp, struct intel_gt, pxp); @@ -15,6 +21,8 @@ void intel_pxp_init(struct intel_pxp *pxp) intel_pxp_ctx_init(&pxp->ctx); + intel_uncore_write(gt->uncore, KCR_INIT, KCR_INIT_ALLOW_DISPLAY_ME_WRITES); + drm_info(>->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n"); }
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(+)