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

[Pierre Moreau]

[…]

> 
> > +   const int nb = atomic_inc_return(_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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Pierre Moreau]

On 02:40 PM - Apr 15 2016, Nick Tenney wrote:
> On Fri, Apr 15, 2016 at 11:25 AM, Ilia Mirkin  wrote:
> 
> > On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau 
> > wrote:
> > > On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> > >> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau  > >> 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 
> > >> > ---
> > >> >  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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> >

> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

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

[Pierre Moreau]

On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau  
> 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 
> > ---
> >  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 = >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(, 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,
> >_bl_ops, );
> > +
> > +   // 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(, 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, );
> > +
> > +   // 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(_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;
> > +}
> > --
> > 

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

[Nick Tenney]

On Fri, Apr 15, 2016 at 11:25 AM, Ilia Mirkin  wrote:

> On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau 
> wrote:
> > On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> >> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau 
> 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 
> >> > ---
> >> >  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
-- next part --
An HTML attachment was scrubbed...
URL: 

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

[Ilia Mirkin]

On Fri, Apr 15, 2016 at 11:22 AM, Pierre Moreau  
wrote:
> On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
>> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau  
>> 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 
>> > ---
>> >  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] [PATCH 1/2] nouveau/bl: Assign different names to interfaces

[Ilia Mirkin]

On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau  
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 
> ---
>  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 = >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(, 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,
>_bl_ops, );
> +
> +   // 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(, 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, );
> +
> +   // 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(_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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau