Hi Joonas,
> This should really have its own function intel_pxp_context_foobar() that is called from this point.
DONE, remove global_state_attacked and flag_display_hm_surface_keys from this patch and only added them if necessary.
> Also, as you see "ctx_mutex" is tautology and "mutex" is enough when it's member of "ctx".
DONE
> We seem to have two separate interrupts at the top level handler. Either we should handle the interrupts separately or just have a single variable "teardown_requested" that is flagged from here.
I would prefer to handle them separately because they are indeed coming from different irq, having different handler would be more readable.
> The effects of setting these variables can't be reviewed as not even the initialization sequence has been added by the series, so this should definitely be much more towards the end of the series.
DONE, got it, let me re-order the commit sequence and move this patch to the tail.
> The if() check is pointless. Again, we should not directly poke such deeply, but wrap it in a function.
DONE
> I think this should be -ENOMEM.
DONE, no need to allocate or return error.
> As we only intend to support a single context, we should avoid a pointer
> + alloc here and just use intel_pxp_context_init(&pxp->ctx)
DONE
> intel_pxp_context_fini(&pxp->ctx);
DONE
> These should be prefixed with PXP_ also, we should not need these at all if we only intend to support single-session.
DONE, remove them since not required by single session usage.
> Adding "new_" is tautology here. Also, we try to separate the allocation and init to separate functions so that we can embed, like I suggested above to embed the singleton context to intel_pxp as member, not pointer.
DONE
> "ctx_id" is again repeating as it's member of "ctx", so "id" should be fine for member name.
DONE
> We shouldn't need any session tracking as we only have single session.
I have trimmed the code only for single session usage only, but hopefully we could have at least one tag for this single session.
> We should only add each variable only when the handler code is introduced. For now the names don't really give a good hint about what their usage model will be, so can't recommend better names.
DONE
> In this series, there will be no user space context ID, but only a singleton implicit session. So we should not need any tracking code.
DONE
-----Original Message-----
From: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Sent: Monday, December 7, 2020 2:51 AM
To: Huang, Sean Z <sean.z.huang@xxxxxxxxx>; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Subject: Re: [RFC-v1 03/16] drm/i915/pxp: Add PXP context for logical hardware states.
Quoting Huang, Sean Z (2020-12-07 02:21:21)
> Add PXP context which represents combined view of driver and logical
> HW states.
>
> Signed-off-by: Huang, Sean Z <sean.z.huang@xxxxxxxxx>
<SNIP>
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -5,6 +5,7 @@
>
> #include "i915_drv.h"
> #include "intel_pxp.h"
> +#include "intel_pxp_context.h"
>
> static void intel_pxp_write_irq_mask_reg(struct drm_i915_private
> *i915, u32 mask) { @@ -28,12 +29,28 @@ static void
> intel_pxp_mask_irq(struct intel_gt *gt, u32 mask)
>
> static int intel_pxp_teardown_required_callback(struct
> drm_i915_private *i915) {
> + mutex_lock(&i915->pxp.ctx->ctx_mutex);
> +
> + i915->pxp.ctx->global_state_attacked = true;
> + i915->pxp.ctx->flag_display_hm_surface_keys = false;
> +
> + mutex_unlock(&i915->pxp.ctx->ctx_mutex);
This should really have its own function intel_pxp_context_foobar() that is called from this point. Also, as you see "ctx_mutex" is tautology and "mutex" is enough when it's member of "ctx".
We seem to have two separate interrupts at the top level handler. Either we should handle the interrupts separately or just have a single variable "teardown_requested" that is flagged from here.
The effects of setting these variables can't be reviewed as not even the initialization sequence has been added by the series, so this should definitely be much more towards the end of the series.
> +
> return 0;
> }
>
> static int intel_pxp_global_terminate_complete_callback(struct
> drm_i915_private *i915) {
> - return 0;
> + int ret = 0;
> +
> + mutex_lock(&i915->pxp.ctx->ctx_mutex);
> +
> + if (i915->pxp.ctx->global_state_attacked)
> + i915->pxp.ctx->global_state_attacked = false;
The if() check is pointless. Again, we should not directly poke such deeply, but wrap it in a function.
> +
> + mutex_unlock(&i915->pxp.ctx->ctx_mutex);
> +
> + return ret;
> }
>
> static void intel_pxp_irq_work(struct work_struct *work) @@ -69,6
> +86,12 @@ int intel_pxp_init(struct drm_i915_private *i915)
>
> drm_info(&i915->drm, "i915 PXP is inited with i915=[%p]\n",
> i915);
>
> + i915->pxp.ctx = intel_pxp_create_ctx(i915);
> + if (!i915->pxp.ctx) {
> + drm_err(&i915->drm, "Failed to create pxp ctx\n");
> + return -EFAULT;
I think this should be -ENOMEM.
> + }
As we only intend to support a single context, we should avoid a pointer
+ alloc here and just use intel_pxp_context_init(&pxp->ctx)
> @@ -80,6 +103,10 @@ int intel_pxp_init(struct drm_i915_private *i915)
>
> void intel_pxp_uninit(struct drm_i915_private *i915) {
> + if (!i915 || INTEL_GEN(i915) < 12)
> + return;
> +
> + intel_pxp_destroy_ctx(i915);
intel_pxp_context_fini(&pxp->ctx);
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -12,6 +12,9 @@
> #define PXP_IRQ_VECTOR_DISPLAY_APP_TERM_PER_FW_REQ BIT(2) #define
> PXP_IRQ_VECTOR_PXP_DISP_STATE_RESET_COMPLETE BIT(3)
>
> +#define MAX_TYPE0_SESSIONS 16
> +#define MAX_TYPE1_SESSIONS 6
These should be prefixed with PXP_ also, we should not need these at all if we only intend to support single-session.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_context.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#include "intel_pxp_context.h"
> +
> +/**
> + * intel_pxp_create_ctx - To create a new pxp context.
> + * @i915: i915 device handle.
> + *
> + * Return: pointer to new_ctx, NULL for failure */ struct
> +pxp_context *intel_pxp_create_ctx(struct drm_i915_private *i915) {
> + struct pxp_context *new_ctx = NULL;
Adding "new_" is tautology here. Also, we try to separate the allocation and init to separate functions so that we can embed, like I suggested above to embed the singleton context to intel_pxp as member, not pointer.
> +
> + new_ctx = kzalloc(sizeof(*new_ctx), GFP_KERNEL);
> + if (!new_ctx)
> + return NULL;
> +
> + get_random_bytes(&new_ctx->ctx_id, sizeof(new_ctx->ctx_id));
"ctx_id" is again repeating as it's member of "ctx", so "id" should be fine for member name.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_context.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_CONTEXT_H__
> +#define __INTEL_PXP_CONTEXT_H__
> +
> +#include <linux/list.h>
> +#include "i915_drv.h"
> +#include "pxp/intel_pxp.h"
> +
> +/* struct pxp_context - Represents combined view of driver and
> +logical HW states. */ struct pxp_context {
> + /** @ctx_mutex: mutex to protect the pxp context */
> + struct mutex ctx_mutex;
> +
> + struct list_head active_pxp_type0_sessions;
> + struct list_head active_pxp_type1_sessions;
We shouldn't need any session tracking as we only have single session.
> + struct list_head user_ctx_list;
> +
> + u32 type0_session_pxp_tag[MAX_TYPE0_SESSIONS];
> + u32 type1_session_pxp_tag[MAX_TYPE1_SESSIONS];
We shouldn't need any of these arrays as we only have single session.
> +
> + int ctx_id;
> +
> + bool global_state_attacked;
> + bool global_state_in_suspend;
> + bool flag_display_hm_surface_keys;
We should only add each variable only when the handler code is introduced. For now the names don't really give a good hint about what their usage model will be, so can't recommend better names.
> +};
> +
> +struct pxp_user_ctx {
> + /** @listhead: linked list infrastructure, do not change its order. */
> + struct list_head listhead;
> +
> + /** @user_ctx: user space context id */
> + u32 user_ctx;
> +};
In this series, there will be no user space context ID, but only a singleton implicit session. So we should not need any tracking code.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
