[PATCH] drm/i915/guc: Always initialize action_lock

[PATCH] drm/i915/guc: Always initialize action_lock

[Arkadiusz Hiler]

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.

Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4462112..17c06d5 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1484,6 +1484,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 	struct intel_guc *guc = &dev_priv->guc;
 	struct i915_vma *vma;
 
+	mutex_init(&guc->action_lock);
+
 	/* Wipe bitmap & delete client in case of reinitialisation */
 	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
 	i915_guc_submission_disable(dev_priv);
@@ -1500,7 +1502,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
 
 	guc->ctx_pool_vma = vma;
 	ida_init(&guc->ctx_ids);
-	mutex_init(&guc->action_lock);
 	guc_log_create(guc);
 	guc_addon_create(guc);
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Always initialize action_lock

[Chris Wilson]

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.
> 
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Seems like you want to split uc_send(), uc_recv() out of
i915_guc_submission.c
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915/guc: Always initialize action_lock

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Always initialize action_lock
URL   : https://patchwork.freedesktop.org/series/15755/
State : failure

== Summary ==

Series 15755v1 drm/i915/guc: Always initialize action_lock
https://patchwork.freedesktop.org/api/1.0/series/15755/revisions/1/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> INCOMPLETE (fi-skl-6700k)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:209  pass:187  dwarn:1   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

eeec5e7742b23082dd20523c8baa08fe495175e4 drm-intel-nightly: 2016y-11m-21d-18h-22m-22s UTC integration manifest
6500d86 drm/i915/guc: Always initialize action_lock

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3083/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Always initialize action_lock

[Srivatsa, Anusha]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Chris Wilson
>Sent: Tuesday, November 22, 2016 9:06 AM
>To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com>
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Always initialize action_lock
>
>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.
>>
>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Cc: Jeff McGee <jeff.mcgee@intel.com>
>> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>
>Seems like you want to split uc_send(), uc_recv() out of i915_guc_submission.c -
>Chris
 Hiler, this fixes the guc_loading enabled and guc_submission disabled scenario that I was facing.
Anusha
>--
>Chris Wilson, Intel Open Source Technology Centre
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Always initialize action_lock

[Arkadiusz Hiler]

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?

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Always initialize action_lock

[Chris Wilson]

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.

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Always initialize action_lock

[Arkadiusz Hiler]

On Tue, Nov 22, 2016 at 10:25:40PM +0100, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Chris Wilson
> >Sent: Tuesday, November 22, 2016 9:06 AM
> >To: Hiler, Arkadiusz <arkadiusz.hiler@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Always initialize action_lock
> >
> >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
>  Hiler, this fixes the guc_loading enabled and guc_submission disabled scenario that I was facing.
> Anusha

As you can see from the discussion in this thread I'll approach this
differently - by doing some code reorganization.

It will obsolete the patch (and will create cosy place for functions
renamed in "Make the GuC fw loading helper functions general").

Feel free to include this patch in your series for now.

PS. Just call me Arek :)

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx