[Intel-gfx] [PATCH] drm/i915/glk: Disable Guc and HuC on GLK

[Srivatsa, Anusha]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Friday, December 15, 2017 3:03 PM
>To: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Srivatsa, Anusha
><anusha.srivatsa at 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.srivatsa at intel.com> wrote:
>>
>> >
>> >
>> > > -----Original Message-----
>> > > From: Wajdeczko, Michal
>> > > Sent: Thursday, December 14, 2017 2:18 PM
>> > > To: intel-gfx at lists.freedesktop.org; Srivatsa, Anusha
>> > > <anusha.srivatsa at intel.com>
>> > > Cc: Vivi, Rodrigo <rodrigo.vivi at 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.srivatsa at intel.com> wrote:
>> > >
>> > > > Since the firmwares are released yet to public repo, disable
>> > > > them on Geminilake.
>> > > >
>> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at 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.

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,
>> > > >  };