Re: [Intel-gfx] [PATCH 05/14] drm/i915/guc/slpc: Allocate, initialize and release SLPC

Belgaumkar, Vinay
2021-07-23T19:30:19Z

On 7/21/2021 10:26 AM, Michal Wajdeczko wrote:
>
>
> On 21.07.2021 18:11, Vinay Belgaumkar wrote:
>> Allocate data structures for SLPC and functions for
>> initializing on host side.
>>
>> v2: Address review comments (Michal W)
>>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> Signed-off-by: Sundaresan Sujaritha <sujaritha.sundaresan@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 11 ++++++
>> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 36 ++++++++++++++++++-
>> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 1 +
>> .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h | 3 ++
>> 4 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index fcccb103a21a..686cb978662d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -337,6 +337,12 @@ int intel_guc_init(struct intel_guc *guc)
>> goto err_ct;
>> }
>>
>> + if (intel_guc_slpc_is_used(guc)) {
>> + ret = intel_guc_slpc_init(&guc->slpc);
>> + if (ret)
>> + goto err_submission;
>> + }
>> +
>> /* now that everything is perma-pinned, initialize the parameters */
>> guc_init_params(guc);
>>
>> @@ -347,6 +353,8 @@ int intel_guc_init(struct intel_guc *guc)
>>
>> return 0;
>>
>> +err_submission:
>> + intel_guc_submission_fini(guc);
>> err_ct:
>> intel_guc_ct_fini(&guc->ct);
>> err_ads:
>> @@ -369,6 +377,9 @@ void intel_guc_fini(struct intel_guc *guc)
>>
>> i915_ggtt_disable_guc(gt->ggtt);
>>
>> + if (intel_guc_slpc_is_used(guc))
>> + intel_guc_slpc_fini(&guc->slpc);
>> +
>> if (intel_guc_submission_is_used(guc))
>> intel_guc_submission_fini(guc);
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> index d9feb430ce35..a99d727b5bf0 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> @@ -12,6 +12,16 @@ static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc)
>> return container_of(slpc, struct intel_guc, slpc);
>> }
>>
>> +static inline struct intel_gt *slpc_to_gt(struct intel_guc_slpc *slpc)
>> +{
>> + return guc_to_gt(slpc_to_guc(slpc));
>> +}
>> +
>> +static inline struct drm_i915_private *slpc_to_i915(struct intel_guc_slpc *slpc)
>> +{
>> + return (slpc_to_gt(slpc))->i915;
>
> redundant ( )

ok.

>
>> +}
>> +
>> static bool __detect_slpc_supported(struct intel_guc *guc)
>> {
>> /* GuC SLPC is unavailable for pre-Gen12 */
>> @@ -35,9 +45,29 @@ void intel_guc_slpc_init_early(struct intel_guc_slpc *slpc)
>> guc->slpc_selected = __guc_slpc_selected(guc);
>> }
>>
>> +static int slpc_shared_data_init(struct intel_guc_slpc *slpc)
>> +{
>> + struct intel_guc *guc = slpc_to_guc(slpc);
>> + struct drm_i915_private *i915 = slpc_to_i915(slpc);
>> + u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));
>> + int err;
>> +
>> + err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, (void **)&slpc->vaddr);
>> + if (unlikely(err)) {
>> + drm_err(&i915->drm,
>> + "Failed to allocate SLPC struct (err=%pe)\n",
>> + ERR_PTR(err));
>> + return err;
>> + }
>> +
>> + return err;
>> +}
>> +
>> int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>> {
>> - return 0;
>> + GEM_BUG_ON(slpc->vma);
>> +
>> + return slpc_shared_data_init(slpc);
>> }
>>
>> /*
>> @@ -60,4 +90,8 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>>
>> void intel_guc_slpc_fini(struct intel_guc_slpc *slpc)
>> {
>> + if (!slpc->vma)
>> + return;
>> +
>> + i915_vma_unpin_and_release(&slpc->vma, I915_VMA_RELEASE_MAP);
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> index c3b0ad7f0f93..f02249ff5f1b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> @@ -8,6 +8,7 @@
>>
>> #include "intel_guc_submission.h"
>> #include "intel_guc_slpc_types.h"
>> +#include "abi/guc_actions_slpc_abi.h"
>
> is this is needed here ?
> maybe abi.h could be included only in slcp.c ?

ok, removed.

>
>>
>> static inline bool intel_guc_slpc_is_supported(struct intel_guc *guc)
>> {
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> index b85148265b1f..214a449e78f2 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
>> @@ -7,9 +7,12 @@
>> #define _INTEL_GUC_SLPC_TYPES_H_
>>
>> #include <linux/types.h>
>> +#include "abi/guc_actions_slpc_abi.h"
>
> for below pointers you don't need this header(s) to be included

ok.
Thanks,
Vinay.
>
> Michal
>
>>
>> struct intel_guc_slpc {
>>
>> + struct i915_vma *vma;
>> + struct slpc_shared_data *vaddr;
>> };
>>
>> #endif
>>