On 11:06 AM - Apr 15 2016, Ilia Mirkin wrote:
> On Fri, Apr 15, 2016 at 10:57 AM, Pierre Moreau <pierre.morrow@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
