[PATCH] drm/xe/display: check for error on drmm_mutex_init

[PATCH] drm/xe/display: check for error on drmm_mutex_init

[Arun R Murthy]

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 | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index e4db069f0db3..c59fa832758d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -107,12 +107,24 @@ 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);
+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
+	if (err)
+		return err;
+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
+	if (err)
+		return err;
 	xe->enabled_irq_mask = ~0;
 
 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
-- 
2.25.1

✗ Fi.CI.BAT: failure for drm/xe/display: check for error on drmm_mutex_init

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8942 bytes --]

== Series Details ==

Series: drm/xe/display: check for error on drmm_mutex_init
URL   : https://patchwork.freedesktop.org/series/131301/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14444 -> Patchwork_131301v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_131301v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_131301v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/index.html

Participating hosts (34 -> 35)
------------------------------

  Additional (2): bat-adlm-1 fi-kbl-8809g 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - bat-arls-1:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14444/bat-arls-1/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-arls-1/igt@i915_selftest@live@hangcheck.html

  
Known issues
------------

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

### CI changes ###

#### Issues hit ####

  * boot:
    - bat-arls-3:         [PASS][3] -> [FAIL][4] ([i915#10234])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14444/bat-arls-3/boot.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-arls-3/boot.html
    - fi-kbl-8809g:       NOTRUN -> [FAIL][5] ([i915#8293])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/fi-kbl-8809g/boot.html

  
#### Possible fixes ####

  * boot:
    - fi-apl-guc:         [FAIL][6] ([i915#8293]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14444/fi-apl-guc/boot.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/fi-apl-guc/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-adlm-1:         NOTRUN -> [SKIP][8] ([i915#3826])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@eof:
    - bat-adlm-1:         NOTRUN -> [SKIP][9] ([i915#2582]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@fbdev@eof.html

  * igt@fbdev@info:
    - bat-adlm-1:         NOTRUN -> [SKIP][10] ([i915#1849] / [i915#2582])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@fbdev@info.html

  * igt@gem_lmem_swapping@basic:
    - fi-apl-guc:         NOTRUN -> [SKIP][11] ([i915#4613]) +3 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/fi-apl-guc/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adlm-1:         NOTRUN -> [SKIP][12] ([i915#4613]) +3 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_tiled_pread_basic:
    - bat-adlm-1:         NOTRUN -> [SKIP][13] ([i915#3282])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-adlm-1:         NOTRUN -> [SKIP][14] ([i915#6621])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@i915_pm_rps@basic-api.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - bat-adlm-1:         NOTRUN -> [SKIP][15] ([i915#9875] / [i915#9900]) +16 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_flip@basic-plain-flip:
    - bat-adlm-1:         NOTRUN -> [SKIP][16] ([i915#3637]) +3 other tests skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_flip@basic-plain-flip.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-adlm-1:         NOTRUN -> [SKIP][17]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-adlm-1:         NOTRUN -> [SKIP][18] ([i915#1849] / [i915#4342])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-apl-guc:         NOTRUN -> [SKIP][19] +17 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/fi-apl-guc/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-adlm-1:         NOTRUN -> [SKIP][20] ([i915#5354])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - bat-adlm-1:         NOTRUN -> [SKIP][21] ([i915#9673] / [i915#9732]) +3 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-adlm-1:         NOTRUN -> [SKIP][22] ([i915#3555])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-adlm-1:         NOTRUN -> [SKIP][23] ([i915#3708] / [i915#9900])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-write:
    - bat-adlm-1:         NOTRUN -> [SKIP][24] ([i915#3708]) +2 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-adlm-1/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gem_contexts:
    - bat-atsm-1:         [ABORT][25] ([i915#10366]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14444/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@hangcheck:
    - bat-rpls-3:         [DMESG-WARN][27] ([i915#5591]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14444/bat-rpls-3/igt@i915_selftest@live@hangcheck.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/bat-rpls-3/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10234]: https://gitlab.freedesktop.org/drm/intel/issues/10234
  [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366
  [i915#10436]: https://gitlab.freedesktop.org/drm/intel/issues/10436
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#4342]: https://gitlab.freedesktop.org/drm/intel/issues/4342
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
  [i915#9875]: https://gitlab.freedesktop.org/drm/intel/issues/9875
  [i915#9900]: https://gitlab.freedesktop.org/drm/intel/issues/9900


Build changes
-------------

  * Linux: CI_DRM_14444 -> Patchwork_131301v1

  CI-20190529: 20190529
  CI_DRM_14444: e8f2ef9ff5486b3bbfc599bf89e10e3bdecd96b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7769: 7769
  Patchwork_131301v1: e8f2ef9ff5486b3bbfc599bf89e10e3bdecd96b9 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

9df520c0eda2 drm/xe/display: check for error on drmm_mutex_init

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_131301v1/index.html

[-- Attachment #2: Type: text/html, Size: 10458 bytes --]

Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init

[Lucas De Marchi]

On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
>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 | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>index e4db069f0db3..c59fa832758d 100644
>--- a/drivers/gpu/drm/xe/display/xe_display.c
>+++ b/drivers/gpu/drm/xe/display/xe_display.c
>@@ -107,12 +107,24 @@ 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);
>+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
>+	if (err)
>+		return err;
>+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
>+	if (err)
>+		return err;


humn... but not very pretty. What about?

	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
	    (err = ...))
		return err;

I think there are few places in life for assignment + check in single
statement, but IMO this is one of them where the alternative is uglier
and more error prone.

thoughts?

Lucas De Marchi

> 	xe->enabled_irq_mask = ~0;
>
> 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
>-- 
>2.25.1
>

RE: [PATCH] drm/xe/display: check for error on drmm_mutex_init

[Murthy, Arun R]

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi@intel.com>
> Sent: Wednesday, March 20, 2024 6:06 AM
> To: Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init
> 
> On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
> >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 | 24 ++++++++++++++++++------
> > 1 file changed, 18 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/xe/display/xe_display.c
> >b/drivers/gpu/drm/xe/display/xe_display.c
> >index e4db069f0db3..c59fa832758d 100644
> >--- a/drivers/gpu/drm/xe/display/xe_display.c
> >+++ b/drivers/gpu/drm/xe/display/xe_display.c
> >@@ -107,12 +107,24 @@ 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);
> >+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
> >+	if (err)
> >+		return err;
> >+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
> >+	if (err)
> >+		return err;
> 
> 
> humn... but not very pretty. What about?
> 
> 	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
> 	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
> 	    (err = ...))
> 		return err;
> 
> I think there are few places in life for assignment + check in single statement,
> but IMO this is one of them where the alternative is uglier and more error
> prone.
> 
> thoughts?
> 

We should not proceed with the remaining mutex_init in case of failures. As an alternative we can have 
drmm_mutex_init(var1) ? (drmm_mutex_init(var2) ? drmm_mutex_init(var3) : return ret) : return ret;

With the existing one traversing the code is more easier, these optimization might make the code look complex.

Thanks and Regards,
Arun R Murthy
--------------------
> Lucas De Marchi
> 
> > 	xe->enabled_irq_mask = ~0;
> >
> > 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
> >--
> >2.25.1
> >

Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init

[Lucas De Marchi]

On Thu, Mar 21, 2024 at 05:04:51AM +0000, Murthy, Arun R wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi@intel.com>
>> Sent: Wednesday, March 20, 2024 6:06 AM
>> To: Murthy, Arun R <arun.r.murthy@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init
>>
>> On Tue, Mar 19, 2024 at 08:33:41AM +0530, Arun R Murthy wrote:
>> >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 | 24 ++++++++++++++++++------
>> > 1 file changed, 18 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/xe/display/xe_display.c
>> >b/drivers/gpu/drm/xe/display/xe_display.c
>> >index e4db069f0db3..c59fa832758d 100644
>> >--- a/drivers/gpu/drm/xe/display/xe_display.c
>> >+++ b/drivers/gpu/drm/xe/display/xe_display.c
>> >@@ -107,12 +107,24 @@ 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);
>> >+	err = drmm_mutex_init(&xe->drm, &xe->sb_lock);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.audio.mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.wm.wm_mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.pps.mutex);
>> >+	if (err)
>> >+		return err;
>> >+	err = drmm_mutex_init(&xe->drm, &xe->display.hdcp.hdcp_mutex);
>> >+	if (err)
>> >+		return err;
>>
>>
>> humn... but not very pretty. What about?
>>
>> 	if ((err = drmm_mutex_init(&xe->drm, &xe->sb_lock)) ||
>> 	    (err = drmm_mutex_init(&xe->drm, &xe->display.backlight.lock)) ||
>> 	    (err = ...))
>> 		return err;
>>
>> I think there are few places in life for assignment + check in single statement,
>> but IMO this is one of them where the alternative is uglier and more error
>> prone.
>>
>> thoughts?
>>
>
>We should not proceed with the remaining mutex_init in case of failures. As an alternative we can have

with the code above, we are not proceeding with the other drmm_mutex_init() initializations.

foo() || bar() doesn't execute bar() if foo() returned != 0.

Lucas De Marchi

>drmm_mutex_init(var1) ? (drmm_mutex_init(var2) ? drmm_mutex_init(var3) : return ret) : return ret;
>
>With the existing one traversing the code is more easier, these optimization might make the code look complex.
>
>Thanks and Regards,
>Arun R Murthy
>--------------------
>> Lucas De Marchi
>>
>> > 	xe->enabled_irq_mask = ~0;
>> >
>> > 	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
>> >--
>> >2.25.1
>> >

[PATCH] drm/xe/display: check for error on drmm_mutex_init

[Arun R Murthy]

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(-)

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index e4db069f0db3..ac2e58d1fa82 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);
-- 
2.25.1

Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init

[Jani Nikula]

On Thu, 21 Mar 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> 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(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index e4db069f0db3..ac2e58d1fa82 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)))

Excessive parentheses.

BR,
Jani.

> +		return -ENOMEM;
> +
>  	xe->enabled_irq_mask = ~0;
>  
>  	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);

-- 
Jani Nikula, Intel

Re: [PATCH] drm/xe/display: check for error on drmm_mutex_init

[Lucas De Marchi]

On Thu, Mar 21, 2024 at 11:31:24AM +0530, Arun R Murthy wrote:
>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(-)
>
>diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
>index e4db069f0db3..ac2e58d1fa82 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)) ||

     	   ^^

you only need 2 parenthesis if you were going to record the return.

Lucas De Marchi