[PATCH 1/2] nouveau/bl: Assign different names to interfaces

[PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

Currently, every backlight interface created by Nouveau uses the same name,
nv_backlight. This leads to a sysfs warning as it tries to create an already
existing folder. This patch adds a incremented number to the name, but keeps
the initial name as nv_backlight, to avoid possibly breaking userspace; the
second interface will be named nv_backlight1, and so on.

Fixes: fdo#86539
Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 89eb460..914e2cb 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -36,6 +36,10 @@
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
 
+static atomic_t bl_interfaces_nb = { 0 };
+
+static char* nouveau_get_backlight_name(void);
+
 static int
 nv40_get_intensity(struct backlight_device *bd)
 {
@@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector)
 	struct nvif_object *device = &drm->device.object;
 	struct backlight_properties props;
 	struct backlight_device *bd;
+	char* backlight_name = NULL;
 
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
 		return 0;
@@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 31;
-	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
+	backlight_name = nouveau_get_backlight_name();
+	bd = backlight_device_register(backlight_name , connector->kdev, drm,
 				       &nv40_bl_ops, &props);
+
+	// backlight_device_register() makes a copy
+	kfree(backlight_name);
+	backlight_name = NULL;
+
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 	drm->backlight = bd;
@@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector)
 	struct backlight_properties props;
 	struct backlight_device *bd;
 	const struct backlight_ops *ops;
+	char* backlight_name = NULL;
 
 	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
 	if (!nv_encoder) {
@@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 100;
-	bd = backlight_device_register("nv_backlight", connector->kdev,
+	backlight_name = nouveau_get_backlight_name();
+	bd = backlight_device_register(backlight_name, connector->kdev,
 				       nv_encoder, ops, &props);
+
+	// backlight_device_register() makes a copy
+	kfree(backlight_name);
+	backlight_name = NULL;
+
 	if (IS_ERR(bd))
 		return PTR_ERR(bd);
 
@@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev)
 		drm->backlight = NULL;
 	}
 }
+
+static char*
+nouveau_get_backlight_name(void)
+{
+	// 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0'
+	char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL);
+	const int nb = atomic_inc_return(&bl_interfaces_nb) - 1;
+	if (nb > 0 && nb < 100)
+		sprintf(backlight_name, "nv_backlight%d", nb);
+	else
+		sprintf(backlight_name, "nv_backlight");
+	return backlight_name;
+}
-- 
2.8.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 2/2] nouveau/bl: Do not register interface if Apple GMUX detected

[Pierre Moreau]

The Apple GMUX is the one managing the backlight, so there is no need for
Nouveau to register its own backlight interface.

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 914e2cb..37a1577 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -30,6 +30,7 @@
  * Register locations derived from NVClock by Roderick Colenbrander
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/backlight.h>
 
 #include "nouveau_drm.h"
@@ -239,6 +240,12 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	if (apple_gmux_present()) {
+		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau"
+		    " backlight interface");
+		return 0;
+	}
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
 		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
-- 
2.8.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Nouveau] [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Ilia Mirkin]

On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> Currently, every backlight interface created by Nouveau uses the same name,
> nv_backlight. This leads to a sysfs warning as it tries to create an already
> existing folder. This patch adds a incremented number to the name, but keeps
> the initial name as nv_backlight, to avoid possibly breaking userspace; the
> second interface will be named nv_backlight1, and so on.
>
> Fixes: fdo#86539
> Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
> ---
>  drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> index 89eb460..914e2cb 100644
> --- a/drm/nouveau/nouveau_backlight.c
> +++ b/drm/nouveau/nouveau_backlight.c
> @@ -36,6 +36,10 @@
>  #include "nouveau_reg.h"
>  #include "nouveau_encoder.h"
>
> +static atomic_t bl_interfaces_nb = { 0 };

static data is initialized to 0, this should be unnecessary.

I'd also call it "backlights" or something like that. No need for
multiple words...

> +
> +static char* nouveau_get_backlight_name(void);

Please organize the code to avoid forward declarations.

> +
>  static int
>  nv40_get_intensity(struct backlight_device *bd)
>  {
> @@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector)
>         struct nvif_object *device = &drm->device.object;
>         struct backlight_properties props;
>         struct backlight_device *bd;
> +       char* backlight_name = NULL;
>
>         if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
>                 return 0;
> @@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector)
>         memset(&props, 0, sizeof(struct backlight_properties));
>         props.type = BACKLIGHT_RAW;
>         props.max_brightness = 31;
> -       bd = backlight_device_register("nv_backlight", connector->kdev, drm,
> +       backlight_name = nouveau_get_backlight_name();
> +       bd = backlight_device_register(backlight_name , connector->kdev, drm,
>                                        &nv40_bl_ops, &props);
> +
> +       // backlight_device_register() makes a copy
> +       kfree(backlight_name);
> +       backlight_name = NULL;
> +
>         if (IS_ERR(bd))
>                 return PTR_ERR(bd);
>         drm->backlight = bd;
> @@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector)
>         struct backlight_properties props;
>         struct backlight_device *bd;
>         const struct backlight_ops *ops;
> +       char* backlight_name = NULL;
>
>         nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
>         if (!nv_encoder) {
> @@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector)
>         memset(&props, 0, sizeof(struct backlight_properties));
>         props.type = BACKLIGHT_RAW;
>         props.max_brightness = 100;
> -       bd = backlight_device_register("nv_backlight", connector->kdev,
> +       backlight_name = nouveau_get_backlight_name();
> +       bd = backlight_device_register(backlight_name, connector->kdev,
>                                        nv_encoder, ops, &props);
> +
> +       // backlight_device_register() makes a copy
> +       kfree(backlight_name);
> +       backlight_name = NULL;
> +
>         if (IS_ERR(bd))
>                 return PTR_ERR(bd);
>
> @@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev)
>                 drm->backlight = NULL;
>         }
>  }
> +
> +static char*
> +nouveau_get_backlight_name(void)
> +{
> +       // 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0'
> +       char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL);

Making this stack-allocated in the caller would be so much simpler...

> +       const int nb = atomic_inc_return(&bl_interfaces_nb) - 1;

This kinda sucks if you reload nouveau a bunch. How about using an
"ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that
one.

> +       if (nb > 0 && nb < 100)
> +               sprintf(backlight_name, "nv_backlight%d", nb);
> +       else
> +               sprintf(backlight_name, "nv_backlight");
> +       return backlight_name;
> +}
> --
> 2.8.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 5445 bytes --]

On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org> wrote:
> > Currently, every backlight interface created by Nouveau uses the same name,
> > nv_backlight. This leads to a sysfs warning as it tries to create an already
> > existing folder. This patch adds a incremented number to the name, but keeps
> > the initial name as nv_backlight, to avoid possibly breaking userspace; the
> > second interface will be named nv_backlight1, and so on.
> >
> > Fixes: fdo#86539
> > Signed-off-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> > ---
> >  drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> > index 89eb460..914e2cb 100644
> > --- a/drm/nouveau/nouveau_backlight.c
> > +++ b/drm/nouveau/nouveau_backlight.c
> > @@ -36,6 +36,10 @@
> >  #include "nouveau_reg.h"
> >  #include "nouveau_encoder.h"
> >
> > +static atomic_t bl_interfaces_nb = { 0 };
> 
> static data is initialized to 0, this should be unnecessary.

I didn’t know that. But on the other hand, I like having it explicit, and it
should not add any overhead.

> 
> I'd also call it "backlights" or something like that. No need for
> multiple words...

I prefer to use a plural noun when talking about a container of those nouns,
rathter than a counter of those nouns. But I’ll change it.

> 
> > +
> > +static char* nouveau_get_backlight_name(void);
> 
> Please organize the code to avoid forward declarations.
> 
> > +
> >  static int
> >  nv40_get_intensity(struct backlight_device *bd)
> >  {
> > @@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector)
> >         struct nvif_object *device = &drm->device.object;
> >         struct backlight_properties props;
> >         struct backlight_device *bd;
> > +       char* backlight_name = NULL;
> >
> >         if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
> >                 return 0;
> > @@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector)
> >         memset(&props, 0, sizeof(struct backlight_properties));
> >         props.type = BACKLIGHT_RAW;
> >         props.max_brightness = 31;
> > -       bd = backlight_device_register("nv_backlight", connector->kdev, drm,
> > +       backlight_name = nouveau_get_backlight_name();
> > +       bd = backlight_device_register(backlight_name , connector->kdev, drm,
> >                                        &nv40_bl_ops, &props);
> > +
> > +       // backlight_device_register() makes a copy
> > +       kfree(backlight_name);
> > +       backlight_name = NULL;
> > +
> >         if (IS_ERR(bd))
> >                 return PTR_ERR(bd);
> >         drm->backlight = bd;
> > @@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector)
> >         struct backlight_properties props;
> >         struct backlight_device *bd;
> >         const struct backlight_ops *ops;
> > +       char* backlight_name = NULL;
> >
> >         nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
> >         if (!nv_encoder) {
> > @@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector)
> >         memset(&props, 0, sizeof(struct backlight_properties));
> >         props.type = BACKLIGHT_RAW;
> >         props.max_brightness = 100;
> > -       bd = backlight_device_register("nv_backlight", connector->kdev,
> > +       backlight_name = nouveau_get_backlight_name();
> > +       bd = backlight_device_register(backlight_name, connector->kdev,
> >                                        nv_encoder, ops, &props);
> > +
> > +       // backlight_device_register() makes a copy
> > +       kfree(backlight_name);
> > +       backlight_name = NULL;
> > +
> >         if (IS_ERR(bd))
> >                 return PTR_ERR(bd);
> >
> > @@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev)
> >                 drm->backlight = NULL;
> >         }
> >  }
> > +
> > +static char*
> > +nouveau_get_backlight_name(void)
> > +{
> > +       // 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0'
> > +       char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL);
> 
> Making this stack-allocated in the caller would be so much simpler...

--" I was planning to use a dynamic size to allow numbers bigger than 99, and
lesser than 10, but stopped midway through the process. I’ll revert to a plain
stack-allocated.

> 
> > +       const int nb = atomic_inc_return(&bl_interfaces_nb) - 1;
> 
> This kinda sucks if you reload nouveau a bunch. How about using an
> "ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that
> one.

Yeah, I’m not really fond of that part. I’ll have a look at drm_crtc.c!

Pierre


> 
> > +       if (nb > 0 && nb < 100)
> > +               sprintf(backlight_name, "nv_backlight%d", nb);
> > +       else
> > +               sprintf(backlight_name, "nv_backlight");
> > +       return backlight_name;
> > +}
> > --
> > 2.8.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Ilia Mirkin]

On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
>> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
>> > Currently, every backlight interface created by Nouveau uses the same name,
>> > nv_backlight. This leads to a sysfs warning as it tries to create an already
>> > existing folder. This patch adds a incremented number to the name, but keeps
>> > the initial name as nv_backlight, to avoid possibly breaking userspace; the
>> > second interface will be named nv_backlight1, and so on.
>> >
>> > Fixes: fdo#86539
>> > Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
>> > ---
>> >  drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
>> >  1 file changed, 33 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
>> > index 89eb460..914e2cb 100644
>> > --- a/drm/nouveau/nouveau_backlight.c
>> > +++ b/drm/nouveau/nouveau_backlight.c
>> > @@ -36,6 +36,10 @@
>> >  #include "nouveau_reg.h"
>> >  #include "nouveau_encoder.h"
>> >
>> > +static atomic_t bl_interfaces_nb = { 0 };
>>
>> static data is initialized to 0, this should be unnecessary.
>
> I didn’t know that. But on the other hand, I like having it explicit, and it
> should not add any overhead.

It increases the size of the object file. I believe it's kernel policy
to avoid static initializations to 0. (Note that this doesn't hold in
regular user applications, just the kernel.)

  -ilia
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Nick Tenney]

[-- Attachment #1.1: Type: text/plain, Size: 2224 bytes --]

On Fri, Apr 15, 2016 at 11:25 AM, Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> wrote:

> On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> wrote:
> > On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> >> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> wrote:
> >> > Currently, every backlight interface created by Nouveau uses the same
> name,
> >> > nv_backlight. This leads to a sysfs warning as it tries to create an
> already
> >> > existing folder. This patch adds a incremented number to the name,
> but keeps
> >> > the initial name as nv_backlight, to avoid possibly breaking
> userspace; the
> >> > second interface will be named nv_backlight1, and so on.
> >> >
> >> > Fixes: fdo#86539
>
 I believe Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539 is
the preferred format. I think this is picked up by the mesa release scripts
or some such.

> >> > Signed-off-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> >> > ---
> >> >  drm/nouveau/nouveau_backlight.c | 35
> +++++++++++++++++++++++++++++++++--
> >> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drm/nouveau/nouveau_backlight.c
> b/drm/nouveau/nouveau_backlight.c
> >> > index 89eb460..914e2cb 100644
> >> > --- a/drm/nouveau/nouveau_backlight.c
> >> > +++ b/drm/nouveau/nouveau_backlight.c
> >> > @@ -36,6 +36,10 @@
> >> >  #include "nouveau_reg.h"
> >> >  #include "nouveau_encoder.h"
> >> >
> >> > +static atomic_t bl_interfaces_nb = { 0 };
> >>
> >> static data is initialized to 0, this should be unnecessary.
> >
> > I didn’t know that. But on the other hand, I like having it explicit,
> and it
> > should not add any overhead.
>
> It increases the size of the object file. I believe it's kernel policy
> to avoid static initializations to 0. (Note that this doesn't hold in
> regular user applications, just the kernel.)
>
>   -ilia
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>

[-- Attachment #1.2: Type: text/html, Size: 3604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 2627 bytes --]

On 02:40 PM - Apr 15 2016, Nick Tenney wrote:
> On Fri, Apr 15, 2016 at 11:25 AM, Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> wrote:
> 
> > On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> > wrote:
> > > On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> > >> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@free.fr>
> > wrote:
> > >> > Currently, every backlight interface created by Nouveau uses the same
> > name,
> > >> > nv_backlight. This leads to a sysfs warning as it tries to create an
> > already
> > >> > existing folder. This patch adds a incremented number to the name,
> > but keeps
> > >> > the initial name as nv_backlight, to avoid possibly breaking
> > userspace; the
> > >> > second interface will be named nv_backlight1, and so on.
> > >> >
> > >> > Fixes: fdo#86539
> >
>  I believe Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539 is
> the preferred format. I think this is picked up by the mesa release scripts
> or some such.

Ack’ed. I’ll fix that in the v2.

Thanks!
Pierre

> 
> > >> > Signed-off-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> > >> > ---
> > >> >  drm/nouveau/nouveau_backlight.c | 35
> > +++++++++++++++++++++++++++++++++--
> > >> >  1 file changed, 33 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/drm/nouveau/nouveau_backlight.c
> > b/drm/nouveau/nouveau_backlight.c
> > >> > index 89eb460..914e2cb 100644
> > >> > --- a/drm/nouveau/nouveau_backlight.c
> > >> > +++ b/drm/nouveau/nouveau_backlight.c
> > >> > @@ -36,6 +36,10 @@
> > >> >  #include "nouveau_reg.h"
> > >> >  #include "nouveau_encoder.h"
> > >> >
> > >> > +static atomic_t bl_interfaces_nb = { 0 };
> > >>
> > >> static data is initialized to 0, this should be unnecessary.
> > >
> > > I didn’t know that. But on the other hand, I like having it explicit,
> > and it
> > > should not add any overhead.
> >
> > It increases the size of the object file. I believe it's kernel policy
> > to avoid static initializations to 0. (Note that this doesn't hold in
> > regular user applications, just the kernel.)
> >
> >   -ilia
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> >

> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 1557 bytes --]

[…]

> 
> > +       const int nb = atomic_inc_return(&bl_interfaces_nb) - 1;
> 
> This kinda sucks if you reload nouveau a bunch. How about using an
> "ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that
> one.

I had a quick look at drm_crtc.c. This seems to be exactly what I need, except
that I do not see how I can enforce to have one of the active ones being named
`nv_backlight`, to not break (possibly) existing userspace applications. (I’m
guessing that I would be doing the same as in drm_crtc.c: looping over all
connectors and assigning them an ida, regardless of them being active or not,
as they might become active later on.)

There is another issue that I failed to address with this patch, and just
realised about it. Each time `nvXX_backlight_init(connector)` is called on a
connector, a new backlight device is created, and is stored in
`drm->backlight`, which is unique at a device level, not connector level,
meaning the old one just gets overriden (and leaked). Having a list
`drm->backlights` should solve this issue. Would that be fine with you Ben?

Pierre

> 
> > +       if (nb > 0 && nb < 100)
> > +               sprintf(backlight_name, "nv_backlight%d", nb);
> > +       else
> > +               sprintf(backlight_name, "nv_backlight");
> > +       return backlight_name;
> > +}
> > --
> > 2.8.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

Currently, every backlight interface created by Nouveau uses the same name,
nv_backlight. This leads to a sysfs warning as it tries to create an already
existing folder. This patch adds a incremented number to the name, but keeps
the initial name as nv_backlight, to avoid possibly breaking userspace; the
second interface will be named nv_backlight1, and so on.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539

v2:
* Switch to using ida for generating unique IDs, as suggested by Ilia Mirkin;
* Allocate backlight name on the stack, as suggested by Ilia Mirkin;
* Move `nouveau_get_backlight_name()` to avoid forward declaration, as
  suggested by Ilia Mirkin;
* Fix reference to bug report formatting, as reported by Nick Tenney.

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 64 ++++++++++++++++++++++++++++++++++++++---
 drm/nouveau/nouveau_display.h   | 10 +++++++
 drm/nouveau/nouveau_drm.c       |  2 ++
 drm/nouveau/nouveau_drm.h       |  1 +
 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 89eb460..41330e4 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -31,11 +31,31 @@
  */
 
 #include <linux/backlight.h>
+#include <linux/idr.h>
 
 #include "nouveau_drm.h"
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
 
+static struct ida bl_ida;
+
+struct backlight_connector {
+	struct list_head head;
+	int id;
+};
+
+static void
+nouveau_get_backlight_name(char backlight_name[15], struct backlight_connector
+		*connector)
+{
+	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
+	if (nb > 0 && nb < 100)
+		sprintf(backlight_name, "nv_backlight%d", nb);
+	else
+		sprintf(backlight_name, "nv_backlight");
+	connector->id = nb;
+}
+
 static int
 nv40_get_intensity(struct backlight_device *bd)
 {
@@ -74,6 +94,8 @@ nv40_backlight_init(struct drm_connector *connector)
 	struct nvif_object *device = &drm->device.object;
 	struct backlight_properties props;
 	struct backlight_device *bd;
+	struct backlight_connector bl_connector;
+	char backlight_name[15]; // 12 for name + 2 for digits + 1 for '\0'
 
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
 		return 0;
@@ -81,10 +103,16 @@ nv40_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 31;
-	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
+	nouveau_get_backlight_name(backlight_name, &bl_connector);
+	bd = backlight_device_register(backlight_name , connector->kdev, drm,
 				       &nv40_bl_ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = nv40_get_intensity(bd);
 	backlight_update_status(bd);
@@ -182,6 +210,8 @@ nv50_backlight_init(struct drm_connector *connector)
 	struct backlight_properties props;
 	struct backlight_device *bd;
 	const struct backlight_ops *ops;
+	struct backlight_connector bl_connector;
+	char backlight_name[15]; // 12 for name + 2 for digits + 1 for '\0'
 
 	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
 	if (!nv_encoder) {
@@ -203,11 +233,17 @@ nv50_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 100;
-	bd = backlight_device_register("nv_backlight", connector->kdev,
+	nouveau_get_backlight_name(backlight_name, &bl_connector);
+	bd = backlight_device_register(backlight_name , connector->kdev,
 				       nv_encoder, ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
 
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = bd->ops->get_brightness(bd);
 	backlight_update_status(bd);
@@ -221,6 +257,8 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	INIT_LIST_HEAD(&drm->bl_connectors);
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
 		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
@@ -246,9 +284,27 @@ void
 nouveau_backlight_exit(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct backlight_connector *connector;
+
+	list_for_each_entry(connector, &drm->bl_connectors, head) {
+		if (connector->id >= 0)
+			ida_simple_remove(&bl_ida, connector->id);
+	}
 
 	if (drm->backlight) {
 		backlight_device_unregister(drm->backlight);
 		drm->backlight = NULL;
 	}
 }
+
+void
+nouveau_backlight_ctor(void)
+{
+	ida_init(&bl_ida);
+}
+
+void
+nouveau_backlight_dtor(void)
+{
+	ida_destroy(&bl_ida);
+}
diff --git a/drm/nouveau/nouveau_display.h b/drm/nouveau/nouveau_display.h
index 5a57d8b..bb07cc8 100644
--- a/drm/nouveau/nouveau_display.h
+++ b/drm/nouveau/nouveau_display.h
@@ -90,6 +90,8 @@ int nouveau_crtc_set_config(struct drm_mode_set *set);
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
 extern int nouveau_backlight_init(struct drm_device *);
 extern void nouveau_backlight_exit(struct drm_device *);
+extern void nouveau_backlight_ctor(void);
+extern void nouveau_backlight_dtor(void);
 #else
 static inline int
 nouveau_backlight_init(struct drm_device *dev)
@@ -100,6 +102,14 @@ nouveau_backlight_init(struct drm_device *dev)
 static inline void
 nouveau_backlight_exit(struct drm_device *dev) {
 }
+
+static inline void
+nouveau_backlight_ctor(void) {
+}
+
+static inline void
+nouveau_backlight_dtor(void) {
+}
 #endif
 
 #endif
diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
index d06877d..0d89d7c 100644
--- a/drm/nouveau/nouveau_drm.c
+++ b/drm/nouveau/nouveau_drm.c
@@ -1097,6 +1097,7 @@ nouveau_drm_init(void)
 #endif
 
 	nouveau_register_dsm_handler();
+	nouveau_backlight_ctor();
 	return drm_pci_init(&driver_pci, &nouveau_drm_pci_driver);
 }
 
@@ -1107,6 +1108,7 @@ nouveau_drm_exit(void)
 		return;
 
 	drm_pci_exit(&driver_pci, &nouveau_drm_pci_driver);
+	nouveau_backlight_dtor();
 	nouveau_unregister_dsm_handler();
 
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
diff --git a/drm/nouveau/nouveau_drm.h b/drm/nouveau/nouveau_drm.h
index 5c363ed..593828e 100644
--- a/drm/nouveau/nouveau_drm.h
+++ b/drm/nouveau/nouveau_drm.h
@@ -161,6 +161,7 @@ struct nouveau_drm {
 	struct nvbios vbios;
 	struct nouveau_display *display;
 	struct backlight_device *backlight;
+	struct list_head bl_connectors;
 
 	/* power management */
 	struct nouveau_hwmon *hwmon;
-- 
2.8.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH REBASED 2/2] nouveau/bl: Do not register interface if Apple GMUX detected

[Pierre Moreau]

The Apple GMUX is the one managing the backlight, so there is no need for
Nouveau to register its own backlight interface.

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 41330e4..94ac3cb 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -30,6 +30,7 @@
  * Register locations derived from NVClock by Roderick Colenbrander
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/backlight.h>
 #include <linux/idr.h>
 
@@ -257,6 +258,12 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	if (apple_gmux_present()) {
+		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau"
+		    " backlight interface");
+		return 0;
+	}
+
 	INIT_LIST_HEAD(&drm->bl_connectors);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-- 
2.8.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH REBASED 2/2] nouveau/bl: Do not register interface if Apple GMUX detected

[Lukas Wunner]

Hi,

On Sun, Apr 17, 2016 at 09:57:42PM +0200, Pierre Moreau wrote:
> The Apple GMUX is the one managing the backlight, so there is no need for
> Nouveau to register its own backlight interface.
> 
> Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
> ---
>  drm/nouveau/nouveau_backlight.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> index 41330e4..94ac3cb 100644
> --- a/drm/nouveau/nouveau_backlight.c
> +++ b/drm/nouveau/nouveau_backlight.c
> @@ -30,6 +30,7 @@
>   * Register locations derived from NVClock by Roderick Colenbrander
>   */
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/backlight.h>
>  #include <linux/idr.h>
>  
> @@ -257,6 +258,12 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->device;
>  	struct drm_connector *connector;
>  
> +	if (apple_gmux_present()) {
> +		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau"
> +		    " backlight interface");

Small nit -- Documentation/CodingStyle, chapter 2 says:

"However, never break user-visible strings such as
printk messages, because that breaks the ability to grep for them."

Thanks,

Lukas

> +		return 0;
> +	}
> +
>  	INIT_LIST_HEAD(&drm->bl_connectors);
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -- 
> 2.8.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] nouveau/bl: Do not register interface if Apple GMUX detected

