Em Mon, 16 Apr 2018 21:27:01 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 16 Apr 2018 15:21:16 +0200
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >
> >> From: Hans Verkuil <hansverk@xxxxxxxxx>
> >>
> >> This adds support for 'proper' functions to the existing API.
> >> This information was before only available through the new v2
> >> API, with this change it's available to both.
> >>
> >> Yes, the plan is to allow entities to expose multiple functions for
> >> multi-function devices, but we do not support it anywhere so this
> >> is still vaporware.
> >
> > I'm not convinced about that. I would, instead, just keep it as-is
> > and be sure that applications stop use the legacy calls.
>
> You can't. First of all, since the new API does not provide the pad index
> (fixed in patch 6/9) it is impossible to use the new API with any driver
> that supports SETUP_LINK.
Yeah, unfortunately, the properties API was just an empty promise.
Anyway, as you said, patch 6/9 solves it.
> So any such driver that uses any of the newer
> subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
> is currently not reporting that correctly. A good example is the
> imx driver. But also others if they are combined with such newer subdevs.
As far as I remember, other drivers also return MEDIA_ENT_F_UNKNOWN
(with also maps to MEDIA_ENT_T_DEVNODE_UNKNOWN) even via the new API,
as the developer never cared to fill the entity function, even
producing warnings.
> There is nothing wrong with the old API, except for not reporting the
> proper function value in field 'type' due to historical concerns.
There is. That's why we took about one year developing a new API.
> There is NO WAY we can suddenly prohibit applications from using the old
> API since the new API was never usable. And besides that, we have no method
> of knowing who uses the old API since such applications are likely custom
> for specific hardware.
Nobody is forbidding anything. We're just freezing it, as its
functionality was superseded.
> All that is really missing in the 'old' API (I hate the terms 'old' and
> 'new', they are misleading) is a proper 'function' field. Let's just add it
> and make it consistent with the documentation about entity functions.
It misses interfaces - with is needed to identify what interface controls
what.
>
> >
> >>
> >> Signed-off-by: Hans Verkuil <hansverk@xxxxxxxxx>
> >> ---
> >> drivers/media/media-device.c | 1 +
> >> include/uapi/linux/media.h | 7 ++++++-
> >> 2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 7c3ab37c258a..dca1e5a3e0f9 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
> >> if (ent->name)
> >> strlcpy(entd->name, ent->name, sizeof(entd->name));
> >> entd->type = ent->function;
> >> + entd->function = ent->function;
> >> entd->revision = 0; /* Unused */
> >
> > I got confused here, until I went to the code and noticed that
> > entd->type is actually touched after this.
> >
> > If we're willing to do that, you should add a comment there explaining
> > why we need to pass both type and function to userspace.
>
> True.
>
> Regards,
>
> Hans
>
> >
> >> entd->flags = ent->flags;
> >> entd->group_id = 0; /* Unused */
> >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >> index 86c7dcc9cba3..ac08acffdb65 100644
> >> --- a/include/uapi/linux/media.h
> >> +++ b/include/uapi/linux/media.h
> >> @@ -146,6 +146,10 @@ struct media_device_info {
> >> /* OR with the entity id value to find the next entity */
> >> #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
> >>
> >> +/* Appeared in 4.18.0 */
> >> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
> >> + ((media_version) >= 0x00041200)
> >> +
> >> struct media_entity_desc {
> >> __u32 id;
> >> char name[32];
> >> @@ -155,8 +159,9 @@ struct media_entity_desc {
> >> __u32 group_id;
> >> __u16 pads;
> >> __u16 links;
> >> + __u32 function;
> >>
> >> - __u32 reserved[4];
> >> + __u32 reserved[3];
> >>
> >> union {
> >> /* Node specifications */
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
