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

2024-03-22 Thread 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 
---
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(>drm, >sb_lock);
-   drmm_mutex_init(>drm, >display.backlight.lock);
-   drmm_mutex_init(>drm, >display.audio.mutex);
-   drmm_mutex_init(>drm, >display.wm.wm_mutex);
-   drmm_mutex_init(>drm, >display.pps.mutex);
-   drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   if ((drmm_mutex_init(>drm, >sb_lock)) ||


   ^^

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

Lucas De Marchi

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

2024-03-21 Thread Jani Nikula 
On Thu, 21 Mar 2024, 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 
> ---
>  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(>drm, >sb_lock);
> - drmm_mutex_init(>drm, >display.backlight.lock);
> - drmm_mutex_init(>drm, >display.audio.mutex);
> - drmm_mutex_init(>drm, >display.wm.wm_mutex);
> - drmm_mutex_init(>drm, >display.pps.mutex);
> - drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
> + if ((drmm_mutex_init(>drm, >sb_lock)) ||
> + (drmm_mutex_init(>drm, >display.backlight.lock)) ||
> + (drmm_mutex_init(>drm, >display.audio.mutex)) ||
> + (drmm_mutex_init(>drm, >display.wm.wm_mutex)) ||
> + (drmm_mutex_init(>drm, >display.pps.mutex)) ||
> + (drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex)))

Excessive parentheses.

BR,
Jani.

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

-- 
Jani Nikula, Intel

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

2024-03-21 Thread Arun R Murthy 
Check return value for drmm_mutex_init as it can fail and return on
failure.

Signed-off-by: Arun R Murthy 
---
 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(>drm, >sb_lock);
-   drmm_mutex_init(>drm, >display.backlight.lock);
-   drmm_mutex_init(>drm, >display.audio.mutex);
-   drmm_mutex_init(>drm, >display.wm.wm_mutex);
-   drmm_mutex_init(>drm, >display.pps.mutex);
-   drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   if ((drmm_mutex_init(>drm, >sb_lock)) ||
+   (drmm_mutex_init(>drm, >display.backlight.lock)) ||
+   (drmm_mutex_init(>drm, >display.audio.mutex)) ||
+   (drmm_mutex_init(>drm, >display.wm.wm_mutex)) ||
+   (drmm_mutex_init(>drm, >display.pps.mutex)) ||
+   (drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex)))
+   return -ENOMEM;
+
xe->enabled_irq_mask = ~0;
 
err = drmm_add_action_or_reset(>drm, display_destroy, NULL);
-- 
2.25.1

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

2024-03-20 Thread Lucas De Marchi 

On Thu, Mar 21, 2024 at 05:04:51AM +, Murthy, Arun R wrote:




-Original Message-
From: De Marchi, Lucas 
Sent: Wednesday, March 20, 2024 6:06 AM
To: Murthy, Arun R 
Cc: intel-gfx@lists.freedesktop.org; intel...@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 
>---
> 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(>drm, >sb_lock);
>-   drmm_mutex_init(>drm, >display.backlight.lock);
>-   drmm_mutex_init(>drm, >display.audio.mutex);
>-   drmm_mutex_init(>drm, >display.wm.wm_mutex);
>-   drmm_mutex_init(>drm, >display.pps.mutex);
>-   drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
>+   err = drmm_mutex_init(>drm, >sb_lock);
>+   if (err)
>+   return err;
>+   err = drmm_mutex_init(>drm, >display.backlight.lock);
>+   if (err)
>+   return err;
>+   err = drmm_mutex_init(>drm, >display.audio.mutex);
>+   if (err)
>+   return err;
>+   err = drmm_mutex_init(>drm, >display.wm.wm_mutex);
>+   if (err)
>+   return err;
>+   err = drmm_mutex_init(>drm, >display.pps.mutex);
>+   if (err)
>+   return err;
>+   err = drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
>+   if (err)
>+   return err;


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

if ((err = drmm_mutex_init(>drm, >sb_lock)) ||
(err = drmm_mutex_init(>drm, >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(>drm, display_destroy, NULL);
>--
>2.25.1
>

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

2024-03-20 Thread Murthy, Arun R 



> -Original Message-
> From: De Marchi, Lucas 
> Sent: Wednesday, March 20, 2024 6:06 AM
> To: Murthy, Arun R 
> Cc: intel-gfx@lists.freedesktop.org; intel...@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 
> >---
> > 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(>drm, >sb_lock);
> >-drmm_mutex_init(>drm, >display.backlight.lock);
> >-drmm_mutex_init(>drm, >display.audio.mutex);
> >-drmm_mutex_init(>drm, >display.wm.wm_mutex);
> >-drmm_mutex_init(>drm, >display.pps.mutex);
> >-drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
> >+err = drmm_mutex_init(>drm, >sb_lock);
> >+if (err)
> >+return err;
> >+err = drmm_mutex_init(>drm, >display.backlight.lock);
> >+if (err)
> >+return err;
> >+err = drmm_mutex_init(>drm, >display.audio.mutex);
> >+if (err)
> >+return err;
> >+err = drmm_mutex_init(>drm, >display.wm.wm_mutex);
> >+if (err)
> >+return err;
> >+err = drmm_mutex_init(>drm, >display.pps.mutex);
> >+if (err)
> >+return err;
> >+err = drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
> >+if (err)
> >+return err;
> 
> 
> humn... but not very pretty. What about?
> 
>   if ((err = drmm_mutex_init(>drm, >sb_lock)) ||
>   (err = drmm_mutex_init(>drm, >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(>drm, display_destroy, NULL);
> >--
> >2.25.1
> >

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

2024-03-19 Thread 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 
---
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(>drm, >sb_lock);
-   drmm_mutex_init(>drm, >display.backlight.lock);
-   drmm_mutex_init(>drm, >display.audio.mutex);
-   drmm_mutex_init(>drm, >display.wm.wm_mutex);
-   drmm_mutex_init(>drm, >display.pps.mutex);
-   drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   err = drmm_mutex_init(>drm, >sb_lock);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.backlight.lock);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.audio.mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.wm.wm_mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.pps.mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   if (err)
+   return err;



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

if ((err = drmm_mutex_init(>drm, >sb_lock)) ||
(err = drmm_mutex_init(>drm, >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(>drm, display_destroy, NULL);
--
2.25.1

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

2024-03-18 Thread Arun R Murthy 
Check return value for drmm_mutex_init as it can fail and return on
failure.

Signed-off-by: Arun R Murthy 
---
 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(>drm, >sb_lock);
-   drmm_mutex_init(>drm, >display.backlight.lock);
-   drmm_mutex_init(>drm, >display.audio.mutex);
-   drmm_mutex_init(>drm, >display.wm.wm_mutex);
-   drmm_mutex_init(>drm, >display.pps.mutex);
-   drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   err = drmm_mutex_init(>drm, >sb_lock);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.backlight.lock);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.audio.mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.wm.wm_mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.pps.mutex);
+   if (err)
+   return err;
+   err = drmm_mutex_init(>drm, >display.hdcp.hdcp_mutex);
+   if (err)
+   return err;
xe->enabled_irq_mask = ~0;
 
err = drmm_add_action_or_reset(>drm, display_destroy, NULL);
-- 
2.25.1