On Mon, 18 Dec 2017 20:25:17 +0100, Srivatsa, Anusha <anusha.sriva...@intel.com> wrote:



-----Original Message-----
From: Vivi, Rodrigo
Sent: Friday, December 15, 2017 3:03 PM
To: Wajdeczko, Michal <michal.wajdec...@intel.com>
Cc: intel-gfx@lists.freedesktop.org; Srivatsa, Anusha
<anusha.sriva...@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/glk: Disable Guc and HuC on GLK

On Fri, Dec 15, 2017 at 06:07:27AM +0000, Michal Wajdeczko wrote:
On Thu, 14 Dec 2017 23:30:56 +0100, Srivatsa, Anusha
<anusha.sriva...@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: Wajdeczko, Michal
> > Sent: Thursday, December 14, 2017 2:18 PM
> > To: intel-gfx@lists.freedesktop.org; Srivatsa, Anusha
> > <anusha.sriva...@intel.com>
> > Cc: Vivi, Rodrigo <rodrigo.v...@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/glk: Disable Guc and HuC
> > on GLK
> >
> > On Thu, 14 Dec 2017 22:58:37 +0100, Anusha Srivatsa
> > <anusha.sriva...@intel.com> wrote:
> >
> > > Since the firmwares are released yet to public repo, disable
> > > them on Geminilake.
> > >
> > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.sriva...@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_pci.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > > b/drivers/gpu/drm/i915/i915_pci.c index fa67d3d..ddf7530 100644
> > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > @@ -521,6 +521,11 @@ static const struct intel_device_info
> > > intel_geminilake_info __initconst = {
> > >          GEN9_LP_FEATURES,
> > >          .platform = INTEL_GEMINILAKE,
> > >          .ddb_size = 1024,
> > > +        /* FIXME Geminilake supports GuC but currently firmwares
> > > +         * have not made it to public repo. Lets disable the support
> > > +         * as a temporary fix.
> > > +         */
> > > +        .has_guc = 0,
> >
> > Maybe better place to put this fix is __get_platform_enable_guc()
> > like in [1] [1] https://patchwork.freedesktop.org/patch/192006/
>
> Michal,
>  Hmm the reference patch  is controlling guc/huc through  a module
> parameter but wont the above approach be neater platform wise?

With your patch it would be impossible to check even preliminary
firmwares from non-public repo without recompilation of the driver.

While with variant below it would just require changing modparams like:
        enable_guc=1
        guc_firmware_path=preliminary/glk.bin

This is a fair point imo. With has_guc=0 you remove the possibility of testing
preliminary GuC/HuC without a kernel patch...


Note that auto mode (enable_guc=-1) will still keep GuC disabled for GLK.

... but I believe that the main point here is that if we don't have a public image it
doesn't exist from the upstream perspective.

Correct
The ideal is not need this patch and releasing firmware sooner. I see same
happening for all upcoming platforms...

So, whatever we decide for this case needs to cover future cases as well.

I agree. While both the places discussed is good to make this change, I am inclining towards making at the platform definition level. I do need more feedback on this since we want that to be the norm moving forward.


There is a yet another option for now: remove GuC/Huc firmware names.

        } else if (IS_GEMINILAKE(dev_priv)) {
-               guc_fw->path = I915_GLK_GUC_UCODE;
-               guc_fw->major_ver_wanted = GLK_FW_MAJOR;
-               guc_fw->minor_ver_wanted = GLK_FW_MINOR;
        } else {

And the norm for the future: do not define GuC/HuC firmware names without
having those images available in public.

Michal

Anusha
Thanks,
Rodrigo.


Michal

>
> Anusha
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > b/drivers/gpu/drm/i915/intel_uc.c index 49bccc9..22b0afe 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -60,6 +60,8 @@ static int __get_platform_enable_guc(struct
> > drm_i915_private *dev_priv)
> >               enable_guc |= ENABLE_GUC_LOAD_HUC;
> >
> >       /* Any platform specific fine-tuning can be done here */
> > +     if (IS_GEMINILAKE(dev_priv))
> > +             enable_guc = 0; /* no firmware on CI machines */
> >
> >       return enable_guc;
> >  }
> >
> >
> > >          GLK_COLORS,
> > >  };
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to