| Message ID | 20180501075203.12458-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > >
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
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>
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
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);
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(-)