LinuxLists.cc - [PATCH] OMAPDSS: restore "name" sysfs entry.

2015-02-24 10:40:51

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

Hi,

On 24/02/15 11:37, NeilBrown wrote:
>
>
> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> OMAPDSS: rename display-sysfs 'name' entry
>
> broke the xorg X server on my device as it couldn't find the display
> any more. It needs the 'name' file and now there isn't one.
>
> That commit claims that 'name' is not compatible with i2c or spi.
> i2c does register it own 'name' file, but spi does not, hence my
> problem - I have an spi display.
>
> So create a special case for i2c: add the name attribute for non-i2c
> devices.

What X driver is that? What's it doing with the display name? Is it just
using the display name to show something for the user, and the returned
value can be essentially any string?

Tomi

Attachments:

2015-02-24 17:08:20

by H. Nikolaus Schaller

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

I have tested it with

X.Org X Server 1.12.4
Release Date: 2012-08-27
X Protocol Version 11, Revision 0
Build Operating System: Linux 3.2.0-4-mx5 armv7l Debian

(coming from Debian Wheezy) on:

* gta04 (SPI panel)
* openpandora (SPI panel)
* BeagleBoard XM (with RGB panel)
* PandaBoard ES (w/o panel)
* omap5432-evm + Pyra prototype (MIPI panel)

Without this patch, X Server simply fails with:

Fatal server error:
no screens found

So please add
Tested-by: Nikolaus Schaller <[email protected]>

Thanks,
Nikolaus

Am 24.02.2015 um 10:37 schrieb NeilBrown <[email protected]>:

>
>
> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> OMAPDSS: rename display-sysfs 'name' entry
>
> broke the xorg X server on my device as it couldn't find the display
> any more. It needs the 'name' file and now there isn't one.
>
> That commit claims that 'name' is not compatible with i2c or spi.
> i2c does register it own 'name' file, but spi does not, hence my
> problem - I have an spi display.
>
> So create a special case for i2c: add the name attribute for non-i2c
> devices.
>
> Fixes: 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> Signed-off-by: NeilBrown <[email protected]>
>
> --
>
> Hi Tomi,
> I wonder if you would consider this for the next merge window (or maybe even sooner as a bug-fix).
> I haven't tested it with an i2c display, but it certainly allows xorg to work
> on my spi-attached display.
>
> Thanks,
> NeilBrown
>
>
> diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> index 5a2095a98ed8..53897b743130 100644
> --- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
> +++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> @@ -25,6 +25,15 @@
> #include <linux/platform_device.h>
> #include <linux/sysfs.h>
>
> +#if IS_ENABLED(CONFIG_I2C)
> +#include <linux/i2c.h>
> +#else
> +static inline int i2c_verify_client(struct device *dev)
> +{
> + return NULL;
> +}
> +#endif
> +
> #include <video/omapdss.h>
> #include "dss.h"
>
> @@ -278,6 +287,7 @@ static ssize_t display_wss_store(struct device *dev,
> }
>
> static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> +static DEVICE_ATTR(name, S_IRUGO, display_name_show, NULL);
> static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
> display_enabled_show, display_enabled_store);
> static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
> @@ -315,6 +325,17 @@ int display_init_sysfs(struct platform_device *pdev)
> DSSERR("failed to create sysfs files\n");
> goto err;
> }
> + if (!i2c_verify_client(dssdev->dev)) {
> + /*
> + * Add 'name' file - not compatible with i2c, but
> + * need by Xorg for other devices.
> + */
> + r = sysfs_create_file(kobj, &dev_attr_name.attr);
> + if (r) {
> + DSSERR("fail to create 'name' sysfs file\n");
> + goto err;
> + }
> + }
>
> r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
> if (r) {
> @@ -341,5 +362,7 @@ void display_uninit_sysfs(struct platform_device *pdev)
> sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
> sysfs_remove_files(&dssdev->dev->kobj,
> display_sysfs_attrs);
> + sysfs_remove_file(&dssdev->dev->kobj,
> + &dev_attr_name.attr);
> }
> }

2015-02-24 20:31:43

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <[email protected]>
wrote:

