Hi Mauro,
On Friday 26 February 2016 10:21:41 Mauro Carvalho Chehab wrote:
> Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart escreveu:
> > On Monday 22 February 2016 23:20:58 Sakari Ailus wrote:
> >> On Mon, Feb 22, 2016 at 06:46:01AM -0300, Mauro Carvalho Chehab wrote:
> >>> Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart escreveu:
> >>>> Code that processes media entities can require knowledge of the
> >>>> structure type that embeds a particular media entity instance in
> >>>> order to use the API provided by that structure. This needs is shown
> >>>> by the presence of the is_media_entity_v4l2_io and
> >>>> is_media_entity_v4l2_subdev functions.
> >>>>
> >>>> The implementation of those two functions relies on the entity
> >>>> function field, which is both a wrong and an inefficient design,
> >>>> without even
> >>
> >> I wouldn't necessarily say "wrong", but it is risky. A device's function
> >> not only defines the interface it offers but also which struct is
> >> considered to contain the media entity. Having a wrong value in the
> >> function field may thus lead memory corruption and / or system crash.
> >>
> >>>> mentioning the maintenance issue involved in updating the functions
> >>>> every time a new entity function is added. Fix this by adding add a
> >>>> type field to the media entity structure to carry the information.
> >>>>
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>
> >>>> drivers/media/v4l2-core/v4l2-dev.c | 1 +
> >>>> drivers/media/v4l2-core/v4l2-subdev.c | 1 +
> >>>> include/media/media-entity.h | 65 ++++++++++-----------------
> >>>> 3 files changed, 30 insertions(+), 37 deletions(-)
> >
> > [snip]
> >
> >>>> diff --git a/include/media/media-entity.h
> >>>> b/include/media/media-entity.h
> >>>> index fe485d367985..2be38483f3a4 100644
> >>>> --- a/include/media/media-entity.h
> >>>> +++ b/include/media/media-entity.h
> >>>> @@ -187,10 +187,27 @@ struct media_entity_operations {
> >>>> };
> >>>>
> >>>> /**
> >>>> + * enum MEDIA_ENTITY_TYPE_NONE - Media entity type
> >>>> + *
[snip]
> >>> I don't like the idea of calling it as "type", as this is confusing,
> >>> specially since we used to call entity.type for what we now call
> >>> function.
> >>
> >> What that field essentially defines is which struct embeds the media
> >> entity. (Well, there's some cleanups to be done there, as we have extra
> >> entity for V4L2 subdevices, but that's another story.)
> >>
> >> The old type field had that information, plus the "function" of the
> >> entity.
> >>
> >> I think "type" isn't a bad name for this field, as what we would really
> >> need is inheritance. It refers to the object type. What would you think
> >> of "class"?
> >
> > I'd prefer type as class has other meanings in the kernel, but I can live
> > with it. Mauro, I agree with Sakari here, what the field contains is
> > really the object type in an object-oriented programming context.
>
> Well, as we could have entities not embedded on some other object, this
> is actually not an object type, even on OO programming.
I don't agree with that. From an OOP point of view, object instances have a
type. A media_entity instance that is not embedded in another structure has
the media_entity type.
> What we're actually representing here is a graph object class.
No we're not. It's not about graph objects. It's about knowing the type of a
particular media_entity instance in order to safely cast it to the proper
object for its type. It's the same concept as run-time type identification
(see https://en.wikipedia.org/wiki/Run-time_type_information) in C++ for
instance.
> The problem is that "type" is a very generic term, and, as we used it before
> with some other meaning, so I prefer to call it as something else.
>
> I'm ok with any other name, although I agree that Kernel uses "class" for
> other things. Maybe gr_class or obj_class?
I could go for entity_type, but given that the field is located in a struct
media_entity, the entity_ prefix is really redundant.
> >>> What we're actually wanting to represent is the Linux kABI group where
> >>> the entity belongs. So, maybe we could call it as
> >>> media_entity_kabi_type, instead.
> >>>
> >>>> + * @MEDIA_ENTITY_TYPE_NONE:
> >>>> + * The entity isn't embedded in a standard structure.
> >>>
> >>> I also don't like having a NONE here. All objects belong to some
> >>> kABI type, but not all subsystems need to use this field
> >>> (so far, DVB doesn't need nor ALSA).
> >>>
> >>> So, I would either call it as DEFAULT or UNDEFINED.
> >>
> >> I prefer UNDEFINED from the two. There really is no interface in that
> >> case, and we don't have a "default" interface either.
> >
> > I prefer UNDEFINED too, I'll fix that.
>
> OK.
>
> >>>> + * @MEDIA_ENTITY_TYPE_VIDEO_DEVICE:
> >>>> + * The media entity is embedded in a struct video_device.
> >>>> + * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> >>>> + * The media entity is embedded in a struct v4l2_subdev.
> >>>> + */
> >>>> +enum media_entity_type {
> >>>> + MEDIA_ENTITY_TYPE_NONE,
> >>>> + MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> >>>> + MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> >>>> +};
> >
> > [snip]
> >
> >>>> @@ -328,56 +346,29 @@ static inline u32 media_gobj_gen_id(enum
> >>>> media_gobj_type type, u64 local_id)
> >>>> }
> >>>>
> >>>> /**
> >>>> - * is_media_entity_v4l2_io() - identify if the entity main function
> >>>> - * is a V4L2 I/O
> >>>> - *
> >>>> + * is_media_entity_v4l2_io() - Check if the entity implements the
> >>>> video_device
> >>>> + * API
> >>>> * @entity: pointer to entity
> >>>> *
> >>>> - * Return: true if the entity main function is one of the V4L2 I/O
> >>>> types
> >>>> - * (video, VBI or SDR radio); false otherwise.
> >>>> + * Return: true if the entity implement the video_device API (is
> >>>> directly
> >>>> + * embedded in a struct video_device instance) or false otherwise.
> >>>
> >>> s/implement/implements/
> >
> > Will fix.
> >
> >>> Yet, I don't think the above comment is ok. First of all, video_device
> >>> is a kABI. We're nowadays calling the kernel APIs as kABI, and the
> >>> userspace ones as uAPI.
> >>
> >> Are the exact definitions of the two available somewhere? ABI doesn't
> >> matter much in the kernel itself but towards user space both ABI and
> >> API are important...
> >
> > the Kernel ABI is defined as the binary interface exported by the kernel
> > to modules. I don't think that's relevant here.
>
> Maybe we could then call it as kAPI, do distinguish from uAPI.
Or just stop talking about ABI/API completely and just mention that the entity
instance is a video_device or v4l2_subdev instance.
> >>> Also, it doesn't make clear that it would be used also for radio, and
> >>> it is repeating the same thing twice.
> >>>
> >>> So, I would either keep the original comment or change it to:
> >>>
> >>> "Return: true if the entity implements the video_device kABI for
> >>> video,
> >>>
> >>> VBI or SDR radio (e. g. if the entity is embeddded at a struct
> >>> video_device instance) or false otherwise."
> >
> > The original comment isn't correct, as the is_media_entity_v4l2_io()
> > doesn't care about the entity main function. I don't think we need to
> > mention video, VBI or radio, this is really about whether the entity is
> > embedded in a video_device structure. How about
> >
> > "Return: true if the entity is embedded in a struct video_device instance
> > or false otherwise."
>
> Works for me.
OK, I'll change the patch accordingly.
> >>>> */
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
