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

Groups > linux.kernel > #1603846 > unrolled thread

Re: [PATCH v5 00/39] 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 v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-18 20:30 +0100
    Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <steve_longerbeam@mentor.com> - 2017-03-18 21:00 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-18 21:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 01:50 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 02:10 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 16:00 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 16:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 11:00 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 15:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-03-19 15:00 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 15:30 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-03-19 16:10 +0100
              Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 16:20 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 15:30 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 15:50 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 11:50 +0100
        [PATCH 2/4] media: imx: allow bayer pixel formats to be looked up Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 11:50 +0100
          Re: [PATCH 2/4] media: imx: allow bayer pixel formats to be looked up Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:20 +0100
        [PATCH 1/4] media: imx-media-csi: fix v4l2-compliance check Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 11:50 +0100
          Re: [PATCH 1/4] media: imx-media-csi: fix v4l2-compliance check Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:20 +0100
        [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 12:00 +0100
          Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:30 +0100
            Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 23:50 +0100
          Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval         enumeration Philippe De Muyter <phdm@macq.eu> - 2017-03-20 10:00 +0100
            Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 10:10 +0100
              Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval         enumeration Philippe De Muyter <phdm@macq.eu> - 2017-03-20 10:30 +0100
                Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 11:50 +0100
        [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 12:10 +0100
          Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:30 +0100
          Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-22 00:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 19:00 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 19:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 14:10 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 14:40 +0100
              Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 15:00 +0100
                Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 15:20 +0100
                  Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 17:10 +0100
                    Re: [PATCH v5 00/39] i.MX Media Driver "Niklas Söderlund" <niklas.soderlund@ragnatech.se> - 2017-03-21 11:50 +0100
                      Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-21 12:10 +0100
                        Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-21 12:50 +0100
                          Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-22 19:20 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 13:20 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 20:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 21:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 21:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 14:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 14:30 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 16:50 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 17:40 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 17:40 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 14:20 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1606879

[Multipart message — attachments visible in raw view] — view raw

Le mardi 21 mars 2017 à 11:36 +0000, Russell King - ARM Linux a écrit :
>                 warn: v4l2-test-formats.cpp(1187): S_PARM is
> supported for buftype 2, but not ENUM_FRAMEINTERVALS
>                 warn: v4l2-test-formats.cpp(1194): S_PARM is
> supported but doesn't report V4L2_CAP_TIMEPERFRAME

For encoders, the framerate value is used as numerical value to
implement bitrate control. So in most cases any interval is accepted.
Though, it would be cleaner to just implement the enumeration. It's
quite simple when you support everything.

Nicolas

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

#1603967

On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> >0:00:01.955927879 20954  0x15ffe90 INFO                    v4l2 gstv4l2object.c:3811:gst_v4l2_object_get_caps:<v4l2src0> probed caps: video/x-bayer, format=(string)rggb, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)BGR, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
> >
> >    despite the media pipeline actually being configured for 60fps.
> >
> >    Forcing it by adjusting the pipeline only results in gstreamer
> >    failing, because it believes that v4l2 is unable to operate at
> >    60fps.
> >
> >    Also note the complaints from v4l2src about the non-compliance...
> 
> Thanks, I've fixed most of v4l2-compliance issues, but this is not
> done yet. Is that something you can help with?

I've looked at this, and IMHO it's yet another media control API mess.

- media-ctl itself allows setting the format on subdev pads via
  struct v4l2_subdev_format.

- struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.

- struct v4l2_mbus_framefmt contains:
  * @width:      frame width
  * @height:     frame height
  * @code:       data format code (from enum v4l2_mbus_pixelcode)
  * @field:      used interlacing type (from enum v4l2_field)
  * @colorspace: colorspace of the data (from enum v4l2_colorspace)
  * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)

- media-ctl sets width, height, code and field, but nothing else.

We're already agreed that the fields that media-ctl are part of the
format negotiation between the ultimate source, flowing down to the
capture device.  However, there's no support in media-ctl to deal
with these other fields - so media-ctl in itself is only half-
implemented.

From what I can tell, _we_ are doing the right thing in imx-media-capture.

However, I think part of the problem is the set_fmt implementation.
When a source pad is configured via set_fmt(), any fields that can
not be altered (eg, because the subdev doesn't support colorspace
conversion) need to be preserved from the subdev's sink pad.

Right now, CSI doesn't do that - it only looks at the width, height,
code, and field.

I think we've got other bugs though that haven't been picked up by any
review - csi_try_fmt() adjusts the format using the _current_
configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
This seems wrong according to the docs: the purpose of the try
mechanism is to be able to setup the _entire_ pipeline using the TRY
mechanism to work out whether the configuration works, before then
setting for real.  If we're validating the TRY formats against the
live configuration, then we're not doing that.

There's calls for:

v4l2_subdev_get_try_format
v4l2_subdev_get_try_crop
v4l2_subdev_get_try_compose

to get the try configuration - we hardly make use of all of these.  I
would suggest that we change the approach to implementing the various
subdevs such that:

1) like __csi_get_fmt(), we have accessors that gets a pointer to the
   correct state for the TRY/live settings.

2) everywhere we're asked to get or set parameters that can be TRY/live,
   we use these accessors to retrieve a pointer to the correct state to
   not only read, but also modify.

3) when we're evaluating parameters against another pad, we use these
   accessors to obtain the other pad's configuration, rather than poking
   about in the state saved in the subdev's priv-> (which is irrelevant
   for the TRY variant.)

4) ensure that all parameters which the subdev itself does not support
   modification of are correctly propagated from the sink pad to all
   source pads, and are unable to be modified via the source pad.

-- 
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] | [next] | [standalone]

#1604069

On Sun, Mar 19, 2017 at 11:37:15AM -0700, Steve Longerbeam wrote:
> On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:
> >Right now, CSI doesn't do that - it only looks at the width, height,
> >code, and field.
> 
> Correct, there is currently no propagation of the colorimetry
> parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
> For the most part, those are just ignored ATM. Philipp Zabel did
> do some work earlier to start propagating those, but that's still
> TODO.


> 
> >
> >I think we've got other bugs though that haven't been picked up by any
> >review - csi_try_fmt() adjusts the format using the _current_
> >configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> >This seems wrong according to the docs: the purpose of the try
> >mechanism is to be able to setup the _entire_ pipeline using the TRY
> >mechanism to work out whether the configuration works, before then
> >setting for real.  If we're validating the TRY formats against the
> >live configuration, then we're not doing that.
> 
> I don't believe that is correct. csi_try_fmt() for the source pads calls
> __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
> the sink format, and for the TRY trial-run from csi_set_fmt(),
> sdformat->which will be set to TRY, so the returned sink format
> is the TRY format.

Look at csi_try_fmt() - it validates the source pad against
priv->crop, which is the actively live cropping rectangle, not the
one which has been configured for the TRY trial-run.

Also, as I mention elsewhere, I believe the way we're doing scaling
is completely wrong...

-- 
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] | [next] | [standalone]

#1604072

On 03/19/2017 11:51 AM, Russell King - ARM Linux wrote:
> On Sun, Mar 19, 2017 at 11:37:15AM -0700, Steve Longerbeam wrote:
>> On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:
>>> Right now, CSI doesn't do that - it only looks at the width, height,
>>> code, and field.
>> Correct, there is currently no propagation of the colorimetry
>> parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
>> For the most part, those are just ignored ATM. Philipp Zabel did
>> do some work earlier to start propagating those, but that's still
>> TODO.
>
>>> I think we've got other bugs though that haven't been picked up by any
>>> review - csi_try_fmt() adjusts the format using the _current_
>>> configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
>>> This seems wrong according to the docs: the purpose of the try
>>> mechanism is to be able to setup the _entire_ pipeline using the TRY
>>> mechanism to work out whether the configuration works, before then
>>> setting for real.  If we're validating the TRY formats against the
>>> live configuration, then we're not doing that.
>> I don't believe that is correct. csi_try_fmt() for the source pads calls
>> __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
>> the sink format, and for the TRY trial-run from csi_set_fmt(),
>> sdformat->which will be set to TRY, so the returned sink format
>> is the TRY format.
> Look at csi_try_fmt() - it validates the source pad against
> priv->crop, which is the actively live cropping rectangle, not the
> one which has been configured for the TRY trial-run.

Ah yes, crop, I missed that. Yes you are right, looks like we
need to add a __csi_get_crop().

>
> Also, as I mention elsewhere, I believe the way we're doing scaling
> is completely wrong...

You might be right there too. Initially, I had no support for the 
down-scaling
in the CSI. That was added later by Philipp, I will respond with more 
there...

Steve

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

#1604071

On 03/19/2017 05:14 AM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
>>> 0:00:01.955927879 20954  0x15ffe90 INFO                    v4l2 gstv4l2object.c:3811:gst_v4l2_object_get_caps:<v4l2src0> probed caps: video/x-bayer, format=(string)rggb, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)BGR, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
>>>
>>>     despite the media pipeline actually being configured for 60fps.
>>>
>>>     Forcing it by adjusting the pipeline only results in gstreamer
>>>     failing, because it believes that v4l2 is unable to operate at
>>>     60fps.
>>>
>>>     Also note the complaints from v4l2src about the non-compliance...
>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>> done yet. Is that something you can help with?
> I've looked at this, and IMHO it's yet another media control API mess.
>
> - media-ctl itself allows setting the format on subdev pads via
>    struct v4l2_subdev_format.
>
> - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.
>
> - struct v4l2_mbus_framefmt contains:
>    * @width:      frame width
>    * @height:     frame height
>    * @code:       data format code (from enum v4l2_mbus_pixelcode)
>    * @field:      used interlacing type (from enum v4l2_field)
>    * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>    * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>    * @quantization: quantization of the data (from enum v4l2_quantization)
>    * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
>
> - media-ctl sets width, height, code and field, but nothing else.
>
> We're already agreed that the fields that media-ctl are part of the
> format negotiation between the ultimate source, flowing down to the
> capture device.  However, there's no support in media-ctl to deal
> with these other fields - so media-ctl in itself is only half-
> implemented.
>
>  From what I can tell, _we_ are doing the right thing in imx-media-capture.
>
> However, I think part of the problem is the set_fmt implementation.
> When a source pad is configured via set_fmt(), any fields that can
> not be altered (eg, because the subdev doesn't support colorspace
> conversion) need to be preserved from the subdev's sink pad.
>
> Right now, CSI doesn't do that - it only looks at the width, height,
> code, and field.

Correct, there is currently no propagation of the colorimetry
parameters (colorspace, ycbcr_enc, quantization, and xfer_func).
For the most part, those are just ignored ATM. Philipp Zabel did
do some work earlier to start propagating those, but that's still
TODO.

>
> I think we've got other bugs though that haven't been picked up by any
> review - csi_try_fmt() adjusts the format using the _current_
> configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> This seems wrong according to the docs: the purpose of the try
> mechanism is to be able to setup the _entire_ pipeline using the TRY
> mechanism to work out whether the configuration works, before then
> setting for real.  If we're validating the TRY formats against the
> live configuration, then we're not doing that.

I don't believe that is correct. csi_try_fmt() for the source pads calls
__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which) to get
the sink format, and for the TRY trial-run from csi_set_fmt(),
sdformat->which will be set to TRY, so the returned sink format
is the TRY format.

But I haven't tested a complete pipeline configuration under the
TRY case, there still could be issues there. But I've checked the
CSI, VDIC, and PRPENCVF subdevs, and for set_fmt() trial-runs,
those should be working correctly using the TRY mechanism.


> There's calls for:
>
> v4l2_subdev_get_try_format
> v4l2_subdev_get_try_crop
> v4l2_subdev_get_try_compose
>
> to get the try configuration - we hardly make use of all of these.

Not sure what you mean, the first two are currently
being used for TRY setup. And I don't think
v4l2_subdev_get_try_compose() is needed.

>   I
> would suggest that we change the approach to implementing the various
> subdevs such that:
>
> 1) like __csi_get_fmt(), we have accessors that gets a pointer to the
>     correct state for the TRY/live settings.

I've verified that CSI, VDIC, and PRPENCVF subdevs do that.

>
> 2) everywhere we're asked to get or set parameters that can be TRY/live,
>     we use these accessors to retrieve a pointer to the correct state to
>     not only read, but also modify.

Yes, that is currently being done in CSI, VDIC, and PRPENCVF subdevs.

>
> 3) when we're evaluating parameters against another pad, we use these
>     accessors to obtain the other pad's configuration, rather than poking
>     about in the state saved in the subdev's priv-> (which is irrelevant
>     for the TRY variant.)

Again, that is being done already:

__vdic_get_fmt()
__prp_get_fmt() (in both prp and prpencvf subdevs)
__csi_get_fmt()


>
> 4) ensure that all parameters which the subdev itself does not support
>     modification of are correctly propagated from the sink pad to all
>     source pads, and are unable to be modified via the source pad.

That is currently true except for the colorimetry params as I mentioned.

Steve

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

#1604522

On 03/19/2017 01:14 PM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
>>> 0:00:01.955927879 20954  0x15ffe90 INFO                    v4l2 gstv4l2object.c:3811:gst_v4l2_object_get_caps:<v4l2src0> probed caps: video/x-bayer, format=(string)rggb, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)BGR, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
>>>
>>>    despite the media pipeline actually being configured for 60fps.
>>>
>>>    Forcing it by adjusting the pipeline only results in gstreamer
>>>    failing, because it believes that v4l2 is unable to operate at
>>>    60fps.
>>>
>>>    Also note the complaints from v4l2src about the non-compliance...
>>
>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>> done yet. Is that something you can help with?
> 
> I've looked at this, and IMHO it's yet another media control API mess.
> 
> - media-ctl itself allows setting the format on subdev pads via
>   struct v4l2_subdev_format.
> 
> - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.
> 
> - struct v4l2_mbus_framefmt contains:
>   * @width:      frame width
>   * @height:     frame height
>   * @code:       data format code (from enum v4l2_mbus_pixelcode)
>   * @field:      used interlacing type (from enum v4l2_field)
>   * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> 
> - media-ctl sets width, height, code and field, but nothing else.
> 
> We're already agreed that the fields that media-ctl are part of the
> format negotiation between the ultimate source, flowing down to the
> capture device.  However, there's no support in media-ctl to deal
> with these other fields - so media-ctl in itself is only half-
> implemented.

Correct. The colorspace et al fields are in practice unimportant for sensors.
For HDMI/DP they are very important, though.

It's the reason why nobody worked on adding support for this to media-ctl,
it's almost exclusively used with sensors. Not saying that it is right that
it hasn't been added to media-ctl, just that it never had a high enough prio.

Regards,

	Hans

> 
> From what I can tell, _we_ are doing the right thing in imx-media-capture.
> 
> However, I think part of the problem is the set_fmt implementation.
> When a source pad is configured via set_fmt(), any fields that can
> not be altered (eg, because the subdev doesn't support colorspace
> conversion) need to be preserved from the subdev's sink pad.
> 
> Right now, CSI doesn't do that - it only looks at the width, height,
> code, and field.
> 
> I think we've got other bugs though that haven't been picked up by any
> review - csi_try_fmt() adjusts the format using the _current_
> configuration of the sink pad, even when using V4L2_SUBDEV_FORMAT_TRY.
> This seems wrong according to the docs: the purpose of the try
> mechanism is to be able to setup the _entire_ pipeline using the TRY
> mechanism to work out whether the configuration works, before then
> setting for real.  If we're validating the TRY formats against the
> live configuration, then we're not doing that.
> 
> There's calls for:
> 
> v4l2_subdev_get_try_format
> v4l2_subdev_get_try_crop
> v4l2_subdev_get_try_compose
> 
> to get the try configuration - we hardly make use of all of these.  I
> would suggest that we change the approach to implementing the various
> subdevs such that:
> 
> 1) like __csi_get_fmt(), we have accessors that gets a pointer to the
>    correct state for the TRY/live settings.
> 
> 2) everywhere we're asked to get or set parameters that can be TRY/live,
>    we use these accessors to retrieve a pointer to the correct state to
>    not only read, but also modify.
> 
> 3) when we're evaluating parameters against another pad, we use these
>    accessors to obtain the other pad's configuration, rather than poking
>    about in the state saved in the subdev's priv-> (which is irrelevant
>    for the TRY variant.)
> 
> 4) ensure that all parameters which the subdev itself does not support
>    modification of are correctly propagated from the sink pad to all
>    source pads, and are unable to be modified via the source pad.
> 

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

#1604565

On Sun, 2017-03-19 at 12:14 +0000, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> > On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> > >0:00:01.955927879 20954  0x15ffe90 INFO                    v4l2 gstv4l2object.c:3811:gst_v4l2_object_get_caps:<v4l2src0> probed caps: video/x-bayer, format=(string)rggb, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)I420, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)BGR, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, framerate=(fraction)30000/1001, width=(int)816, height=(int)616, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1
> > >
> > >    despite the media pipeline actually being configured for 60fps.
> > >
> > >    Forcing it by adjusting the pipeline only results in gstreamer
> > >    failing, because it believes that v4l2 is unable to operate at
> > >    60fps.
> > >
> > >    Also note the complaints from v4l2src about the non-compliance...
> > 
> > Thanks, I've fixed most of v4l2-compliance issues, but this is not
> > done yet. Is that something you can help with?
> 
> I've looked at this, and IMHO it's yet another media control API mess.
> 
> - media-ctl itself allows setting the format on subdev pads via
>   struct v4l2_subdev_format.
> 
> - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt.
> 
> - struct v4l2_mbus_framefmt contains:
>   * @width:      frame width
>   * @height:     frame height
>   * @code:       data format code (from enum v4l2_mbus_pixelcode)
>   * @field:      used interlacing type (from enum v4l2_field)
>   * @colorspace: colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:  YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> 
> - media-ctl sets width, height, code and field, but nothing else.
> 
> We're already agreed that the fields that media-ctl are part of the
> format negotiation between the ultimate source, flowing down to the
> capture device.  However, there's no support in media-ctl to deal
> with these other fields - so media-ctl in itself is only half-
> implemented.

To set and read colorimetry information:
https://patchwork.linuxtv.org/patch/39350/

regards
Philipp

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

#1604706

On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> To set and read colorimetry information:
> https://patchwork.linuxtv.org/patch/39350/

Thanks, I've applied all four of your patches, but there's a side effect
from that.  Old media-ctl (modified by me):

- entity 53: imx219 0-0010 (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev9
        pad0: Source
                [fmt:SRGGB8/816x616 field:none
                 frame_interval:1/25]
                -> "imx6-mipi-csi2":0 [ENABLED]
        pad1: Sink
                [fmt:SRGGB10/3280x2464 field:none
                 crop.bounds:(0,0)/3280x2464
                 crop:(0,0)/3264x2464
                 compose.bounds:(0,0)/3264x2464
                 compose:(0,0)/816x616]
                <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

New media-ctl:

- entity 53: imx219 0-0010 (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev9
        pad0: Source
                [fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb xfer:srgb]
                -> "imx6-mipi-csi2":0 [ENABLED]
        pad1: Sink
                <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]

It looks like we successfully retrieve the frame interval for pad 0
and print it, but when we try to retrieve the frame interval for pad 1,
we get EINVAL (because that's what I'm returning, but I'm wondering if
that's the correct thing to do...) and that prevents _all_ format
information being output.

Maybe something like the following would be a better idea?

 utils/media-ctl/media-ctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index f61963a..a50a559 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_fract interval = { 0, 0 };
 	struct v4l2_rect rect;
-	int ret;
+	int ret, err_fi;
 
 	ret = v4l2_subdev_get_format(entity, &format, pad, which);
 	if (ret != 0)
 		return;
 
-	ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
-	if (ret != 0 && ret != -ENOTTY)
-		return;
+	err_fi = v4l2_subdev_get_frame_interval(entity, &interval, pad);
 
 	printf("\t\t[fmt:%s/%ux%u",
 	       v4l2_subdev_pixelcode_to_string(format.code),
 	       format.width, format.height);
 
-	if (interval.numerator || interval.denominator)
+	if (err_fi == 0 && (interval.numerator || interval.denominator))
 		printf("@%u/%u", interval.numerator, interval.denominator);
+	else if (err_fi != -ENOTTY)
+		printf("@<error: %s>", strerror(-err_fi));
 
 	if (format.field)
 		printf(" field:%s", v4l2_subdev_field_to_string(format.field));


-- 
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] | [next] | [standalone]

#1604759

On Mon, Mar 20, 2017 at 05:29:07PM +0100, Philipp Zabel wrote:
> According to the documentation [1], you are doing the right thing:
> 
>     The struct v4l2_subdev_frame_interval pad references a non-existing
>     pad, or the pad doesn’t support frame intervals.
> 
> But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
> not implemented at all, which is turned into -ENOTTY by video_usercopy.
> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value

Thanks for confirming.

> > Maybe something like the following would be a better idea?
> > 
> >  utils/media-ctl/media-ctl.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> > index f61963a..a50a559 100644
> > --- a/utils/media-ctl/media-ctl.c
> > +++ b/utils/media-ctl/media-ctl.c
> > @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
> >  	struct v4l2_mbus_framefmt format;
> >  	struct v4l2_fract interval = { 0, 0 };
> >  	struct v4l2_rect rect;
> > -	int ret;
> > +	int ret, err_fi;
> >  
> >  	ret = v4l2_subdev_get_format(entity, &format, pad, which);
> >  	if (ret != 0)
> >  		return;
> >  
> > -	ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
> > -	if (ret != 0 && ret != -ENOTTY)
> > -		return;
> > +	err_fi = v4l2_subdev_get_frame_interval(entity, &interval, pad);
> 
> Not supporting frame intervals doesn't warrant a visible error message,
> I think -EINVAL should also be ignored above, if the spec is to be
> believed.
> 
> >  
> >  	printf("\t\t[fmt:%s/%ux%u",
> >  	       v4l2_subdev_pixelcode_to_string(format.code),
> >  	       format.width, format.height);
> >  
> > -	if (interval.numerator || interval.denominator)
> > +	if (err_fi == 0 && (interval.numerator || interval.denominator))
> >  		printf("@%u/%u", interval.numerator, interval.denominator);
> > +	else if (err_fi != -ENOTTY)
> > +		printf("@<error: %s>", strerror(-err_fi));
> 
> Or here.

I don't mind which - I could change this to:

	else if (err_fi != -ENOTTY && err_fi != -EINVAL)

Or an alternative would be to print an error (ignoring ENOTTY and EINVAL)
to stderr at the "v4l2_subdev_get_frame_interval" callsite and continue
on (ensuring that interval is zeroed).

-- 
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] | [next] | [standalone]

#1604770

On Mon, 2017-03-20 at 15:43 +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> > To set and read colorimetry information:
> > https://patchwork.linuxtv.org/patch/39350/
> 
> Thanks, I've applied all four of your patches, but there's a side effect
> from that.  Old media-ctl (modified by me):
> 
> - entity 53: imx219 0-0010 (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev9
>         pad0: Source
>                 [fmt:SRGGB8/816x616 field:none
>                  frame_interval:1/25]
>                 -> "imx6-mipi-csi2":0 [ENABLED]
>         pad1: Sink
>                 [fmt:SRGGB10/3280x2464 field:none
>                  crop.bounds:(0,0)/3280x2464
>                  crop:(0,0)/3264x2464
>                  compose.bounds:(0,0)/3264x2464
>                  compose:(0,0)/816x616]
>                 <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
> 
> New media-ctl:
> 
> - entity 53: imx219 0-0010 (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev9
>         pad0: Source
>                 [fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb xfer:srgb]
>                 -> "imx6-mipi-csi2":0 [ENABLED]
>         pad1: Sink
>                 <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
> 
> It looks like we successfully retrieve the frame interval for pad 0
> and print it, but when we try to retrieve the frame interval for pad 1,
> we get EINVAL (because that's what I'm returning, but I'm wondering if
> that's the correct thing to do...) and that prevents _all_ format
> information being output.

According to the documentation [1], you are doing the right thing:

    The struct v4l2_subdev_frame_interval pad references a non-existing
    pad, or the pad doesn’t support frame intervals.

But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
not implemented at all, which is turned into -ENOTTY by video_usercopy.

[1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value

> Maybe something like the following would be a better idea?
> 
>  utils/media-ctl/media-ctl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> index f61963a..a50a559 100644
> --- a/utils/media-ctl/media-ctl.c
> +++ b/utils/media-ctl/media-ctl.c
> @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
>  	struct v4l2_mbus_framefmt format;
>  	struct v4l2_fract interval = { 0, 0 };
>  	struct v4l2_rect rect;
> -	int ret;
> +	int ret, err_fi;
>  
>  	ret = v4l2_subdev_get_format(entity, &format, pad, which);
>  	if (ret != 0)
>  		return;
>  
> -	ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
> -	if (ret != 0 && ret != -ENOTTY)
> -		return;
> +	err_fi = v4l2_subdev_get_frame_interval(entity, &interval, pad);

Not supporting frame intervals doesn't warrant a visible error message,
I think -EINVAL should also be ignored above, if the spec is to be
believed.

>  
>  	printf("\t\t[fmt:%s/%ux%u",
>  	       v4l2_subdev_pixelcode_to_string(format.code),
>  	       format.width, format.height);
>  
> -	if (interval.numerator || interval.denominator)
> +	if (err_fi == 0 && (interval.numerator || interval.denominator))
>  		printf("@%u/%u", interval.numerator, interval.denominator);
> +	else if (err_fi != -ENOTTY)
> +		printf("@<error: %s>", strerror(-err_fi));

Or here.

>  
>  	if (format.field)
>  		printf(" field:%s", v4l2_subdev_field_to_string(format.field));
> 
> 

regards
Philipp

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

#1604545

On Sat, 2017-03-18 at 12:58 -0700, Steve Longerbeam wrote:
> 
> On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote:
> > Hi Steve,
> >
> > I've just been trying to get gstreamer to capture and h264 encode
> > video from my camera at various frame rates, and what I've discovered
> > does not look good.
> >
> > 1) when setting frame rates, media-ctl _always_ calls
> >     VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0.

To allow setting pad > 0:
https://patchwork.linuxtv.org/patch/39348/

> > 2) media-ctl never retrieves the frame interval information, so there's
> >     no way to read it back with standard tools, and no indication that
> >     this is going on...
> 
> I think Philipp Zabel submitted a patch which addresses these
> in media-ctl. Check with him.

To read back and propagate the frame interval:
https://patchwork.linuxtv.org/patch/39349/
https://patchwork.linuxtv.org/patch/39351/

regards
Philipp

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web