All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] Restructure video_device
@ 2009-10-23 14:25 Laurent Pinchart
  2009-11-05 14:19 ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2009-10-23 14:25 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil, Sakari Ailus

Hi everybody,

while working on device node support for subdevs I ran into an issue with the 
way v4l2 objects are structured.

We currently have the following structure:

- video_device represents a device that complies with the V4L1 or V4L2 API. 
Every video_device has a corresponding device node.

- v4l2_device represents a high-level media device that handles sub-devices. 
With the new media controller infrastructure a v4l2_device will have a device 
node as well.

- v4l2_subdev represents a sub-device. As for v4l2_device's, the new media 
controller infrastructure will give a device node for every sub-device.

- v4l2_entity is the structure that both v4l2_subdev and video_device derive 
from. Most of the media controller code will deal with entities rather than 
sub-devices or video devices, as most operations (such as discovering the 
topology and create links) do not depend on the exact nature of the entity. 
New types of entities could be introduced later.

Both the video_device and v4l2_subdev structure inherit from v4l2_entity, so 
both of them have a v4l2_entity field. With v4l2_device and v4l2_subdev now 
needing to devices to have device nodes created, the v4l2_device and 
v4l2_subdev structure both have a video_device field.

This isn't clean for two reasons:

- v4l2_device isn't a v4l2_entity, so it should inherit from a structure 
(video_device) that itself inherits from v4l2_entity. 

- v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and once 
through video_device.

To fix this I would like to refactor the video_device structure and cut it in 
two pieces. One of them will deal with device node related tasks, being mostly 
V4L1/V4L2 agnostic, and the other will inherit from the first and add 
V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields from the current 
video_device structure), as well as media controller support (inheriting from 
v4l2_entity).

My plan was to create a video_devnode structure for the low-level device node 
related structure, and keeping the video_device name for the higher level 
structure. v4l2_device, v4l2_subdev and video_device would then all have a 
video_devnode field.

While this isn't exactly difficult, it would require changing a lot of 
drivers, as some field will be moved from video_device to 
video_device::video_devnode. Some of those fields are internal, some of them 
are accessed by drivers while they shouldn't in most cases (the minor field 
for instance), and some are public (name, parent).

I would like to have your opinion on whether you think this proposal is 
acceptable or whether you see a better and cleaner way to restructure the 
video device code structures.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2009-10-23 14:25 [RFC] Restructure video_device Laurent Pinchart
@ 2009-11-05 14:19 ` Hans Verkuil
  2009-11-06 10:23   ` Laurent Pinchart
  2011-09-26 11:17   ` Guennadi Liakhovetski
  0 siblings, 2 replies; 7+ messages in thread
From: Hans Verkuil @ 2009-11-05 14:19 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media, Sakari Ailus

On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> Hi everybody,
> 
> while working on device node support for subdevs I ran into an issue with the 
> way v4l2 objects are structured.
> 
> We currently have the following structure:
> 
> - video_device represents a device that complies with the V4L1 or V4L2 API. 
> Every video_device has a corresponding device node.
> 
> - v4l2_device represents a high-level media device that handles sub-devices. 
> With the new media controller infrastructure a v4l2_device will have a device 
> node as well.
> 
> - v4l2_subdev represents a sub-device. As for v4l2_device's, the new media 
> controller infrastructure will give a device node for every sub-device.
> 
> - v4l2_entity is the structure that both v4l2_subdev and video_device derive 
> from. Most of the media controller code will deal with entities rather than 
> sub-devices or video devices, as most operations (such as discovering the 
> topology and create links) do not depend on the exact nature of the entity. 
> New types of entities could be introduced later.
> 
> Both the video_device and v4l2_subdev structure inherit from v4l2_entity, so 
> both of them have a v4l2_entity field. With v4l2_device and v4l2_subdev now 
> needing to devices to have device nodes created, the v4l2_device and 
> v4l2_subdev structure both have a video_device field.
> 
> This isn't clean for two reasons:
> 
> - v4l2_device isn't a v4l2_entity, so it should inherit from a structure 
> (video_device) that itself inherits from v4l2_entity. 
> 
> - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and once 
> through video_device.

I agree.

> To fix this I would like to refactor the video_device structure and cut it in 
> two pieces. One of them will deal with device node related tasks, being mostly 
> V4L1/V4L2 agnostic, and the other will inherit from the first and add 
> V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields from the current 
> video_device structure), as well as media controller support (inheriting from 
> v4l2_entity).
> 
> My plan was to create a video_devnode structure for the low-level device node 

Let's call it v4l2_devnode to be consistent with the current naming convention.

> related structure, and keeping the video_device name for the higher level 
> structure. v4l2_device, v4l2_subdev and video_device would then all have a 
> video_devnode field.
> 
> While this isn't exactly difficult, it would require changing a lot of 
> drivers, as some field will be moved from video_device to 
> video_device::video_devnode. Some of those fields are internal, some of them 
> are accessed by drivers while they shouldn't in most cases (the minor field 
> for instance), and some are public (name, parent).
> 
> I would like to have your opinion on whether you think this proposal is 
> acceptable or whether you see a better and cleaner way to restructure the 
> video device code structures.
> 

I have two issues with this:

1) Is it really necessary to do this now? We are still in the prototyping
phase and I think it is probably more efficient right now to hack around this
and postpone the real fix (as described above) until we are sure that the mc
concept is working correctly.

2) I'm not sure whether the final media controller will and should be part
of the v4l framework at all. I think that this is something that can be used
separately from the v4l subsystem. So we should be very careful about
integrating this too closely in v4l. Again, this is not much of an issue
while prototyping, but it definitely will need some careful thinking when we
do the final implementation.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2009-11-05 14:19 ` Hans Verkuil
@ 2009-11-06 10:23   ` Laurent Pinchart
  2009-11-07 12:36     ` Hans Verkuil
  2011-09-26 11:17   ` Guennadi Liakhovetski
  1 sibling, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2009-11-06 10:23 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Sakari Ailus

Hi Hans,

On Thursday 05 November 2009 15:19:06 Hans Verkuil wrote:
> On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> > Hi everybody,
> >
> > while working on device node support for subdevs I ran into an issue with
> > the way v4l2 objects are structured.
> >
> > We currently have the following structure:
> >
> > - video_device represents a device that complies with the V4L1 or V4L2
> > API. Every video_device has a corresponding device node.
> >
> > - v4l2_device represents a high-level media device that handles
> > sub-devices. With the new media controller infrastructure a v4l2_device
> > will have a device node as well.
> >
> > - v4l2_subdev represents a sub-device. As for v4l2_device's, the new
> > media controller infrastructure will give a device node for every
> > sub-device.
> >
> > - v4l2_entity is the structure that both v4l2_subdev and video_device
> > derive from. Most of the media controller code will deal with entities
> > rather than sub-devices or video devices, as most operations (such as
> > discovering the topology and create links) do not depend on the exact
> > nature of the entity. New types of entities could be introduced later.
> >
> > Both the video_device and v4l2_subdev structure inherit from v4l2_entity,
> > so both of them have a v4l2_entity field. With v4l2_device and
> > v4l2_subdev now needing to devices to have device nodes created, the
> > v4l2_device and v4l2_subdev structure both have a video_device field.
> >
> > This isn't clean for two reasons:
> >
> > - v4l2_device isn't a v4l2_entity, so it should inherit from a structure
> > (video_device) that itself inherits from v4l2_entity.
> >
> > - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and
> > once through video_device.
> 
> I agree.
> 
> > To fix this I would like to refactor the video_device structure and cut
> > it in two pieces. One of them will deal with device node related tasks,
> > being mostly V4L1/V4L2 agnostic, and the other will inherit from the
> > first and add V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields
> > from the current video_device structure), as well as media controller
> > support (inheriting from v4l2_entity).
> >
> > My plan was to create a video_devnode structure for the low-level device
> > node
> 
> Let's call it v4l2_devnode to be consistent with the current naming
>  convention.

Ok.

> > related structure, and keeping the video_device name for the higher level
> > structure. v4l2_device, v4l2_subdev and video_device would then all have
> > a video_devnode field.
> >
> > While this isn't exactly difficult, it would require changing a lot of
> > drivers, as some field will be moved from video_device to
> > video_device::video_devnode. Some of those fields are internal, some of
> > them are accessed by drivers while they shouldn't in most cases (the
> > minor field for instance), and some are public (name, parent).
> >
> > I would like to have your opinion on whether you think this proposal is
> > acceptable or whether you see a better and cleaner way to restructure the
> > video device code structures.
> 
> I have two issues with this:
> 
> 1) Is it really necessary to do this now? We are still in the prototyping
> phase and I think it is probably more efficient right now to hack around
>  this and postpone the real fix (as described above) until we are sure that
>  the mc concept is working correctly.

The media controller prototyping code is, as usual with prototyping codes, a 
bit messy. Splitting the device node management part from video_device into 
v4l2_devnode will make the media controller code easier to understand for 
outsiders (by outsider I mean every person who haven't been actively working 
on the code, so that includes pretty much everybody). I think it's worth it, 
especially given that I've already written the patches. They can live in the 
media controller tree of course, we don't have to apply them to mainline at 
the moment.

> 2) I'm not sure whether the final media controller will and should be part
> of the v4l framework at all. I think that this is something that can be
>  used separately from the v4l subsystem.

I think it should not be part of the v4l subsystem. ALSA will benefit from the 
media controller, and so might other subsystems such as GPU. A media_ prefix 
would be much nicer.

>  So we should be very careful about integrating this too closely in v4l.
>  Again, this is not much of an issue while prototyping, but it definitely
>  will need some careful thinking when we do the final implementation.

Agreed. Let's rename v4l2_devnode to media_devnode in the future then :-)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2009-11-06 10:23   ` Laurent Pinchart
@ 2009-11-07 12:36     ` Hans Verkuil
  2009-11-09  9:36       ` Laurent Pinchart
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2009-11-07 12:36 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media, Sakari Ailus

On Friday 06 November 2009 11:23:59 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 05 November 2009 15:19:06 Hans Verkuil wrote:
> > On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> > > Hi everybody,
> > >
> > > while working on device node support for subdevs I ran into an issue with
> > > the way v4l2 objects are structured.
> > >
> > > We currently have the following structure:
> > >
> > > - video_device represents a device that complies with the V4L1 or V4L2
> > > API. Every video_device has a corresponding device node.
> > >
> > > - v4l2_device represents a high-level media device that handles
> > > sub-devices. With the new media controller infrastructure a v4l2_device
> > > will have a device node as well.
> > >
> > > - v4l2_subdev represents a sub-device. As for v4l2_device's, the new
> > > media controller infrastructure will give a device node for every
> > > sub-device.
> > >
> > > - v4l2_entity is the structure that both v4l2_subdev and video_device
> > > derive from. Most of the media controller code will deal with entities
> > > rather than sub-devices or video devices, as most operations (such as
> > > discovering the topology and create links) do not depend on the exact
> > > nature of the entity. New types of entities could be introduced later.
> > >
> > > Both the video_device and v4l2_subdev structure inherit from v4l2_entity,
> > > so both of them have a v4l2_entity field. With v4l2_device and
> > > v4l2_subdev now needing to devices to have device nodes created, the
> > > v4l2_device and v4l2_subdev structure both have a video_device field.
> > >
> > > This isn't clean for two reasons:
> > >
> > > - v4l2_device isn't a v4l2_entity, so it should inherit from a structure
> > > (video_device) that itself inherits from v4l2_entity.
> > >
> > > - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and
> > > once through video_device.
> > 
> > I agree.
> > 
> > > To fix this I would like to refactor the video_device structure and cut
> > > it in two pieces. One of them will deal with device node related tasks,
> > > being mostly V4L1/V4L2 agnostic, and the other will inherit from the
> > > first and add V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields
> > > from the current video_device structure), as well as media controller
> > > support (inheriting from v4l2_entity).
> > >
> > > My plan was to create a video_devnode structure for the low-level device
> > > node
> > 
> > Let's call it v4l2_devnode to be consistent with the current naming
> >  convention.
> 
> Ok.
> 
> > > related structure, and keeping the video_device name for the higher level
> > > structure. v4l2_device, v4l2_subdev and video_device would then all have
> > > a video_devnode field.
> > >
> > > While this isn't exactly difficult, it would require changing a lot of
> > > drivers, as some field will be moved from video_device to
> > > video_device::video_devnode. Some of those fields are internal, some of
> > > them are accessed by drivers while they shouldn't in most cases (the
> > > minor field for instance), and some are public (name, parent).
> > >
> > > I would like to have your opinion on whether you think this proposal is
> > > acceptable or whether you see a better and cleaner way to restructure the
> > > video device code structures.
> > 
> > I have two issues with this:
> > 
> > 1) Is it really necessary to do this now? We are still in the prototyping
> > phase and I think it is probably more efficient right now to hack around
> >  this and postpone the real fix (as described above) until we are sure that
> >  the mc concept is working correctly.
> 
> The media controller prototyping code is, as usual with prototyping codes, a 
> bit messy. Splitting the device node management part from video_device into 
> v4l2_devnode will make the media controller code easier to understand for 
> outsiders (by outsider I mean every person who haven't been actively working 
> on the code, so that includes pretty much everybody). I think it's worth it, 
> especially given that I've already written the patches. They can live in the 
> media controller tree of course, we don't have to apply them to mainline at 
> the moment.

Ah, it's only for the mc tree. I was getting the impression that you wanted to
do this for the mainline tree as well. But if it is just for the mc tree, then
go ahead. You can just do it in your own tree; as far as I am concerned your
tree is leading for now.

> > 2) I'm not sure whether the final media controller will and should be part
> > of the v4l framework at all. I think that this is something that can be
> >  used separately from the v4l subsystem.
> 
> I think it should not be part of the v4l subsystem. ALSA will benefit from the 
> media controller, and so might other subsystems such as GPU. A media_ prefix 
> would be much nicer.

I agree, but let's postpone such decisions until later.

> >  So we should be very careful about integrating this too closely in v4l.
> >  Again, this is not much of an issue while prototyping, but it definitely
> >  will need some careful thinking when we do the final implementation.
> 
> Agreed. Let's rename v4l2_devnode to media_devnode in the future then :-)
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2009-11-07 12:36     ` Hans Verkuil
@ 2009-11-09  9:36       ` Laurent Pinchart
  0 siblings, 0 replies; 7+ messages in thread
From: Laurent Pinchart @ 2009-11-09  9:36 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Sakari Ailus

Hi Hans,

On Saturday 07 November 2009 13:36:33 Hans Verkuil wrote:
> On Friday 06 November 2009 11:23:59 Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Thursday 05 November 2009 15:19:06 Hans Verkuil wrote:
> > > On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> > > > Hi everybody,
> > > >
> > > > while working on device node support for subdevs I ran into an issue
> > > > with the way v4l2 objects are structured.
> > > >
> > > > We currently have the following structure:
> > > >
> > > > - video_device represents a device that complies with the V4L1 or
> > > > V4L2 API. Every video_device has a corresponding device node.
> > > >
> > > > - v4l2_device represents a high-level media device that handles
> > > > sub-devices. With the new media controller infrastructure a
> > > > v4l2_device will have a device node as well.
> > > >
> > > > - v4l2_subdev represents a sub-device. As for v4l2_device's, the new
> > > > media controller infrastructure will give a device node for every
> > > > sub-device.
> > > >
> > > > - v4l2_entity is the structure that both v4l2_subdev and video_device
> > > > derive from. Most of the media controller code will deal with
> > > > entities rather than sub-devices or video devices, as most operations
> > > > (such as discovering the topology and create links) do not depend on
> > > > the exact nature of the entity. New types of entities could be
> > > > introduced later.
> > > >
> > > > Both the video_device and v4l2_subdev structure inherit from
> > > > v4l2_entity, so both of them have a v4l2_entity field. With
> > > > v4l2_device and v4l2_subdev now needing to devices to have device
> > > > nodes created, the v4l2_device and v4l2_subdev structure both have a
> > > > video_device field.
> > > >
> > > > This isn't clean for two reasons:
> > > >
> > > > - v4l2_device isn't a v4l2_entity, so it should inherit from a
> > > > structure (video_device) that itself inherits from v4l2_entity.
> > > >
> > > > - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly
> > > > and once through video_device.
> > >
> > > I agree.
> > >
> > > > To fix this I would like to refactor the video_device structure and
> > > > cut it in two pieces. One of them will deal with device node related
> > > > tasks, being mostly V4L1/V4L2 agnostic, and the other will inherit
> > > > from the first and add V4L1/V4L2 support
> > > > (tvnorms/current_norm/ioctl_ops fields from the current video_device
> > > > structure), as well as media controller support (inheriting from
> > > > v4l2_entity).
> > > >
> > > > My plan was to create a video_devnode structure for the low-level
> > > > device node
> > >
> > > Let's call it v4l2_devnode to be consistent with the current naming
> > >  convention.
> >
> > Ok.
> >
> > > > related structure, and keeping the video_device name for the higher
> > > > level structure. v4l2_device, v4l2_subdev and video_device would then
> > > > all have a video_devnode field.
> > > >
> > > > While this isn't exactly difficult, it would require changing a lot
> > > > of drivers, as some field will be moved from video_device to
> > > > video_device::video_devnode. Some of those fields are internal, some
> > > > of them are accessed by drivers while they shouldn't in most cases
> > > > (the minor field for instance), and some are public (name, parent).
> > > >
> > > > I would like to have your opinion on whether you think this proposal
> > > > is acceptable or whether you see a better and cleaner way to
> > > > restructure the video device code structures.
> > >
> > > I have two issues with this:
> > >
> > > 1) Is it really necessary to do this now? We are still in the
> > > prototyping phase and I think it is probably more efficient right now
> > > to hack around this and postpone the real fix (as described above)
> > > until we are sure that the mc concept is working correctly.
> >
> > The media controller prototyping code is, as usual with prototyping
> > codes, a bit messy. Splitting the device node management part from
> > video_device into v4l2_devnode will make the media controller code easier
> > to understand for outsiders (by outsider I mean every person who haven't
> > been actively working on the code, so that includes pretty much
> > everybody). I think it's worth it, especially given that I've already
> > written the patches. They can live in the media controller tree of
> > course, we don't have to apply them to mainline at the moment.
> 
> Ah, it's only for the mc tree. I was getting the impression that you wanted
>  to do this for the mainline tree as well. But if it is just for the mc
>  tree, then go ahead. You can just do it in your own tree; as far as I am
>  concerned your tree is leading for now.

Ok. I just wanted to make sure there was no huge issue with the proposed 
change. I want to avoid writing code that I'll have to completely redesign 
later.

> > > 2) I'm not sure whether the final media controller will and should be
> > > part of the v4l framework at all. I think that this is something that
> > > can be used separately from the v4l subsystem.
> >
> > I think it should not be part of the v4l subsystem. ALSA will benefit
> > from the media controller, and so might other subsystems such as GPU. A
> > media_ prefix would be much nicer.
> 
> I agree, but let's postpone such decisions until later.
> 
> > >  So we should be very careful about integrating this too closely in
> > > v4l. Again, this is not much of an issue while prototyping, but it
> > > definitely will need some careful thinking when we do the final
> > > implementation.
> >
> > Agreed. Let's rename v4l2_devnode to media_devnode in the future then :-)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2009-11-05 14:19 ` Hans Verkuil
  2009-11-06 10:23   ` Laurent Pinchart
@ 2011-09-26 11:17   ` Guennadi Liakhovetski
  2011-09-28  9:34     ` Hans Verkuil
  1 sibling, 1 reply; 7+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-26 11:17 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Laurent Pinchart, linux-media, Sakari Ailus

Hi Hans

Sorry for reviving an almost 2 year old thread, but the topic, discussed 
back then is still relevant (I'll include a complete quote to refresh the 
old discussion):

On Thu, 5 Nov 2009, Hans Verkuil wrote:

> On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> > Hi everybody,
> > 
> > while working on device node support for subdevs I ran into an issue with the 
> > way v4l2 objects are structured.
> > 
> > We currently have the following structure:
> > 
> > - video_device represents a device that complies with the V4L1 or V4L2 API. 
> > Every video_device has a corresponding device node.
> > 
> > - v4l2_device represents a high-level media device that handles sub-devices. 
> > With the new media controller infrastructure a v4l2_device will have a device 
> > node as well.
> > 
> > - v4l2_subdev represents a sub-device. As for v4l2_device's, the new media 
> > controller infrastructure will give a device node for every sub-device.
> > 
> > - v4l2_entity is the structure that both v4l2_subdev and video_device derive 
> > from. Most of the media controller code will deal with entities rather than 
> > sub-devices or video devices, as most operations (such as discovering the 
> > topology and create links) do not depend on the exact nature of the entity. 
> > New types of entities could be introduced later.
> > 
> > Both the video_device and v4l2_subdev structure inherit from v4l2_entity, so 
> > both of them have a v4l2_entity field. With v4l2_device and v4l2_subdev now 
> > needing to devices to have device nodes created, the v4l2_device and 
> > v4l2_subdev structure both have a video_device field.
> > 
> > This isn't clean for two reasons:
> > 
> > - v4l2_device isn't a v4l2_entity, so it should inherit from a structure 
> > (video_device) that itself inherits from v4l2_entity. 
> > 
> > - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and once 
> > through video_device.
> 
> I agree.
> 
> > To fix this I would like to refactor the video_device structure and cut it in 
> > two pieces. One of them will deal with device node related tasks, being mostly 
> > V4L1/V4L2 agnostic, and the other will inherit from the first and add 
> > V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields from the current 
> > video_device structure), as well as media controller support (inheriting from 
> > v4l2_entity).
> > 
> > My plan was to create a video_devnode structure for the low-level device node 
> 
> Let's call it v4l2_devnode to be consistent with the current naming convention.
> 
> > related structure, and keeping the video_device name for the higher level 
> > structure. v4l2_device, v4l2_subdev and video_device would then all have a 
> > video_devnode field.
> > 
> > While this isn't exactly difficult, it would require changing a lot of 
> > drivers, as some field will be moved from video_device to 
> > video_device::video_devnode. Some of those fields are internal, some of them 
> > are accessed by drivers while they shouldn't in most cases (the minor field 
> > for instance), and some are public (name, parent).
> > 
> > I would like to have your opinion on whether you think this proposal is 
> > acceptable or whether you see a better and cleaner way to restructure the 
> > video device code structures.
> > 
> 
> I have two issues with this:
> 
> 1) Is it really necessary to do this now? We are still in the prototyping
> phase and I think it is probably more efficient right now to hack around this
> and postpone the real fix (as described above) until we are sure that the mc
> concept is working correctly.

Here comes my question: is it the right time for this now?;-) I've relaxed 
the problem a bit with this my patch:

http://patchwork.linuxtv.org/patch/7817/

But the problem, described above, when MC _is_ used - that of double 
inheritance - still remains. I really think it should be fixed now.

Thanks
Guennadi

> 
> 2) I'm not sure whether the final media controller will and should be part
> of the v4l framework at all. I think that this is something that can be used
> separately from the v4l subsystem. So we should be very careful about
> integrating this too closely in v4l. Again, this is not much of an issue
> while prototyping, but it definitely will need some careful thinking when we
> do the final implementation.
> 
> Regards,
> 
> 	Hans
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Restructure video_device
  2011-09-26 11:17   ` Guennadi Liakhovetski
@ 2011-09-28  9:34     ` Hans Verkuil
  0 siblings, 0 replies; 7+ messages in thread
From: Hans Verkuil @ 2011-09-28  9:34 UTC (permalink / raw)
  To: Guennadi Liakhovetski; +Cc: Laurent Pinchart, linux-media, Sakari Ailus

On Monday, September 26, 2011 13:17:00 Guennadi Liakhovetski wrote:
> Hi Hans
> 
> Sorry for reviving an almost 2 year old thread, but the topic, discussed 
> back then is still relevant (I'll include a complete quote to refresh the 
> old discussion):
> 
> On Thu, 5 Nov 2009, Hans Verkuil wrote:
> 
> > On Friday 23 October 2009 16:25:40 Laurent Pinchart wrote:
> > > Hi everybody,
> > > 
> > > while working on device node support for subdevs I ran into an issue with the 
> > > way v4l2 objects are structured.
> > > 
> > > We currently have the following structure:
> > > 
> > > - video_device represents a device that complies with the V4L1 or V4L2 API. 
> > > Every video_device has a corresponding device node.
> > > 
> > > - v4l2_device represents a high-level media device that handles sub-devices. 
> > > With the new media controller infrastructure a v4l2_device will have a device 
> > > node as well.
> > > 
> > > - v4l2_subdev represents a sub-device. As for v4l2_device's, the new media 
> > > controller infrastructure will give a device node for every sub-device.
> > > 
> > > - v4l2_entity is the structure that both v4l2_subdev and video_device derive 
> > > from. Most of the media controller code will deal with entities rather than 
> > > sub-devices or video devices, as most operations (such as discovering the 
> > > topology and create links) do not depend on the exact nature of the entity. 
> > > New types of entities could be introduced later.
> > > 
> > > Both the video_device and v4l2_subdev structure inherit from v4l2_entity, so 
> > > both of them have a v4l2_entity field. With v4l2_device and v4l2_subdev now 
> > > needing to devices to have device nodes created, the v4l2_device and 
> > > v4l2_subdev structure both have a video_device field.
> > > 
> > > This isn't clean for two reasons:
> > > 
> > > - v4l2_device isn't a v4l2_entity, so it should inherit from a structure 
> > > (video_device) that itself inherits from v4l2_entity. 
> > > 
> > > - v4l2_subdev shouldn't inherit twice from v4l2_entity, once directly and once 
> > > through video_device.
> > 
> > I agree.
> > 
> > > To fix this I would like to refactor the video_device structure and cut it in 
> > > two pieces. One of them will deal with device node related tasks, being mostly 
> > > V4L1/V4L2 agnostic, and the other will inherit from the first and add 
> > > V4L1/V4L2 support (tvnorms/current_norm/ioctl_ops fields from the current 
> > > video_device structure), as well as media controller support (inheriting from 
> > > v4l2_entity).
> > > 
> > > My plan was to create a video_devnode structure for the low-level device node 
> > 
> > Let's call it v4l2_devnode to be consistent with the current naming convention.
> > 
> > > related structure, and keeping the video_device name for the higher level 
> > > structure. v4l2_device, v4l2_subdev and video_device would then all have a 
> > > video_devnode field.
> > > 
> > > While this isn't exactly difficult, it would require changing a lot of 
> > > drivers, as some field will be moved from video_device to 
> > > video_device::video_devnode. Some of those fields are internal, some of them 
> > > are accessed by drivers while they shouldn't in most cases (the minor field 
> > > for instance), and some are public (name, parent).
> > > 
> > > I would like to have your opinion on whether you think this proposal is 
> > > acceptable or whether you see a better and cleaner way to restructure the 
> > > video device code structures.
> > > 
> > 
> > I have two issues with this:
> > 
> > 1) Is it really necessary to do this now? We are still in the prototyping
> > phase and I think it is probably more efficient right now to hack around this
> > and postpone the real fix (as described above) until we are sure that the mc
> > concept is working correctly.
> 
> Here comes my question: is it the right time for this now?;-) I've relaxed 
> the problem a bit with this my patch:
> 
> http://patchwork.linuxtv.org/patch/7817/
> 
> But the problem, described above, when MC _is_ used - that of double 
> inheritance - still remains. I really think it should be fixed now.

I'd say, make a proposal for this. I'm not against it and we have enough
experience now to have a good feel for this issue.

I think I would like to put this perhaps in a slightly bigger picture
as well. There are some naming problems as well that I would like to address:

1) v4l2_device is very poorly named. It really represents a root structure from
which to find all other pieces. It does not really represent a device. My proposal
would be to rename it v4l2_root (alternatives are: v4l2_core, v4l2_top, v4l2_sys).

Comments?

2) If v4l2_device becomes available as a name, then I would like to rename
video_device to v4l2_device. Originally I attempted to rename video_device to
v4l2_devnode, but that was opposed by Mauro. I gathered from him that he thinks
v4l2_device would be a proper name for this, but of course it is currently in
use.

Comments?

Regards,

	Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-09-28  9:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-23 14:25 [RFC] Restructure video_device Laurent Pinchart
2009-11-05 14:19 ` Hans Verkuil
2009-11-06 10:23   ` Laurent Pinchart
2009-11-07 12:36     ` Hans Verkuil
2009-11-09  9:36       ` Laurent Pinchart
2011-09-26 11:17   ` Guennadi Liakhovetski
2011-09-28  9:34     ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.