Hi Joonas,
Yes, I have removed this commit for single session patch series.
-----Original Message-----
From: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Sent: Monday, December 7, 2020 3:52 AM
To: Huang, Sean Z <sean.z.huang@xxxxxxxxx>; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [RFC-v1 06/16] drm/i915/pxp: Implement funcs to get/set PXP tag
Quoting Huang, Sean Z (2020-12-07 02:21:24)
> Implement the functions to get/set the PXP tag, which is 32-bit
> bitwise value containing the hardware session info, such as its
> session id, protection mode or whether it's enabled.
>
> Signed-off-by: Huang, Sean Z <sean.z.huang@xxxxxxxxx>
By my understanding, this patch should not be needed at all for singleton session? So I'm mostly skipping review here.
<SNIP>
> -/**
> - * check_if_protected_type0_sessions_are_attacked - To check if type0 active sessions are attacked.
> - * @i915: i915 device handle.
> - *
> - * Return: true if HW shows protected sessions are attacked, false otherwise.
> - */
> -static bool check_if_protected_type0_sessions_are_attacked(struct
> drm_i915_private *i915) -{
> - i915_reg_t kcr_status_reg = KCR_STATUS_1;
> - u32 reg_value = 0;
> - u32 mask = 0x80000000;
> - int ret;
> -
> - if (!i915)
> - return false;
> -
> - if (i915->pxp.ctx->global_state_attacked)
> - return true;
> -
> - ret = pxp_sm_reg_read(i915, kcr_status_reg.reg, ®_value);
> - if (ret) {
> - drm_err(&i915->drm, "Failed to pxp_sm_reg_read\n");
> - goto end;
> - }
> -
> - if (reg_value & mask)
> - return true;
> -end:
> - return false;
> -}
Removal of code added previously in the series?
> int pxp_sm_set_kcr_init_reg(struct drm_i915_private *i915) {
> int ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> index 222a879be96d..b5012948f971 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_sm.h
> @@ -20,6 +20,9 @@
> #define GEN12_KCR_TSIP_LOW _MMIO(0x32264) /* KCR type1 session in play 0-31 */
> #define GEN12_KCR_TSIP_HIGH _MMIO(0x32268) /* KCR type1 session in play 32-63 */
>
> +#define SESSION_TYPE_MASK BIT(7)
> +#define SESSION_ID_MASK (BIT(7) - 1)
> +
> enum pxp_session_types {
> SESSION_TYPE_TYPE0 = 0,
> SESSION_TYPE_TYPE1 = 1,
> @@ -36,6 +39,21 @@ enum pxp_protection_modes {
> PROTECTION_MODE_ALL
> };
>
> +struct pxp_tag {
> + union {
> + u32 value;
> + struct {
> + u32 session_id : 8;
> + u32 instance_id : 8;
> + u32 enable : 1;
> + u32 hm : 1;
> + u32 reserved_1 : 1;
> + u32 sm : 1;
> + u32 reserved_2 : 12;
> + };
It is not obvious if this is a software-only field. If it's software only, we should just make these into normal variables. If it's hardware related, it should be documented as a bitfield, like other hardware writes. We avoid using this construct in i915.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
