[PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[Chris Wilson]

As our early doorbell is split between early allocation and a late setup
after we have a channel to the GuC, it may happen due to a lapse of
programmer judgement that we try to setup an invalid doorbell. Make use
of our has_doorbell() function to check the doorbell does exist for the
client before we try and tell the guc about it. In doing so, we prevent
the compiler from warning about the otherwise unused function in some
configurations.

Reported-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 22 +++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 6e6ed0f46bd3..c6bb5bebddfc 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -124,9 +124,17 @@ static int reserve_doorbell(struct intel_guc_client *client)
 	return 0;
 }
 
+static bool has_doorbell(struct intel_guc_client *client)
+{
+	if (client->doorbell_id == GUC_DOORBELL_INVALID)
+		return false;
+
+	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
+}
+
 static void unreserve_doorbell(struct intel_guc_client *client)
 {
-	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
+	GEM_BUG_ON(!has_doorbell(client));
 
 	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
 	client->doorbell_id = GUC_DOORBELL_INVALID;
@@ -184,14 +192,6 @@ static struct guc_doorbell_info *__get_doorbell(struct intel_guc_client *client)
 	return client->vaddr + client->doorbell_offset;
 }
 
-static bool has_doorbell(struct intel_guc_client *client)
-{
-	if (client->doorbell_id == GUC_DOORBELL_INVALID)
-		return false;
-
-	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
-}
-
 static void __create_doorbell(struct intel_guc_client *client)
 {
 	struct guc_doorbell_info *doorbell;
@@ -207,7 +207,6 @@ static void __destroy_doorbell(struct intel_guc_client *client)
 	struct guc_doorbell_info *doorbell;
 	u16 db_id = client->doorbell_id;
 
-
 	doorbell = __get_doorbell(client);
 	doorbell->db_status = GUC_DOORBELL_DISABLED;
 	doorbell->cookie = 0;
@@ -224,6 +223,9 @@ static int create_doorbell(struct intel_guc_client *client)
 {
 	int ret;
 
+	if (WARN_ON(!has_doorbell(client)))
+		return -ENODEV; /* internal setup error, should never happen */
+
 	__update_doorbell_desc(client, client->doorbell_id);
 	__create_doorbell(client);
 
-- 
2.17.0

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

✓ Fi.CI.BAT: success for drm/i915/guc: Assert we have the doorbell before setting it up

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Assert we have the doorbell before setting it up
URL   : https://patchwork.freedesktop.org/series/42509/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4116 -> Patchwork_8854 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42509/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8854 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-ivb-3520m:       PASS -> DMESG-WARN (fdo#106084)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (38 -> 35) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4116 -> Patchwork_8854

  CI_DRM_4116: 7ff375cb94000d93f7a9d541cb0c8180fbea80f2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4453: 29ae12bd764e3b1876356e7628a32192b4ec9066 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8854: 3c7cfe517d43568f8810ec93c15d51c2d8b645fe @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4453: d0a0bca2194a673c4d9a70a2256837c59213c64b @ git://anongit.freedesktop.org/piglit


== Linux commits ==

3c7cfe517d43 drm/i915/guc: Assert we have the doorbell before setting it up

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8854/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/guc: Assert we have the doorbell before setting it up

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Assert we have the doorbell before setting it up
URL   : https://patchwork.freedesktop.org/series/42509/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4116_full -> Patchwork_8854_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8854_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8854_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42509/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8854_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd2:
      shard-kbl:          SKIP -> PASS

    igt@gem_mocs_settings@mocs-rc6-bsd1:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in Patchwork_8854_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#106087)

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@flip-vs-absolute-wf_vblank:
      shard-hsw:          PASS -> FAIL (fdo#103928)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
      shard-hsw:          PASS -> FAIL (fdo#103481)

    igt@kms_plane@plane-position-hole-dpms-pipe-c-planes:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_workarounds@suspend-resume:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    igt@kms_flip@flip-vs-blocking-wf-vblank:
      shard-kbl:          DMESG-FAIL (fdo#103558) -> PASS +1

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-kbl:          DMESG-WARN (fdo#103313, fdo#105602, fdo#103558) -> PASS

    igt@kms_flip@plain-flip-fb-recreate-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_flip_event_leak:
      shard-kbl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +33

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (9 -> 8) ==

  Missing    (1): shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4116 -> Patchwork_8854

  CI_DRM_4116: 7ff375cb94000d93f7a9d541cb0c8180fbea80f2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4453: 29ae12bd764e3b1876356e7628a32192b4ec9066 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8854: 3c7cfe517d43568f8810ec93c15d51c2d8b645fe @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4453: d0a0bca2194a673c4d9a70a2256837c59213c64b @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8854/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[Michel Thierry]

On 5/1/2018 12:52 AM, Chris Wilson wrote:
> As our early doorbell is split between early allocation and a late setup
> after we have a channel to the GuC, it may happen due to a lapse of
> programmer judgement that we try to setup an invalid doorbell. Make use
> of our has_doorbell() function to check the doorbell does exist for the
> client before we try and tell the guc about it. In doing so, we prevent
> the compiler from warning about the otherwise unused function in some
> configurations.
> 

Looks ok to me, but the new place has_doorbell is called is inside a 
GEM_BUG_ON...
So the warning will still be there when CONFIG_DRM_I915_DEBUG_GEM=n, right?

(btw, until late last year, there where more users of that function).

> Reported-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_submission.c | 22 +++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 6e6ed0f46bd3..c6bb5bebddfc 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -124,9 +124,17 @@ static int reserve_doorbell(struct intel_guc_client *client)
>   	return 0;
>   }
>   
> +static bool has_doorbell(struct intel_guc_client *client)
> +{
> +	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> +		return false;
> +
> +	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> +}
> +
>   static void unreserve_doorbell(struct intel_guc_client *client)
>   {
> -	GEM_BUG_ON(client->doorbell_id == GUC_DOORBELL_INVALID);
> +	GEM_BUG_ON(!has_doorbell(client));
>   
>   	__clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
>   	client->doorbell_id = GUC_DOORBELL_INVALID;
> @@ -184,14 +192,6 @@ static struct guc_doorbell_info *__get_doorbell(struct intel_guc_client *client)
>   	return client->vaddr + client->doorbell_offset;
>   }
>   
> -static bool has_doorbell(struct intel_guc_client *client)
> -{
> -	if (client->doorbell_id == GUC_DOORBELL_INVALID)
> -		return false;
> -
> -	return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> -}
> -
>   static void __create_doorbell(struct intel_guc_client *client)
>   {
>   	struct guc_doorbell_info *doorbell;
> @@ -207,7 +207,6 @@ static void __destroy_doorbell(struct intel_guc_client *client)
>   	struct guc_doorbell_info *doorbell;
>   	u16 db_id = client->doorbell_id;
>   
> -
>   	doorbell = __get_doorbell(client);
>   	doorbell->db_status = GUC_DOORBELL_DISABLED;
>   	doorbell->cookie = 0;
> @@ -224,6 +223,9 @@ static int create_doorbell(struct intel_guc_client *client)
>   {
>   	int ret;
>   
> +	if (WARN_ON(!has_doorbell(client)))
> +		return -ENODEV; /* internal setup error, should never happen */
> +
>   	__update_doorbell_desc(client, client->doorbell_id);
>   	__create_doorbell(client);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[Chris Wilson]

Quoting Michel Thierry (2018-05-01 15:21:53)
> On 5/1/2018 12:52 AM, Chris Wilson wrote:
> > As our early doorbell is split between early allocation and a late setup
> > after we have a channel to the GuC, it may happen due to a lapse of
> > programmer judgement that we try to setup an invalid doorbell. Make use
> > of our has_doorbell() function to check the doorbell does exist for the
> > client before we try and tell the guc about it. In doing so, we prevent
> > the compiler from warning about the otherwise unused function in some
> > configurations.
> > 
> 
> Looks ok to me, but the new place has_doorbell is called is inside a 
> GEM_BUG_ON...
> So the warning will still be there when CONFIG_DRM_I915_DEBUG_GEM=n, right?

> > @@ -224,6 +223,9 @@ static int create_doorbell(struct intel_guc_client *client)
> >   {
> >       int ret;
> >   
> > +     if (WARN_ON(!has_doorbell(client)))
> > +             return -ENODEV; /* internal setup error, should never happen */

This is the one I added to make sure we had at least one user. If it
weren't for the compiler warning I'd be happy for this to be
GEM_BUG_ON() as well.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[Michel Thierry]

On 05/02/2018 02:11 AM, Chris Wilson wrote:
> Quoting Michel Thierry (2018-05-01 15:21:53)
>> On 5/1/2018 12:52 AM, Chris Wilson wrote:
>>> As our early doorbell is split between early allocation and a late setup
>>> after we have a channel to the GuC, it may happen due to a lapse of
>>> programmer judgement that we try to setup an invalid doorbell. Make use
>>> of our has_doorbell() function to check the doorbell does exist for the
>>> client before we try and tell the guc about it. In doing so, we prevent
>>> the compiler from warning about the otherwise unused function in some
>>> configurations.
>>>
>>
>> Looks ok to me, but the new place has_doorbell is called is inside a
>> GEM_BUG_ON...
>> So the warning will still be there when CONFIG_DRM_I915_DEBUG_GEM=n, right?
> 
>>> @@ -224,6 +223,9 @@ static int create_doorbell(struct intel_guc_client *client)
>>>    {
>>>        int ret;
>>>    
>>> +     if (WARN_ON(!has_doorbell(client)))
>>> +             return -ENODEV; /* internal setup error, should never happen */

Sorry, somehow I read that line as GEM_WARN_ON...

> 
> This is the one I added to make sure we had at least one user. If it
> weren't for the compiler warning I'd be happy for this to be
> GEM_BUG_ON() as well.
> -Chris
> 

Reviewed-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert we have the doorbell before setting it up

[Chris Wilson]

Quoting Michel Thierry (2018-05-02 17:29:41)
> On 05/02/2018 02:11 AM, Chris Wilson wrote:
> > Quoting Michel Thierry (2018-05-01 15:21:53)
> >> On 5/1/2018 12:52 AM, Chris Wilson wrote:
> >>> As our early doorbell is split between early allocation and a late setup
> >>> after we have a channel to the GuC, it may happen due to a lapse of
> >>> programmer judgement that we try to setup an invalid doorbell. Make use
> >>> of our has_doorbell() function to check the doorbell does exist for the
> >>> client before we try and tell the guc about it. In doing so, we prevent
> >>> the compiler from warning about the otherwise unused function in some
> >>> configurations.
> >>>
> >>
> >> Looks ok to me, but the new place has_doorbell is called is inside a
> >> GEM_BUG_ON...
> >> So the warning will still be there when CONFIG_DRM_I915_DEBUG_GEM=n, right?
> > 
> >>> @@ -224,6 +223,9 @@ static int create_doorbell(struct intel_guc_client *client)
> >>>    {
> >>>        int ret;
> >>>    
> >>> +     if (WARN_ON(!has_doorbell(client)))
> >>> +             return -ENODEV; /* internal setup error, should never happen */
> 
> Sorry, somehow I read that line as GEM_WARN_ON...
> 
> > 
> > This is the one I added to make sure we had at least one user. If it
> > weren't for the compiler warning I'd be happy for this to be
> > GEM_BUG_ON() as well.
> > -Chris
> > 
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Ta, pushed alongside the clang warning suppressions.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx