Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1563608 > unrolled thread

Re: [PATCH v3 00/24] i.MX Media Driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 00/24] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-01-20 15:00 +0100
    Re: [PATCH v3 00/24] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-20 17:40 +0100
      Re: [PATCH v3 00/24] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-20 19:50 +0100
        Re: [PATCH v3 00/24] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-01-20 21:50 +0100
          Re: [PATCH v3 00/24] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-23 12:10 +0100
            Re: [PATCH v3 00/24] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-01-23 12:10 +0100
              Re: [PATCH v3 00/24] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-24 12:30 +0100
            Re: [PATCH v3 00/24] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-24 00:10 +0100
              Re: [PATCH v3 00/24] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-24 12:30 +0100
                Re: [PATCH v3 00/24] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-28 20:30 +0100
                Re: [PATCH v3 00/24] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-30 14:10 +0100

#1563608 — Re: [PATCH v3 00/24] i.MX Media Driver

Hi Steve, Philipp,

On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> In version 3:
> 
> Changes suggested by Rob Herring <robh@kernel.org>:
> 
>   - prepended FIM node properties with vendor prefix "fsl,".
> 
>   - make mipi csi-2 receiver compatible string SoC specific:
>     "fsl,imx6-mipi-csi2" instead of "fsl,imx-mipi-csi2".
> 
>   - redundant "_clk" removed from mipi csi-2 receiver clock-names property.
> 
>   - removed board-specific info from the media driver binding doc. These
>     were all related to sensor bindings, which already are (adv7180)
>     or will be (ov564x) covered in separate binding docs. All reference
>     board info not related to DT bindings has been moved to
>     Documentation/media/v4l-drivers/imx.rst.
> 
>   - removed "_mipi" from the OV5640 compatible string.
> 
> Changes suggested by Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>:
> 
>   Mostly cosmetic/non-functional changes which I won't list here, except
>   for the following:
> 
>   - spin_lock_irqsave() changed to spin_lock() in a couple interrupt handlers.
> 
>   - fixed some unnecessary of_node_put()'s in for_each_child_of_node() loops.
> 
>   - check/handle return code from required reg property of CSI port nodes.
> 
>   - check/handle return code from clk_prepare_enable().
> 
> Changes suggested by Fabio Estevam <festevam@gmail.com>:
> 
>   - switch to VGEN3 Analog Vdd supply assuming rev. C SabreSD boards.
> 
>   - finally got around to passing valid IOMUX pin config values to the
>     pin groups.
> 
> Other changes:
> 
>   - removed the FIM properties that overrided the v4l2 FIM control defaults
>     values. This was left-over from a requirement of a customer and is not
>     necessary here.
> 
>   - The FIM must be explicitly enabled in the fim child node under the CSI
>     port nodes, using the status property. If not enabled, FIM v4l2 controls
>     will not appear in the video capture driver.
> 
>   - brought in additional media types patch from Philipp Zabel. Use new
>     MEDIA_ENT_F_VID_IF_BRIDGE in mipi csi-2 receiver subdev.
> 
>   - brought in latest platform generic video multiplexer subdevice driver
>     from Philipp Zabel (squashed with patch that uses new MEDIA_ENT_F_MUX).
> 
>   - removed imx-media-of.h, moved those prototypes into imx-media.h.

Based on the discussion on the mailinglist it seems everyone agrees that this
is the preferred driver, correct?

There are a bunch of review comments, so I will wait for a v4. I plan to merge
that staging driver unless there are major issues with it.

I am not sure if I would merge those sensor drivers in staging, I'll have to
take a closer look at those once v4 is posted.

Regards,

	Hans

[toc] | [next] | [standalone]

#1563771

Hi Hans,

On Fri, 2017-01-20 at 14:52 +0100, Hans Verkuil wrote:
> Hi Steve, Philipp,
> 
> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> > In version 3:
> > 
> > Changes suggested by Rob Herring <robh@kernel.org>:
> > 
> >   - prepended FIM node properties with vendor prefix "fsl,".
> > 
> >   - make mipi csi-2 receiver compatible string SoC specific:
> >     "fsl,imx6-mipi-csi2" instead of "fsl,imx-mipi-csi2".
> > 
> >   - redundant "_clk" removed from mipi csi-2 receiver clock-names property.
> > 
> >   - removed board-specific info from the media driver binding doc. These
> >     were all related to sensor bindings, which already are (adv7180)
> >     or will be (ov564x) covered in separate binding docs. All reference
> >     board info not related to DT bindings has been moved to
> >     Documentation/media/v4l-drivers/imx.rst.
> > 
> >   - removed "_mipi" from the OV5640 compatible string.
> > 
> > Changes suggested by Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>:
> > 
> >   Mostly cosmetic/non-functional changes which I won't list here, except
> >   for the following:
> > 
> >   - spin_lock_irqsave() changed to spin_lock() in a couple interrupt handlers.
> > 
> >   - fixed some unnecessary of_node_put()'s in for_each_child_of_node() loops.
> > 
> >   - check/handle return code from required reg property of CSI port nodes.
> > 
> >   - check/handle return code from clk_prepare_enable().
> > 
> > Changes suggested by Fabio Estevam <festevam@gmail.com>:
> > 
> >   - switch to VGEN3 Analog Vdd supply assuming rev. C SabreSD boards.
> > 
> >   - finally got around to passing valid IOMUX pin config values to the
> >     pin groups.
> > 
> > Other changes:
> > 
> >   - removed the FIM properties that overrided the v4l2 FIM control defaults
> >     values. This was left-over from a requirement of a customer and is not
> >     necessary here.
> > 
> >   - The FIM must be explicitly enabled in the fim child node under the CSI
> >     port nodes, using the status property. If not enabled, FIM v4l2 controls
> >     will not appear in the video capture driver.
> > 
> >   - brought in additional media types patch from Philipp Zabel. Use new
> >     MEDIA_ENT_F_VID_IF_BRIDGE in mipi csi-2 receiver subdev.
> > 
> >   - brought in latest platform generic video multiplexer subdevice driver
> >     from Philipp Zabel (squashed with patch that uses new MEDIA_ENT_F_MUX).
> > 
> >   - removed imx-media-of.h, moved those prototypes into imx-media.h.
> 
> Based on the discussion on the mailinglist it seems everyone agrees that this
> is the preferred driver, correct?

No. I have some major reservations against the custom mem2mem framework
embedded in Steve's driver.
I think it is a misuse of the media entity links (which should describe
hardware connections) for something that should be done at the vb2 level
(letting one device's capture EOF interrupt trigger the next device's
m2m device_run without going through userspace).
Steve and I disagree on that point, so we'd appreciate if we could get
some more eyes on the above issue.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1563864

Hi Hans, Philipp,


On 01/20/2017 08:31 AM, Philipp Zabel wrote:
> Hi Hans,
>
> On Fri, 2017-01-20 at 14:52 +0100, Hans Verkuil wrote:
>> Hi Steve, Philipp,
>>
>> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
>>> In version 3:
>>>
>>> Changes suggested by Rob Herring <robh@kernel.org>:
>>>
>>>    - prepended FIM node properties with vendor prefix "fsl,".
>>>
>>>    - make mipi csi-2 receiver compatible string SoC specific:
>>>      "fsl,imx6-mipi-csi2" instead of "fsl,imx-mipi-csi2".
>>>
>>>    - redundant "_clk" removed from mipi csi-2 receiver clock-names property.
>>>
>>>    - removed board-specific info from the media driver binding doc. These
>>>      were all related to sensor bindings, which already are (adv7180)
>>>      or will be (ov564x) covered in separate binding docs. All reference
>>>      board info not related to DT bindings has been moved to
>>>      Documentation/media/v4l-drivers/imx.rst.
>>>
>>>    - removed "_mipi" from the OV5640 compatible string.
>>>
>>> Changes suggested by Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>:
>>>
>>>    Mostly cosmetic/non-functional changes which I won't list here, except
>>>    for the following:
>>>
>>>    - spin_lock_irqsave() changed to spin_lock() in a couple interrupt handlers.
>>>
>>>    - fixed some unnecessary of_node_put()'s in for_each_child_of_node() loops.
>>>
>>>    - check/handle return code from required reg property of CSI port nodes.
>>>
>>>    - check/handle return code from clk_prepare_enable().
>>>
>>> Changes suggested by Fabio Estevam <festevam@gmail.com>:
>>>
>>>    - switch to VGEN3 Analog Vdd supply assuming rev. C SabreSD boards.
>>>
>>>    - finally got around to passing valid IOMUX pin config values to the
>>>      pin groups.
>>>
>>> Other changes:
>>>
>>>    - removed the FIM properties that overrided the v4l2 FIM control defaults
>>>      values. This was left-over from a requirement of a customer and is not
>>>      necessary here.
>>>
>>>    - The FIM must be explicitly enabled in the fim child node under the CSI
>>>      port nodes, using the status property. If not enabled, FIM v4l2 controls
>>>      will not appear in the video capture driver.
>>>
>>>    - brought in additional media types patch from Philipp Zabel. Use new
>>>      MEDIA_ENT_F_VID_IF_BRIDGE in mipi csi-2 receiver subdev.
>>>
>>>    - brought in latest platform generic video multiplexer subdevice driver
>>>      from Philipp Zabel (squashed with patch that uses new MEDIA_ENT_F_MUX).
>>>
>>>    - removed imx-media-of.h, moved those prototypes into imx-media.h.
>> Based on the discussion on the mailinglist it seems everyone agrees that this
>> is the preferred driver, correct?
> No. I have some major reservations against the custom mem2mem framework
> embedded in Steve's driver.
> I think it is a misuse of the media entity links (which should describe
> hardware connections) for something that should be done at the vb2 level
> (letting one device's capture EOF interrupt trigger the next device's
> m2m device_run without going through userspace).
> Steve and I disagree on that point, so we'd appreciate if we could get
> some more eyes on the above issue.

This needs some background first, so let me first describe one example
pipeline in this driver.

There is a VDIC entity in the i.MX IPU that performs de-interlacing with
hardware filters for motion compensation. Some of the motion compensation
modes ("low" and "medium" motion) require that the VDIC receive video
frame fields from memory buffers (dedicated dma channels in the
IPU are used to transfer those buffers into the VDIC).

So one option to support those modes would be to pass the raw buffers
from a camera sensor up to userspace to a capture device, and then pass
them back to the VDIC for de-interlacing using a mem2mem device.

Philipp and I are both in agreement that, since userland is not interested
in the intermediate interlaced buffers in this case, but only the final
result (motion compensated, de-interlaced frames), it is more efficient
to provide a media link that allows passing those intermediate frames
directly from a camera source pad to VDIC sink pad, without having
to route them through userspace.

So in order to support that, I've implemented a simple FIFO dma buffer
queue in the driver to allow passing video buffers directly from a source
to a sink. It is modeled loosely off the vb2 state machine and API, but
simpler (for instance it only allows contiguous, cache-coherent buffers).

This is where Philipp has an argument, that this should be done with a
new API in videobuf2.

And I'm actually in total agreement with that. I definitely agree that there
should be a mechanism in the media framework that allows passing video
buffers from a source pad to a sink pad using a software queue, with no
involvement from userland.

My only disagreement is when this should be implemented. I think it is
fine to keep my custom implementation of this in the driver for now. Once
an extension of vb2 is ready to support this feature, it would be fairly
straightforward to strip out my custom implementation and go with the
new API.

Steve

[toc] | [prev] | [next] | [standalone]

#1563909

On 01/20/2017 07:40 PM, Steve Longerbeam wrote:
> Hi Hans, Philipp,
> 
> 
> On 01/20/2017 08:31 AM, Philipp Zabel wrote:
>> Hi Hans,
>>
>> On Fri, 2017-01-20 at 14:52 +0100, Hans Verkuil wrote:
>>> Hi Steve, Philipp,
>>>
>>> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
>>>> In version 3:
>>>>
>>>> Changes suggested by Rob Herring <robh@kernel.org>:
>>>>
>>>>    - prepended FIM node properties with vendor prefix "fsl,".
>>>>
>>>>    - make mipi csi-2 receiver compatible string SoC specific:
>>>>      "fsl,imx6-mipi-csi2" instead of "fsl,imx-mipi-csi2".
>>>>
>>>>    - redundant "_clk" removed from mipi csi-2 receiver clock-names property.
>>>>
>>>>    - removed board-specific info from the media driver binding doc. These
>>>>      were all related to sensor bindings, which already are (adv7180)
>>>>      or will be (ov564x) covered in separate binding docs. All reference
>>>>      board info not related to DT bindings has been moved to
>>>>      Documentation/media/v4l-drivers/imx.rst.
>>>>
>>>>    - removed "_mipi" from the OV5640 compatible string.
>>>>
>>>> Changes suggested by Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>:
>>>>
>>>>    Mostly cosmetic/non-functional changes which I won't list here, except
>>>>    for the following:
>>>>
>>>>    - spin_lock_irqsave() changed to spin_lock() in a couple interrupt handlers.
>>>>
>>>>    - fixed some unnecessary of_node_put()'s in for_each_child_of_node() loops.
>>>>
>>>>    - check/handle return code from required reg property of CSI port nodes.
>>>>
>>>>    - check/handle return code from clk_prepare_enable().
>>>>
>>>> Changes suggested by Fabio Estevam <festevam@gmail.com>:
>>>>
>>>>    - switch to VGEN3 Analog Vdd supply assuming rev. C SabreSD boards.
>>>>
>>>>    - finally got around to passing valid IOMUX pin config values to the
>>>>      pin groups.
>>>>
>>>> Other changes:
>>>>
>>>>    - removed the FIM properties that overrided the v4l2 FIM control defaults
>>>>      values. This was left-over from a requirement of a customer and is not
>>>>      necessary here.
>>>>
>>>>    - The FIM must be explicitly enabled in the fim child node under the CSI
>>>>      port nodes, using the status property. If not enabled, FIM v4l2 controls
>>>>      will not appear in the video capture driver.
>>>>
>>>>    - brought in additional media types patch from Philipp Zabel. Use new
>>>>      MEDIA_ENT_F_VID_IF_BRIDGE in mipi csi-2 receiver subdev.
>>>>
>>>>    - brought in latest platform generic video multiplexer subdevice driver
>>>>      from Philipp Zabel (squashed with patch that uses new MEDIA_ENT_F_MUX).
>>>>
>>>>    - removed imx-media-of.h, moved those prototypes into imx-media.h.
>>> Based on the discussion on the mailinglist it seems everyone agrees that this
>>> is the preferred driver, correct?
>> No. I have some major reservations against the custom mem2mem framework
>> embedded in Steve's driver.
>> I think it is a misuse of the media entity links (which should describe
>> hardware connections) for something that should be done at the vb2 level
>> (letting one device's capture EOF interrupt trigger the next device's
>> m2m device_run without going through userspace).
>> Steve and I disagree on that point, so we'd appreciate if we could get
>> some more eyes on the above issue.
> 
> This needs some background first, so let me first describe one example
> pipeline in this driver.
> 
> There is a VDIC entity in the i.MX IPU that performs de-interlacing with
> hardware filters for motion compensation. Some of the motion compensation
> modes ("low" and "medium" motion) require that the VDIC receive video
> frame fields from memory buffers (dedicated dma channels in the
> IPU are used to transfer those buffers into the VDIC).
> 
> So one option to support those modes would be to pass the raw buffers
> from a camera sensor up to userspace to a capture device, and then pass
> them back to the VDIC for de-interlacing using a mem2mem device.
> 
> Philipp and I are both in agreement that, since userland is not interested
> in the intermediate interlaced buffers in this case, but only the final
> result (motion compensated, de-interlaced frames), it is more efficient
> to provide a media link that allows passing those intermediate frames
> directly from a camera source pad to VDIC sink pad, without having
> to route them through userspace.
> 
> So in order to support that, I've implemented a simple FIFO dma buffer
> queue in the driver to allow passing video buffers directly from a source
> to a sink. It is modeled loosely off the vb2 state machine and API, but
> simpler (for instance it only allows contiguous, cache-coherent buffers).
> 
> This is where Philipp has an argument, that this should be done with a
> new API in videobuf2.
> 
> And I'm actually in total agreement with that. I definitely agree that there
> should be a mechanism in the media framework that allows passing video
> buffers from a source pad to a sink pad using a software queue, with no
> involvement from userland.
> 
> My only disagreement is when this should be implemented. I think it is
> fine to keep my custom implementation of this in the driver for now. Once
> an extension of vb2 is ready to support this feature, it would be fairly
> straightforward to strip out my custom implementation and go with the
> new API.

For a staging driver this isn't necessary, as long as it is documented in
the TODO file that this needs to be fixed before it can be moved out of
staging. The whole point of staging is that there is still work to be
done in the driver, after all :-)

BTW, did you look at the vb2_thread_* functions in videobuf2-core.c? A lot
of what you need is already there. Making a new version that has producer
and consumer queues shouldn't be hard given that code.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1564868

On Fri, 2017-01-20 at 21:39 +0100, Hans Verkuil wrote:
[...]
> > There is a VDIC entity in the i.MX IPU that performs de-interlacing with
> > hardware filters for motion compensation. Some of the motion compensation
> > modes ("low" and "medium" motion) require that the VDIC receive video
> > frame fields from memory buffers (dedicated dma channels in the
> > IPU are used to transfer those buffers into the VDIC).
> > 
> > So one option to support those modes would be to pass the raw buffers
> > from a camera sensor up to userspace to a capture device, and then pass
> > them back to the VDIC for de-interlacing using a mem2mem device.
> > 
> > Philipp and I are both in agreement that, since userland is not interested
> > in the intermediate interlaced buffers in this case, but only the final
> > result (motion compensated, de-interlaced frames), it is more efficient
> > to provide a media link that allows passing those intermediate frames
> > directly from a camera source pad to VDIC sink pad, without having
> > to route them through userspace.
> > 
> > So in order to support that, I've implemented a simple FIFO dma buffer
> > queue in the driver to allow passing video buffers directly from a source
> > to a sink. It is modeled loosely off the vb2 state machine and API, but
> > simpler (for instance it only allows contiguous, cache-coherent buffers).
> > 
> > This is where Philipp has an argument, that this should be done with a
> > new API in videobuf2.

That is one part of the argument. I'm glad to understand now that we
agree about this.

> > And I'm actually in total agreement with that. I definitely agree that there
> > should be a mechanism in the media framework that allows passing video
> > buffers from a source pad to a sink pad using a software queue, with no
> > involvement from userland.

That is the other part of the argument. I do not agree that these
software queue "links" should be presented to userspace as media pad
links between two entities of a media device. 

First, that would limit the links to subdevices contained in the same
media graph, while this should work between any two capture and output
queues of different devices.
Assume for example, we want to encode the captured, deinterlaced video
to h.264 with the coda VPU driver. A software queue link could be
established between the CSI capture and the VDIC deinterlacer input,
just as between the VDIC deinterlacer output and the coda VPU input.
Technically, there would be no difference between those two linked
capture/output queue pairs. But the coda driver is a completely separate
mem2mem device. And since it is not part of the i.MX media graph, there
is no entity pad to link to.
Or assume there is an USB analog capture device that produces interlaced
frames. I think it should be possible to connect its capture queue to
the VDIC deinterlacer output queue just the same way as linking the CSI
to the VDIC (in software queue mode).

Second, the subdevice pad formats describe wire formats, not memory
formats. The user might want to choose between 4:2:2 and 4:2:0
subsampled YUV formats for the intermediate buffer, for example,
depending on memory bandwidth constraints and quality requirements. This
is impossible with the media entity / subdevice pad links.

I think an interface where userspace configures the capture and output
queues via v4l2 API, passes dma buffers around from one to the other
queue, and then puts both queues into a free running mode would be a
much better fit for this mechanism.

> > My only disagreement is when this should be implemented. I think it is
> > fine to keep my custom implementation of this in the driver for now. Once
> > an extension of vb2 is ready to support this feature, it would be fairly
> > straightforward to strip out my custom implementation and go with the
> > new API.
> 
> For a staging driver this isn't necessary, as long as it is documented in
> the TODO file that this needs to be fixed before it can be moved out of
> staging. The whole point of staging is that there is still work to be
> done in the driver, after all :-)

Absolutely. The reason I am arguing against merging the mem2mem media
control links so vehemently is that I am convinced the userspace
interface is wrong, and I am afraid that even though in staging, it
might become established.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1564874

On 01/23/2017 12:00 PM, Philipp Zabel wrote:
> On Fri, 2017-01-20 at 21:39 +0100, Hans Verkuil wrote:
> [...]
>>> There is a VDIC entity in the i.MX IPU that performs de-interlacing with
>>> hardware filters for motion compensation. Some of the motion compensation
>>> modes ("low" and "medium" motion) require that the VDIC receive video
>>> frame fields from memory buffers (dedicated dma channels in the
>>> IPU are used to transfer those buffers into the VDIC).
>>>
>>> So one option to support those modes would be to pass the raw buffers
>>> from a camera sensor up to userspace to a capture device, and then pass
>>> them back to the VDIC for de-interlacing using a mem2mem device.
>>>
>>> Philipp and I are both in agreement that, since userland is not interested
>>> in the intermediate interlaced buffers in this case, but only the final
>>> result (motion compensated, de-interlaced frames), it is more efficient
>>> to provide a media link that allows passing those intermediate frames
>>> directly from a camera source pad to VDIC sink pad, without having
>>> to route them through userspace.
>>>
>>> So in order to support that, I've implemented a simple FIFO dma buffer
>>> queue in the driver to allow passing video buffers directly from a source
>>> to a sink. It is modeled loosely off the vb2 state machine and API, but
>>> simpler (for instance it only allows contiguous, cache-coherent buffers).
>>>
>>> This is where Philipp has an argument, that this should be done with a
>>> new API in videobuf2.
> 
> That is one part of the argument. I'm glad to understand now that we
> agree about this.
> 
>>> And I'm actually in total agreement with that. I definitely agree that there
>>> should be a mechanism in the media framework that allows passing video
>>> buffers from a source pad to a sink pad using a software queue, with no
>>> involvement from userland.
> 
> That is the other part of the argument. I do not agree that these
> software queue "links" should be presented to userspace as media pad
> links between two entities of a media device. 
> 
> First, that would limit the links to subdevices contained in the same
> media graph, while this should work between any two capture and output
> queues of different devices.
> Assume for example, we want to encode the captured, deinterlaced video
> to h.264 with the coda VPU driver. A software queue link could be
> established between the CSI capture and the VDIC deinterlacer input,
> just as between the VDIC deinterlacer output and the coda VPU input.
> Technically, there would be no difference between those two linked
> capture/output queue pairs. But the coda driver is a completely separate
> mem2mem device. And since it is not part of the i.MX media graph, there
> is no entity pad to link to.
> Or assume there is an USB analog capture device that produces interlaced
> frames. I think it should be possible to connect its capture queue to
> the VDIC deinterlacer output queue just the same way as linking the CSI
> to the VDIC (in software queue mode).
> 
> Second, the subdevice pad formats describe wire formats, not memory
> formats. The user might want to choose between 4:2:2 and 4:2:0
> subsampled YUV formats for the intermediate buffer, for example,
> depending on memory bandwidth constraints and quality requirements. This
> is impossible with the media entity / subdevice pad links.
> 
> I think an interface where userspace configures the capture and output
> queues via v4l2 API, passes dma buffers around from one to the other
> queue, and then puts both queues into a free running mode would be a
> much better fit for this mechanism.
> 
>>> My only disagreement is when this should be implemented. I think it is
>>> fine to keep my custom implementation of this in the driver for now. Once
>>> an extension of vb2 is ready to support this feature, it would be fairly
>>> straightforward to strip out my custom implementation and go with the
>>> new API.
>>
>> For a staging driver this isn't necessary, as long as it is documented in
>> the TODO file that this needs to be fixed before it can be moved out of
>> staging. The whole point of staging is that there is still work to be
>> done in the driver, after all :-)
> 
> Absolutely. The reason I am arguing against merging the mem2mem media
> control links so vehemently is that I am convinced the userspace
> interface is wrong, and I am afraid that even though in staging, it
> might become established.

As long as it is mentioned in the TODO, and ideally in the Kconfig as well,
then I'm fine with it.

The big advantage of being in the kernel is that it is much easier to start
providing fixes, improvements, etc. If you use a staging driver you know
that there is no guarantee whatsoever with respect to stable ABI/APIs.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1565791

On Mon, 2017-01-23 at 12:08 +0100, Hans Verkuil wrote:
> On 01/23/2017 12:00 PM, Philipp Zabel wrote:
> > On Fri, 2017-01-20 at 21:39 +0100, Hans Verkuil wrote:
[...]
> As long as it is mentioned in the TODO, and ideally in the Kconfig as well,
> then I'm fine with it.
>
> The big advantage of being in the kernel is that it is much easier to start
> providing fixes, improvements, etc. If you use a staging driver you know
> that there is no guarantee whatsoever with respect to stable ABI/APIs.

Of course, but there should be a clear way how to progress on those
issues that are documented as blockers, otherwise the driver will linger
in staging.
Worse, currently we are not even in agreement what to put into the TODO.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1565360

On 01/23/2017 03:00 AM, Philipp Zabel wrote:
> On Fri, 2017-01-20 at 21:39 +0100, Hans Verkuil wrote:
> [...]
>>> There is a VDIC entity in the i.MX IPU that performs de-interlacing with
>>> hardware filters for motion compensation. Some of the motion compensation
>>> modes ("low" and "medium" motion) require that the VDIC receive video
>>> frame fields from memory buffers (dedicated dma channels in the
>>> IPU are used to transfer those buffers into the VDIC).
>>>
>>> So one option to support those modes would be to pass the raw buffers
>>> from a camera sensor up to userspace to a capture device, and then pass
>>> them back to the VDIC for de-interlacing using a mem2mem device.
>>>
>>> Philipp and I are both in agreement that, since userland is not interested
>>> in the intermediate interlaced buffers in this case, but only the final
>>> result (motion compensated, de-interlaced frames), it is more efficient
>>> to provide a media link that allows passing those intermediate frames
>>> directly from a camera source pad to VDIC sink pad, without having
>>> to route them through userspace.
>>>
>>> So in order to support that, I've implemented a simple FIFO dma buffer
>>> queue in the driver to allow passing video buffers directly from a source
>>> to a sink. It is modeled loosely off the vb2 state machine and API, but
>>> simpler (for instance it only allows contiguous, cache-coherent buffers).
>>>
>>> This is where Philipp has an argument, that this should be done with a
>>> new API in videobuf2.
> That is one part of the argument. I'm glad to understand now that we
> agree about this.
>
>>> And I'm actually in total agreement with that. I definitely agree that there
>>> should be a mechanism in the media framework that allows passing video
>>> buffers from a source pad to a sink pad using a software queue, with no
>>> involvement from userland.
> That is the other part of the argument. I do not agree that these
> software queue "links" should be presented to userspace as media pad
> links between two entities of a media device.
> First, that would limit the links to subdevices contained in the same
> media graph, while this should work between any two capture and output
> queues of different devices.

It sounds like we are talking about two different new proposed features.

My proposal is to implement a software buffer queue between pads.
Beyond enabling the link between pads using the existing media controller
API, userspace is not involved after that. The fact that this link is 
accomplished
with a software buffer queue is not known, and doesn't need to be known,
by userspace.

Your proposal, if I have it right, is to allow linking two v4l2 device 
vb2 queues
(i.e. /dev/videoX -> /dev/videoY), using a new user level API, in a free-run
mode such that v4l2 buffers get passed from one device's vb2 queue to the
other without requiring the v4l2 user program to actively forward those 
buffers.

There isn't anything that would preclude one from the other, they can
both exist. But they are different ideas. One implements software queues
at the _pad level_ and is opaque to userspace, the other links queues
at the _device level_ using a new user API, but once the link is 
established,
also does not require any involvement from userspace.

What I'm saying is we can do _both_.


> Assume for example, we want to encode the captured, deinterlaced video
> to h.264 with the coda VPU driver. A software queue link could be
> established between the CSI capture and the VDIC deinterlacer input,

That's already available in the media graph. By linking CSI and
VDIC entities. The capture device will then already be providing
de-interlaced video, and ...

> just as between the VDIC deinterlacer output and the coda VPU input.
> Technically, there would be no difference between those two linked
> capture/output queue pairs. But the coda driver is a completely separate
> mem2mem device. And since it is not part of the i.MX media graph, there
> is no entity pad to link to.

your free-run queue linking could then be used to link the (already)
de-interlaced stream to the coda device for h.264 encode.

The other idea would be to eventually make the coda device part of
the media graph as an entity. Then this link would instead be via pads.

> Or assume there is an USB analog capture device that produces interlaced
> frames. I think it should be possible to connect its capture queue to
> the VDIC deinterlacer output queue just the same way as linking the CSI
> to the VDIC (in software queue mode).

Right, for devices that are outside the i.MX media graph, such as a USB
capture device (or coda), access to the i.MX entities such as the VDIC would
require an i.MX mem2mem device with media links to the VDIC. The USB
capture device would forward its captured frames to mem2mem (maybe
using your free-run vb2 queue linking idea):

usb device -> i.mx mem2mem device -> VDIC entity -> i.mx mem2mem device



> Second, the subdevice pad formats describe wire formats, not memory
> formats. The user might want to choose between 4:2:2 and 4:2:0
> subsampled YUV formats for the intermediate buffer, for example,
> depending on memory bandwidth constraints and quality requirements. This
> is impossible with the media entity / subdevice pad links.

It's true that there are currently no defined planar media bus
pixel formats. We just need to add new definitions for them. Once
that is done, the media driver will support planar YUV formats
simply by adding the new codes to imx_media_formats[].

Perhaps this gets to the root of the issue.

Is the media bus concept an abstract one, or is the media bus
intended to represent actual physical buses (as the lack of planar
media bus formats would imply)?

Can we break with the physical-bus-only idea if that is the case, and
loosen the definition of a media bus to mean the passage of media
data from one pad to another by whatever means?

In my view the idea of a physical bus at the sensor makes sense, but
beyond that, keeping that restriction limits how data can pass between
pads.

Hans, any input here?

If this is really anathema, then I'm willing to remove the software queues
between pads, but it will be giving up some functionality in the media 
driver.
It would also mean splitting the VDIC in two. The VDIC entity would be 
limited
to only one motion compensation mode, and the full functionality would have
to be added somewhere else. Currently all functionality of the VDIC is
implemented in a single media entity.

> I think an interface where userspace configures the capture and output
> queues via v4l2 API, passes dma buffers around from one to the other
> queue, and then puts both queues into a free running mode would be a
> much better fit for this mechanism.

As I said, I see these as two different ideas that can both be
implemented.

>>> My only disagreement is when this should be implemented. I think it is
>>> fine to keep my custom implementation of this in the driver for now. Once
>>> an extension of vb2 is ready to support this feature, it would be fairly
>>> straightforward to strip out my custom implementation and go with the
>>> new API.
>> For a staging driver this isn't necessary, as long as it is documented in
>> the TODO file that this needs to be fixed before it can be moved out of
>> staging. The whole point of staging is that there is still work to be
>> done in the driver, after all :-)
> Absolutely. The reason I am arguing against merging the mem2mem media
> control links so vehemently is that I am convinced the userspace
> interface is wrong, and I am afraid that even though in staging, it
> might become established.

I don't believe there is anything wrong with the userspace interface,
In fact it hasn't even changed. The fact two pads are passing memory
buffers is "under the hood".

Steve

[toc] | [prev] | [next] | [standalone]

#1565790

Hi Steve, Hans,

[added Laurent to Cc: who I believe might have an opinion on the media
bus formats, too. Sorry for the wall of text, I have put a marker where
the MEDIA_BUS argument starts]

The central issue seems to be that I think media pad links / media bus
formats should describe physical links, such as parallel or serial
buses, and the formats of pixels flowing through them, whereas Steve
would like to extend them to describe software transports and in-memory
formats.

On Mon, 2017-01-23 at 15:08 -0800, Steve Longerbeam wrote:
[...]
> >>> And I'm actually in total agreement with that. I definitely agree that there
> >>> should be a mechanism in the media framework that allows passing video
> >>> buffers from a source pad to a sink pad using a software queue, with no
> >>> involvement from userland.
> > That is the other part of the argument. I do not agree that these
> > software queue "links" should be presented to userspace as media pad
> > links between two entities of a media device.
> > First, that would limit the links to subdevices contained in the same
> > media graph, while this should work between any two capture and output
> > queues of different devices.
> 
> It sounds like we are talking about two different new proposed features.

We are talking about the same thing, but we both want a different user
interface.
Technically, the issue is to trigger the DMA read channel of a mem2mem
device automatically whenever another capture device's DMA write channel
signals a finished frame. Where we disagree is how to present this to
userspace.

You represent the capture DMA write channel and mem2mem DMA read channel
as pads on media entites and configure the in-kernel software queue
between the two using a media pad link. At the same time a different
representation of the same DMA write and read channels (the capture
vb2_queue of the capture device and the output vb2_queue of the mem2mem
device) would be used for operation in the classic, userspace controlled
mode via dmabuf passing.

I don't want the software-only link in the media graph, but instead use
the vb2_queue representation for both cases, and implement the in-kernel
queue link on top of the vb2_queue interface. This would allow userspace
to have control over buffer allocations and format, and thus avoid
unexpected performance implications: it is impossible for userspace to
understand which media entity link, when enabled, will cause a
significant increase in memory bandwidth usage, or even how much.
Also the same mechanism could then be used to link any two devices in a
generic manner, instead of special casing the software queue link for
two devices that happen to be part of the same media graph.

> My proposal is to implement a software buffer queue between pads.
> Beyond enabling the link between pads using the existing media controller
> API, userspace is not involved after that. The fact that this link is 
> accomplished with a software buffer queue is not known, and doesn't
> need to be known, by userspace.

I don't think this is a good thing for the reasons stated above and
below:
Since the software buffer queue is opaque to userspace, it is completely
out of userspace control which format is chosen and how the buffers are
allocated.
By using media bus formats to configure software links the kernel
pretends to userspace that there is a physical connection where there
isn't one.
Also, the media entity graph would quickly become very unreadable if we
were to add all devices to it that could reasonably be linked with
software queues.

> Your proposal, if I have it right, is to allow linking two v4l2 device 
> vb2 queues
> (i.e. /dev/videoX -> /dev/videoY), using a new user level API, in a free-run
> mode such that v4l2 buffers get passed from one device's vb2 queue to the
> other without requiring the v4l2 user program to actively forward those 
> buffers.

Yes.

> There isn't anything that would preclude one from the other, they can
> both exist. But they are different ideas. One implements software queues
> at the _pad level_ and is opaque to userspace, the other links queues
> at the _device level_ using a new user API, but once the link is 
> established, also does not require any involvement from userspace.

Well, they are different ideas of how the userspace interface _for the
same thing_ should look like.

> What I'm saying is we can do _both_.

What I am saying is we shouldn't do the pad link interface for the
software queues. In my opinion it is the wrong abstraction, and apart
from the convenience of being able to switch the links on with a single
media-ctl invocation, I see too many downsides.

> > Assume for example, we want to encode the captured, deinterlaced video
> > to h.264 with the coda VPU driver. A software queue link could be
> > established between the CSI capture and the VDIC deinterlacer input,
> 
> That's already available in the media graph. By linking CSI and
> VDIC entities. The capture device will then already be providing
> de-interlaced video, and ...

I know it is in your code. That is the cause for my concern. The link
between CSI and VDIC entity should only describe the direct physical
connection through the VDIC FIFO1 in my opinion.
For the indirect CSI -> SMFC -> IDMAC -> RAM, RAM -> IDMAC -> VDIC
software queue, I would strongly prefer to use linked vb2_queues instead
of the media entity link.

> > just as between the VDIC deinterlacer output and the coda VPU input.
> > Technically, there would be no difference between those two linked
> > capture/output queue pairs. But the coda driver is a completely separate
> > mem2mem device. And since it is not part of the i.MX media graph, there
> > is no entity pad to link to.
> 
> your free-run queue linking could then be used to link the (already)
> de-interlaced stream to the coda device for h.264 encode.

Yes, and I see no reason why that should use a different interface than
what is exactly the same process between CSI and VDIC.

> The other idea would be to eventually make the coda device part of
> the media graph as an entity. Then this link would instead be via pads.

I would only want to do this if there was a direct connection between
the IPU FIFOs and the coda VPU device somehow. But since the devices are
completely separate, they should be described as such.

> > Or assume there is an USB analog capture device that produces interlaced
> > frames. I think it should be possible to connect its capture queue to
> > the VDIC deinterlacer output queue just the same way as linking the CSI
> > to the VDIC (in software queue mode).
> 
> Right, for devices that are outside the i.MX media graph, such as a USB
> capture device (or coda), access to the i.MX entities such as the VDIC would
> require an i.MX mem2mem device with media links to the VDIC. The USB
> capture device would forward its captured frames to mem2mem (maybe
> using your free-run vb2 queue linking idea):
> 
> usb device -> i.mx mem2mem device -> VDIC entity -> i.mx mem2mem device

The VDIC doesn't have a direct to memory channel, so that would be
mem2mem -> VDIC -> IC -> mem2mem, I think?

========== MEDIA_BUS formats below =====================================

> > Second, the subdevice pad formats describe wire formats, not memory
> > formats. The user might want to choose between 4:2:2 and 4:2:0
> > subsampled YUV formats for the intermediate buffer, for example,
> > depending on memory bandwidth constraints and quality requirements. This
> > is impossible with the media entity / subdevice pad links.
> 
> It's true that there are currently no defined planar media bus
> pixel formats. We just need to add new definitions for them. Once
> that is done, the media driver will support planar YUV formats
> simply by adding the new codes to imx_media_formats[].

I am not comfortable with starting to mix MEDIA_BUS_FMT and V4L2_PIX_FMT
this way.

> Perhaps this gets to the root of the issue.
> 
> Is the media bus concept an abstract one, or is the media bus
> intended to represent actual physical buses (as the lack of planar
> media bus formats would imply)?

Yes, maybe that is the root of our disconnect. As I understand it, the
media bus formats are describing "image formats as flowing over physical
busses" [1].

[1] Linux Media Subsystem Documentation, Chapter 4.15.3.4.1.1. Media Bus Pixel Codes
    https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/subdev-formats.html?highlight=media%20bus#v4l2-mbus-pixelcode

I would like to keep it that way and not soften up that description.

> Can we break with the physical-bus-only idea if that is the case, and
> loosen the definition of a media bus to mean the passage of media
> data from one pad to another by whatever means?
> 
> In my view the idea of a physical bus at the sensor makes sense, but
> beyond that, keeping that restriction limits how data can pass between
> pads.
> 
> Hans, any input here?
> 
> If this is really anathema, then I'm willing to remove the software queues
> between pads, but it will be giving up some functionality in the media 
> driver.

So far I am the only one arguing against this, and I haven't yet heard
anything yet that would have convinced me otherwise. That's why I'd too
like some more input on this issue.

Certainly removing the controversial pad link controlled software queues
would remove the point of contention. I think losing some functionality
(in this case "higher quality" deinterlacing without userspace
intervention) for now would be worth it to achieve consensus, and also
reduce the list of things that have to be done for this driver to leave
staging, but that is of course from the point of view of the guy arguing
against that interface.

> It would also mean splitting the VDIC in two. The VDIC entity would be
> limited to only one motion compensation mode,

Currently, yes. That direct mode is the only one that should be
described by the media pad link between CSI and VDIC in my opinion.

> and the full functionality would have
> to be added somewhere else.

I don't understand why the other functionality would necessarily have to
live somewere else, but I can see that it might make sense to do so. In
any case, the separate control of the VDIC/IC via a mem2mem video device
will be needed anyway, as pointed out in the USB example above, or to
deinterlace streams received via network or played back from files.

>  Currently all functionality of the VDIC is implemented in a single media entity.

Yes, at least the mem2mem part should move into its own mem2mem video
device.

> > I think an interface where userspace configures the capture and output
> > queues via v4l2 API, passes dma buffers around from one to the other
> > queue, and then puts both queues into a free running mode would be a
> > much better fit for this mechanism.
> 
> As I said, I see these as two different ideas that can both be
> implemented.

I think we should have cleared up now where we disagree. These are two
different ideas for userspace interfaces for the same functionality. My
opinion is that both should not be implemented

> >>> My only disagreement is when this should be implemented. I think it is
> >>> fine to keep my custom implementation of this in the driver for now. Once
> >>> an extension of vb2 is ready to support this feature, it would be fairly
> >>> straightforward to strip out my custom implementation and go with the
> >>> new API.
> >> For a staging driver this isn't necessary, as long as it is documented in
> >> the TODO file that this needs to be fixed before it can be moved out of
> >> staging. The whole point of staging is that there is still work to be
> >> done in the driver, after all :-)
> > Absolutely. The reason I am arguing against merging the mem2mem media
> > control links so vehemently is that I am convinced the userspace
> > interface is wrong, and I am afraid that even though in staging, it
> > might become established.
> 
> I don't believe there is anything wrong with the userspace interface,
> In fact it hasn't even changed. The fact two pads are passing memory
> buffers is "under the hood".

Which I disagree with. Doing things like this under the hood would be
fine only if they were properly introspectable and if the interface
wouldn't break assumptions that I believe to be there, such as media
links describing physical connections between hardware entities, and
media bus formats describing the image format on a physical bus.
Also with videobuf2 we already have a userspace interface for DMA read
and write queues, and I'd prefer to extend and improve that instead of
reimplementing the same functionality, even simplified, under the hood.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1568999

On 01/24/2017 03:27 AM, Philipp Zabel wrote:
> Hi Steve, Hans,
>
> [added Laurent to Cc: who I believe might have an opinion on the media
> bus formats, too. Sorry for the wall of text, I have put a marker where
> the MEDIA_BUS argument starts]
>
> The central issue seems to be that I think media pad links / media bus
> formats should describe physical links, such as parallel or serial
> buses, and the formats of pixels flowing through them, whereas Steve
> would like to extend them to describe software transports and in-memory
> formats.

Hi Philipp, Hans,

I've decided to pull the dma read/write channel linking between
pads. Although I haven't heard any other feedback yet, I agree it is
controversial and it is fairly clear it violates the current media bus
concept that describes physical links.

So the VDIC entity will support only the high motion mode, and
the post-processor entities will be removed. We've talked in the
past of adding full VDIC support to the IPU image conversion API
in the IPUv3 driver, so the complete motion compensation modes
can live there eventually, accessed from a mem2mem device. I'm
also considering leaving the low/medium motion support in the VDIC
entity, but accessed eventually from a separate device node sink pad
(from a future output device), instead of from a subdev pad.

So now there will be four capture device nodes per IPU: two linked
directly from each CSI IDMAC source pad, one from the prp-encode
source pad, and one from the prp-viewfinder source pad.

I will post version 4 in a couple days.

Steve


> On Mon, 2017-01-23 at 15:08 -0800, Steve Longerbeam wrote:
> [...]
>>>>> And I'm actually in total agreement with that. I definitely agree that there
>>>>> should be a mechanism in the media framework that allows passing video
>>>>> buffers from a source pad to a sink pad using a software queue, with no
>>>>> involvement from userland.
>>> That is the other part of the argument. I do not agree that these
>>> software queue "links" should be presented to userspace as media pad
>>> links between two entities of a media device.
>>> First, that would limit the links to subdevices contained in the same
>>> media graph, while this should work between any two capture and output
>>> queues of different devices.
>> It sounds like we are talking about two different new proposed features.
> We are talking about the same thing, but we both want a different user
> interface.
> Technically, the issue is to trigger the DMA read channel of a mem2mem
> device automatically whenever another capture device's DMA write channel
> signals a finished frame. Where we disagree is how to present this to
> userspace.
>
> You represent the capture DMA write channel and mem2mem DMA read channel
> as pads on media entites and configure the in-kernel software queue
> between the two using a media pad link. At the same time a different
> representation of the same DMA write and read channels (the capture
> vb2_queue of the capture device and the output vb2_queue of the mem2mem
> device) would be used for operation in the classic, userspace controlled
> mode via dmabuf passing.
>
> I don't want the software-only link in the media graph, but instead use
> the vb2_queue representation for both cases, and implement the in-kernel
> queue link on top of the vb2_queue interface. This would allow userspace
> to have control over buffer allocations and format, and thus avoid
> unexpected performance implications: it is impossible for userspace to
> understand which media entity link, when enabled, will cause a
> significant increase in memory bandwidth usage, or even how much.
> Also the same mechanism could then be used to link any two devices in a
> generic manner, instead of special casing the software queue link for
> two devices that happen to be part of the same media graph.
>
>> My proposal is to implement a software buffer queue between pads.
>> Beyond enabling the link between pads using the existing media controller
>> API, userspace is not involved after that. The fact that this link is
>> accomplished with a software buffer queue is not known, and doesn't
>> need to be known, by userspace.
> I don't think this is a good thing for the reasons stated above and
> below:
> Since the software buffer queue is opaque to userspace, it is completely
> out of userspace control which format is chosen and how the buffers are
> allocated.
> By using media bus formats to configure software links the kernel
> pretends to userspace that there is a physical connection where there
> isn't one.
> Also, the media entity graph would quickly become very unreadable if we
> were to add all devices to it that could reasonably be linked with
> software queues.
>
>> Your proposal, if I have it right, is to allow linking two v4l2 device
>> vb2 queues
>> (i.e. /dev/videoX -> /dev/videoY), using a new user level API, in a free-run
>> mode such that v4l2 buffers get passed from one device's vb2 queue to the
>> other without requiring the v4l2 user program to actively forward those
>> buffers.
> Yes.
>
>> There isn't anything that would preclude one from the other, they can
>> both exist. But they are different ideas. One implements software queues
>> at the _pad level_ and is opaque to userspace, the other links queues
>> at the _device level_ using a new user API, but once the link is
>> established, also does not require any involvement from userspace.
> Well, they are different ideas of how the userspace interface _for the
> same thing_ should look like.
>
>> What I'm saying is we can do _both_.
> What I am saying is we shouldn't do the pad link interface for the
> software queues. In my opinion it is the wrong abstraction, and apart
> from the convenience of being able to switch the links on with a single
> media-ctl invocation, I see too many downsides.
>
>>> Assume for example, we want to encode the captured, deinterlaced video
>>> to h.264 with the coda VPU driver. A software queue link could be
>>> established between the CSI capture and the VDIC deinterlacer input,
>> That's already available in the media graph. By linking CSI and
>> VDIC entities. The capture device will then already be providing
>> de-interlaced video, and ...
> I know it is in your code. That is the cause for my concern. The link
> between CSI and VDIC entity should only describe the direct physical
> connection through the VDIC FIFO1 in my opinion.
> For the indirect CSI -> SMFC -> IDMAC -> RAM, RAM -> IDMAC -> VDIC
> software queue, I would strongly prefer to use linked vb2_queues instead
> of the media entity link.
>
>>> just as between the VDIC deinterlacer output and the coda VPU input.
>>> Technically, there would be no difference between those two linked
>>> capture/output queue pairs. But the coda driver is a completely separate
>>> mem2mem device. And since it is not part of the i.MX media graph, there
>>> is no entity pad to link to.
>> your free-run queue linking could then be used to link the (already)
>> de-interlaced stream to the coda device for h.264 encode.
> Yes, and I see no reason why that should use a different interface than
> what is exactly the same process between CSI and VDIC.
>
>> The other idea would be to eventually make the coda device part of
>> the media graph as an entity. Then this link would instead be via pads.
> I would only want to do this if there was a direct connection between
> the IPU FIFOs and the coda VPU device somehow. But since the devices are
> completely separate, they should be described as such.
>
>>> Or assume there is an USB analog capture device that produces interlaced
>>> frames. I think it should be possible to connect its capture queue to
>>> the VDIC deinterlacer output queue just the same way as linking the CSI
>>> to the VDIC (in software queue mode).
>> Right, for devices that are outside the i.MX media graph, such as a USB
>> capture device (or coda), access to the i.MX entities such as the VDIC would
>> require an i.MX mem2mem device with media links to the VDIC. The USB
>> capture device would forward its captured frames to mem2mem (maybe
>> using your free-run vb2 queue linking idea):
>>
>> usb device -> i.mx mem2mem device -> VDIC entity -> i.mx mem2mem device
> The VDIC doesn't have a direct to memory channel, so that would be
> mem2mem -> VDIC -> IC -> mem2mem, I think?
>
> ========== MEDIA_BUS formats below =====================================
>
>>> Second, the subdevice pad formats describe wire formats, not memory
>>> formats. The user might want to choose between 4:2:2 and 4:2:0
>>> subsampled YUV formats for the intermediate buffer, for example,
>>> depending on memory bandwidth constraints and quality requirements. This
>>> is impossible with the media entity / subdevice pad links.
>> It's true that there are currently no defined planar media bus
>> pixel formats. We just need to add new definitions for them. Once
>> that is done, the media driver will support planar YUV formats
>> simply by adding the new codes to imx_media_formats[].
> I am not comfortable with starting to mix MEDIA_BUS_FMT and V4L2_PIX_FMT
> this way.
>
>> Perhaps this gets to the root of the issue.
>>
>> Is the media bus concept an abstract one, or is the media bus
>> intended to represent actual physical buses (as the lack of planar
>> media bus formats would imply)?
> Yes, maybe that is the root of our disconnect. As I understand it, the
> media bus formats are describing "image formats as flowing over physical
> busses" [1].
>
> [1] Linux Media Subsystem Documentation, Chapter 4.15.3.4.1.1. Media Bus Pixel Codes
>      https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/subdev-formats.html?highlight=media%20bus#v4l2-mbus-pixelcode
>
> I would like to keep it that way and not soften up that description.
>
>> Can we break with the physical-bus-only idea if that is the case, and
>> loosen the definition of a media bus to mean the passage of media
>> data from one pad to another by whatever means?
>>
>> In my view the idea of a physical bus at the sensor makes sense, but
>> beyond that, keeping that restriction limits how data can pass between
>> pads.
>>
>> Hans, any input here?
>>
>> If this is really anathema, then I'm willing to remove the software queues
>> between pads, but it will be giving up some functionality in the media
>> driver.
> So far I am the only one arguing against this, and I haven't yet heard
> anything yet that would have convinced me otherwise. That's why I'd too
> like some more input on this issue.
>
> Certainly removing the controversial pad link controlled software queues
> would remove the point of contention. I think losing some functionality
> (in this case "higher quality" deinterlacing without userspace
> intervention) for now would be worth it to achieve consensus, and also
> reduce the list of things that have to be done for this driver to leave
> staging, but that is of course from the point of view of the guy arguing
> against that interface.
>
>> It would also mean splitting the VDIC in two. The VDIC entity would be
>> limited to only one motion compensation mode,
> Currently, yes. That direct mode is the only one that should be
> described by the media pad link between CSI and VDIC in my opinion.
>
>> and the full functionality would have
>> to be added somewhere else.
> I don't understand why the other functionality would necessarily have to
> live somewere else, but I can see that it might make sense to do so. In
> any case, the separate control of the VDIC/IC via a mem2mem video device
> will be needed anyway, as pointed out in the USB example above, or to
> deinterlace streams received via network or played back from files.
>
>>   Currently all functionality of the VDIC is implemented in a single media entity.
> Yes, at least the mem2mem part should move into its own mem2mem video
> device.
>
>>> I think an interface where userspace configures the capture and output
>>> queues via v4l2 API, passes dma buffers around from one to the other
>>> queue, and then puts both queues into a free running mode would be a
>>> much better fit for this mechanism.
>> As I said, I see these as two different ideas that can both be
>> implemented.
> I think we should have cleared up now where we disagree. These are two
> different ideas for userspace interfaces for the same functionality. My
> opinion is that both should not be implemented
>
>>>>> My only disagreement is when this should be implemented. I think it is
>>>>> fine to keep my custom implementation of this in the driver for now. Once
>>>>> an extension of vb2 is ready to support this feature, it would be fairly
>>>>> straightforward to strip out my custom implementation and go with the
>>>>> new API.
>>>> For a staging driver this isn't necessary, as long as it is documented in
>>>> the TODO file that this needs to be fixed before it can be moved out of
>>>> staging. The whole point of staging is that there is still work to be
>>>> done in the driver, after all :-)
>>> Absolutely. The reason I am arguing against merging the mem2mem media
>>> control links so vehemently is that I am convinced the userspace
>>> interface is wrong, and I am afraid that even though in staging, it
>>> might become established.
>> I don't believe there is anything wrong with the userspace interface,
>> In fact it hasn't even changed. The fact two pads are passing memory
>> buffers is "under the hood".
> Which I disagree with. Doing things like this under the hood would be
> fine only if they were properly introspectable and if the interface
> wouldn't break assumptions that I believe to be there, such as media
> links describing physical connections between hardware entities, and
> media bus formats describing the image format on a physical bus.
> Also with videobuf2 we already have a userspace interface for DMA read
> and write queues, and I'd prefer to extend and improve that instead of
> reimplementing the same functionality, even simplified, under the hood.
>
> regards
> Philipp
>

[toc] | [prev] | [next] | [standalone]

#1569720

> The central issue seems to be that I think media pad links / media bus
> formats should describe physical links, such as parallel or serial
> buses, and the formats of pixels flowing through them, whereas Steve
> would like to extend them to describe software transports and in-memory
> formats.

This probably isn't the right place to attach this comment in this
thread, but... the issue of media bus formats matching physical buses
is an argument that I think is already lost.

For example, take the 10-bit bayer formats:

#define MEDIA_BUS_FMT_SBGGR10_1X10              0x3007
#define MEDIA_BUS_FMT_SGBRG10_1X10              0x300e
#define MEDIA_BUS_FMT_SGRBG10_1X10              0x300a
#define MEDIA_BUS_FMT_SRGGB10_1X10              0x300f

These are commonly used on CSI serial buses (see the smiapp driver for
example).  From the description at the top of the file, it says the
1X10 means that one pixel is transferred as one 10-bit sample.

However, the format on wire is somewhat different - four pixels are
transmitted over five bytes:

	P0	P1	P2	P3	P0	P1	P2	P3
	8-bit	8-bit	8-bit	8-bit	2-bit	2-bit	2-bit	2-bit

This gives two problems:
1) it doesn't fit in any sensible kind of "one pixel transferred as
   N M-bit samples" description because the pixel/sample values
   (depending how you look at them) are broken up.

2) changing this will probably be a user visible change, as things
   like smiapp are already in use.

So, I think what we actually have is the media bus formats describing
the _logical_ bus format.  Yes, one pixel is transferred as one 10-bit
sample in this case.

To help illustrate my point, consider the difference between
MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or
MEDIA_BUS_FMT_RGB565_2X8_LE.  RGB565_1X16 means 1 pixel over an effective
16-bit wide bus (if it's not 16-bit, then it has to be broken up into
separate "samples".)  RGB565_2X8 means 1 pixel as two 8-bit samples.

So, the 10-bit bayer is 1 pixel as 1.25 bytes.  Or is it, over a serial
bus.  Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes
interesting:

	first byte	2nd	3rd
lane 1	P0 9:2		S0	P7 9:2
lane 2	P1 9:2		P4 9:2	S1
lane 3	P2 9:2		P5 9:2	P8 9:2
lane 4	P3 9:2		P6 9:2	P9 9:2

S0 = P0/P1/P2/P3 least significant two bits
S1 = P4/P5/P6/P7 least significant two bits

or 2 lane CSI:
	first byte	2nd	3rd	4th	5th
lane 1	P0 9:2		P2	S0	P5	P7
lane 2	P1 9:2		P3	P4	P6	S1

or 1 lane CSI:
lane 1	P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ...

etc.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web