Re: [PATCH 06/16] drm/i915/guc/slpc: Allocate, initialize and release slpc

["Belgaumkar, Vinay" <vinay.belgaumkar@xxxxxxxxx>]

On 7/10/2021 9:05 AM, Michal Wajdeczko wrote:
> On 10.07.2021 03:20, Vinay Belgaumkar wrote:
> > Allocate data structures for SLPC and functions for
> > initializing on host side.
> >
> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > Signed-off-by: Sundaresan Sujaritha <sujaritha.sundaresan@xxxxxxxxx>
> > ---
> >   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 | 20 ++++++++++++
> >   3 files changed, 66 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 9d61b2d54de4..82863a9bc8e8 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > @@ -336,6 +336,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);
> >   
> > @@ -346,6 +352,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:
> > @@ -368,6 +376,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 c1f569d2300d..94e2f19951aa 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > @@ -4,11 +4,41 @@
> >    * Copyright © 2020 Intel Corporation
> >    */
> >   
> > +#include <asm/msr-index.h>
>
> hmm, what exactly is needed from this header ?

Was being used in a previous version for MSR reads, removed.

> > +
> > +#include "gt/intel_gt.h"
> > +#include "gt/intel_rps.h"
> > +
> > +#include "i915_drv.h"
> >   #include "intel_guc_slpc.h"
> > +#include "intel_pm.h"
> > +
> > +static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc)
> > +{
> > +	return container_of(slpc, struct intel_guc, slpc);
> > +}
> > +
> > +static int slpc_shared_data_init(struct intel_guc_slpc *slpc)
> > +{
> > +	struct intel_guc *guc = slpc_to_guc(slpc);
> > +	int err;
> > +	u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));
>
> move err decl here
>
> > +
> > +	err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, &slpc->vaddr);
> > +	if (unlikely(err)) {
> > +		DRM_ERROR("Failed to allocate slpc struct (err=%d)\n", err);
>
> s/slpc/SLPC
>
> and use drm_err instead
> and you may also want to print error as %pe

added.

> > +		i915_vma_unpin_and_release(&slpc->vma, I915_VMA_RELEASE_MAP);
>
> do you really need this ?

removed.
> > +		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);
> >   }
> >   
> >   /*
> > @@ -31,4 +61,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 98036459a1a3..a2643b904165 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
> > @@ -3,12 +3,32 @@
> >    *
> >    * Copyright © 2020 Intel Corporation
> >    */
> > +
>
> should be fixed in earlier patch
>
> >   #ifndef _INTEL_GUC_SLPC_H_
> >   #define _INTEL_GUC_SLPC_H_
> >   
> > +#include <linux/mutex.h>
> >   #include "intel_guc_slpc_fwif.h"
> >   
> >   struct intel_guc_slpc {
> > +	/*Protects access to vma and SLPC actions */
>
> hmm, missing mutex ;)

Removed.

> > +	struct i915_vma *vma;
> > +	void *vaddr;
>
> no need to be void, define it as ptr to slpc_shared_data
>
> > +
> > +	/* platform frequency limits */
> > +	u32 min_freq;
> > +	u32 rp0_freq;
> > +	u32 rp1_freq;
> > +
> > +	/* frequency softlimits */
> > +	u32 min_freq_softlimit;
> > +	u32 max_freq_softlimit;
> > +
> > +	struct {
> > +		u32 param_id;
> > +		u32 param_value;
> > +		u32 param_override;
> > +	} debug;
>
> can you add all these extra fields in patches which will need them?
>
> Michal

Done.

Thanks,
Vinay.

> >   };
> >   
> >   int intel_guc_slpc_init(struct intel_guc_slpc *slpc);