| Message ID | 1460732242-4161-1-git-send-email-pierre.morrow@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@free.fr> wrote: > Currently, every backlight interface created by Nouveau uses the same name, > nv_backlight. This leads to a sysfs warning as it tries to create an already > existing folder. This patch adds a incremented number to the name, but keeps > the initial name as nv_backlight, to avoid possibly breaking userspace; the > second interface will be named nv_backlight1, and so on. > > Fixes: fdo#86539 > Signed-off-by: Pierre Moreau <pierre.morrow@free.fr> > --- > drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++-- > 1 file changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c > index 89eb460..914e2cb 100644 > --- a/drm/nouveau/nouveau_backlight.c > +++ b/drm/nouveau/nouveau_backlight.c > @@ -36,6 +36,10 @@ > #include "nouveau_reg.h" > #include "nouveau_encoder.h" > > +static atomic_t bl_interfaces_nb = { 0 }; static data is initialized to 0, this should be unnecessary. I'd also call it "backlights" or something like that. No need for multiple words... > + > +static char* nouveau_get_backlight_name(void); Please organize the code to avoid forward declarations. > + > static int > nv40_get_intensity(struct backlight_device *bd) > { > @@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector) > struct nvif_object *device = &drm->device.object; > struct backlight_properties props; > struct backlight_device *bd; > + char* backlight_name = NULL; > > if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK)) > return 0; > @@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector) > memset(&props, 0, sizeof(struct backlight_properties)); > props.type = BACKLIGHT_RAW; > props.max_brightness = 31; > - bd = backlight_device_register("nv_backlight", connector->kdev, drm, > + backlight_name = nouveau_get_backlight_name(); > + bd = backlight_device_register(backlight_name , connector->kdev, drm, > &nv40_bl_ops, &props); > + > + // backlight_device_register() makes a copy > + kfree(backlight_name); > + backlight_name = NULL; > + > if (IS_ERR(bd)) > return PTR_ERR(bd); > drm->backlight = bd; > @@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector) > struct backlight_properties props; > struct backlight_device *bd; > const struct backlight_ops *ops; > + char* backlight_name = NULL; > > nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS); > if (!nv_encoder) { > @@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector) > memset(&props, 0, sizeof(struct backlight_properties)); > props.type = BACKLIGHT_RAW; > props.max_brightness = 100; > - bd = backlight_device_register("nv_backlight", connector->kdev, > + backlight_name = nouveau_get_backlight_name(); > + bd = backlight_device_register(backlight_name, connector->kdev, > nv_encoder, ops, &props); > + > + // backlight_device_register() makes a copy > + kfree(backlight_name); > + backlight_name = NULL; > + > if (IS_ERR(bd)) > return PTR_ERR(bd); > > @@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev) > drm->backlight = NULL; > } > } > + > +static char* > +nouveau_get_backlight_name(void) > +{ > + // 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0' > + char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL); Making this stack-allocated in the caller would be so much simpler... > + const int nb = atomic_inc_return(&bl_interfaces_nb) - 1; This kinda sucks if you reload nouveau a bunch. How about using an "ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that one. > + if (nb > 0 && nb < 100) > + sprintf(backlight_name, "nv_backlight%d", nb); > + else > + sprintf(backlight_name, "nv_backlight"); > + return backlight_name; > +} > -- > 2.8.0 > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
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:22 AM, Pierre Moreau <pierre.morrow@free.fr> wrote: > On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote: >> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@free.fr> wrote: >> > Currently, every backlight interface created by Nouveau uses the same name, >> > nv_backlight. This leads to a sysfs warning as it tries to create an already >> > existing folder. This patch adds a incremented number to the name, but keeps >> > the initial name as nv_backlight, to avoid possibly breaking userspace; the >> > second interface will be named nv_backlight1, and so on. >> > >> > Fixes: fdo#86539 >> > Signed-off-by: Pierre Moreau <pierre.morrow@free.fr> >> > --- >> > drm/nouveau/nouveau_backlight.c | 35 +++++++++++++++++++++++++++++++++-- >> > 1 file changed, 33 insertions(+), 2 deletions(-) >> > >> > diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c >> > index 89eb460..914e2cb 100644 >> > --- a/drm/nouveau/nouveau_backlight.c >> > +++ b/drm/nouveau/nouveau_backlight.c >> > @@ -36,6 +36,10 @@ >> > #include "nouveau_reg.h" >> > #include "nouveau_encoder.h" >> > >> > +static atomic_t bl_interfaces_nb = { 0 }; >> >> static data is initialized to 0, this should be unnecessary. > > I didn’t know that. But on the other hand, I like having it explicit, and it > should not add any overhead. It increases the size of the object file. I believe it's kernel policy to avoid static initializations to 0. (Note that this doesn't hold in regular user applications, just the kernel.) -ilia
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 >
On 02:40 PM - Apr 15 2016, Nick Tenney wrote: > 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. Ack’ed. I’ll fix that in the v2. Thanks! Pierre > > > >> > 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 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[…] > > > + const int nb = atomic_inc_return(&bl_interfaces_nb) - 1; > > This kinda sucks if you reload nouveau a bunch. How about using an > "ida". Have a look in drivers/gpu/drm/drm_crtc.c for how I use that > one. I had a quick look at drm_crtc.c. This seems to be exactly what I need, except that I do not see how I can enforce to have one of the active ones being named `nv_backlight`, to not break (possibly) existing userspace applications. (I’m guessing that I would be doing the same as in drm_crtc.c: looping over all connectors and assigning them an ida, regardless of them being active or not, as they might become active later on.) There is another issue that I failed to address with this patch, and just realised about it. Each time `nvXX_backlight_init(connector)` is called on a connector, a new backlight device is created, and is stored in `drm->backlight`, which is unique at a device level, not connector level, meaning the old one just gets overriden (and leaked). Having a list `drm->backlights` should solve this issue. Would that be fine with you Ben? Pierre > > > + if (nb > 0 && nb < 100) > > + sprintf(backlight_name, "nv_backlight%d", nb); > > + else > > + sprintf(backlight_name, "nv_backlight"); > > + return backlight_name; > > +} > > -- > > 2.8.0 > > > > _______________________________________________ > > Nouveau mailing list > > Nouveau@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/nouveau
diff --git a/drm/nouveau/nouveau_backlight.c b/drm/nouveau/nouveau_backlight.c index 89eb460..914e2cb 100644 --- a/drm/nouveau/nouveau_backlight.c +++ b/drm/nouveau/nouveau_backlight.c @@ -36,6 +36,10 @@ #include "nouveau_reg.h" #include "nouveau_encoder.h" +static atomic_t bl_interfaces_nb = { 0 }; + +static char* nouveau_get_backlight_name(void); + static int nv40_get_intensity(struct backlight_device *bd) { @@ -74,6 +78,7 @@ nv40_backlight_init(struct drm_connector *connector) struct nvif_object *device = &drm->device.object; struct backlight_properties props; struct backlight_device *bd; + char* backlight_name = NULL; if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK)) return 0; @@ -81,8 +86,14 @@ nv40_backlight_init(struct drm_connector *connector) memset(&props, 0, sizeof(struct backlight_properties)); props.type = BACKLIGHT_RAW; props.max_brightness = 31; - bd = backlight_device_register("nv_backlight", connector->kdev, drm, + backlight_name = nouveau_get_backlight_name(); + bd = backlight_device_register(backlight_name , connector->kdev, drm, &nv40_bl_ops, &props); + + // backlight_device_register() makes a copy + kfree(backlight_name); + backlight_name = NULL; + if (IS_ERR(bd)) return PTR_ERR(bd); drm->backlight = bd; @@ -182,6 +193,7 @@ nv50_backlight_init(struct drm_connector *connector) struct backlight_properties props; struct backlight_device *bd; const struct backlight_ops *ops; + char* backlight_name = NULL; nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS); if (!nv_encoder) { @@ -203,8 +215,14 @@ nv50_backlight_init(struct drm_connector *connector) memset(&props, 0, sizeof(struct backlight_properties)); props.type = BACKLIGHT_RAW; props.max_brightness = 100; - bd = backlight_device_register("nv_backlight", connector->kdev, + backlight_name = nouveau_get_backlight_name(); + bd = backlight_device_register(backlight_name, connector->kdev, nv_encoder, ops, &props); + + // backlight_device_register() makes a copy + kfree(backlight_name); + backlight_name = NULL; + if (IS_ERR(bd)) return PTR_ERR(bd); @@ -252,3 +270,16 @@ nouveau_backlight_exit(struct drm_device *dev) drm->backlight = NULL; } } + +static char* +nouveau_get_backlight_name(void) +{ + // 12 chars for "nv_backlight" + 2 for two digits number + 1 for '\0' + char* backlight_name = (char*)kmalloc(sizeof(char[15]), GFP_KERNEL); + const int nb = atomic_inc_return(&bl_interfaces_nb) - 1; + if (nb > 0 && nb < 100) + sprintf(backlight_name, "nv_backlight%d", nb); + else + sprintf(backlight_name, "nv_backlight"); + return backlight_name; +}
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(-)