Hello,
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
> >> + *
> >
> > s/MEDIA_ENTITY_TYPE_NONE/media_entity_type/
> >
> > (it seems you didn't test producing the docbook, otherwise you would
> > have seen this error - Please always generate the docbook when the
> > patch contains kernel-doc markups)
Oops, sorry. I'll fix that.
> > 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.
> > 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.
> >> + * @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.
> > 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."
> >> */
> >> static inline bool is_media_entity_v4l2_io(struct media_entity *entity)
> >> {
> >> - if (!entity)
> >> - return false;
> >> -
> >> - switch (entity->function) {
> >> - case MEDIA_ENT_F_IO_V4L:
> >> - case MEDIA_ENT_F_IO_VBI:
> >> - case MEDIA_ENT_F_IO_SWRADIO:
> >> - return true;
> >> - default:
> >> - return false;
> >> - }
> >> + return entity && entity->type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> >> }
[snip]
I'm happy to see that the comments are mostly about details and that the
overall idea doesn't raise any strong opposition :-) Let's try to sort the
last issues out and get this merged.
By the way, this patch is a prerequisite to fix the warnings printed by the MC
core for the vsp1 driver since v4.5-rc1. Should it thus be merged as a fix for
v4.5 ?
--
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
