[Intel-gfx] [RFC-v1 04/16] drm/i915/pxp: set KCR reg init during the boot time

[Huang, Sean Z]

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 <joonas.lahtinen at linux.intel.com> 
Sent: Monday, December 7, 2020 3:11 AM
To: Huang, Sean Z <sean.z.huang at intel.com>; Intel-gfx at 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 <sean.z.huang at intel.com>

<SNIP>

> +++ 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(&i915->runtime_pm, wakeref) {
> +               i915_reg_t reg_offset = {offset};

See below, here we convert from u32 to i915_reg_t.

> +
> +               intel_uncore_write(&i915->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(&i915->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