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

[Michal Wajdeczko]

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

>
>
>> -----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.
>

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