[Pierre Moreau]

The Apple GMUX is the one managing the backlight, so there is no need for
Nouveau to register its own backlight interface.

v2: Do not split information message on two lines as it prevents from grepping
    it, as pointed out by Hans de Goede

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 41330e4..8429ceb 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -30,6 +30,7 @@
  * Register locations derived from NVClock by Roderick Colenbrander
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/backlight.h>
 #include <linux/idr.h>
 
@@ -257,6 +258,11 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	if (apple_gmux_present()) {
+		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface");
+		return 0;
+	}
+
 	INIT_LIST_HEAD(&drm->bl_connectors);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-- 
2.8.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 2/2] nouveau/bl: Do not register interface if Apple GMUX detected

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 1629 bytes --]

On 02:32 PM - May 01 2016, Pierre Moreau wrote:
> The Apple GMUX is the one managing the backlight, so there is no need for
> Nouveau to register its own backlight interface.
> 
> v2: Do not split information message on two lines as it prevents from grepping
>     it, as pointed out by Hans de Goede

It should be "it, as pointed out by Lukas Wunner", sorry for the mistake… :-/

Pierre

> 
> Signed-off-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/nouveau_backlight.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> index 41330e4..8429ceb 100644
> --- a/drm/nouveau/nouveau_backlight.c
> +++ b/drm/nouveau/nouveau_backlight.c
> @@ -30,6 +30,7 @@
>   * Register locations derived from NVClock by Roderick Colenbrander
>   */
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/backlight.h>
>  #include <linux/idr.h>
>  
> @@ -257,6 +258,11 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->device;
>  	struct drm_connector *connector;
>  
> +	if (apple_gmux_present()) {
> +		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface");
> +		return 0;
> +	}
> +
>  	INIT_LIST_HEAD(&drm->bl_connectors);
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -- 
> 2.8.2
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v3 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

From: Pierre Moreau <pierre.morrow@free.fr>

Currently, every backlight interface created by Nouveau uses the same name,
nv_backlight. This leads to a sysfs warning as it tries to create an already
existing folder. This patch adds a incremented number to the name, but keeps
the initial name as nv_backlight, to avoid possibly breaking userspace; the
second interface will be named nv_backlight1, and so on.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539

v2:
* Switch to using ida for generating unique IDs, as suggested by Ilia Mirkin;
* Allocate backlight name on the stack, as suggested by Ilia Mirkin;
* Move `nouveau_get_backlight_name()` to avoid forward declaration, as
  suggested by Ilia Mirkin;
* Fix reference to bug report formatting, as reported by Nick Tenney.

v3:
* Define a macro for the size of the backlight name, to avoid defining
  it multiple times;
* Use snprintf in place of sprintf.

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 65 +++++++++++++++++++++++++++--
 drivers/gpu/drm/nouveau/nouveau_display.h   | 10 +++++
 drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +
 drivers/gpu/drm/nouveau/nouveau_drv.h       |  1 +
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 5e2c568..0e69612 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -31,11 +31,32 @@
  */
 
 #include <linux/backlight.h>
+#include <linux/idr.h>
 
 #include "nouveau_drv.h"
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
 
+static struct ida bl_ida;
+#define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0'
+
+struct backlight_connector {
+	struct list_head head;
+	int id;
+};
+
+static void
+nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct
+		backlight_connector *connector)
+{
+	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
+	if (nb > 0 && nb < 100)
+		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
+	else
+		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
+	connector->id = nb;
+}
+
 static int
 nv40_get_intensity(struct backlight_device *bd)
 {
@@ -74,6 +95,8 @@ nv40_backlight_init(struct drm_connector *connector)
 	struct nvif_object *device = &drm->device.object;
 	struct backlight_properties props;
 	struct backlight_device *bd;
+	struct backlight_connector bl_connector;
+	char backlight_name[BL_NAME_SIZE];
 
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
 		return 0;
@@ -81,10 +104,16 @@ nv40_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 31;
-	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
+	nouveau_get_backlight_name(backlight_name, &bl_connector);
+	bd = backlight_device_register(backlight_name , connector->kdev, drm,
 				       &nv40_bl_ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = nv40_get_intensity(bd);
 	backlight_update_status(bd);
@@ -182,6 +211,8 @@ nv50_backlight_init(struct drm_connector *connector)
 	struct backlight_properties props;
 	struct backlight_device *bd;
 	const struct backlight_ops *ops;
+	struct backlight_connector bl_connector;
+	char backlight_name[BL_NAME_SIZE];
 
 	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
 	if (!nv_encoder) {
@@ -203,11 +234,17 @@ nv50_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 100;
-	bd = backlight_device_register("nv_backlight", connector->kdev,
+	nouveau_get_backlight_name(backlight_name, &bl_connector);
+	bd = backlight_device_register(backlight_name , connector->kdev,
 				       nv_encoder, ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
 
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = bd->ops->get_brightness(bd);
 	backlight_update_status(bd);
@@ -221,6 +258,8 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	INIT_LIST_HEAD(&drm->bl_connectors);
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
 		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
@@ -247,9 +286,27 @@ void
 nouveau_backlight_exit(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct backlight_connector *connector;
+
+	list_for_each_entry(connector, &drm->bl_connectors, head) {
+		if (connector->id >= 0)
+			ida_simple_remove(&bl_ida, connector->id);
+	}
 
 	if (drm->backlight) {
 		backlight_device_unregister(drm->backlight);
 		drm->backlight = NULL;
 	}
 }
+
+void
+nouveau_backlight_ctor(void)
+{
+	ida_init(&bl_ida);
+}
+
+void
+nouveau_backlight_dtor(void)
+{
+	ida_destroy(&bl_ida);
+}
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
index 330fe0f..4a75df0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -91,6 +91,8 @@ int nouveau_crtc_set_config(struct drm_mode_set *set);
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
 extern int nouveau_backlight_init(struct drm_device *);
 extern void nouveau_backlight_exit(struct drm_device *);
+extern void nouveau_backlight_ctor(void);
+extern void nouveau_backlight_dtor(void);
 #else
 static inline int
 nouveau_backlight_init(struct drm_device *dev)
@@ -101,6 +103,14 @@ nouveau_backlight_init(struct drm_device *dev)
 static inline void
 nouveau_backlight_exit(struct drm_device *dev) {
 }
+
+static inline void
+nouveau_backlight_ctor(void) {
+}
+
+static inline void
+nouveau_backlight_dtor(void) {
+}
 #endif
 
 struct drm_framebuffer *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 9876e6f..2b93b55 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1113,6 +1113,7 @@ nouveau_drm_init(void)
 #endif
 
 	nouveau_register_dsm_handler();
+	nouveau_backlight_ctor();
 	return drm_pci_init(&driver_pci, &nouveau_drm_pci_driver);
 }
 
@@ -1123,6 +1124,7 @@ nouveau_drm_exit(void)
 		return;
 
 	drm_pci_exit(&driver_pci, &nouveau_drm_pci_driver);
+	nouveau_backlight_dtor();
 	nouveau_unregister_dsm_handler();
 
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 4cd47ba..923dbce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -161,6 +161,7 @@ struct nouveau_drm {
 	struct nvbios vbios;
 	struct nouveau_display *display;
 	struct backlight_device *backlight;
+	struct list_head bl_connectors;
 
 	/* power management */
 	struct nouveau_hwmon *hwmon;
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH REBASED 2/2] Do not register interface if Apple GMUX detected

[Pierre Moreau]

From: Pierre Moreau <pierre.morrow@free.fr>

The Apple GMUX is the one managing the backlight, so there is no need for
Nouveau to register its own backlight interface.

v2: Do not split information message on two lines as it prevents from grepping
    it, as pointed out by Lukas Wunner

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 0e69612..3c91c24 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -30,6 +30,7 @@
  * Register locations derived from NVClock by Roderick Colenbrander
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/backlight.h>
 #include <linux/idr.h>
 
@@ -258,6 +259,11 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	if (apple_gmux_present()) {
+		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface");
+		return 0;
+	}
+
 	INIT_LIST_HEAD(&drm->bl_connectors);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] nouveau/bl: Assign different names to interfaces

[Lukas Wunner]

On Sun, Nov 13, 2016 at 08:57:06PM +0100, Pierre Moreau wrote:
> From: Pierre Moreau <pierre.morrow@free.fr>
> 
> Currently, every backlight interface created by Nouveau uses the same name,
> nv_backlight. This leads to a sysfs warning as it tries to create an already
> existing folder. This patch adds a incremented number to the name, but keeps
> the initial name as nv_backlight, to avoid possibly breaking userspace; the
> second interface will be named nv_backlight1, and so on.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539
> 
> v2:
> * Switch to using ida for generating unique IDs, as suggested by Ilia Mirkin;
> * Allocate backlight name on the stack, as suggested by Ilia Mirkin;
> * Move `nouveau_get_backlight_name()` to avoid forward declaration, as
>   suggested by Ilia Mirkin;
> * Fix reference to bug report formatting, as reported by Nick Tenney.
> 
> v3:
> * Define a macro for the size of the backlight name, to avoid defining
>   it multiple times;
> * Use snprintf in place of sprintf.
> 
> Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
> ---
>  drivers/gpu/drm/nouveau/nouveau_backlight.c | 65 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/nouveau/nouveau_display.h   | 10 +++++
>  drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +
>  drivers/gpu/drm/nouveau/nouveau_drv.h       |  1 +
>  4 files changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index 5e2c568..0e69612 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> @@ -31,11 +31,32 @@
>   */
>  
>  #include <linux/backlight.h>
> +#include <linux/idr.h>
>  
>  #include "nouveau_drv.h"
>  #include "nouveau_reg.h"
>  #include "nouveau_encoder.h"
>  
> +static struct ida bl_ida;
> +#define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0'
> +
> +struct backlight_connector {
> +	struct list_head head;
> +	int id;
> +};
> +
> +static void
> +nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct
> +		backlight_connector *connector)
> +{
> +	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
> +	if (nb > 0 && nb < 100)
> +		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
> +	else
> +		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
> +	connector->id = nb;
> +}
> +

Just a minor issue, in the >= 100 case, backlight creation should probably
fail rather than reverting to "nv_backlight".  Other than that LGTM.

Thanks,

Lukas

>  static int
>  nv40_get_intensity(struct backlight_device *bd)
>  {
> @@ -74,6 +95,8 @@ nv40_backlight_init(struct drm_connector *connector)
>  	struct nvif_object *device = &drm->device.object;
>  	struct backlight_properties props;
>  	struct backlight_device *bd;
> +	struct backlight_connector bl_connector;
> +	char backlight_name[BL_NAME_SIZE];
>  
>  	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
>  		return 0;
> @@ -81,10 +104,16 @@ nv40_backlight_init(struct drm_connector *connector)
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW;
>  	props.max_brightness = 31;
> -	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
> +	nouveau_get_backlight_name(backlight_name, &bl_connector);
> +	bd = backlight_device_register(backlight_name , connector->kdev, drm,
>  				       &nv40_bl_ops, &props);
> -	if (IS_ERR(bd))
> +
> +	if (IS_ERR(bd)) {
> +		if (bl_connector.id > 0)
> +			ida_simple_remove(&bl_ida, bl_connector.id);
>  		return PTR_ERR(bd);
> +	}
> +	list_add(&bl_connector.head, &drm->bl_connectors);
>  	drm->backlight = bd;
>  	bd->props.brightness = nv40_get_intensity(bd);
>  	backlight_update_status(bd);
> @@ -182,6 +211,8 @@ nv50_backlight_init(struct drm_connector *connector)
>  	struct backlight_properties props;
>  	struct backlight_device *bd;
>  	const struct backlight_ops *ops;
> +	struct backlight_connector bl_connector;
> +	char backlight_name[BL_NAME_SIZE];
>  
>  	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
>  	if (!nv_encoder) {
> @@ -203,11 +234,17 @@ nv50_backlight_init(struct drm_connector *connector)
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW;
>  	props.max_brightness = 100;
> -	bd = backlight_device_register("nv_backlight", connector->kdev,
> +	nouveau_get_backlight_name(backlight_name, &bl_connector);
> +	bd = backlight_device_register(backlight_name , connector->kdev,
>  				       nv_encoder, ops, &props);
> -	if (IS_ERR(bd))
> +
> +	if (IS_ERR(bd)) {
> +		if (bl_connector.id > 0)
> +			ida_simple_remove(&bl_ida, bl_connector.id);
>  		return PTR_ERR(bd);
> +	}
>  
> +	list_add(&bl_connector.head, &drm->bl_connectors);
>  	drm->backlight = bd;
>  	bd->props.brightness = bd->ops->get_brightness(bd);
>  	backlight_update_status(bd);
> @@ -221,6 +258,8 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->device;
>  	struct drm_connector *connector;
>  
> +	INIT_LIST_HEAD(&drm->bl_connectors);
> +
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
>  		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> @@ -247,9 +286,27 @@ void
>  nouveau_backlight_exit(struct drm_device *dev)
>  {
>  	struct nouveau_drm *drm = nouveau_drm(dev);
> +	struct backlight_connector *connector;
> +
> +	list_for_each_entry(connector, &drm->bl_connectors, head) {
> +		if (connector->id >= 0)
> +			ida_simple_remove(&bl_ida, connector->id);
> +	}
>  
>  	if (drm->backlight) {
>  		backlight_device_unregister(drm->backlight);
>  		drm->backlight = NULL;
>  	}
>  }
> +
> +void
> +nouveau_backlight_ctor(void)
> +{
> +	ida_init(&bl_ida);
> +}
> +
> +void
> +nouveau_backlight_dtor(void)
> +{
> +	ida_destroy(&bl_ida);
> +}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> index 330fe0f..4a75df0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> @@ -91,6 +91,8 @@ int nouveau_crtc_set_config(struct drm_mode_set *set);
>  #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
>  extern int nouveau_backlight_init(struct drm_device *);
>  extern void nouveau_backlight_exit(struct drm_device *);
> +extern void nouveau_backlight_ctor(void);
> +extern void nouveau_backlight_dtor(void);
>  #else
>  static inline int
>  nouveau_backlight_init(struct drm_device *dev)
> @@ -101,6 +103,14 @@ nouveau_backlight_init(struct drm_device *dev)
>  static inline void
>  nouveau_backlight_exit(struct drm_device *dev) {
>  }
> +
> +static inline void
> +nouveau_backlight_ctor(void) {
> +}
> +
> +static inline void
> +nouveau_backlight_dtor(void) {
> +}
>  #endif
>  
>  struct drm_framebuffer *
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 9876e6f..2b93b55 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1113,6 +1113,7 @@ nouveau_drm_init(void)
>  #endif
>  
>  	nouveau_register_dsm_handler();
> +	nouveau_backlight_ctor();
>  	return drm_pci_init(&driver_pci, &nouveau_drm_pci_driver);
>  }
>  
> @@ -1123,6 +1124,7 @@ nouveau_drm_exit(void)
>  		return;
>  
>  	drm_pci_exit(&driver_pci, &nouveau_drm_pci_driver);
> +	nouveau_backlight_dtor();
>  	nouveau_unregister_dsm_handler();
>  
>  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 4cd47ba..923dbce 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -161,6 +161,7 @@ struct nouveau_drm {
>  	struct nvbios vbios;
>  	struct nouveau_display *display;
>  	struct backlight_device *backlight;
> +	struct list_head bl_connectors;
>  
>  	/* power management */
>  	struct nouveau_hwmon *hwmon;
> -- 
> 2.10.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH REBASED 2/2] Do not register interface if Apple GMUX detected

[Lukas Wunner]

On Sun, Nov 13, 2016 at 08:57:07PM +0100, Pierre Moreau wrote:
> From: Pierre Moreau <pierre.morrow@free.fr>
> 
> The Apple GMUX is the one managing the backlight, so there is no need for
> Nouveau to register its own backlight interface.
> 
> v2: Do not split information message on two lines as it prevents from grepping
>     it, as pointed out by Lukas Wunner
> 
> Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

> ---
>  drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index 0e69612..3c91c24 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> @@ -30,6 +30,7 @@
>   * Register locations derived from NVClock by Roderick Colenbrander
>   */
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/backlight.h>
>  #include <linux/idr.h>
>  
> @@ -258,6 +259,11 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->device;
>  	struct drm_connector *connector;
>  
> +	if (apple_gmux_present()) {
> +		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface");
> +		return 0;
> +	}
> +
>  	INIT_LIST_HEAD(&drm->bl_connectors);
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -- 
> 2.10.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v3 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 9819 bytes --]

On 11:17 am - Nov 14 2016, Lukas Wunner wrote:
> On Sun, Nov 13, 2016 at 08:57:06PM +0100, Pierre Moreau wrote:
> > From: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> > 
> > Currently, every backlight interface created by Nouveau uses the same name,
> > nv_backlight. This leads to a sysfs warning as it tries to create an already
> > existing folder. This patch adds a incremented number to the name, but keeps
> > the initial name as nv_backlight, to avoid possibly breaking userspace; the
> > second interface will be named nv_backlight1, and so on.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539
> > 
> > v2:
> > * Switch to using ida for generating unique IDs, as suggested by Ilia Mirkin;
> > * Allocate backlight name on the stack, as suggested by Ilia Mirkin;
> > * Move `nouveau_get_backlight_name()` to avoid forward declaration, as
> >   suggested by Ilia Mirkin;
> > * Fix reference to bug report formatting, as reported by Nick Tenney.
> > 
> > v3:
> > * Define a macro for the size of the backlight name, to avoid defining
> >   it multiple times;
> > * Use snprintf in place of sprintf.
> > 
> > Signed-off-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_backlight.c | 65 +++++++++++++++++++++++++++--
> >  drivers/gpu/drm/nouveau/nouveau_display.h   | 10 +++++
> >  drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +
> >  drivers/gpu/drm/nouveau/nouveau_drv.h       |  1 +
> >  4 files changed, 74 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > index 5e2c568..0e69612 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > @@ -31,11 +31,32 @@
> >   */
> >  
> >  #include <linux/backlight.h>
> > +#include <linux/idr.h>
> >  
> >  #include "nouveau_drv.h"
> >  #include "nouveau_reg.h"
> >  #include "nouveau_encoder.h"
> >  
> > +static struct ida bl_ida;
> > +#define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0'
> > +
> > +struct backlight_connector {
> > +	struct list_head head;
> > +	int id;
> > +};
> > +
> > +static void
> > +nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct
> > +		backlight_connector *connector)
> > +{
> > +	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
> > +	if (nb > 0 && nb < 100)
> > +		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
> > +	else
> > +		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
> > +	connector->id = nb;
> > +}
> > +
> 
> Just a minor issue, in the >= 100 case, backlight creation should probably
> fail rather than reverting to "nv_backlight".  Other than that LGTM.

I don’t see how you could get to more than 100 backlight interfaces, but you
are absolutely right, it should fail rather than reverting to "nv_backlight".
I’ll add a return code to `nouveau_get_backlight_name()` and check for that in
the `nvX0_backlight_init()` functions. If it is non-zero, I’m not sure whether
I should return an error code (and if so, which one) or just a plain 0.

Thanks,
Pierre

PS: I have no idea what went wrong with the e-mail address, and why it decided
to send it from my other e-mail address rather than the usual one… I’ll have to
fix that before sending the updated version.

> 
> Thanks,
> 
> Lukas
> 
> >  static int
> >  nv40_get_intensity(struct backlight_device *bd)
> >  {
> > @@ -74,6 +95,8 @@ nv40_backlight_init(struct drm_connector *connector)
> >  	struct nvif_object *device = &drm->device.object;
> >  	struct backlight_properties props;
> >  	struct backlight_device *bd;
> > +	struct backlight_connector bl_connector;
> > +	char backlight_name[BL_NAME_SIZE];
> >  
> >  	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
> >  		return 0;
> > @@ -81,10 +104,16 @@ nv40_backlight_init(struct drm_connector *connector)
> >  	memset(&props, 0, sizeof(struct backlight_properties));
> >  	props.type = BACKLIGHT_RAW;
> >  	props.max_brightness = 31;
> > -	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
> > +	nouveau_get_backlight_name(backlight_name, &bl_connector);
> > +	bd = backlight_device_register(backlight_name , connector->kdev, drm,
> >  				       &nv40_bl_ops, &props);
> > -	if (IS_ERR(bd))
> > +
> > +	if (IS_ERR(bd)) {
> > +		if (bl_connector.id > 0)
> > +			ida_simple_remove(&bl_ida, bl_connector.id);
> >  		return PTR_ERR(bd);
> > +	}
> > +	list_add(&bl_connector.head, &drm->bl_connectors);
> >  	drm->backlight = bd;
> >  	bd->props.brightness = nv40_get_intensity(bd);
> >  	backlight_update_status(bd);
> > @@ -182,6 +211,8 @@ nv50_backlight_init(struct drm_connector *connector)
> >  	struct backlight_properties props;
> >  	struct backlight_device *bd;
> >  	const struct backlight_ops *ops;
> > +	struct backlight_connector bl_connector;
> > +	char backlight_name[BL_NAME_SIZE];
> >  
> >  	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
> >  	if (!nv_encoder) {
> > @@ -203,11 +234,17 @@ nv50_backlight_init(struct drm_connector *connector)
> >  	memset(&props, 0, sizeof(struct backlight_properties));
> >  	props.type = BACKLIGHT_RAW;
> >  	props.max_brightness = 100;
> > -	bd = backlight_device_register("nv_backlight", connector->kdev,
> > +	nouveau_get_backlight_name(backlight_name, &bl_connector);
> > +	bd = backlight_device_register(backlight_name , connector->kdev,
> >  				       nv_encoder, ops, &props);
> > -	if (IS_ERR(bd))
> > +
> > +	if (IS_ERR(bd)) {
> > +		if (bl_connector.id > 0)
> > +			ida_simple_remove(&bl_ida, bl_connector.id);
> >  		return PTR_ERR(bd);
> > +	}
> >  
> > +	list_add(&bl_connector.head, &drm->bl_connectors);
> >  	drm->backlight = bd;
> >  	bd->props.brightness = bd->ops->get_brightness(bd);
> >  	backlight_update_status(bd);
> > @@ -221,6 +258,8 @@ nouveau_backlight_init(struct drm_device *dev)
> >  	struct nvif_device *device = &drm->device;
> >  	struct drm_connector *connector;
> >  
> > +	INIT_LIST_HEAD(&drm->bl_connectors);
> > +
> >  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> >  		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
> >  		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > @@ -247,9 +286,27 @@ void
> >  nouveau_backlight_exit(struct drm_device *dev)
> >  {
> >  	struct nouveau_drm *drm = nouveau_drm(dev);
> > +	struct backlight_connector *connector;
> > +
> > +	list_for_each_entry(connector, &drm->bl_connectors, head) {
> > +		if (connector->id >= 0)
> > +			ida_simple_remove(&bl_ida, connector->id);
> > +	}
> >  
> >  	if (drm->backlight) {
> >  		backlight_device_unregister(drm->backlight);
> >  		drm->backlight = NULL;
> >  	}
> >  }
> > +
> > +void
> > +nouveau_backlight_ctor(void)
> > +{
> > +	ida_init(&bl_ida);
> > +}
> > +
> > +void
> > +nouveau_backlight_dtor(void)
> > +{
> > +	ida_destroy(&bl_ida);
> > +}
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> > index 330fe0f..4a75df0 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> > @@ -91,6 +91,8 @@ int nouveau_crtc_set_config(struct drm_mode_set *set);
> >  #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
> >  extern int nouveau_backlight_init(struct drm_device *);
> >  extern void nouveau_backlight_exit(struct drm_device *);
> > +extern void nouveau_backlight_ctor(void);
> > +extern void nouveau_backlight_dtor(void);
> >  #else
> >  static inline int
> >  nouveau_backlight_init(struct drm_device *dev)
> > @@ -101,6 +103,14 @@ nouveau_backlight_init(struct drm_device *dev)
> >  static inline void
> >  nouveau_backlight_exit(struct drm_device *dev) {
> >  }
> > +
> > +static inline void
> > +nouveau_backlight_ctor(void) {
> > +}
> > +
> > +static inline void
> > +nouveau_backlight_dtor(void) {
> > +}
> >  #endif
> >  
> >  struct drm_framebuffer *
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 9876e6f..2b93b55 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -1113,6 +1113,7 @@ nouveau_drm_init(void)
> >  #endif
> >  
> >  	nouveau_register_dsm_handler();
> > +	nouveau_backlight_ctor();
> >  	return drm_pci_init(&driver_pci, &nouveau_drm_pci_driver);
> >  }
> >  
> > @@ -1123,6 +1124,7 @@ nouveau_drm_exit(void)
> >  		return;
> >  
> >  	drm_pci_exit(&driver_pci, &nouveau_drm_pci_driver);
> > +	nouveau_backlight_dtor();
> >  	nouveau_unregister_dsm_handler();
> >  
> >  #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 4cd47ba..923dbce 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -161,6 +161,7 @@ struct nouveau_drm {
> >  	struct nvbios vbios;
> >  	struct nouveau_display *display;
> >  	struct backlight_device *backlight;
> > +	struct list_head bl_connectors;
> >  
> >  	/* power management */
> >  	struct nouveau_hwmon *hwmon;
> > -- 
> > 2.10.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 1/2] nouveau/bl: Assign different names to interfaces

[Pierre Moreau]

Currently, every backlight interface created by Nouveau uses the same name,
nv_backlight. This leads to a sysfs warning as it tries to create an already
existing folder. This patch adds a incremented number to the name, but keeps
the initial name as nv_backlight, to avoid possibly breaking userspace; the
second interface will be named nv_backlight1, and so on.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86539

v2:
* Switch to using ida for generating unique IDs, as suggested by Ilia Mirkin;
* Allocate backlight name on the stack, as suggested by Ilia Mirkin;
* Move `nouveau_get_backlight_name()` to avoid forward declaration, as
  suggested by Ilia Mirkin;
* Fix reference to bug report formatting, as reported by Nick Tenney.

v3:
* Define a macro for the size of the backlight name, to avoid defining
  it multiple times;
* Use snprintf in place of sprintf.

v4:
* Do not create similarly named interfaces when reaching the maximum
  amount of unique names, but fail instead, as pointed out by Lukas Wunner

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 74 ++++++++++++++++++++++++++++++++++++++---
 drm/nouveau/nouveau_display.h   | 10 ++++++
 drm/nouveau/nouveau_drm.c       |  2 ++
 drm/nouveau/nouveau_drv.h       |  1 +
 4 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index 5e2c568..a34cd35 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -31,11 +31,35 @@
  */
 
 #include <linux/backlight.h>
+#include <linux/idr.h>
 
 #include "nouveau_drv.h"
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
 
+static struct ida bl_ida;
+#define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0'
+
+struct backlight_connector {
+	struct list_head head;
+	int id;
+};
+
+static bool
+nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct backlight_connector
+		*connector)
+{
+	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
+	if (nb < 0 || nb >= 100)
+		return false;
+	if (nb > 0)
+		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
+	else
+		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
+	connector->id = nb;
+	return true;
+}
+
 static int
 nv40_get_intensity(struct backlight_device *bd)
 {
@@ -74,6 +98,8 @@ nv40_backlight_init(struct drm_connector *connector)
 	struct nvif_object *device = &drm->device.object;
 	struct backlight_properties props;
 	struct backlight_device *bd;
+	struct backlight_connector bl_connector;
+	char backlight_name[BL_NAME_SIZE];
 
 	if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK))
 		return 0;
@@ -81,10 +107,19 @@ nv40_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 31;
-	bd = backlight_device_register("nv_backlight", connector->kdev, drm,
+	if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) {
+		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n");
+		return 0;
+	}
+	bd = backlight_device_register(backlight_name , connector->kdev, drm,
 				       &nv40_bl_ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = nv40_get_intensity(bd);
 	backlight_update_status(bd);
@@ -182,6 +217,8 @@ nv50_backlight_init(struct drm_connector *connector)
 	struct backlight_properties props;
 	struct backlight_device *bd;
 	const struct backlight_ops *ops;
+	struct backlight_connector bl_connector;
+	char backlight_name[BL_NAME_SIZE];
 
 	nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS);
 	if (!nv_encoder) {
@@ -203,11 +240,20 @@ nv50_backlight_init(struct drm_connector *connector)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = 100;
-	bd = backlight_device_register("nv_backlight", connector->kdev,
+	if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) {
+		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n");
+		return 0;
+	}
+	bd = backlight_device_register(backlight_name , connector->kdev,
 				       nv_encoder, ops, &props);
-	if (IS_ERR(bd))
+
+	if (IS_ERR(bd)) {
+		if (bl_connector.id > 0)
+			ida_simple_remove(&bl_ida, bl_connector.id);
 		return PTR_ERR(bd);
+	}
 
+	list_add(&bl_connector.head, &drm->bl_connectors);
 	drm->backlight = bd;
 	bd->props.brightness = bd->ops->get_brightness(bd);
 	backlight_update_status(bd);
@@ -221,6 +267,8 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	INIT_LIST_HEAD(&drm->bl_connectors);
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
 		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
@@ -247,9 +295,27 @@ void
 nouveau_backlight_exit(struct drm_device *dev)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct backlight_connector *connector;
+
+	list_for_each_entry(connector, &drm->bl_connectors, head) {
+		if (connector->id >= 0)
+			ida_simple_remove(&bl_ida, connector->id);
+	}
 
 	if (drm->backlight) {
 		backlight_device_unregister(drm->backlight);
 		drm->backlight = NULL;
 	}
 }
+
+void
+nouveau_backlight_ctor(void)
+{
+	ida_init(&bl_ida);
+}
+
+void
+nouveau_backlight_dtor(void)
+{
+	ida_destroy(&bl_ida);
+}
diff --git a/drm/nouveau/nouveau_display.h b/drm/nouveau/nouveau_display.h
index 330fe0f..4a75df0 100644
--- a/drm/nouveau/nouveau_display.h
+++ b/drm/nouveau/nouveau_display.h
@@ -91,6 +91,8 @@ int nouveau_crtc_set_config(struct drm_mode_set *set);
 #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
 extern int nouveau_backlight_init(struct drm_device *);
 extern void nouveau_backlight_exit(struct drm_device *);
+extern void nouveau_backlight_ctor(void);
+extern void nouveau_backlight_dtor(void);
 #else
 static inline int
 nouveau_backlight_init(struct drm_device *dev)
@@ -101,6 +103,14 @@ nouveau_backlight_init(struct drm_device *dev)
 static inline void
 nouveau_backlight_exit(struct drm_device *dev) {
 }
+
+static inline void
+nouveau_backlight_ctor(void) {
+}
+
+static inline void
+nouveau_backlight_dtor(void) {
+}
 #endif
 
 struct drm_framebuffer *
diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
index 064a925..59348fc 100644
--- a/drm/nouveau/nouveau_drm.c
+++ b/drm/nouveau/nouveau_drm.c
@@ -1122,6 +1122,7 @@ nouveau_drm_init(void)
 #endif
 
 	nouveau_register_dsm_handler();
+	nouveau_backlight_ctor();
 	return drm_pci_init(&driver_pci, &nouveau_drm_pci_driver);
 }
 
@@ -1132,6 +1133,7 @@ nouveau_drm_exit(void)
 		return;
 
 	drm_pci_exit(&driver_pci, &nouveau_drm_pci_driver);
+	nouveau_backlight_dtor();
 	nouveau_unregister_dsm_handler();
 
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
diff --git a/drm/nouveau/nouveau_drv.h b/drm/nouveau/nouveau_drv.h
index 9730c0e..8d5ed5b 100644
--- a/drm/nouveau/nouveau_drv.h
+++ b/drm/nouveau/nouveau_drv.h
@@ -163,6 +163,7 @@ struct nouveau_drm {
 	struct nvbios vbios;
 	struct nouveau_display *display;
 	struct backlight_device *backlight;
+	struct list_head bl_connectors;
 	struct work_struct hpd_work;
 #ifdef CONFIG_ACPI
 	struct notifier_block acpi_nb;
-- 
2.10.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v3 2/2] Do not register interface if Apple GMUX detected

[Pierre Moreau]

The Apple GMUX is the one managing the backlight, so there is no need for
Nouveau to register its own backlight interface.

v2: Do not split information message on two lines as it prevents from grepping
    it, as pointed out by Lukas Wunner

v3: Add a missing end-of-line character to the printed message

Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
index a34cd35..8b1ca4a 100644
--- a/drm/nouveau/nouveau_backlight.c
+++ b/drm/nouveau/nouveau_backlight.c
@@ -30,6 +30,7 @@
  * Register locations derived from NVClock by Roderick Colenbrander
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/backlight.h>
 #include <linux/idr.h>
 
@@ -267,6 +268,11 @@ nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->device;
 	struct drm_connector *connector;
 
+	if (apple_gmux_present()) {
+		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
+		return 0;
+	}
+
 	INIT_LIST_HEAD(&drm->bl_connectors);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-- 
2.10.2

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v3 2/2] Do not register interface if Apple GMUX detected

[Lukas Wunner]

On Thu, Dec 08, 2016 at 12:57:09AM +0100, Pierre Moreau wrote:
> The Apple GMUX is the one managing the backlight, so there is no need for
> Nouveau to register its own backlight interface.
> 
> v2: Do not split information message on two lines as it prevents from grepping
>     it, as pointed out by Lukas Wunner
> 
> v3: Add a missing end-of-line character to the printed message
> 
> Signed-off-by: Pierre Moreau <pierre.morrow@free.fr>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

> ---
>  drm/nouveau/nouveau_backlight.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c
> index a34cd35..8b1ca4a 100644
> --- a/drm/nouveau/nouveau_backlight.c
> +++ b/drm/nouveau/nouveau_backlight.c
> @@ -30,6 +30,7 @@
>   * Register locations derived from NVClock by Roderick Colenbrander
>   */
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/backlight.h>
>  #include <linux/idr.h>
>  
> @@ -267,6 +268,11 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->device;
>  	struct drm_connector *connector;
>  
> +	if (apple_gmux_present()) {
> +		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
> +		return 0;
> +	}
> +
>  	INIT_LIST_HEAD(&drm->bl_connectors);
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -- 
> 2.10.2
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau