| Message ID | 1456105996-20845-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Mon, 22 Feb 2016 03:53:16 +0200 Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> 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 > 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(-) > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index d8e5994cccf1..7e766a92e3d9 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type) > if (!vdev->v4l2_dev->mdev) > return 0; > > + vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > > switch (type) { > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index d63083803144..bb6e79f14bb8 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > sd->host_priv = NULL; > #if defined(CONFIG_MEDIA_CONTROLLER) > sd->entity.name = sd->name; > + sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV; > sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN; > #endif > } > 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) 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 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. > + * @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, > +}; > + > +/** > * struct media_entity - A media entity graph object. > * > * @graph_obj: Embedded structure containing the media object common data. > * @name: Entity name. > + * @type: Type of the object that embeds the media_entity instance. > * @function: Entity main function, as defined in uapi/media.h > * (MEDIA_ENT_F_*) > * @flags: Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*) > @@ -219,6 +236,7 @@ struct media_entity_operations { > struct media_entity { > struct media_gobj graph_obj; /* must be first field in struct */ > const char *name; > + enum media_entity_type type; Same as before: I would call it as kabi_type. > u32 function; > unsigned long flags; > > @@ -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/ 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. 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." > */ > 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; > } > > /** > - * is_media_entity_v4l2_subdev - return true if the entity main function is > - * associated with the V4L2 API subdev usage > - * > + * is_media_entity_v4l2_subdev() - Check if the entity implements the > + * v4l2_subdev API > * @entity: pointer to entity > * > - * This is an ancillary function used by subdev-based V4L2 drivers. > - * It checks if the entity function is one of functions used by a V4L2 subdev, > - * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs. > + * Return: true if the entity implement the v4l2_subdev API (is directly > + * embedded in a struct v4l2_subdev instance) or false otherwise. > */ s/API/kABI/ > static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity) > { > - if (!entity) > - return false; > - > - switch (entity->function) { > - case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN: > - case MEDIA_ENT_F_CAM_SENSOR: > - case MEDIA_ENT_F_FLASH: > - case MEDIA_ENT_F_LENS: > - case MEDIA_ENT_F_ATV_DECODER: > - case MEDIA_ENT_F_TUNER: > - return true; > - > - default: > - return false; > - } > + return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV; > } > > /**
Hi Mauro and Laurent, 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 <laurent.pinchart+renesas@ideasonboard.com> 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(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > > index d8e5994cccf1..7e766a92e3d9 100644 > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > @@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type) > > if (!vdev->v4l2_dev->mdev) > > return 0; > > > > + vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > > > > switch (type) { > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index d63083803144..bb6e79f14bb8 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > > sd->host_priv = NULL; > > #if defined(CONFIG_MEDIA_CONTROLLER) > > sd->entity.name = sd->name; > > + sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV; > > sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN; > > #endif > > } > > 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) > > 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"? > > 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. > > > + * @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, > > +}; > > > + > > +/** > > * struct media_entity - A media entity graph object. > > * > > * @graph_obj: Embedded structure containing the media object common data. > > * @name: Entity name. > > + * @type: Type of the object that embeds the media_entity instance. > > * @function: Entity main function, as defined in uapi/media.h > > * (MEDIA_ENT_F_*) > > * @flags: Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*) > > @@ -219,6 +236,7 @@ struct media_entity_operations { > > struct media_entity { > > struct media_gobj graph_obj; /* must be first field in struct */ > > const char *name; > > + enum media_entity_type type; > > Same as before: I would call it as kabi_type. > > > u32 function; > > unsigned long flags; > > > > @@ -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/ > > 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... > > 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." > > > > */ > > 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; > > } > > > > /** > > - * is_media_entity_v4l2_subdev - return true if the entity main function is > > - * associated with the V4L2 API subdev usage > > - * > > + * is_media_entity_v4l2_subdev() - Check if the entity implements the > > + * v4l2_subdev API > > * @entity: pointer to entity > > * > > - * This is an ancillary function used by subdev-based V4L2 drivers. > > - * It checks if the entity function is one of functions used by a V4L2 subdev, > > - * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs. > > + * Return: true if the entity implement the v4l2_subdev API (is directly > > + * embedded in a struct v4l2_subdev instance) or false otherwise. > > */ > > s/API/kABI/ > > > static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity) > > { > > - if (!entity) > > - return false; > > - > > - switch (entity->function) { > > - case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN: > > - case MEDIA_ENT_F_CAM_SENSOR: > > - case MEDIA_ENT_F_FLASH: > > - case MEDIA_ENT_F_LENS: > > - case MEDIA_ENT_F_ATV_DECODER: > > - case MEDIA_ENT_F_TUNER: > > - return true; > > - > > - default: > > - return false; > > - } > > + return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV; > > } > > > > /** > >
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 ?
Em Fri, 26 Feb 2016 13:18:30 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > 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. Well, as we could have entities not embedded on some other object, this is actually not an object type, even on OO programming. What we're actually representing here is a graph object class. 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? > > > > 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. > > > > 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. > > > >> */ > > >> 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 ? This patch itself won't remove the warnings, but the followup ones that you're likely writing adding the proper entity functions to each subdev used by vsp1. The very same thing is also needed for omap3 and some other drivers. I guess there's no hurry to merge such warnings removal on v4.5. The entity types on vsp1 driver are not properly reported even before 4.5. The only thing is that, before 4.5, those undefined entities were silently ignored by the MC core. So, I don't see any urgency. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote: > Em Fri, 26 Feb 2016 13:18:30 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > >> 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. > > Well, as we could have entities not embedded on some other object, this > is actually not an object type, even on OO programming. What we're actually > representing here is a graph object class. > > 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 had to think about this a bit, but IMHO it is an entity classification that a subsystem sets when creating the entity. So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all entities of the V4L2_SUBDEV classification are indeed embedded in a struct v4l2_subdev, that is not true for V4L2_IO (radio interface entities are embedded in struct video_device, but are not of the V4L2_IO class). Other subsystems may need other classifications. So what about this: enum media_entity_class { MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too MEDIA_ENTITY_CLASS_V4L2_IO, MEDIA_ENTITY_CLASS_V4L2_SUBDEV, }; and the field enum media_entity_class class; in struct media_entity with documentation: @class: Classification of the media_entity, subsystems can set this to quickly classify what sort of media_entity this is. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Fri, 26 Feb 2016 15:00:06 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 02/26/2016 02:21 PM, Mauro Carvalho Chehab wrote: > > Em Fri, 26 Feb 2016 13:18:30 +0200 > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > > >> 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. > > > > Well, as we could have entities not embedded on some other object, this > > is actually not an object type, even on OO programming. What we're actually > > representing here is a graph object class. > > > > 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 had to think about this a bit, but IMHO it is an entity classification that > a subsystem sets when creating the entity. > > So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all entities of the > V4L2_SUBDEV classification are indeed embedded in a struct v4l2_subdev, that is not > true for V4L2_IO (radio interface entities are embedded in struct video_device, but > are not of the V4L2_IO class). > > Other subsystems may need other classifications. > > So what about this: > > enum media_entity_class { > MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too > MEDIA_ENTITY_CLASS_V4L2_IO, > MEDIA_ENTITY_CLASS_V4L2_SUBDEV, > }; > > and the field enum media_entity_class class; in struct media_entity with documentation: > > @class: Classification of the media_entity, subsystems can set this to quickly classify > what sort of media_entity this is. Works for me. > > Regards, > > Hans
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@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 > >>>> + * [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. > >>>> */
Hi Hans, On Friday 26 February 2016 15:00:06 Hans Verkuil wrote: > On 02/26/2016 02:21 PM, 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@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. > > > > Well, as we could have entities not embedded on some other object, this > > is actually not an object type, even on OO programming. What we're > > actually representing here is a graph object class. > > > > 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 had to think about this a bit, but IMHO it is an entity classification > that a subsystem sets when creating the entity. > > So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all > entities of the V4L2_SUBDEV classification are indeed embedded in a struct > v4l2_subdev, that is not true for V4L2_IO (radio interface entities are > embedded in struct video_device, but are not of the V4L2_IO class). > > Other subsystems may need other classifications. > > So what about this: > > enum media_entity_class { > MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too > MEDIA_ENTITY_CLASS_V4L2_IO, > MEDIA_ENTITY_CLASS_V4L2_SUBDEV, > }; The purpose of the type is solely to identify the type of the media_entity instance to safely cast it to the proper object type (in an OOP sense). That's what I want the name of the field to describe. It's not about a classification, it's about object instance type identification. From that point of view, the V4L2_IO class/type is wrong. We want to tell that the entity instance is a video_device instance (and given that we use C, this OOP construct is implemented by embedding the struct media_entity in a struct video_device). We really want VIDEO_DEVICE here, there is no struct v4l2_io. > and the field enum media_entity_class class; in struct media_entity with > documentation: > > @class: Classification of the media_entity, subsystems can set this to > quickly classify what sort of media_entity this is.
On 02/28/2016 08:09 PM, Laurent Pinchart wrote: > Hi Hans, > > On Friday 26 February 2016 15:00:06 Hans Verkuil wrote: >> On 02/26/2016 02:21 PM, 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@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. >>> >>> Well, as we could have entities not embedded on some other object, this >>> is actually not an object type, even on OO programming. What we're >>> actually representing here is a graph object class. >>> >>> 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 had to think about this a bit, but IMHO it is an entity classification >> that a subsystem sets when creating the entity. >> >> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all >> entities of the V4L2_SUBDEV classification are indeed embedded in a struct >> v4l2_subdev, that is not true for V4L2_IO (radio interface entities are >> embedded in struct video_device, but are not of the V4L2_IO class). >> >> Other subsystems may need other classifications. >> >> So what about this: >> >> enum media_entity_class { >> MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here too >> MEDIA_ENTITY_CLASS_V4L2_IO, >> MEDIA_ENTITY_CLASS_V4L2_SUBDEV, >> }; > > The purpose of the type is solely to identify the type of the media_entity > instance to safely cast it to the proper object type (in an OOP sense). That's > what I want the name of the field to describe. It's not about a > classification, it's about object instance type identification. No, it's not. The way it is used is to find entities that are embedded in a video_device AND that do I/O. While all I/O V4L2 entities are a video_device, the reverse is not true. Most obviously radio devices, but video devices can also be used for control only and not for I/O. For example (a real-life example) a sensor -> FPGA -> HDMI TX pipeline where video devices are used to control the sensor and HDMI transmitter, but no I/O to/from memory happens since that's all inside the FPGA. So this really is CLASS_V4L2_IO. And the name is_media_entity_v4l2_io is OK too. This could be done differently, though, but it requires more work. I do think it would be useful to know that the entity is embedded in the video_device, so I agree with you on that. But to make is_media_entity_v4l2_io work you would need to know if the device could do I/O. One way this can be done is to make the device_caps of v4l2_capabilities part of struct video_device. This would have other beneficial results as well since the core can now know the caps of the device and it can take decisions accordingly. I've thought about this before but never had enough of a reason to implement it. So the is_media_entity_v4l2_io() function would check if the entity is of type VIDEO_DEVICE, then use container_of to get the caps from video_device and check for (CAP_STREAMING | CAP_READWRITE). In this case I would call it: enum media_entity_is_of_type { MEDIA_ENTITY_TYPE_UNDEFINED, MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE, MEDIA_ENTITY_TYPE_V4L2_SUBDEV, }; And the field name would be: enum media_entity_is_of_type type; But this requires more work since all drivers need to be modified, starting with those used by those drivers are also use the is_media_entity_* functions. I do think it would be beneficial to make the device_caps part of video_device regardless. Regards, Hans > From that point of view, the V4L2_IO class/type is wrong. We want to tell that > the entity instance is a video_device instance (and given that we use C, this > OOP construct is implemented by embedding the struct media_entity in a struct > video_device). We really want VIDEO_DEVICE here, there is no struct v4l2_io. > >> and the field enum media_entity_class class; in struct media_entity with >> documentation: >> >> @class: Classification of the media_entity, subsystems can set this to >> quickly classify what sort of media_entity this is. > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Monday 29 February 2016 09:28:18 Hans Verkuil wrote: > On 02/28/2016 08:09 PM, Laurent Pinchart wrote: > > On Friday 26 February 2016 15:00:06 Hans Verkuil wrote: > >> On 02/26/2016 02:21 PM, 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@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. > >>> > >>> Well, as we could have entities not embedded on some other object, this > >>> is actually not an object type, even on OO programming. What we're > >>> actually representing here is a graph object class. > >>> > >>> 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 had to think about this a bit, but IMHO it is an entity classification > >> that a subsystem sets when creating the entity. > >> > >> So v4l2 has the classifications V4L2_SUBDEV and V4L2_IO. And while all > >> entities of the V4L2_SUBDEV classification are indeed embedded in a > >> struct v4l2_subdev, that is not true for V4L2_IO (radio interface > >> entities are embedded in struct video_device, but are not of the V4L2_IO > >> class). > >> > >> Other subsystems may need other classifications. > >> > >> So what about this: > >> > >> enum media_entity_class { > >> > >> MEDIA_ENTITY_CLASS_UNDEFINED, // Actually, CLASS_NONE would work here > >> too > >> MEDIA_ENTITY_CLASS_V4L2_IO, > >> MEDIA_ENTITY_CLASS_V4L2_SUBDEV, > >> > >> }; > > > > The purpose of the type is solely to identify the type of the media_entity > > instance to safely cast it to the proper object type (in an OOP sense). > > That's what I want the name of the field to describe. It's not about a > > classification, it's about object instance type identification. > > No, it's not. The way it is used is to find entities that are embedded in a > video_device AND that do I/O. Used by who ? There are currently 5 drivers using the is_media_entity_v4l2_io() function (exynos4-is, omap3isp, vsp1, omap4iss, davinci_vpfe). They all use two entity types only (video_device and v4l2_subdev), and use is_media_entity_v4l2_io() to check whether the entity is implemented by a video_device (and in the case of the vsp1 driver to check whether the entity is not a subdev, I should probably use !is_media_entity_v4l2_subdev() instead). Those are the current use cases, and they make sense to me. They might not align with the function name though, which is why I propose renaming it in the v2 I've sent yesterday. Furthermore, the type of the entity embedded in struct video_device is unconditionally set to MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE in the video device registration function. In practice is_media_entity_v4l2_io() will return true for all video_device instances, regardless of whether they can do I/O. I agree that knowing whether a particular video_device instance can perform I/O could be useful to drivers, but I believe that information shouldn't be carried by the media_entity type field but discovered through a method specific to struct video_device instances. We have no driver requiring that information today, so there's no urgency. > While all I/O V4L2 entities are a video_device, the reverse is not true. > Most obviously radio devices, but video devices can also be used for control > only and not for I/O. For example (a real-life example) a sensor -> FPGA -> > HDMI TX pipeline where video devices are used to control the sensor and HDMI > transmitter, but no I/O to/from memory happens since that's all inside the > FPGA. > > So this really is CLASS_V4L2_IO. > > And the name is_media_entity_v4l2_io is OK too. > > This could be done differently, though, but it requires more work. > > I do think it would be useful to know that the entity is embedded in the > video_device, so I agree with you on that. > > But to make is_media_entity_v4l2_io work you would need to know if the > device could do I/O. One way this can be done is to make the device_caps of > v4l2_capabilities part of struct video_device. > > This would have other beneficial results as well since the core can now know > the caps of the device and it can take decisions accordingly. I've thought > about this before but never had enough of a reason to implement it. Given that caps are static (at least in the use cases I can think of, and we could allow drivers to override the capabilities operation if really needed) it would indeed make sense and would allow getting rid of a bunch of trivial functions. At the expense of 8 more bytes per video_device instance I believe we could still decrease memory usage. > So the is_media_entity_v4l2_io() function would check if the entity is of > type VIDEO_DEVICE, then use container_of to get the caps from video_device > and check for (CAP_STREAMING | CAP_READWRITE). > > In this case I would call it: > > enum media_entity_is_of_type { > MEDIA_ENTITY_TYPE_UNDEFINED, > MEDIA_ENTITY_TYPE_V4L2_VIDEO_DEVICE, > MEDIA_ENTITY_TYPE_V4L2_SUBDEV, > }; > > And the field name would be: > > enum media_entity_is_of_type type; > > But this requires more work since all drivers need to be modified, starting > with those used by those drivers are also use the is_media_entity_* > functions. I do think it would be beneficial to make the device_caps part > of video_device regardless. > > Regards, > > Hans > > > From that point of view, the V4L2_IO class/type is wrong. We want to tell > > that the entity instance is a video_device instance (and given that we > > use C, this OOP construct is implemented by embedding the struct > > media_entity in a struct video_device). We really want VIDEO_DEVICE here, > > there is no struct v4l2_io.> > >> and the field enum media_entity_class class; in struct media_entity with > >> documentation: > >> > >> @class: Classification of the media_entity, subsystems can set this to > >> quickly classify what sort of media_entity this is.
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index d8e5994cccf1..7e766a92e3d9 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -735,6 +735,7 @@ static int video_register_media_controller(struct video_device *vdev, int type) if (!vdev->v4l2_dev->mdev) return 0; + vdev->entity.type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; vdev->entity.function = MEDIA_ENT_F_UNKNOWN; switch (type) { diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index d63083803144..bb6e79f14bb8 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -584,6 +584,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) sd->host_priv = NULL; #if defined(CONFIG_MEDIA_CONTROLLER) sd->entity.name = sd->name; + sd->entity.type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV; sd->entity.function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN; #endif } 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 + * + * @MEDIA_ENTITY_TYPE_NONE: + * The entity isn't embedded in a standard structure. + * @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, +}; + +/** * struct media_entity - A media entity graph object. * * @graph_obj: Embedded structure containing the media object common data. * @name: Entity name. + * @type: Type of the object that embeds the media_entity instance. * @function: Entity main function, as defined in uapi/media.h * (MEDIA_ENT_F_*) * @flags: Entity flags, as defined in uapi/media.h (MEDIA_ENT_FL_*) @@ -219,6 +236,7 @@ struct media_entity_operations { struct media_entity { struct media_gobj graph_obj; /* must be first field in struct */ const char *name; + enum media_entity_type type; u32 function; unsigned long flags; @@ -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. */ 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; } /** - * is_media_entity_v4l2_subdev - return true if the entity main function is - * associated with the V4L2 API subdev usage - * + * is_media_entity_v4l2_subdev() - Check if the entity implements the + * v4l2_subdev API * @entity: pointer to entity * - * This is an ancillary function used by subdev-based V4L2 drivers. - * It checks if the entity function is one of functions used by a V4L2 subdev, - * e. g. camera-relatef functions, analog TV decoder, TV tuner, V4L2 DSPs. + * Return: true if the entity implement the v4l2_subdev API (is directly + * embedded in a struct v4l2_subdev instance) or false otherwise. */ static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity) { - if (!entity) - return false; - - switch (entity->function) { - case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN: - case MEDIA_ENT_F_CAM_SENSOR: - case MEDIA_ENT_F_FLASH: - case MEDIA_ENT_F_LENS: - case MEDIA_ENT_F_ATV_DECODER: - case MEDIA_ENT_F_TUNER: - return true; - - default: - return false; - } + return entity && entity->type == MEDIA_ENTITY_TYPE_V4L2_SUBDEV; } /**
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 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(-)