Re: [Intel-gfx] [RFC-v1 04/16] drm/i915/pxp: set KCR reg init during the boot time
Hi Joonas,
> I think this should just be intel_pxp_sm_init() and then do whatever it needs
> to initialize. Also as we plan on having only a single session, I don't see
> why would we want a separate session management file/header.
> So I would be inclined to just inline the KCR_INIT macro write here. If this
> is moved to appropriate spot during intel_gt initialization, we should have
> the hardware wakeref, so would be just a single intel_uncore_write.
DONE, move to intel_gt so don't need the wakeref anymore, good suggestion,
thanks!
> Again, GEM_BUG_ON(!i915) should suffice.
DONE, remove the check
> I don't think we want to grab the wakeref at a low level reg_write function
> but at a higher levels to clearly distinct functions that need to access
> hardware and those who don't.
DONE.
> There is an error message in the upper level function, so one of these
> becomes redundant.
> After this has been moved to intel_gt init, the hardware wakeref is
> definitely held
DONE
> See above related to offset. Here we convert to u32. We shouldn't escape the
> protection offered by _MMIO macro.
DONE
> Based on the register name this feels like it should somehow be related to
> display init?
This register is more like a PXP related reg rather than display, so I prefer
still keep this in here.
> If this is only used from single place, it should go to the .c file that uses
> it.
DONE
Best regards,
Sean
-Original Message-
From: Joonas Lahtinen
Sent: Monday, December 7, 2020 3:11 AM
To: Huang, Sean Z ; Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v1 04/16] drm/i915/pxp: set KCR reg init during
the boot time
Quoting Huang, Sean Z (2020-12-07 02:21:22)
> 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
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -6,6 +6,7 @@
> #include "i915_drv.h"
> #include "intel_pxp.h"
> #include "intel_pxp_context.h"
> +#include "intel_pxp_sm.h"
>
> static void intel_pxp_write_irq_mask_reg(struct drm_i915_private
> *i915, u32 mask) { @@ -77,6 +78,8 @@ static void
> intel_pxp_irq_work(struct work_struct *work)
>
> int intel_pxp_init(struct drm_i915_private *i915) {
> + int ret;
> +
> if (!i915)
> return -EINVAL;
>
> @@ -92,13 +95,19 @@ int intel_pxp_init(struct drm_i915_private *i915)
> return -EFAULT;
> }
>
> + ret = pxp_sm_set_kcr_init_reg(i915);
I think this should just be intel_pxp_sm_init() and then do whatever it needs
to initialize. Also as we plan on having only a single session, I don't see why
would we want a separate session management file/header.
So I would be inclined to just inline the KCR_INIT macro write here. If this is
moved to appropriate spot during intel_gt initialization, we should have the
hardware wakeref, so would be just a single intel_uncore_write.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#include "gt/intel_context.h"
> +#include "gt/intel_engine_pm.h"
> +
> +#include "intel_pxp.h"
> +#include "intel_pxp_sm.h"
> +#include "intel_pxp_context.h"
> +
> +static int pxp_reg_write(struct drm_i915_private *i915, u32 offset,
> +u32 regval) {
> + intel_wakeref_t wakeref;
> +
> + if (!i915)
> + return -EINVAL;
Again, GEM_BUG_ON(!i915) should suffice.
> +
> + with_intel_runtime_pm(>runtime_pm, wakeref) {
> + i915_reg_t reg_offset = {offset};
See below, here we convert from u32 to i915_reg_t.
> +
> + intel_uncore_write(>uncore, reg_offset, regval);
> + }
I don't think we want to grab the wakeref at a low level reg_write function but
at a higher levels to clearly distinct functions that need to access hardware
and those who don't.
> + return 0;
> +}
> +
> +int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915) {
> + int ret;
> +
> + ret = pxp_reg_write(i915, KCR_INIT.reg,
> + KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
See above related to offset. Here we convert to u32. We shouldn't escape the
protection offered by _MMIO macro.
Based on the register name this feels like it should somehow be related to
display init?
> + if (ret)
> + drm_err(>drm, "Failed to write()\n");
There is an error message in the upper level function, so one of these becomes
redundant.
After this has been moved t
Re: [Intel-gfx] [RFC-v1 04/16] drm/i915/pxp: set KCR reg init during the boot time
Quoting Huang, Sean Z (2020-12-07 02:21:22)
> 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
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -6,6 +6,7 @@
> #include "i915_drv.h"
> #include "intel_pxp.h"
> #include "intel_pxp_context.h"
> +#include "intel_pxp_sm.h"
>
> static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
> mask)
> {
> @@ -77,6 +78,8 @@ static void intel_pxp_irq_work(struct work_struct *work)
>
> int intel_pxp_init(struct drm_i915_private *i915)
> {
> + int ret;
> +
> if (!i915)
> return -EINVAL;
>
> @@ -92,13 +95,19 @@ int intel_pxp_init(struct drm_i915_private *i915)
> return -EFAULT;
> }
>
> + ret = pxp_sm_set_kcr_init_reg(i915);
I think this should just be intel_pxp_sm_init() and then do whatever it
needs to initialize. Also as we plan on having only a single session, I
don't see why would we want a separate session management file/header.
So I would be inclined to just inline the KCR_INIT macro write here. If
this is moved to appropriate spot during intel_gt initialization, we
should have the hardware wakeref, so would be just a single
intel_uncore_write.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#include "gt/intel_context.h"
> +#include "gt/intel_engine_pm.h"
> +
> +#include "intel_pxp.h"
> +#include "intel_pxp_sm.h"
> +#include "intel_pxp_context.h"
> +
> +static int pxp_reg_write(struct drm_i915_private *i915, u32 offset, u32
> regval)
> +{
> + intel_wakeref_t wakeref;
> +
> + if (!i915)
> + return -EINVAL;
Again, GEM_BUG_ON(!i915) should suffice.
> +
> + with_intel_runtime_pm(>runtime_pm, wakeref) {
> + i915_reg_t reg_offset = {offset};
See below, here we convert from u32 to i915_reg_t.
> +
> + intel_uncore_write(>uncore, reg_offset, regval);
> + }
I don't think we want to grab the wakeref at a low level reg_write
function but at a higher levels to clearly distinct functions that need
to access hardware and those who don't.
> + return 0;
> +}
> +
> +int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915)
> +{
> + int ret;
> +
> + ret = pxp_reg_write(i915, KCR_INIT.reg,
> KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
See above related to offset. Here we convert to u32. We shouldn't escape
the protection offered by _MMIO macro.
Based on the register name this feels like it should somehow be related
to display init?
> + if (ret)
> + drm_err(>drm, "Failed to write()\n");
There is an error message in the upper level function, so one of these
becomes redundant.
After this has been moved to intel_gt init, the hardware wakeref is
definitely held
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_SM_H__
> +#define __INTEL_PXP_SM_H__
> +
> +#include "i915_drv.h"
> +#include "i915_reg.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))
If this is only used from single place, it should go to the .c file
that uses it.
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC-v1 04/16] drm/i915/pxp: set KCR reg init during the boot time
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
---
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c| 11 ++-
drivers/gpu/drm/i915/pxp/intel_pxp_sm.c | 38 +
drivers/gpu/drm/i915/pxp/intel_pxp_sm.h | 20 +
4 files changed, 70 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 99efac469cc2..131bd8921565 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -257,7 +257,8 @@ i915-y += i915_perf.o
# Protected execution platform (PXP) support
i915-$(CONFIG_DRM_I915_PXP) += \
pxp/intel_pxp.o \
- pxp/intel_pxp_context.o
+ pxp/intel_pxp_context.o \
+ pxp/intel_pxp_sm.o
# Post-mortem debug and GPU hang state capture
i915-$(CONFIG_DRM_I915_CAPTURE_ERROR) += i915_gpu_error.o
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 769bfd9bc6b8..d74a32b29716 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -6,6 +6,7 @@
#include "i915_drv.h"
#include "intel_pxp.h"
#include "intel_pxp_context.h"
+#include "intel_pxp_sm.h"
static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
mask)
{
@@ -77,6 +78,8 @@ static void intel_pxp_irq_work(struct work_struct *work)
int intel_pxp_init(struct drm_i915_private *i915)
{
+ int ret;
+
if (!i915)
return -EINVAL;
@@ -92,13 +95,19 @@ int intel_pxp_init(struct drm_i915_private *i915)
return -EFAULT;
}
+ ret = pxp_sm_set_kcr_init_reg(i915);
+ if (ret) {
+ drm_err(>drm, "Failed to set kcr init reg\n");
+ return ret;
+ }
+
INIT_WORK(>pxp.irq_work, intel_pxp_irq_work);
i915->pxp.handled_irr = (PXP_IRQ_VECTOR_DISPLAY_PXP_STATE_TERMINATED |
PXP_IRQ_VECTOR_DISPLAY_APP_TERM_PER_FW_REQ |
PXP_IRQ_VECTOR_PXP_DISP_STATE_RESET_COMPLETE);
- return 0;
+ return ret;
}
void intel_pxp_uninit(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
new file mode 100644
index ..a2c9c71d2372
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2020, Intel Corporation. All rights reserved.
+ */
+
+#include "gt/intel_context.h"
+#include "gt/intel_engine_pm.h"
+
+#include "intel_pxp.h"
+#include "intel_pxp_sm.h"
+#include "intel_pxp_context.h"
+
+static int pxp_reg_write(struct drm_i915_private *i915, u32 offset, u32 regval)
+{
+ intel_wakeref_t wakeref;
+
+ if (!i915)
+ return -EINVAL;
+
+ with_intel_runtime_pm(>runtime_pm, wakeref) {
+ i915_reg_t reg_offset = {offset};
+
+ intel_uncore_write(>uncore, reg_offset, regval);
+ }
+
+ return 0;
+}
+
+int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915)
+{
+ int ret;
+
+ ret = pxp_reg_write(i915, KCR_INIT.reg,
KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
+ if (ret)
+ drm_err(>drm, "Failed to write()\n");
+
+ return ret;
+}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
new file mode 100644
index ..d061f395aa16
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2020, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_PXP_SM_H__
+#define __INTEL_PXP_SM_H__
+
+#include "i915_drv.h"
+#include "i915_reg.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))
+
+int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915);
+
+#endif /* __INTEL_PXP_SM_H__ */
--
2.17.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
