| Message ID | 20180803143626.48191-1-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| Headers | show |
| Series | Media Controller Properties | expand |
On 08/03/2018 04:36 PM, Hans Verkuil wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > This RFC patch series implements properties for the media controller. > > This is not finished, but I wanted to post this so people can discuss > this further. > > No documentation yet (too early for that). > > An updated v4l2-ctl and v4l2-compliance that can report properties > is available here: > > https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props > > There is one main missing piece: currently the properties are effectively > laid out in random order. My plan is to change that so they are grouped > by object type and object owner. So first all properties for each entity, > then for each pad, etc. I started to work on that, but it's a bit more > work than expected and I wanted to post this before the weekend. > > While it is possible to have nested properties, this is not currently > implemented. Only properties for entities and pads are supported in this > code, but that's easy to extend to interfaces and links. > > I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest > option by renaming the old ioctl and adding a new one with property support. > > I think this needs to change (at the very least the old and new should > share the same ioctl NR), but that's something for the future. > > Currently I support u64, s64 and const char * property types. But it > can be anything including binary data if needed. No array support (as we > have for controls), but there are enough reserved fields in media_v2_prop > to add this if needed. > > I added properties for entities and pads to vimc, so I could test this. I forgot to mention that there are known issues with the initialization of the entity props list (it happens in two places, I need to look into that) and that mdev is expected to be valid when adding properties, but I don't think that is necessarily the case. So just be aware of that. Regards, Hans > > Regards, > > Hans > > Hans Verkuil (3): > uapi/linux/media.h: add property support > media: add support for properties > vimc: add test properties > > drivers/media/media-device.c | 98 +++++++++++- > drivers/media/media-entity.c | 65 ++++++++ > drivers/media/platform/vimc/vimc-common.c | 18 +++ > drivers/media/platform/vimc/vimc-core.c | 6 +- > include/media/media-device.h | 6 + > include/media/media-entity.h | 172 ++++++++++++++++++++++ > include/uapi/linux/media.h | 62 +++++++- > 7 files changed, 420 insertions(+), 7 deletions(-) >
Em Fri, 3 Aug 2018 17:03:20 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 08/03/2018 04:36 PM, Hans Verkuil wrote: > > From: Hans Verkuil <hans.verkuil@cisco.com> > > > > This RFC patch series implements properties for the media controller. > > > > This is not finished, but I wanted to post this so people can discuss > > this further. > > > > No documentation yet (too early for that). That's my first complain :-D Anyway, just 3 patches should be good enough to review without docs (famous last words). > > > > An updated v4l2-ctl and v4l2-compliance that can report properties > > is available here: > > > > https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props > > > > There is one main missing piece: currently the properties are effectively > > laid out in random order. My plan is to change that so they are grouped > > by object type and object owner. So first all properties for each entity, > > then for each pad, etc. I started to work on that, but it's a bit more > > work than expected and I wanted to post this before the weekend. IMO, the best is to use some tree struct. The Kernel has some types that could help setting it, although we never used on media. See, for example Documentation/rbtree.txt for one such type. > > > > While it is possible to have nested properties, this is not currently > > implemented. Only properties for entities and pads are supported in this > > code, but that's easy to extend to interfaces and links. With a tree-based data structure, this would likely come for free. > > > > I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest > > option by renaming the old ioctl and adding a new one with property support. Why? No need for that at the public header. Just add the needed fields at the end of the code and check for struct size at the ioctl handler. It could make sense to have the old struct inside media-device.c, just to allow using sizeof() there. > > > > I think this needs to change (at the very least the old and new should > > share the same ioctl NR), but that's something for the future. You don't need that. Just be sure to mask the size when checking for the ioctl, and then the code will check for the struct size, comparing if it matches _v1 and the latest version (with should keep the same name). > > > > Currently I support u64, s64 and const char * property types. But it > > can be anything including binary data if needed. No array support (as we > > have for controls), but there are enough reserved fields in media_v2_prop > > to add this if needed. That's u64/s64/char* are probably enough for now. > > > > I added properties for entities and pads to vimc, so I could test this. > > I forgot to mention that there are known issues with the initialization > of the entity props list (it happens in two places, I need to look into > that) and that mdev is expected to be valid when adding properties, but > I don't think that is necessarily the case. > > So just be aware of that. > > Regards, > > Hans > > > > > Regards, > > > > Hans > > > > Hans Verkuil (3): > > uapi/linux/media.h: add property support > > media: add support for properties > > vimc: add test properties > > > > drivers/media/media-device.c | 98 +++++++++++- > > drivers/media/media-entity.c | 65 ++++++++ > > drivers/media/platform/vimc/vimc-common.c | 18 +++ > > drivers/media/platform/vimc/vimc-core.c | 6 +- > > include/media/media-device.h | 6 + > > include/media/media-entity.h | 172 ++++++++++++++++++++++ > > include/uapi/linux/media.h | 62 +++++++- > > 7 files changed, 420 insertions(+), 7 deletions(-) > > > Thanks, Mauro
On 08/03/2018 05:23 PM, Mauro Carvalho Chehab wrote: > Em Fri, 3 Aug 2018 17:03:20 +0200 <snip> >>> I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest >>> option by renaming the old ioctl and adding a new one with property support. > > Why? No need for that at the public header. Just add the needed fields at the > end of the code and check for struct size at the ioctl handler. > > It could make sense to have the old struct inside media-device.c, just > to allow using sizeof() there. Sorry, you need the old struct. The application may be newer than the kernel, so if the new topology struct (with props support) doesn't work, then it has to fall back to the old ioctl. So applications will need to know the old size. That said, it would be sufficient in this case to just export the old ioctl define since the old struct layout is identical to the new (except of course for the new fields added to the end). E.g. add just this to media.h: /* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */ #define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04 This brings me to another related question: I can easily support both the old and new G_TOPOLOGY ioctls in media-device.c. But what should I do if we add still more fields to the topology struct in the future and so we might be called by a newer application with a G_TOPOLOGY ioctl that has a size larger than we have now. We can either reject this (that's what we do today in fact, hence the need for the TOPOLOGY_OLD), or we can just accept it and zero the unknown fields at the end of the larger struct. I think we need to do the latter, otherwise we will have to keep adding new ioctl variants whenever we add a field. Alternatively, we can just add a pile of reserved fields to struct media_v2_topology which should last us for many years based on past experience with reserved fields. Regards, Hans
Hi Hans, On Fri, Aug 3, 2018 at 11:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > From: Hans Verkuil <hans.verkuil@cisco.com> > > This RFC patch series implements properties for the media controller. > > This is not finished, but I wanted to post this so people can discuss > this further. > > No documentation yet (too early for that). > > An updated v4l2-ctl and v4l2-compliance that can report properties > is available here: > > https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props > > There is one main missing piece: currently the properties are effectively > laid out in random order. My plan is to change that so they are grouped > by object type and object owner. So first all properties for each entity, > then for each pad, etc. I started to work on that, but it's a bit more > work than expected and I wanted to post this before the weekend. > > While it is possible to have nested properties, this is not currently > implemented. Only properties for entities and pads are supported in this > code, but that's easy to extend to interfaces and links. > > I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest > option by renaming the old ioctl and adding a new one with property support. > > I think this needs to change (at the very least the old and new should > share the same ioctl NR), but that's something for the future. > > Currently I support u64, s64 and const char * property types. But it > can be anything including binary data if needed. No array support (as we > have for controls), but there are enough reserved fields in media_v2_prop > to add this if needed. > > I added properties for entities and pads to vimc, so I could test this. I think I'm missing the background and the description doesn't mention it either. What's the use case for those and why not controls? Best regards, Tomasz
On 11/14/2018 09:09 AM, Tomasz Figa wrote: > Hi Hans, > > On Fri, Aug 3, 2018 at 11:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> This RFC patch series implements properties for the media controller. >> >> This is not finished, but I wanted to post this so people can discuss >> this further. >> >> No documentation yet (too early for that). >> >> An updated v4l2-ctl and v4l2-compliance that can report properties >> is available here: >> >> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props >> >> There is one main missing piece: currently the properties are effectively >> laid out in random order. My plan is to change that so they are grouped >> by object type and object owner. So first all properties for each entity, >> then for each pad, etc. I started to work on that, but it's a bit more >> work than expected and I wanted to post this before the weekend. >> >> While it is possible to have nested properties, this is not currently >> implemented. Only properties for entities and pads are supported in this >> code, but that's easy to extend to interfaces and links. >> >> I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest >> option by renaming the old ioctl and adding a new one with property support. >> >> I think this needs to change (at the very least the old and new should >> share the same ioctl NR), but that's something for the future. >> >> Currently I support u64, s64 and const char * property types. But it >> can be anything including binary data if needed. No array support (as we >> have for controls), but there are enough reserved fields in media_v2_prop >> to add this if needed. >> >> I added properties for entities and pads to vimc, so I could test this. > > I think I'm missing the background and the description doesn't mention > it either. What's the use case for those and why not controls? Properties provide additional information about graph objects: main initial use-case is to give camera information: back/front, upside down, and whatever else Sakari can come up with :-) Also we want to use this to describe all the functions that an entity supports (right now you can describe only one function, but there are multi-function devices out there). Other use-cases are for connectors (but these are not yet modeled by the MC): name and/or color on the backplane. I forgot what the pad properties where needed for, I would have to dig. It is something Mauro wanted to use for either DVB or TV capture drivers to provide additional information about the type of data flowing through a pad. This is all information that most likely will come from the device tree. All this is read-only information. And yes, these properties will all have to be documented since they are part of the ABI. Regards, Hans
From: Hans Verkuil <hans.verkuil@cisco.com> This RFC patch series implements properties for the media controller. This is not finished, but I wanted to post this so people can discuss this further. No documentation yet (too early for that). An updated v4l2-ctl and v4l2-compliance that can report properties is available here: https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props There is one main missing piece: currently the properties are effectively laid out in random order. My plan is to change that so they are grouped by object type and object owner. So first all properties for each entity, then for each pad, etc. I started to work on that, but it's a bit more work than expected and I wanted to post this before the weekend. While it is possible to have nested properties, this is not currently implemented. Only properties for entities and pads are supported in this code, but that's easy to extend to interfaces and links. I'm not sure about the G_TOPOLOGY ioctl handling: I went with the quickest option by renaming the old ioctl and adding a new one with property support. I think this needs to change (at the very least the old and new should share the same ioctl NR), but that's something for the future. Currently I support u64, s64 and const char * property types. But it can be anything including binary data if needed. No array support (as we have for controls), but there are enough reserved fields in media_v2_prop to add this if needed. I added properties for entities and pads to vimc, so I could test this. Regards, Hans Hans Verkuil (3): uapi/linux/media.h: add property support media: add support for properties vimc: add test properties drivers/media/media-device.c | 98 +++++++++++- drivers/media/media-entity.c | 65 ++++++++ drivers/media/platform/vimc/vimc-common.c | 18 +++ drivers/media/platform/vimc/vimc-core.c | 6 +- include/media/media-device.h | 6 + include/media/media-entity.h | 172 ++++++++++++++++++++++ include/uapi/linux/media.h | 62 +++++++- 7 files changed, 420 insertions(+), 7 deletions(-)