Re: [PATCH] media: Add type field to struct media_entity

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ideasonboard.com>
> >> ---
> >> 
> >>  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