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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
> > 
> > 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@xxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> 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

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux