[Intel-gfx] [PATCH] drm/i915/guc: Always initialize action_lock

[Arkadiusz Hiler]

On Wed, Nov 23, 2016 at 10:07:27AM +0000, Chris Wilson wrote:
> On Wed, Nov 23, 2016 at 10:41:42AM +0100, Arkadiusz Hiler wrote:
> > On Tue, Nov 22, 2016 at 05:05:32PM +0000, Chris Wilson wrote:
> > > On Tue, Nov 22, 2016 at 05:22:47PM +0100, Arkadiusz Hiler wrote:
> > > > Action lock is not being initialized if the GuC submission is disabled
> > > > (i.e. i915.guc_submission=0).
> > > > 
> > > > host2guc_action(), which uses the action_lock can be used for
> > > > non-submission purposes, e.g. triggering HuC authentication.
> > > > 
> > > > Moving action_lock initialization before enablement check will allow us
> > > > to use the host2guc_action no matter whether submission is enabled or
> > > > not.
> > > Seems like you want to split uc_send(), uc_recv() out of
> > > i915_guc_submission.c
> > > -Chris
> > 
> > The patch I've shared just addressed issue Anusha had with HuC
> > enablement and allowed her to move further.
> > 
> > I was thinking of the split, as the HuC usage scenario rendered those
> > functions more general.
> > 
> > I would like to do it as a learning exercise.
> > 
> > I thought of two ways of approaching that:
> >   1. rename intel_guc_loader.c to something more general
> >      (e.g. intel_guc.c) and move the functions there
> >   2. create intel_guc_comm.c (or similar) for those functions
> > 
> > Since guc_send() and guc_recv() are made up from only a couple of dozens
> > of lines I am more inclined to option number 1.
> > 
> > Any thoughts?
> 
> I was thinking 1 seems ok, but intel_guc_loader.c should be reasonably
> well defined (wrt what we expect to find there) and has some ugly code...
> So starting a new file (intel_guc.c) and moving the common functions
> there is ok. It's ok if it starts small, it'll grow! Anything will be
> better than the surprise that i915_guc_submission.c exports functions
> for intel_huc.c.

I am convinced. I'll start slowly by moving the GuC communication there.

> Should it be intel_guc.c though? intel_agent.c? intel_uc.c? intel_fw.c?
> -Chris

Browsing through already mentioned HuC series you'll see there's a big
patch reusing lot's of GuC code for HuC. It's done by making funtions
and structs more generic and renaming them to include just the 'uc'.
Most of it could end up in the new file as well.

I am in favor of intel_uc.c.

-- 
Cheers,
Arek