Skip to contentSkip to conversation

nouveau/bl: Assign different names to interfaces

PATCH Superseded Pierre Moreau 8 participants 21 messages Apr 15, 2016 – Dec 8, 2016 12 messages not in the archive

Subsystems (3)
DRM drivers DRM misc nouveau
lore
Share & tools
lore

Pierre Moreau

author
#0 lore raw

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

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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
drm/nouveau/nouveau_backlight.c +33 -2
index 89eb460..914e2cb 100644
@@ -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;
+}
Signature · 6 lines
2.8.0

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau
#1 lore raw

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

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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)
drm/nouveau/nouveau_backlight.c +7 -0
index 914e2cb..37a1577 100644
@@ -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)
Signature · 6 lines
2.8.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw
In reply to Pierre Moreau
Quoted 17 lines · 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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
drm/nouveau/nouveau_backlight.c +0 -0
index 89eb460..914e2cb 100644

drm/nouveau/nouveau_backlight.c line 39

Quoted 4 lines
drm/nouveau/nouveau_backlight.c +1 -0
@@ -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...

drm/nouveau/nouveau_backlight.c line 41

Quoted 2 lines
drm/nouveau/nouveau_backlight.c +2 -0
+
+static char* nouveau_get_backlight_name(void);

Please organize the code to avoid forward declarations.

Quoted 53 lines
drm/nouveau/nouveau_backlight.c +17 -2
+
 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);

drm/nouveau/nouveau_backlight.c nouveau_backlight_exit(struct drm_device *dev) · line 278

Quoted 9 lines
drm/nouveau/nouveau_backlight.c +6 -0
@@ -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...

drm/nouveau/nouveau_backlight.c nouveau_backlight_exit(struct drm_device *dev) · line 279

Quoted 1 line
drm/nouveau/nouveau_backlight.c +1 -0
+       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.

Quoted 9 lines
drm/nouveau/nouveau_backlight.c +6 -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

Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): CAKb7Uvg3OxWuooJ=otSb1N_yRr1VcVgvH4K0DDy1zzDs9js4CA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

author
#4 lore raw
Quoted 21 lines · Ilia Mirkin
Quoted 20 lines · 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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
drm/nouveau/nouveau_backlight.c +0 -0
index 89eb460..914e2cb 100644
@@ -36,6 +36,10 @@
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
Quoted 3 lines

drm/nouveau/nouveau_backlight.c line 39

Quoted 1 line
drm/nouveau/nouveau_backlight.c +1 -0
+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.

Quoted 3 lines

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.

Quoted 58 lines

drm/nouveau/nouveau_backlight.c line 41

Quoted 3 lines
drm/nouveau/nouveau_backlight.c +2 -0
+
+static char* nouveau_get_backlight_name(void);

Please organize the code to avoid forward declarations.

Quoted 52 lines
drm/nouveau/nouveau_backlight.c +17 -2
+
 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);
Quoted 12 lines

drm/nouveau/nouveau_backlight.c nouveau_backlight_exit(struct drm_device *dev) · line 278

Quoted 10 lines
drm/nouveau/nouveau_backlight.c +6 -0
@@ -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.

Quoted 6 lines

drm/nouveau/nouveau_backlight.c nouveau_backlight_exit(struct drm_device *dev) · line 279

Quoted 2 lines
drm/nouveau/nouveau_backlight.c +1 -0
+       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

Quoted 10 lines
Quoted 10 lines
drm/nouveau/nouveau_backlight.c +6 -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

Quoted 4 lines
Quoted 4 lines

_______________________________________________
Nouveau mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 20160415152212.GA1697-Klvq+9u5igPhiM2yCknSfMugMpMbD5Xr@public.gmane.org

#6 lore raw
Quoted 22 lines · Pierre Moreau
Quoted 21 lines · Ilia Mirkin
Quoted 20 lines · 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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)
drm/nouveau/nouveau_backlight.c +0 -0
index 89eb460..914e2cb 100644
@@ -36,6 +36,10 @@
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
Quoted 6 lines
Quoted 3 lines

drm/nouveau/nouveau_backlight.c line 39

Quoted 1 line
drm/nouveau/nouveau_backlight.c +1 -0
+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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): CAKb7UviwNACmbjk-TpMzUKhvFO8ES+7fqn6UBvzJA_S-WYb96Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#8 lore raw
Quoted 5 lines · Ilia Mirkin
Quoted 2 lines · Pierre Moreau
Quoted 1 line · Ilia Mirkin

On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <[email protected]>

wrote:

Quoted 12 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

Currently, every backlight interface created by Nouveau uses the same

name,

Quoted 1 line
Quoted 1 line
Quoted 1 line

nv_backlight. This leads to a sysfs warning as it tries to create an

already

Quoted 1 line
Quoted 1 line
Quoted 1 line

existing folder. This patch adds a incremented number to the name,

but keeps

Quoted 1 line
Quoted 1 line
Quoted 1 line

the initial name as nv_backlight, to avoid possibly breaking

userspace; the

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

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.

Quoted 27 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Signed-off-by Pierre Moreau <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35

+++++++++++++++++++++++++++++++++--

Quoted 3 lines
Quoted 3 lines

Review

Quoted 3 lines
 1 file changed, 33 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drm/nouveau/nouveau_backlight.c

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

index 89eb460..914e2cb 100644

drm/nouveau/nouveau_backlight.c +0 -0
@@ -36,6 +36,10 @@
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
Quoted 5 lines
Quoted 3 lines

drm/nouveau/nouveau_backlight.c line 39

Quoted 1 line
drm/nouveau/nouveau_backlight.c +1 -0
+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

Quoted 1 line

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

Quoted 6 lines

-ilia
_______________________________________________
Nouveau mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

author
#9 lore raw
Quoted 14 lines · Nick Tenney
Quoted 12 lines · Ilia Mirkin
Quoted 2 lines · Pierre Moreau
Quoted 1 line · Ilia Mirkin

On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <[email protected]>

Quoted 1 line · wrote:
Quoted 1 line
Quoted 1 line

Currently, every backlight interface created by Nouveau uses the same

name,

Quoted 1 line
Quoted 1 line
Quoted 1 line

nv_backlight. This leads to a sysfs warning as it tries to create an

already

Quoted 1 line
Quoted 1 line
Quoted 1 line

existing folder. This patch adds a incremented number to the name,

but keeps

Quoted 1 line
Quoted 1 line
Quoted 1 line

the initial name as nv_backlight, to avoid possibly breaking

Quoted 8 lines
Quoted 5 lines

userspace; the

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

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

Quoted 35 lines
Quoted 28 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Signed-off-by Pierre Moreau <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 35

+++++++++++++++++++++++++++++++++--

Quoted 3 lines
Quoted 3 lines

Review

Quoted 3 lines
 1 file changed, 33 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drm/nouveau/nouveau_backlight.c

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

index 89eb460..914e2cb 100644

drm/nouveau/nouveau_backlight.c +0 -0
@@ -36,6 +36,10 @@
 #include "nouveau_reg.h"
 #include "nouveau_encoder.h"
Quoted 5 lines
Quoted 3 lines

drm/nouveau/nouveau_backlight.c line 39

Quoted 1 line
drm/nouveau/nouveau_backlight.c +1 -0
+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

Quoted 1 line

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

Quoted 6 lines

-ilia
_______________________________________________
Nouveau mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 4 lines

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

#10 lore raw
In reply to Ilia Mirkin

[…]

Quoted 6 lines
Quoted 2 lines

+ 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

Quoted 10 lines
Quoted 10 lines
drm/nouveau/nouveau_backlight.c +6 -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

Quoted 4 lines
Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 1460732242-4161-1-git-send-email-pierre.morrow-GANU6spQydw@public.gmane.org

author
#12 lore raw

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

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 <[email protected]>
---
 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(-)
drm/nouveau/nouveau_backlight.c +60 -4
index 89eb460..41330e4 100644
@@ -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);
+}
drm/nouveau/nouveau_display.h +10 -0
index 5a57d8b..bb07cc8 100644
@@ -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
drm/nouveau/nouveau_drm.c +2 -0
index d06877d..0d89d7c 100644
@@ -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
drm/nouveau/nouveau_drm.h +1 -0
index 5c363ed..593828e 100644
@@ -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;
Signature · 6 lines
2.8.0

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 1460923062-10849-1-git-send-email-pierre.morrow-GANU6spQydw@public.gmane.org

author
#14 lore raw

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

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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)
drm/nouveau/nouveau_backlight.c +7 -0
index 41330e4..94ac3cb 100644
@@ -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) {
Signature · 6 lines
2.8.0

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau
#15 lore raw

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

Hi,

Quoted 21 lines · 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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 7 +++++++
 1 file changed, 7 insertions(+)
drm/nouveau/nouveau_backlight.c +1 -0
index 41330e4..94ac3cb 100644
@@ -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>

drm/nouveau/nouveau_backlight.c nouveau_backlight_init(struct drm_device *dev) · line 263

Quoted 6 lines
drm/nouveau/nouveau_backlight.c +3 -0
@@ -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

Quoted 8 lines
drm/nouveau/nouveau_backlight.c +3 -0
+		return 0;
+	}
+
 	INIT_LIST_HEAD(&drm->bl_connectors);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
Signature · 1 line
2.8.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 1460923062-10849-2-git-send-email-pierre.morrow-GANU6spQydw@public.gmane.org

author
#17 lore raw

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

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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drm/nouveau/nouveau_backlight.c +6 -0
index 41330e4..8429ceb 100644
@@ -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) {
Signature · 6 lines
2.8.2

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 1462105927-4328-1-git-send-email-pierre.morrow-GANU6spQydw@public.gmane.org

author
#19 lore raw

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

Quoted 5 lines · 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

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

Pierre

Quoted 37 lines
Signed-off-by Pierre Moreau <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drm/nouveau/nouveau_backlight.c +6 -0
index 41330e4..8429ceb 100644
@@ -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) {
Signature · 6 lines
2.8.2

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

author
#20 lore raw

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

In reply to a message not in the archive

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 <[email protected]>
---
 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(-)
drm/nouveau/nouveau_backlight.c +70 -4
index 5e2c568..a34cd35 100644
@@ -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);
+}
drm/nouveau/nouveau_display.h +10 -0
index 330fe0f..4a75df0 100644
@@ -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 *
drm/nouveau/nouveau_drm.c +2 -0
index 064a925..59348fc 100644
@@ -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
drm/nouveau/nouveau_drv.h +1 -0
index 9730c0e..8d5ed5b 100644
@@ -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;
Signature · 6 lines
2.10.2

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#22 lore raw

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

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 <[email protected]>
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drm/nouveau/nouveau_backlight.c +6 -0
index a34cd35..8b1ca4a 100644
@@ -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) {
Signature · 6 lines
2.10.2

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): [email protected]

Reviewed-by
#24 lore raw

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

Quoted 9 lines · 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 <[email protected]>
Reviewed-by Lukas Wunner <[email protected]>

Thanks,

Lukas

Quoted 30 lines
---
 drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drm/nouveau/nouveau_backlight.c +6 -0
index a34cd35..8b1ca4a 100644
@@ -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) {
Signature · 1 line
2.10.2

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

#25 lore raw

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

In reply to Pierre Moreau

From: Pierre Moreau <[email protected]>

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 <[email protected]>
---
 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(-)
drivers/gpu/drm/nouveau/nouveau_backlight.c +61 -4
index 5e2c568..0e69612 100644
@@ -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);
+}
drivers/gpu/drm/nouveau/nouveau_display.h +10 -0
index 330fe0f..4a75df0 100644
@@ -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 *
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -0
index 9876e6f..2b93b55 100644
@@ -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
drivers/gpu/drm/nouveau/nouveau_drv.h +1 -0
index 4cd47ba..923dbce 100644
@@ -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;
Signature · 6 lines
2.10.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#26 lore raw

PATCH 2/2 Do not register interface if Apple GMUX detected

From: Pierre Moreau <[email protected]>

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 <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/gpu/drm/nouveau/nouveau_backlight.c +6 -0
index 0e69612..3c91c24 100644
@@ -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) {
Signature · 6 lines
2.10.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 20161113195707.4113-2-dev-WLoDKDh+7sdAfugRpC6u6w@public.gmane.org

Reviewed-by
#28 lore raw

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

Quoted 9 lines · Pierre Moreau

From: Pierre Moreau <[email protected]>

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 <[email protected]>
Reviewed-by Lukas Wunner <[email protected]>

Thanks,

Lukas

Quoted 35 lines
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/gpu/drm/nouveau/nouveau_backlight.c +6 -0
index 0e69612..3c91c24 100644
@@ -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) {
Signature · 6 lines
2.10.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 20161113195707.4113-1-dev-WLoDKDh+7sdAfugRpC6u6w@public.gmane.org

#30 lore raw
Quoted 54 lines · Pierre Moreau

From: Pierre Moreau <[email protected]>

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 <[email protected]>
---
 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(-)
drivers/gpu/drm/nouveau/nouveau_backlight.c +11 -0
index 5e2c568..0e69612 100644
@@ -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

drivers/gpu/drm/nouveau/nouveau_backlight.c line 59

Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_backlight.c +10 -0
+		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

Quoted 164 lines
drivers/gpu/drm/nouveau/nouveau_backlight.c +40 -4
 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);
+}
drivers/gpu/drm/nouveau/nouveau_display.h +10 -0
index 330fe0f..4a75df0 100644
@@ -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 *
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -0
index 9876e6f..2b93b55 100644
@@ -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
drivers/gpu/drm/nouveau/nouveau_drv.h +1 -0
index 4cd47ba..923dbce 100644
@@ -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;
Signature · 6 lines
2.10.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): 20161114101740.GA9829-JFq808J9C/[email protected]

author
#32 lore raw
Quoted 60 lines · Lukas Wunner
Quoted 59 lines · Pierre Moreau

From: Pierre Moreau <[email protected]>

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 <[email protected]>
---
 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(-)
drivers/gpu/drm/nouveau/nouveau_backlight.c +16 -0
index 5e2c568..0e69612 100644
@@ -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);
Quoted 8 lines

drivers/gpu/drm/nouveau/nouveau_backlight.c line 59

Quoted 5 lines
drivers/gpu/drm/nouveau/nouveau_backlight.c +5 -0
+	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.

Quoted 165 lines

Thanks,

Lukas

Quoted 160 lines
drivers/gpu/drm/nouveau/nouveau_backlight.c +40 -4
 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);
+}
drivers/gpu/drm/nouveau/nouveau_display.h +10 -0
index 330fe0f..4a75df0 100644
@@ -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 *
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -0
index 9876e6f..2b93b55 100644
@@ -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
drivers/gpu/drm/nouveau/nouveau_drv.h +1 -0
index 4cd47ba..923dbce 100644
@@ -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;
Signature · 1 line
2.10.2
Quoted 8 lines
Quoted 4 lines

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau