| Message ID | 20190117111918.31759-1-tomi.valkeinen@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm: support gpu aliases defined in DT data | expand |
On Thu, Jan 17, 2019 at 01:19:18PM +0200, Tomi Valkeinen wrote: > The DRM device minor numbers are allocated according to the registration > order. This causes confusion in cases where the registration order can > change, or when, say, a modesetting capable device is preferred to be > card0, and a rendering device is preferred to be card1. > > This patch adds similar functionality that is used in some other > subsystems, where device minor numbers can be defined in DT bindings' > aliases node. > > For example, this sets the DRM device minor number to 1 for the 'dss' > device. > > aliases { > gpu1 = &dss; > }; > > The logic on how to pick the minor number is: > > - if there's a DT gpu alias for the device, use that > - else, if there are any gpu aliases, pick a minor number that is higher > than any of the aliases. > - else, use the full range of possible numbers > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> I guess we'd need a binding document for this? IIRC, Rob was against introducing new aliases. Rob? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Jan 17, 2019 at 01:19:18PM +0200, Tomi Valkeinen wrote: > The DRM device minor numbers are allocated according to the registration > order. This causes confusion in cases where the registration order can > change, or when, say, a modesetting capable device is preferred to be > card0, and a rendering device is preferred to be card1. > > This patch adds similar functionality that is used in some other > subsystems, where device minor numbers can be defined in DT bindings' > aliases node. What other subsystem? I thought that minor numbers shouldn't be made uapi, and that udev or similar is supposed to give you stable names ... Is that not the case on SoC? If we go with this I think we need a few acks from soc-tree people that this is a reasonable idea which should be copied around. -Daniel > For example, this sets the DRM device minor number to 1 for the 'dss' > device. > > aliases { > gpu1 = &dss; > }; > > The logic on how to pick the minor number is: > > - if there's a DT gpu alias for the device, use that > - else, if there are any gpu aliases, pick a minor number that is higher > than any of the aliases. > - else, use the full range of possible numbers > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/drm_drv.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index a5fe91b8c3c9..f536a2760293 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -110,6 +110,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > struct drm_minor *minor; > unsigned long flags; > int r; > + int min_id, max_id; > + bool of_id_found = false; > > minor = kzalloc(sizeof(*minor), GFP_KERNEL); > if (!minor) > @@ -118,12 +120,37 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > minor->type = type; > minor->dev = dev; > > + min_id = 64 * type; > + max_id = 64 * (type + 1); > + > + if (dev->dev && dev->dev->of_node) { > + int id; > + > + id = of_alias_get_id(dev->dev->of_node, "gpu"); > + > + if (id >= 0) { > + min_id = 64 * type + id; > + max_id = 64 * type + id + 1; > + > + of_id_found = true; > + } > + } > + > + if (!of_id_found) { > + int id; > + > + id = of_alias_get_highest_id("gpu"); > + > + if (id >= 0) > + min_id = id + 1; > + } > + > idr_preload(GFP_KERNEL); > spin_lock_irqsave(&drm_minor_lock, flags); > r = idr_alloc(&drm_minors_idr, > NULL, > - 64 * type, > - 64 * (type + 1), > + min_id, > + max_id, > GFP_NOWAIT); > spin_unlock_irqrestore(&drm_minor_lock, flags); > idr_preload_end(); > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Jan 17, 2019 at 2:04 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > On 17/01/19 14:33, Daniel Vetter wrote: > > On Thu, Jan 17, 2019 at 01:19:18PM +0200, Tomi Valkeinen wrote: > >> The DRM device minor numbers are allocated according to the registration > >> order. This causes confusion in cases where the registration order can > >> change, or when, say, a modesetting capable device is preferred to be > >> card0, and a rendering device is preferred to be card1. > >> > >> This patch adds similar functionality that is used in some other > >> subsystems, where device minor numbers can be defined in DT bindings' > >> aliases node. > > > > What other subsystem? I thought that minor numbers shouldn't be made uapi, > > and that udev or similar is supposed to give you stable names ... Is that > > not the case on SoC? > > I think at least i2c, spi and uart use DT aliases. Commits/code implementing would be best. > I also have my doubts about this, but thought to post this to get some > comments, as it does make life quite a bit easier =). Yeah I think "soc without udev" seems reasonable assumption, I just think this is something the overall soc community should agree on as a good thing to do. I guess since the of stuff you're using is generic that's all happened already, so should amount to gathering a pile of acks and then merging it. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On 17/01/19 15:26, Daniel Vetter wrote: > On Thu, Jan 17, 2019 at 2:04 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >> >> On 17/01/19 14:33, Daniel Vetter wrote: >>> On Thu, Jan 17, 2019 at 01:19:18PM +0200, Tomi Valkeinen wrote: >>>> The DRM device minor numbers are allocated according to the registration >>>> order. This causes confusion in cases where the registration order can >>>> change, or when, say, a modesetting capable device is preferred to be >>>> card0, and a rendering device is preferred to be card1. >>>> >>>> This patch adds similar functionality that is used in some other >>>> subsystems, where device minor numbers can be defined in DT bindings' >>>> aliases node. >>> >>> What other subsystem? I thought that minor numbers shouldn't be made uapi, >>> and that udev or similar is supposed to give you stable names ... Is that >>> not the case on SoC? >> >> I think at least i2c, spi and uart use DT aliases. > > Commits/code implementing would be best. For i2c: ee5c27440cc24d62ec463cce4c000bb32c5692c7 ("i2c: core: Pick i2c bus number from dt alias if present") 03bde7c31a360f814ca42101d60563b1b803dca1 ("i2c: busses with dynamic ids should start after fixed ids for DT") >> I also have my doubts about this, but thought to post this to get some >> comments, as it does make life quite a bit easier =). > > Yeah I think "soc without udev" seems reasonable assumption, I just I'm not that familiar with udev rules. I wonder how it would work... The rule would need to keep all dynamic cards out from the group of reserved card names, and also handle render nodes. All this is probably possible, with a quick study I could find out how to implement something like that. > think this is something the overall soc community should agree on as a > good thing to do. I guess since the of stuff you're using is generic > that's all happened already, so should amount to gathering a pile of > acks and then merging it. Ok. I'm not sure who the "SoC people" would be, but I added some more OF people here. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tomi Valkeinen <tomi.valkeinen@ti.com> writes: > The DRM device minor numbers are allocated according to the registration > order. This causes confusion in cases where the registration order can > change, or when, say, a modesetting capable device is preferred to be > card0, and a rendering device is preferred to be card1. > > This patch adds similar functionality that is used in some other > subsystems, where device minor numbers can be defined in DT bindings' > aliases node. > > For example, this sets the DRM device minor number to 1 for the 'dss' > device. > > aliases { > gpu1 = &dss; > }; This would be really nice to have. Given that there's plenty of userspace code that opens the first available DRM node (whether or not that is Correct Behavior), making their behavior not dependent on the random probe order is a good thing.
On Thu, Jan 17, 2019 at 7:26 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Jan 17, 2019 at 2:04 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > > > On 17/01/19 14:33, Daniel Vetter wrote: > > > On Thu, Jan 17, 2019 at 01:19:18PM +0200, Tomi Valkeinen wrote: > > >> The DRM device minor numbers are allocated according to the registration > > >> order. This causes confusion in cases where the registration order can > > >> change, or when, say, a modesetting capable device is preferred to be > > >> card0, and a rendering device is preferred to be card1. > > >> > > >> This patch adds similar functionality that is used in some other > > >> subsystems, where device minor numbers can be defined in DT bindings' > > >> aliases node. > > > > > > What other subsystem? I thought that minor numbers shouldn't be made uapi, > > > and that udev or similar is supposed to give you stable names ... Is that > > > not the case on SoC? You thought correctly. The problem is lots of people like their stable names. > > I think at least i2c, spi and uart use DT aliases. Well, yes. UARTs were largely to not break 'console=ttySx' and/or getty's on a ttySx when moving to DT. That is largely a solved problem now (though everyone still puts them in). SPI and I2C sneaked in I guess. Those are really only for folks twiddling with SPI and I2C directly from userspace. There's been discussions in the past how not use aliases, but no one has cared enough to implement. > Commits/code implementing would be best. > > > I also have my doubts about this, but thought to post this to get some > > comments, as it does make life quite a bit easier =). > > Yeah I think "soc without udev" seems reasonable assumption, I just > think this is something the overall soc community should agree on as a > good thing to do. I guess since the of stuff you're using is generic > that's all happened already, so should amount to gathering a pile of > acks and then merging it. Mesa/libdrm already has lots of code to open the correct devices and not care about minor numbers. What's the problem here? Rob
On 18/01/19 00:04, Rob Herring wrote: > Mesa/libdrm already has lots of code to open the correct devices and > not care about minor numbers. What's the problem here? Well, maybe the problem is that I don't know how to do this =). So, if we have multiple DRM devices, how does one manage those? Iterating over them and looking for kms-capable ones is easy enough. But if there are multiple kms-capable ones, how should the userspace choose between those? I see that udev creates /dev/dri/by-path/ links to the cards, should the userspace use those to have a persistent link to the card? E.g. first time the app is ran, it'll collect all the kms-capable cards, and store the by-path names somewhere, and in subsequent runs it will instead use the by-path names to keep the order the same. If I have a product with two display controllers, and one of them has the main display connected, how does my custom app know which card to use? Hardcoding the by-path in the app? If udev/systemd is not available, how does the userspace do this? I tried to see if one can get enough information from the card device with ioctls to figure these things out, but I didn't figure this out. Should the drmGetBusid() return something for platform devices too? Maybe I've got something missing from the display driver, and drmGetBusid() should return something similar as what is in the by-path name. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tomi, On Fri, Jan 18, 2019 at 10:29:33AM +0200, Tomi Valkeinen wrote: > On 18/01/19 00:04, Rob Herring wrote: > > > Mesa/libdrm already has lots of code to open the correct devices and > > not care about minor numbers. What's the problem here? > > Well, maybe the problem is that I don't know how to do this =). > > So, if we have multiple DRM devices, how does one manage those? > Iterating over them and looking for kms-capable ones is easy enough. But > if there are multiple kms-capable ones, how should the userspace choose > between those? > > I see that udev creates /dev/dri/by-path/ links to the cards, should the > userspace use those to have a persistent link to the card? E.g. first > time the app is ran, it'll collect all the kms-capable cards, and store > the by-path names somewhere, and in subsequent runs it will instead use > the by-path names to keep the order the same. > > If I have a product with two display controllers, and one of them has > the main display connected, how does my custom app know which card to > use? Hardcoding the by-path in the app? I think it depends on how you define "main display". We have a similar problem with cameras where a system such as a phone or tablet with a front camera and a back camera has no good way to convey the role of each camera all the way to applications. The information belongs to system firmware in my opinion (and thus DT in this case), but probably not in the form of aliases. It would make sense to tag connector and panels with some kind of role (such as main display, various kind of auxiliary displays, ...), parse that information in drivers, and report it to userspace (ideally through standard APIs). > If udev/systemd is not available, how does the userspace do this? I > tried to see if one can get enough information from the card device with > ioctls to figure these things out, but I didn't figure this out. Should > the drmGetBusid() return something for platform devices too? Maybe I've > got something missing from the display driver, and drmGetBusid() should > return something similar as what is in the by-path name. -- Regards, Laurent Pinchart
On 06/03/2019 03:41, Laurent Pinchart wrote: > Hi Tomi, > > On Fri, Jan 18, 2019 at 10:29:33AM +0200, Tomi Valkeinen wrote: >> On 18/01/19 00:04, Rob Herring wrote: >> >>> Mesa/libdrm already has lots of code to open the correct devices and >>> not care about minor numbers. What's the problem here? >> >> Well, maybe the problem is that I don't know how to do this =). >> >> So, if we have multiple DRM devices, how does one manage those? >> Iterating over them and looking for kms-capable ones is easy enough. But >> if there are multiple kms-capable ones, how should the userspace choose >> between those? >> >> I see that udev creates /dev/dri/by-path/ links to the cards, should the >> userspace use those to have a persistent link to the card? E.g. first >> time the app is ran, it'll collect all the kms-capable cards, and store >> the by-path names somewhere, and in subsequent runs it will instead use >> the by-path names to keep the order the same. >> >> If I have a product with two display controllers, and one of them has >> the main display connected, how does my custom app know which card to >> use? Hardcoding the by-path in the app? > > I think it depends on how you define "main display". We have a similar > problem with cameras where a system such as a phone or tablet with a > front camera and a back camera has no good way to convey the role of > each camera all the way to applications. The information belongs to > system firmware in my opinion (and thus DT in this case), but probably > not in the form of aliases. It would make sense to tag connector and > panels with some kind of role (such as main display, various kind of > auxiliary displays, ...), parse that information in drivers, and report > it to userspace (ideally through standard APIs). I agree. I did have "label" in the omapdrm-style panels and connectors, but that's not used anywhere. But this patch is not really about what you describe above, The target was really just to have card0 to be a modesetting card, making simple apps that use card0 for display working. So, obviously a work-around or even a hack, but solves the issue without changing the userspace. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 17/04/2019 20:42, Emil Velikov wrote: > Have you looked at the libdrm drmDevice2 (yes use the second version) API? > It provides various information about the different devices, yet if > it's missing anything do send us a patch ;-) No, I didn't notice that. At least with a quick look, looks good. I'll give it a try. Thanks! Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index a5fe91b8c3c9..f536a2760293 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -110,6 +110,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) struct drm_minor *minor; unsigned long flags; int r; + int min_id, max_id; + bool of_id_found = false; minor = kzalloc(sizeof(*minor), GFP_KERNEL); if (!minor) @@ -118,12 +120,37 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) minor->type = type; minor->dev = dev; + min_id = 64 * type; + max_id = 64 * (type + 1); + + if (dev->dev && dev->dev->of_node) { + int id; + + id = of_alias_get_id(dev->dev->of_node, "gpu"); + + if (id >= 0) { + min_id = 64 * type + id; + max_id = 64 * type + id + 1; + + of_id_found = true; + } + } + + if (!of_id_found) { + int id; + + id = of_alias_get_highest_id("gpu"); + + if (id >= 0) + min_id = id + 1; + } + idr_preload(GFP_KERNEL); spin_lock_irqsave(&drm_minor_lock, flags); r = idr_alloc(&drm_minors_idr, NULL, - 64 * type, - 64 * (type + 1), + min_id, + max_id, GFP_NOWAIT); spin_unlock_irqrestore(&drm_minor_lock, flags); idr_preload_end();
The DRM device minor numbers are allocated according to the registration order. This causes confusion in cases where the registration order can change, or when, say, a modesetting capable device is preferred to be card0, and a rendering device is preferred to be card1. This patch adds similar functionality that is used in some other subsystems, where device minor numbers can be defined in DT bindings' aliases node. For example, this sets the DRM device minor number to 1 for the 'dss' device. aliases { gpu1 = &dss; }; The logic on how to pick the minor number is: - if there's a DT gpu alias for the device, use that - else, if there are any gpu aliases, pick a minor number that is higher than any of the aliases. - else, use the full range of possible numbers Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/drm_drv.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki