>-----Original Message----- >From: Wajdeczko, Michal >Sent: Friday, September 29, 2017 12:10 AM >To: Sundaresan, Sujaritha <sujaritha.sundaresan@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx; Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> >Cc: Wajdeczko, Michal <Michal.Wajdeczko@xxxxxxxxx>; Mateo Lozano, Oscar ><oscar.mateo@xxxxxxxxx>; Ceraolo Spurio, Daniele ><daniele.ceraolospurio@xxxxxxxxx> >Subject: Re: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading >module > >On Fri, 29 Sep 2017 00:36:56 +0200, Srivatsa, Anusha ><anusha.srivatsa@xxxxxxxxx> wrote: > >> >> >>> -----Original Message----- >>> From: Sundaresan, Sujaritha >>> Sent: Thursday, September 21, 2017 11:38 AM >>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@xxxxxxxxx>; Wajdeczko, >>> Michal >>> <Michal.Wajdeczko@xxxxxxxxx>; Srivatsa, Anusha >>> <anusha.srivatsa@xxxxxxxxx>; >>> Mateo Lozano, Oscar <oscar.mateo@xxxxxxxxx>; Ceraolo Spurio, Daniele >>> <daniele.ceraolospurio@xxxxxxxxx> >>> Subject: [PATCH v4 1/2] drm/i915/guc : Removing enable_guc_loading >>> module >>> >>> We currently have two module parameters that control GuC: >>> "enable_guc_loading" and "enable_guc_submission". >>> Whenever we need i915.enable_guc_submission=1, we also need >>> enable_guc_loading=1. >>> We also need enable_guc_loading=1 when we want to verify the HuC, which >>> is >>> every time we have a HuC (but all platforms with HuC have a GuC and >>> viceversa). >>> >>> v2: Clarifying the commit message (Anusha) >>> v3: Unify seq_puts messages, correcting inconsistencies (Michal) >>> v4: Rebased >>> >>> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> >>> Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >>> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> >>> >>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx> >>> --- > > snip > >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h >>> index 6d7d871..bd583f7 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -3138,11 +3138,14 @@ static inline unsigned int >>> i915_sg_segment_size(void) >>> * command submission once loaded. But these are logically independent >>> * properties, so we have separate macros to test them. >>> */ >>> -#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) >>> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) >>> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >>> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) >>> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >>> +#define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) >>> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != >>> NULL) >>> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != >>> NULL) >>> + >>> +#define NEEDS_GUC_LOADING(dev_priv) \ >>> + (HAS_GUC(dev_priv) && \ >>> + (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) >> >> What if there is GuC and we don't want to use it? The above >> NEEDS_GUC_LOADING is still going to load it because it is available. So >> maybe there should only be: >> >> #define NEEDS_GUC_LOADING(dev_priv) \ >> (i915.enable_guc_submission || HAS_HUC_UCODE(dev_priv)) >> That is, load guc since guc submission is enabled or we need guc to >> authenticate HuC. >> > >Note that we used HAS_GUC as prerequisite that is then followed by logical >operator AND what guarantees us that we will try to load Guc only if its >available. In your modified macro we will try to load Guc just due to user >provided enable_guc_submission modparam which may not match hw caps. > >On other hand we can try to rely on earlier modparam sanitization but I >would >rather not trust that too much and keep our macro fully independent. Yes, you are right. Anusha >Michal _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
