| Message ID | 20220602141850.21301-5-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Dual LFP/EDP enablement | expand |
On Thu, 02 Jun 2022, Animesh Manna <animesh.manna@intel.com> wrote: > From: Arun R Murthy <arun.r.murthy@intel.com> > > With the enablement of dual eDP, there will have to exist two entries of > backlight sysfs file. In order to avoid sysfs file name duplication, the > file names are prepended with the connector name. Fixed by 20f85ef89d94 ("drm/i915/backlight: use unique backlight device names") about a year ago. BR, Jani. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_backlight.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c > index 68513206a66a..211fa0f07239 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c > @@ -967,6 +967,8 @@ int intel_backlight_device_register(struct intel_connector *connector) > props.power = FB_BLANK_POWERDOWN; > > name = kstrdup("intel_backlight", GFP_KERNEL); > + name = kasprintf(GFP_KERNEL, "%s.intel_backlight", > + connector->base.name); > if (!name) > return -ENOMEM;
> On Thu, 02 Jun 2022, Animesh Manna <animesh.manna@intel.com> wrote: > > From: Arun R Murthy <arun.r.murthy@intel.com> > > > > With the enablement of dual eDP, there will have to exist two entries > > of backlight sysfs file. In order to avoid sysfs file name > > duplication, the file names are prepended with the connector name. > > Fixed by 20f85ef89d94 ("drm/i915/backlight: use unique backlight device > names") about a year ago. > This patches checks if the return value is -EEXIST and then acts accordingly, but -EEXIST is not returned. struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, const char *name, umode_t mode, kuid_t uid, kgid_t gid, loff_t size, const struct kernfs_ops *ops, void *priv, const void *ns, struct lock_class_key *key) { struct kernfs_node *kn; unsigned flags; int rc; flags = KERNFS_FILE; kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, uid, gid, flags); if (!kn) return ERR_PTR(-ENOMEM); So the condition check with not be satisfied and the backlight registration will fail for the 2nd backlight device. Thanks and Regards, Arun R Murthy --------------------
On Fri, 03 Jun 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> On Thu, 02 Jun 2022, Animesh Manna <animesh.manna@intel.com> wrote: >> > From: Arun R Murthy <arun.r.murthy@intel.com> >> > >> > With the enablement of dual eDP, there will have to exist two entries >> > of backlight sysfs file. In order to avoid sysfs file name >> > duplication, the file names are prepended with the connector name. >> >> Fixed by 20f85ef89d94 ("drm/i915/backlight: use unique backlight device >> names") about a year ago. >> > This patches checks if the return value is -EEXIST and then acts accordingly, but -EEXIST is not returned. > struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, > const char *name, > umode_t mode, kuid_t uid, kgid_t gid, > loff_t size, > const struct kernfs_ops *ops, > void *priv, const void *ns, > struct lock_class_key *key) > { > struct kernfs_node *kn; > unsigned flags; > int rc; > > flags = KERNFS_FILE; > > kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, > uid, gid, flags); > if (!kn) > return ERR_PTR(-ENOMEM); > > So the condition check with not be satisfied and the backlight registration will fail for the 2nd backlight device. But the file isn't added by kernfs_new_node(), it just allocates the node. See the kernfs_add_one() later in __kernfs_create_file(). BR, Jani. > > Thanks and Regards, > Arun R Murthy > --------------------
> On Fri, 03 Jun 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: > >> On Thu, 02 Jun 2022, Animesh Manna <animesh.manna@intel.com> > wrote: > >> > From: Arun R Murthy <arun.r.murthy@intel.com> > >> > > >> > With the enablement of dual eDP, there will have to exist two > >> > entries of backlight sysfs file. In order to avoid sysfs file name > >> > duplication, the file names are prepended with the connector name. > >> > >> Fixed by 20f85ef89d94 ("drm/i915/backlight: use unique backlight > >> device > >> names") about a year ago. > >> > > This patches checks if the return value is -EEXIST and then acts accordingly, > but -EEXIST is not returned. > > struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, > > const char *name, > > umode_t mode, kuid_t uid, kgid_t gid, > > loff_t size, > > const struct kernfs_ops *ops, > > void *priv, const void *ns, > > struct lock_class_key *key) { > > struct kernfs_node *kn; > > unsigned flags; > > int rc; > > > > flags = KERNFS_FILE; > > > > kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, > > uid, gid, flags); > > if (!kn) > > return ERR_PTR(-ENOMEM); > > > > So the condition check with not be satisfied and the backlight registration > will fail for the 2nd backlight device. > > But the file isn't added by kernfs_new_node(), it just allocates the node. See > the kernfs_add_one() later in __kernfs_create_file(). > Moreover now that we will be supporting dual display, wouldn't it be better to have the same file naming convention for both the displays? Without this patch, the first backlight would create an interface with name intel_backlight and for the second it would create as "cardXX-XXX-backlight". There wont be any similarities in the backlight naming convention. Would it be better to maintain the same naming convention across the displays? Thanks and Regards, Arun R Murthy --------------------
On Tue, 21 Jun 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> On Fri, 03 Jun 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> >> On Thu, 02 Jun 2022, Animesh Manna <animesh.manna@intel.com> >> wrote: >> >> > From: Arun R Murthy <arun.r.murthy@intel.com> >> >> > >> >> > With the enablement of dual eDP, there will have to exist two >> >> > entries of backlight sysfs file. In order to avoid sysfs file name >> >> > duplication, the file names are prepended with the connector name. >> >> >> >> Fixed by 20f85ef89d94 ("drm/i915/backlight: use unique backlight >> >> device >> >> names") about a year ago. >> >> >> > This patches checks if the return value is -EEXIST and then acts accordingly, >> but -EEXIST is not returned. >> > struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, >> > const char *name, >> > umode_t mode, kuid_t uid, kgid_t gid, >> > loff_t size, >> > const struct kernfs_ops *ops, >> > void *priv, const void *ns, >> > struct lock_class_key *key) { >> > struct kernfs_node *kn; >> > unsigned flags; >> > int rc; >> > >> > flags = KERNFS_FILE; >> > >> > kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, >> > uid, gid, flags); >> > if (!kn) >> > return ERR_PTR(-ENOMEM); >> > >> > So the condition check with not be satisfied and the backlight registration >> will fail for the 2nd backlight device. >> >> But the file isn't added by kernfs_new_node(), it just allocates the node. See >> the kernfs_add_one() later in __kernfs_create_file(). >> > Moreover now that we will be supporting dual display, wouldn't it > be better to have the same file naming convention for both the > displays? > Without this patch, the first backlight would create an interface > with name intel_backlight and for the second it would create as > "cardXX-XXX-backlight". There wont be any similarities in the > backlight naming convention. > Would it be better to maintain the same naming convention > across the displays? The old name can't be changed. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c index 68513206a66a..211fa0f07239 100644 --- a/drivers/gpu/drm/i915/display/intel_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_backlight.c @@ -967,6 +967,8 @@ int intel_backlight_device_register(struct intel_connector *connector) props.power = FB_BLANK_POWERDOWN; name = kstrdup("intel_backlight", GFP_KERNEL); + name = kasprintf(GFP_KERNEL, "%s.intel_backlight", + connector->base.name); if (!name) return -ENOMEM;