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.

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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau

On Fri, Apr 15, 2016 at 11:25 AM, Ilia Mirkin imirkin@alum.mit.edu wrote:

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

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@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

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

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

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

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

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@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

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