| Message ID | 20240328080005.410961-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] drm/xe/display: check for error on drmm_mutex_init | expand |
Hi Arun, ... > - drmm_mutex_init(&xe->drm, &xe->sb_lock); > - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); > - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); > - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); > - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); > - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); > + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || > + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || > + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || > + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || > + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || > + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) > + return -ENOMEM; why not extract the value from drmm_mutex_init()? it would make the code a bit more complex, but better than forcing a -ENOMEM return. err = drmm_mutex_init(...) if (err) return err; err = drmm_mutex_init(...) if (err) return err; err = drmm_mutex_init(...) if (err) return err; ... On the other hand drmm_mutex_init(), as of now returns only -ENOMEM, but it's a bad practice to assume it will always do. I'd rather prefer not to check the error value at all. Andi > xe->enabled_irq_mask = ~0; > > err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL); > -- > 2.25.1
On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: > Hi Arun, > > ... > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || >> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) >> + return -ENOMEM; > > why not extract the value from drmm_mutex_init()? it would make > the code a bit more complex, but better than forcing a -ENOMEM > return. > > err = drmm_mutex_init(...) > if (err) > return err; > > err = drmm_mutex_init(...) > if (err) > return err; > > err = drmm_mutex_init(...) > if (err) > return err; > > ... > > On the other hand drmm_mutex_init(), as of now returns only > -ENOMEM, but it's a bad practice to assume it will always do. I'd > rather prefer not to check the error value at all. And round and round we go. This is exactly what v1 was [1], but it's not clear because the patch doesn't have a changelog. This is all utterly ridiculous compared to *why* we even have or use drmm_mutex_init(). Managed initialization causes more trouble here than it gains us. Gah. BR, Jani. [1] https://lore.kernel.org/r/ki4ynsl4nmhavf63vzdlt2xkedjo7p7iouzvcksvki3okgz6ak@twlznnlo3g22 > > Andi > >> xe->enabled_irq_mask = ~0; >> >> err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL); >> -- >> 2.25.1
Hi Jani, On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); > >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); > >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); > >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); > >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); > >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); > >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || > >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || > >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || > >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || > >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || > >> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) > >> + return -ENOMEM; > > > > why not extract the value from drmm_mutex_init()? it would make > > the code a bit more complex, but better than forcing a -ENOMEM > > return. > > > > err = drmm_mutex_init(...) > > if (err) > > return err; > > > > err = drmm_mutex_init(...) > > if (err) > > return err; > > > > err = drmm_mutex_init(...) > > if (err) > > return err; > > > > ... > > > > On the other hand drmm_mutex_init(), as of now returns only > > -ENOMEM, but it's a bad practice to assume it will always do. I'd > > rather prefer not to check the error value at all. > > And round and round we go. This is exactly what v1 was [1], but it's not > clear because the patch doesn't have a changelog. ha! funny! I missed v1. > This is all utterly ridiculous compared to *why* we even have or use > drmm_mutex_init(). Managed initialization causes more trouble here than > it gains us. Gah. As I wrote here: > > I'd rather prefer not to check the error value at all. we could rather drop this patch. Checking the error value is always good, but checking implausible errors with this price is not really worth it. At the end drmm_mutex_init() should make our life easier. Andi
> -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Thursday, March 28, 2024 4:16 PM > To: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com>; Murthy, Arun R > <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org; intel- > xe@lists.freedesktop.org > Subject: Re: [PATCHv2] drm/xe/display: check for error on drmm_mutex_init > > Hi Jani, > > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: > > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); > > >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); > > >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); > > >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); > > >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); > > >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); > > >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || > > >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || > > >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || > > >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || > > >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || > > >> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) > > >> + return -ENOMEM; > > > > > > why not extract the value from drmm_mutex_init()? it would make the > > > code a bit more complex, but better than forcing a -ENOMEM return. > > > > > > err = drmm_mutex_init(...) > > > if (err) > > > return err; > > > > > > err = drmm_mutex_init(...) > > > if (err) > > > return err; > > > > > > err = drmm_mutex_init(...) > > > if (err) > > > return err; > > > > > > ... > > > > > > On the other hand drmm_mutex_init(), as of now returns only -ENOMEM, The function is also returning -ENOMEM and there is no other error code returned from the error. > > > but it's a bad practice to assume it will always do. I'd rather > > > prefer not to check the error value at all. > > > > And round and round we go. This is exactly what v1 was [1], but it's > > not clear because the patch doesn't have a changelog. > > ha! funny! I missed v1. > > > This is all utterly ridiculous compared to *why* we even have or use > > drmm_mutex_init(). Managed initialization causes more trouble here > > than it gains us. Gah. > > As I wrote here: > > > > I'd rather prefer not to check the error value at all. > > we could rather drop this patch. Checking the error value is always good, but > checking implausible errors with this price is not really worth it. This is reported as error from Coverity. My suggestion was also to discard this error from Coverity but the same API used in other places in our driver is considering the return value. Thanks and Regards, Arun R Murthy -------------------- > > At the end drmm_mutex_init() should make our life easier. > > Andi
Hi Arun, > > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > > > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: > > > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); > > > >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); > > > >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); > > > >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); > > > >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); > > > >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); > > > >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || > > > >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || > > > >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || > > > >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || > > > >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || > > > >> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) > > > >> + return -ENOMEM; > > > > > > > > why not extract the value from drmm_mutex_init()? it would make the > > > > code a bit more complex, but better than forcing a -ENOMEM return. > > > > > > > > err = drmm_mutex_init(...) > > > > if (err) > > > > return err; > > > > > > > > err = drmm_mutex_init(...) > > > > if (err) > > > > return err; > > > > > > > > err = drmm_mutex_init(...) > > > > if (err) > > > > return err; > > > > > > > > ... > > > > > > > > On the other hand drmm_mutex_init(), as of now returns only -ENOMEM, > > The function is also returning -ENOMEM and there is no other error code returned from the error. yes, but it's wrong to assume this will always be true. > > > > but it's a bad practice to assume it will always do. I'd rather > > > > prefer not to check the error value at all. > > > > > > And round and round we go. This is exactly what v1 was [1], but it's > > > not clear because the patch doesn't have a changelog. > > > > ha! funny! I missed v1. > > > > > This is all utterly ridiculous compared to *why* we even have or use > > > drmm_mutex_init(). Managed initialization causes more trouble here > > > than it gains us. Gah. > > > > As I wrote here: > > > > > > I'd rather prefer not to check the error value at all. > > > > we could rather drop this patch. Checking the error value is always good, but > > checking implausible errors with this price is not really worth it. > > This is reported as error from Coverity. My suggestion was also to discard this error from Coverity but the same API used in other places in our driver is considering the return value. Strictly speaking, coverity is right and if I have to choose, I'd prefer your v1. v2, in my opinion, is wrong, even if it's more nice and readable. Thanks, Andi
> -----Original Message----- > From: Andi Shyti <andi.shyti@linux.intel.com> > Sent: Thursday, March 28, 2024 4:38 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com>; Jani Nikula > <jani.nikula@linux.intel.com>; intel-gfx@lists.freedesktop.org; intel- > xe@lists.freedesktop.org > Subject: Re: [PATCHv2] drm/xe/display: check for error on drmm_mutex_init > > Hi Arun, > > > > On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: > > > > On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: > > > > >> - drmm_mutex_init(&xe->drm, &xe->sb_lock); > > > > >> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); > > > > >> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); > > > > >> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); > > > > >> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); > > > > >> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); > > > > >> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || > > > > >> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || > > > > >> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || > > > > >> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) > || > > > > >> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || > > > > >> + drmm_mutex_init(&xe->drm, &xe- > >display.hdcp.hdcp_mutex)) > > > > >> + return -ENOMEM; > > > > > > > > > > why not extract the value from drmm_mutex_init()? it would make > > > > > the code a bit more complex, but better than forcing a -ENOMEM > return. > > > > > > > > > > err = drmm_mutex_init(...) > > > > > if (err) > > > > > return err; > > > > > > > > > > err = drmm_mutex_init(...) > > > > > if (err) > > > > > return err; > > > > > > > > > > err = drmm_mutex_init(...) > > > > > if (err) > > > > > return err; > > > > > > > > > > ... > > > > > > > > > > On the other hand drmm_mutex_init(), as of now returns only > > > > > -ENOMEM, > > > > The function is also returning -ENOMEM and there is no other error code > returned from the error. > > yes, but it's wrong to assume this will always be true. > > > > > > but it's a bad practice to assume it will always do. I'd rather > > > > > prefer not to check the error value at all. > > > > > > > > And round and round we go. This is exactly what v1 was [1], but > > > > it's not clear because the patch doesn't have a changelog. > > > > > > ha! funny! I missed v1. > > > > > > > This is all utterly ridiculous compared to *why* we even have or > > > > use drmm_mutex_init(). Managed initialization causes more trouble > > > > here than it gains us. Gah. > > > > > > As I wrote here: > > > > > > > > I'd rather prefer not to check the error value at all. > > > > > > we could rather drop this patch. Checking the error value is always > > > good, but checking implausible errors with this price is not really worth it. > > > > This is reported as error from Coverity. My suggestion was also to discard this > error from Coverity but the same API used in other places in our driver is > considering the return value. > > Strictly speaking, coverity is right and if I have to choose, I'd prefer your v1. v2, > in my opinion, is wrong, even if it's more nice and readable. > Thanks for you comments, will wait to see any more comments and if not will refloat v1 patch version. Thanks and Regards, Arun R Murthy ------------------- > Thanks, > Andi
On Thu, Mar 28, 2024 at 12:33:09PM +0200, Jani Nikula wrote: >On Thu, 28 Mar 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote: >> Hi Arun, >> >> ... >> >>> - drmm_mutex_init(&xe->drm, &xe->sb_lock); >>> - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); >>> - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); >>> - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); >>> - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); >>> - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); >>> + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || >>> + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || >>> + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || >>> + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || >>> + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || >>> + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) >>> + return -ENOMEM; My suggestion from v1 was to assign and check the return value, not to hardcode the return like done here. Now we have a v3 going back to v1 and we never had what was suggested. Why? Let me be explicit and type it: if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) || (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) || (err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex)) || (err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex)) || (err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex)) || (err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex))) return err; I also said I usually don't like assign + check in the same statement, but all the alternatives I've seen here are worse. However it turns out all of these display mutex initializations are actually wrong after commit 3fef3e6ff86a ("drm/i915: move display mutex inits to display code"), which predates xe in the tree. drivers/gpu/drm/i915/i915_driver.c: intel_display_driver_early_probe(dev_priv); drivers/gpu/drm/xe/display/xe_display.c: intel_display_driver_early_probe(xe); So intel_display_driver_early_probe() is actually called from xe, which does the mutex_init() (and misses the mutex_destroy()). Am I missing anything? >> why not extract the value from drmm_mutex_init()? it would make >> the code a bit more complex, but better than forcing a -ENOMEM >> return. >> >> err = drmm_mutex_init(...) >> if (err) >> return err; >> >> err = drmm_mutex_init(...) >> if (err) >> return err; >> >> err = drmm_mutex_init(...) >> if (err) >> return err; >> >> ... >> >> On the other hand drmm_mutex_init(), as of now returns only >> -ENOMEM, but it's a bad practice to assume it will always do. I'd >> rather prefer not to check the error value at all. > >And round and round we go. This is exactly what v1 was [1], but it's not >clear because the patch doesn't have a changelog. > >This is all utterly ridiculous compared to *why* we even have or use >drmm_mutex_init(). Managed initialization causes more trouble here than >it gains us. Gah. I think managed initialization make sense to keep the teardown/unwind part sane (which is often not tested). However drmm_mutex_init() maybe is overkill indeed. We started using it because people often forget the mutex_destroy() and drm/ as whole started using it. Compare: git grep mutex_init -- drivers/gpu/drm/i915/ git grep mutex_destroy -- drivers/gpu/drm/i915/ This is only an issue when mutex_init does more than init, which is the case with CONFIG_PREEMPT_RT + CONFIG_DEBUG_MUTEXES, which most people don't have set so they don't see it, CI doesn't see it, but it causes problems for people who have that set. Maybe what we could have would be a drmm_mutex_vinit(mutex, ...) so we can do: err = drmm_mutex_vinit(&xe->drm, &xe->sb_lock, &xe->display.backlight.lock, ..., NULL); if (err) return err; or... just stop using drmm_mutex_init and add the destroy. No need for unwind as mutex_init() can't fail. We still need to keep the destroy explicit, but I think that would be fine (and doesn't cause 1 allocation per mutex). Lucas De Marchi > >BR, >Jani. > > >[1] https://lore.kernel.org/r/ki4ynsl4nmhavf63vzdlt2xkedjo7p7iouzvcksvki3okgz6ak@twlznnlo3g22 > > >> >> Andi >> >>> xe->enabled_irq_mask = ~0; >>> >>> err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL); >>> -- >>> 2.25.1 > >-- >Jani Nikula, Intel
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c index e4db069f0db3..b2f58b3afabe 100644 --- a/drivers/gpu/drm/xe/display/xe_display.c +++ b/drivers/gpu/drm/xe/display/xe_display.c @@ -107,12 +107,14 @@ int xe_display_create(struct xe_device *xe) xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0); - drmm_mutex_init(&xe->drm, &xe->sb_lock); - drmm_mutex_init(&xe->drm, &xe->display.backlight.lock); - drmm_mutex_init(&xe->drm, &xe->display.audio.mutex); - drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex); - drmm_mutex_init(&xe->drm, &xe->display.pps.mutex); - drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex); + if (drmm_mutex_init(&xe->drm, &xe->sb_lock) || + drmm_mutex_init(&xe->drm, &xe->display.backlight.lock) || + drmm_mutex_init(&xe->drm, &xe->display.audio.mutex) || + drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex) || + drmm_mutex_init(&xe->drm, &xe->display.pps.mutex) || + drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex)) + return -ENOMEM; + xe->enabled_irq_mask = ~0; err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
Check return value for drmm_mutex_init as it can fail and return on failure. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/xe/display/xe_display.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)