> Hi,
>
> On 24/02/15 11:37, NeilBrown wrote:
> >
> >
> > commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> > OMAPDSS: rename display-sysfs 'name' entry
> >
> > broke the xorg X server on my device as it couldn't find the display
> > any more. It needs the 'name' file and now there isn't one.
> >
> > That commit claims that 'name' is not compatible with i2c or spi.
> > i2c does register it own 'name' file, but spi does not, hence my
> > problem - I have an spi display.
> >
> > So create a special case for i2c: add the name attribute for non-i2c
> > devices.
>
> What X driver is that? What's it doing with the display name? Is it just
> using the display name to show something for the user, and the returned
> value can be essentially any string?
>
> Tomi
>
>
/usr/lib/xorg/modules/drivers/omapfb_drv.so
from package xserver-xorg-video-omap3 in Debian.

I don't know where the main upstream source is, but here:

https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258

is the code which reads
/sys/devices/platform/omapdss/display0/name
and fails if that file cannot be opened.

Thanks,
NeilBrown

Attachments:

2015-02-24 21:02:22

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

* NeilBrown <[email protected]> [150224 12:35]:
> On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <[email protected]>
> wrote:
>
> > Hi,
> >
> > On 24/02/15 11:37, NeilBrown wrote:
> > >
> > >
> > > commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> > > OMAPDSS: rename display-sysfs 'name' entry
> > >
> > > broke the xorg X server on my device as it couldn't find the display
> > > any more. It needs the 'name' file and now there isn't one.
> > >
> > > That commit claims that 'name' is not compatible with i2c or spi.
> > > i2c does register it own 'name' file, but spi does not, hence my
> > > problem - I have an spi display.
> > >
> > > So create a special case for i2c: add the name attribute for non-i2c
> > > devices.
> >
> > What X driver is that? What's it doing with the display name? Is it just
> > using the display name to show something for the user, and the returned
> > value can be essentially any string?
> >
> > Tomi
> >
> >
> /usr/lib/xorg/modules/drivers/omapfb_drv.so
> from package xserver-xorg-video-omap3 in Debian.
>
> I don't know where the main upstream source is, but here:
>
> https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258
>
> is the code which reads
> /sys/devices/platform/omapdss/display0/name
> and fails if that file cannot be opened.

Sounds like a kernel interface we need to support forever
and fix the regression.

Regards,

Tony

2015-02-25 07:04:29

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

On 24/02/15 22:31, NeilBrown wrote:
> On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <[email protected]>
> wrote:
>
>> Hi,
>>
>> On 24/02/15 11:37, NeilBrown wrote:
>>>
>>>
>>> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
>>> OMAPDSS: rename display-sysfs 'name' entry
>>>
>>> broke the xorg X server on my device as it couldn't find the display
>>> any more. It needs the 'name' file and now there isn't one.
>>>
>>> That commit claims that 'name' is not compatible with i2c or spi.
>>> i2c does register it own 'name' file, but spi does not, hence my
>>> problem - I have an spi display.
>>>
>>> So create a special case for i2c: add the name attribute for non-i2c
>>> devices.
>>
>> What X driver is that? What's it doing with the display name? Is it just
>> using the display name to show something for the user, and the returned
>> value can be essentially any string?
>>
>> Tomi
>>
>>
> /usr/lib/xorg/modules/drivers/omapfb_drv.so
> from package xserver-xorg-video-omap3 in Debian.
>
> I don't know where the main upstream source is, but here:
>
> https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258
>
> is the code which reads
> /sys/devices/platform/omapdss/display0/name
> and fails if that file cannot be opened.

Thanks. Unfortunately it looks to me that the omapfb_drv uses the
display name to configure things, and as in i2c's case the 'name' is not
the correct dss name, X will probably fail in interesting ways.

Of course, it's already broken in that way, so this fix improves the
situation for non-i2c displays. I'll have a look if I can figure out how
to fix this for all displays.

Tomi

Attachments:

2015-02-25 08:50:11

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

Hi,

On 24/02/15 22:31, NeilBrown wrote:
> On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <[email protected]>
> wrote:
>
>> Hi,
>>
>> On 24/02/15 11:37, NeilBrown wrote:
>>>
>>>
>>> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
>>> OMAPDSS: rename display-sysfs 'name' entry
>>>
>>> broke the xorg X server on my device as it couldn't find the display
>>> any more. It needs the 'name' file and now there isn't one.
>>>
>>> That commit claims that 'name' is not compatible with i2c or spi.
>>> i2c does register it own 'name' file, but spi does not, hence my
>>> problem - I have an spi display.
>>>
>>> So create a special case for i2c: add the name attribute for non-i2c
>>> devices.

How about this patch instead. It separates the underlying device's sysfs directory
from the "displayX" directory, and allows us to have name & display_name. So it
should work for any device type.

From 8e411fa684d42fca35628b41837c6d72e87aaff0 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <[email protected]>
Date: Wed, 25 Feb 2015 10:23:58 +0200
Subject: [PATCH] OMAPDSS: fix regression with display sysfs files

omapdss's sysfs directories for displays used to have 'name' file,
giving the name for the display. This file was later renamed to
'display_name' to avoid conflicts with i2c sysfs 'name' file. Looks like
at least xserver-xorg-video-omap3 requires the 'name' file to be
present.

To fix the regression, this patch creates new kobjects for each display,
allowing us to create sysfs directories for the displays. This way we
have the whole directory for omapdss, and there will be no sysfs file
clashes with the underlying display device's sysfs files.

We can thus add the 'name' sysfs file back.

Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/video/fbdev/omap2/dss/display-sysfs.c | 179 ++++++++++++++------------
include/video/omapdss.h | 1 +
2 files changed, 96 insertions(+), 84 deletions(-)

diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
index 5a2095a98ed8..12186557a9d4 100644
--- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
+++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
@@ -28,44 +28,22 @@
#include <video/omapdss.h>
#include "dss.h"

-static struct omap_dss_device *to_dss_device_sysfs(struct device *dev)
+static ssize_t display_name_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = NULL;
-
- for_each_dss_dev(dssdev) {
- if (dssdev->dev == dev) {
- omap_dss_put_device(dssdev);
- return dssdev;
- }
- }
-
- return NULL;
-}
-
-static ssize_t display_name_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
-
return snprintf(buf, PAGE_SIZE, "%s\n",
dssdev->name ?
dssdev->name : "");
}

-static ssize_t display_enabled_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_enabled_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
-
return snprintf(buf, PAGE_SIZE, "%d\n",
omapdss_device_is_enabled(dssdev));
}

-static ssize_t display_enabled_store(struct device *dev,
- struct device_attribute *attr,
+static ssize_t display_enabled_store(struct omap_dss_device *dssdev,
const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int r;
bool enable;

@@ -90,19 +68,16 @@ static ssize_t display_enabled_store(struct device *dev,
return size;
}

-static ssize_t display_tear_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_tear_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
return snprintf(buf, PAGE_SIZE, "%d\n",
dssdev->driver->get_te ?
dssdev->driver->get_te(dssdev) : 0);
}

-static ssize_t display_tear_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t display_tear_store(struct omap_dss_device *dssdev,
+ const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int r;
bool te;

@@ -120,10 +95,8 @@ static ssize_t display_tear_store(struct device *dev,
return size;
}

-static ssize_t display_timings_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_timings_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
struct omap_video_timings t;

if (!dssdev->driver->get_timings)
@@ -137,10 +110,9 @@ static ssize_t display_timings_show(struct device *dev,
t.y_res, t.vfp, t.vbp, t.vsw);
}

-static ssize_t display_timings_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t display_timings_store(struct omap_dss_device *dssdev,
+ const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
struct omap_video_timings t = dssdev->panel.timings;
int r, found;

@@ -176,10 +148,8 @@ static ssize_t display_timings_store(struct device *dev,
return size;
}

-static ssize_t display_rotate_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_rotate_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int rotate;
if (!dssdev->driver->get_rotate)
return -ENOENT;
@@ -187,10 +157,9 @@ static ssize_t display_rotate_show(struct device *dev,
return snprintf(buf, PAGE_SIZE, "%u\n", rotate);
}

-static ssize_t display_rotate_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t display_rotate_store(struct omap_dss_device *dssdev,
+ const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int rot, r;

if (!dssdev->driver->set_rotate || !dssdev->driver->get_rotate)
@@ -207,10 +176,8 @@ static ssize_t display_rotate_store(struct device *dev,
return size;
}

-static ssize_t display_mirror_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_mirror_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int mirror;
if (!dssdev->driver->get_mirror)
return -ENOENT;
@@ -218,10 +185,9 @@ static ssize_t display_mirror_show(struct device *dev,
return snprintf(buf, PAGE_SIZE, "%u\n", mirror);
}

-static ssize_t display_mirror_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t display_mirror_store(struct omap_dss_device *dssdev,
+ const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
int r;
bool mirror;

@@ -239,10 +205,8 @@ static ssize_t display_mirror_store(struct device *dev,
return size;
}

-static ssize_t display_wss_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t display_wss_show(struct omap_dss_device *dssdev, char *buf)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
unsigned int wss;

if (!dssdev->driver->get_wss)
@@ -253,10 +217,9 @@ static ssize_t display_wss_show(struct device *dev,
return snprintf(buf, PAGE_SIZE, "0x%05x\n", wss);
}

-static ssize_t display_wss_store(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t size)
+static ssize_t display_wss_store(struct omap_dss_device *dssdev,
+ const char *buf, size_t size)
{
- struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
u32 wss;
int r;

@@ -277,50 +240,94 @@ static ssize_t display_wss_store(struct device *dev,
return size;
}

-static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
+struct display_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct omap_dss_device *, char *);
+ ssize_t (*store)(struct omap_dss_device *, const char *, size_t);
+};
+
+#define DISPLAY_ATTR(_name, _mode, _show, _store) \
+ struct display_attribute display_attr_##_name = \
+ __ATTR(_name, _mode, _show, _store)
+
+static DISPLAY_ATTR(name, S_IRUGO, display_name_show, NULL);
+static DISPLAY_ATTR(display_name, S_IRUGO, display_name_show, NULL);
+static DISPLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
display_enabled_show, display_enabled_store);
-static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
+static DISPLAY_ATTR(tear_elim, S_IRUGO|S_IWUSR,
display_tear_show, display_tear_store);
-static DEVICE_ATTR(timings, S_IRUGO|S_IWUSR,
+static DISPLAY_ATTR(timings, S_IRUGO|S_IWUSR,
display_timings_show, display_timings_store);
-static DEVICE_ATTR(rotate, S_IRUGO|S_IWUSR,
+static DISPLAY_ATTR(rotate, S_IRUGO|S_IWUSR,
display_rotate_show, display_rotate_store);
-static DEVICE_ATTR(mirror, S_IRUGO|S_IWUSR,
+static DISPLAY_ATTR(mirror, S_IRUGO|S_IWUSR,
display_mirror_show, display_mirror_store);
-static DEVICE_ATTR(wss, S_IRUGO|S_IWUSR,
+static DISPLAY_ATTR(wss, S_IRUGO|S_IWUSR,
display_wss_show, display_wss_store);

-static const struct attribute *display_sysfs_attrs[] = {
- &dev_attr_display_name.attr,
- &dev_attr_enabled.attr,
- &dev_attr_tear_elim.attr,
- &dev_attr_timings.attr,
- &dev_attr_rotate.attr,
- &dev_attr_mirror.attr,
- &dev_attr_wss.attr,
+static struct attribute *display_sysfs_attrs[] = {
+ &display_attr_name.attr,
+ &display_attr_display_name.attr,
+ &display_attr_enabled.attr,
+ &display_attr_tear_elim.attr,
+ &display_attr_timings.attr,
+ &display_attr_rotate.attr,
+ &display_attr_mirror.attr,
+ &display_attr_wss.attr,
NULL
};

+static ssize_t display_attr_show(struct kobject *kobj, struct attribute *attr,
+ char *buf)
+{
+ struct omap_dss_device *dssdev;
+ struct display_attribute *display_attr;
+
+ dssdev = container_of(kobj, struct omap_dss_device, kobj);
+ display_attr = container_of(attr, struct display_attribute, attr);
+
+ if (!display_attr->show)
+ return -ENOENT;
+
+ return display_attr->show(dssdev, buf);
+}
+
+static ssize_t display_attr_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t size)
+{
+ struct omap_dss_device *dssdev;
+ struct display_attribute *display_attr;
+
+ dssdev = container_of(kobj, struct omap_dss_device, kobj);
+ display_attr = container_of(attr, struct display_attribute, attr);
+
+ if (!display_attr->store)
+ return -ENOENT;
+
+ return display_attr->store(dssdev, buf, size);
+}
+
+static const struct sysfs_ops display_sysfs_ops = {
+ .show = display_attr_show,
+ .store = display_attr_store,
+};
+
+static struct kobj_type display_ktype = {
+ .sysfs_ops = &display_sysfs_ops,
+ .default_attrs = display_sysfs_attrs,
+};
+
int display_init_sysfs(struct platform_device *pdev)
{
struct omap_dss_device *dssdev = NULL;
int r;

for_each_dss_dev(dssdev) {
- struct kobject *kobj = &dssdev->dev->kobj;
-
- r = sysfs_create_files(kobj, display_sysfs_attrs);
+ r = kobject_init_and_add(&dssdev->kobj, &display_ktype,
+ &pdev->dev.kobj, dssdev->alias);
if (r) {
DSSERR("failed to create sysfs files\n");
- goto err;
- }
-
- r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
- if (r) {
- sysfs_remove_files(kobj, display_sysfs_attrs);
-
- DSSERR("failed to create sysfs display link\n");
+ omap_dss_put_device(dssdev);
goto err;
}
}
@@ -338,8 +345,12 @@ void display_uninit_sysfs(struct platform_device *pdev)
struct omap_dss_device *dssdev = NULL;

for_each_dss_dev(dssdev) {
- sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
- sysfs_remove_files(&dssdev->dev->kobj,
- display_sysfs_attrs);
+ if (kobject_name(&dssdev->kobj) == NULL)
+ continue;
+
+ kobject_del(&dssdev->kobj);
+ kobject_put(&dssdev->kobj);
+
+ memset(&dssdev->kobj, 0, sizeof(dssdev->kobj));
}
}
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 60de61fea8e3..c8ed15daad02 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -689,6 +689,7 @@ struct omapdss_dsi_ops {
};

struct omap_dss_device {
+ struct kobject kobj;
struct device *dev;

struct module *owner;
--
2.3.0

Attachments:

2015-02-25 09:21:03

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

On Wed, 25 Feb 2015 10:49:58 +0200 Tomi Valkeinen <[email protected]>
wrote:

> Hi,
>
> On 24/02/15 22:31, NeilBrown wrote:
> > On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <[email protected]>
> > wrote:
> >
> >> Hi,
> >>
> >> On 24/02/15 11:37, NeilBrown wrote:
> >>>
> >>>
> >>> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> >>> OMAPDSS: rename display-sysfs 'name' entry
> >>>
> >>> broke the xorg X server on my device as it couldn't find the display
> >>> any more. It needs the 'name' file and now there isn't one.
> >>>
> >>> That commit claims that 'name' is not compatible with i2c or spi.
> >>> i2c does register it own 'name' file, but spi does not, hence my
> >>> problem - I have an spi display.
> >>>
> >>> So create a special case for i2c: add the name attribute for non-i2c
> >>> devices.
>
> How about this patch instead. It separates the underlying device's sysfs directory
> from the "displayX" directory, and allows us to have name & display_name. So it
> should work for any device type.
>
>
> From 8e411fa684d42fca35628b41837c6d72e87aaff0 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <[email protected]>
> Date: Wed, 25 Feb 2015 10:23:58 +0200
> Subject: [PATCH] OMAPDSS: fix regression with display sysfs files
>
> omapdss's sysfs directories for displays used to have 'name' file,
> giving the name for the display. This file was later renamed to
> 'display_name' to avoid conflicts with i2c sysfs 'name' file. Looks like
> at least xserver-xorg-video-omap3 requires the 'name' file to be
> present.
>
> To fix the regression, this patch creates new kobjects for each display,
> allowing us to create sysfs directories for the displays. This way we
> have the whole directory for omapdss, and there will be no sysfs file
> clashes with the underlying display device's sysfs files.
>
> We can thus add the 'name' sysfs file back.
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/video/fbdev/omap2/dss/display-sysfs.c | 179 ++++++++++++++------------
> include/video/omapdss.h | 1 +
> 2 files changed, 96 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> index 5a2095a98ed8..12186557a9d4 100644
> --- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
> +++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> @@ -28,44 +28,22 @@
> #include <video/omapdss.h>
> #include "dss.h"
>
> -static struct omap_dss_device *to_dss_device_sysfs(struct device *dev)
> +static ssize_t display_name_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = NULL;
> -
> - for_each_dss_dev(dssdev) {
> - if (dssdev->dev == dev) {
> - omap_dss_put_device(dssdev);
> - return dssdev;
> - }
> - }
> -
> - return NULL;
> -}
> -
> -static ssize_t display_name_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> -
> return snprintf(buf, PAGE_SIZE, "%s\n",
> dssdev->name ?
> dssdev->name : "");
> }
>
> -static ssize_t display_enabled_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_enabled_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> -
> return snprintf(buf, PAGE_SIZE, "%d\n",
> omapdss_device_is_enabled(dssdev));
> }
>
> -static ssize_t display_enabled_store(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t display_enabled_store(struct omap_dss_device *dssdev,
> const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int r;
> bool enable;
>
> @@ -90,19 +68,16 @@ static ssize_t display_enabled_store(struct device *dev,
> return size;
> }
>
> -static ssize_t display_tear_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_tear_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> return snprintf(buf, PAGE_SIZE, "%d\n",
> dssdev->driver->get_te ?
> dssdev->driver->get_te(dssdev) : 0);
> }
>
> -static ssize_t display_tear_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_tear_store(struct omap_dss_device *dssdev,
> + const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int r;
> bool te;
>
> @@ -120,10 +95,8 @@ static ssize_t display_tear_store(struct device *dev,
> return size;
> }
>
> -static ssize_t display_timings_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_timings_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> struct omap_video_timings t;
>
> if (!dssdev->driver->get_timings)
> @@ -137,10 +110,9 @@ static ssize_t display_timings_show(struct device *dev,
> t.y_res, t.vfp, t.vbp, t.vsw);
> }
>
> -static ssize_t display_timings_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_timings_store(struct omap_dss_device *dssdev,
> + const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> struct omap_video_timings t = dssdev->panel.timings;
> int r, found;
>
> @@ -176,10 +148,8 @@ static ssize_t display_timings_store(struct device *dev,
> return size;
> }
>
> -static ssize_t display_rotate_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_rotate_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int rotate;
> if (!dssdev->driver->get_rotate)
> return -ENOENT;
> @@ -187,10 +157,9 @@ static ssize_t display_rotate_show(struct device *dev,
> return snprintf(buf, PAGE_SIZE, "%u\n", rotate);
> }
>
> -static ssize_t display_rotate_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_rotate_store(struct omap_dss_device *dssdev,
> + const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int rot, r;
>
> if (!dssdev->driver->set_rotate || !dssdev->driver->get_rotate)
> @@ -207,10 +176,8 @@ static ssize_t display_rotate_store(struct device *dev,
> return size;
> }
>
> -static ssize_t display_mirror_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_mirror_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int mirror;
> if (!dssdev->driver->get_mirror)
> return -ENOENT;
> @@ -218,10 +185,9 @@ static ssize_t display_mirror_show(struct device *dev,
> return snprintf(buf, PAGE_SIZE, "%u\n", mirror);
> }
>
> -static ssize_t display_mirror_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_mirror_store(struct omap_dss_device *dssdev,
> + const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> int r;
> bool mirror;
>
> @@ -239,10 +205,8 @@ static ssize_t display_mirror_store(struct device *dev,
> return size;
> }
>
> -static ssize_t display_wss_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +static ssize_t display_wss_show(struct omap_dss_device *dssdev, char *buf)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> unsigned int wss;
>
> if (!dssdev->driver->get_wss)
> @@ -253,10 +217,9 @@ static ssize_t display_wss_show(struct device *dev,
> return snprintf(buf, PAGE_SIZE, "0x%05x\n", wss);
> }
>
> -static ssize_t display_wss_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_wss_store(struct omap_dss_device *dssdev,
> + const char *buf, size_t size)
> {
> - struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> u32 wss;
> int r;
>
> @@ -277,50 +240,94 @@ static ssize_t display_wss_store(struct device *dev,
> return size;
> }
>
> -static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> -static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
> +struct display_attribute {
> + struct attribute attr;
> + ssize_t (*show)(struct omap_dss_device *, char *);
> + ssize_t (*store)(struct omap_dss_device *, const char *, size_t);
> +};
> +
> +#define DISPLAY_ATTR(_name, _mode, _show, _store) \
> + struct display_attribute display_attr_##_name = \
> + __ATTR(_name, _mode, _show, _store)
> +
> +static DISPLAY_ATTR(name, S_IRUGO, display_name_show, NULL);
> +static DISPLAY_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> +static DISPLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
> display_enabled_show, display_enabled_store);
> -static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(tear_elim, S_IRUGO|S_IWUSR,
> display_tear_show, display_tear_store);
> -static DEVICE_ATTR(timings, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(timings, S_IRUGO|S_IWUSR,
> display_timings_show, display_timings_store);
> -static DEVICE_ATTR(rotate, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(rotate, S_IRUGO|S_IWUSR,
> display_rotate_show, display_rotate_store);
> -static DEVICE_ATTR(mirror, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(mirror, S_IRUGO|S_IWUSR,
> display_mirror_show, display_mirror_store);
> -static DEVICE_ATTR(wss, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(wss, S_IRUGO|S_IWUSR,
> display_wss_show, display_wss_store);
>
> -static const struct attribute *display_sysfs_attrs[] = {
> - &dev_attr_display_name.attr,
> - &dev_attr_enabled.attr,
> - &dev_attr_tear_elim.attr,
> - &dev_attr_timings.attr,
> - &dev_attr_rotate.attr,
> - &dev_attr_mirror.attr,
> - &dev_attr_wss.attr,
> +static struct attribute *display_sysfs_attrs[] = {
> + &display_attr_name.attr,
> + &display_attr_display_name.attr,
> + &display_attr_enabled.attr,
> + &display_attr_tear_elim.attr,
> + &display_attr_timings.attr,
> + &display_attr_rotate.attr,
> + &display_attr_mirror.attr,
> + &display_attr_wss.attr,
> NULL
> };
>
> +static ssize_t display_attr_show(struct kobject *kobj, struct attribute *attr,
> + char *buf)
> +{
> + struct omap_dss_device *dssdev;
> + struct display_attribute *display_attr;
> +
> + dssdev = container_of(kobj, struct omap_dss_device, kobj);
> + display_attr = container_of(attr, struct display_attribute, attr);
> +
> + if (!display_attr->show)
> + return -ENOENT;
> +
> + return display_attr->show(dssdev, buf);
> +}
> +
> +static ssize_t display_attr_store(struct kobject *kobj, struct attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct omap_dss_device *dssdev;
> + struct display_attribute *display_attr;
> +
> + dssdev = container_of(kobj, struct omap_dss_device, kobj);
> + display_attr = container_of(attr, struct display_attribute, attr);
> +
> + if (!display_attr->store)
> + return -ENOENT;
> +
> + return display_attr->store(dssdev, buf, size);
> +}
> +
> +static const struct sysfs_ops display_sysfs_ops = {
> + .show = display_attr_show,
> + .store = display_attr_store,
> +};
> +
> +static struct kobj_type display_ktype = {
> + .sysfs_ops = &display_sysfs_ops,
> + .default_attrs = display_sysfs_attrs,
> +};
> +
> int display_init_sysfs(struct platform_device *pdev)
> {
> struct omap_dss_device *dssdev = NULL;
> int r;
>
> for_each_dss_dev(dssdev) {
> - struct kobject *kobj = &dssdev->dev->kobj;
> -
> - r = sysfs_create_files(kobj, display_sysfs_attrs);
> + r = kobject_init_and_add(&dssdev->kobj, &display_ktype,
> + &pdev->dev.kobj, dssdev->alias);
> if (r) {
> DSSERR("failed to create sysfs files\n");
> - goto err;
> - }
> -
> - r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
> - if (r) {
> - sysfs_remove_files(kobj, display_sysfs_attrs);
> -
> - DSSERR("failed to create sysfs display link\n");
> + omap_dss_put_device(dssdev);
> goto err;
> }
> }
> @@ -338,8 +345,12 @@ void display_uninit_sysfs(struct platform_device *pdev)
> struct omap_dss_device *dssdev = NULL;
>
> for_each_dss_dev(dssdev) {
> - sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
> - sysfs_remove_files(&dssdev->dev->kobj,
> - display_sysfs_attrs);
> + if (kobject_name(&dssdev->kobj) == NULL)
> + continue;
> +
> + kobject_del(&dssdev->kobj);
> + kobject_put(&dssdev->kobj);
> +
> + memset(&dssdev->kobj, 0, sizeof(dssdev->kobj));
> }
> }
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index 60de61fea8e3..c8ed15daad02 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -689,6 +689,7 @@ struct omapdss_dsi_ops {
> };
>
> struct omap_dss_device {
> + struct kobject kobj;
> struct device *dev;
>
> struct module *owner;

Tested-by: NeilBrown <[email protected]>

Before the patch:

# ls -l /sys/devices/platform/omapdss/display0
lrwxrwxrwx 1 root root 0 Feb 8 12:57 /sys/devices/platform/omapdss/display0 -> ../spi_lcd/spi_master/spi32766/spi32766.0

After the patch:

# ls -l /sys/devices/platform/omapdss/display0
total 0
-r--r--r-- 1 root root 4096 Feb 8 13:37 display_name
-rw-r--r-- 1 root root 4096 Feb 8 13:37 enabled
-rw-r--r-- 1 root root 4096 Feb 8 13:37 mirror
-r--r--r-- 1 root root 4096 Feb 8 13:37 name
-rw-r--r-- 1 root root 4096 Feb 8 13:37 rotate
-rw-r--r-- 1 root root 4096 Feb 8 13:37 tear_elim
-rw-r--r-- 1 root root 4096 Feb 8 13:37 timings
-rw-r--r-- 1 root root 4096 Feb 8 13:37 wss

So as you say it creates a directory just for the display0 device, and that
has the 'name' that we want.

This works for me, and it seems to me to be a better fit to the general
structure of /sys/devices - symlinks within /sys/devices are a substantial
minority, other than 'subsystem', 'device', 'driver' and 'bdi' which have
very generic meanings.

I guess I'm a little surprised that there doesn't seem to be any linkage from
the display0 to the spi device. Maybe that isn't important.

Thanks a lot!

NeilBrown

Attachments:

2015-02-25 09:32:30

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

On 25/02/15 11:20, NeilBrown wrote:

> Tested-by: NeilBrown <[email protected]>
>
> Before the patch:
>
> # ls -l /sys/devices/platform/omapdss/display0
> lrwxrwxrwx 1 root root 0 Feb 8 12:57 /sys/devices/platform/omapdss/display0 -> ../spi_lcd/spi_master/spi32766/spi32766.0
>
> After the patch:
>
> # ls -l /sys/devices/platform/omapdss/display0
> total 0
> -r--r--r-- 1 root root 4096 Feb 8 13:37 display_name
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 enabled
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 mirror
> -r--r--r-- 1 root root 4096 Feb 8 13:37 name
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 rotate
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 tear_elim
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 timings
> -rw-r--r-- 1 root root 4096 Feb 8 13:37 wss
>
>
> So as you say it creates a directory just for the display0 device, and that
> has the 'name' that we want.

I think this was something similar than how the sysfs files were set up
originally for omapdss. If I remember right, we didn't have proper
devices for the displays then. Things have evolved quite a bit since then.

There's a small chance of this patch breaking things, of course... If
someone accessed those display sysfs files via the display device
(../spi_lcd/spi_master/spi32766/spi32766.0), the files are now gone.

But I think (hope...) they are always accessed via the omapdss's
displayX directories.

> This works for me, and it seems to me to be a better fit to the general
> structure of /sys/devices - symlinks within /sys/devices are a substantial
> minority, other than 'subsystem', 'device', 'driver' and 'bdi' which have
> very generic meanings.
>
> I guess I'm a little surprised that there doesn't seem to be any linkage from
> the display0 to the spi device. Maybe that isn't important.

Yep, I don't think so. In any case, all this is to be deprecated, and as
soon as omapdrm driver works reliably that should be the driver to use.

So of course we need to keep omapfb working for the years to come, but
I'd rather not add any new sysfs files for a soon deprecated driver.

Tomi

Attachments:

2015-02-25 22:33:36

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

On Wed, 25 Feb 2015 11:32:18 +0200 Tomi Valkeinen <[email protected]>
wrote:

> Yep, I don't think so. In any case, all this is to be deprecated, and as
> soon as omapdrm driver works reliably that should be the driver to use.

How close is that? Is it worth experimenting yet? Is there an xorg driver
available? Maybe a wiki page telling me how to set it up?

http://processors.wiki.ti.com/index.php/Linux_Core_DSS_User%27s_Guide

seems to have some useful suggestions, but no mention of Xorg...

Thanks,
NeilBrown

>
> So of course we need to keep omapfb working for the years to come, but
> I'd rather not add any new sysfs files for a soon deprecated driver.
>
> Tomi
>
>

Attachments: