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

Groups > linux.kernel > #1604015 > unrolled thread

Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

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 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 16:30 +0100
    Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 20:20 +0100
      Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 13:20 +0100
        Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 13:40 +0100
          Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 15:30 +0100
            Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 18:20 +0100
            Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 18:30 +0100
              Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 22:00 +0100
              Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-21 05:20 +0100
              Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-21 12:30 +0100
                Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-22 01:00 +0100
              Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-22 00:40 +0100
            Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 18:50 +0100
              Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 19:20 +0100
          Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 15:30 +0100
            Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in  sink set_fmt Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-20 21:10 +0100

#1604015 — Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt

On Thu, Mar 09, 2017 at 08:53:18PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> The csi_try_crop call in set_fmt should compare the cropping rectangle
> to the currently set input format, not to the previous input format.

Are we really sure that the cropping support is implemented correctly?

I came across this while looking at what we're doing with the
V4L2_SEL_FLAG_KEEP_CONFIG flag.

Documentation/media/uapi/v4l/dev-subdev.rst defines the behaviour of
the user API, and "Order of configuration and format propagation" says:

  The coordinates to a step always refer to the actual size of the
  previous step. The exception to this rule is the source compose
  rectangle, which refers to the sink compose bounds rectangle --- if it
  is supported by the hardware.
  
  1. Sink pad format. The user configures the sink pad format. This format
     defines the parameters of the image the entity receives through the
     pad for further processing.
  
  2. Sink pad actual crop selection. The sink pad crop defines the crop
     performed to the sink pad format.
  
  3. Sink pad actual compose selection. The size of the sink pad compose
     rectangle defines the scaling ratio compared to the size of the sink
     pad crop rectangle. The location of the compose rectangle specifies
     the location of the actual sink compose rectangle in the sink compose
     bounds rectangle.
  
  4. Source pad actual crop selection. Crop on the source pad defines crop
     performed to the image in the sink compose bounds rectangle.
  
  5. Source pad format. The source pad format defines the output pixel
     format of the subdev, as well as the other parameters with the
     exception of the image width and height. Width and height are defined
     by the size of the source pad actual crop selection.
  
  Accessing any of the above rectangles not supported by the subdev will
  return ``EINVAL``. Any rectangle referring to a previous unsupported
  rectangle coordinates will instead refer to the previous supported
  rectangle. For example, if sink crop is not supported, the compose
  selection will refer to the sink pad format dimensions instead.

Note step 3 above: scaling is defined by the ratio of the _sink_ crop
rectangle to the _sink_ compose rectangle.

So, lets say that the camera produces a 1280x720 image, and the sink
pad format is configured with 1280x720.  That's step 1.

The sink crop operates within that rectangle, cropping it to an area.
Let's say we're only interested in its centre, so we'd chose 640x360
with the top-left as 320,180.  This is step 2.

Then, if we want to down-scale by a factor of two, we'd set the sink
compose selection to 320x180.

This seems to be at odds with how the scaling is done in CSI at
present: the selection implementations all reject attempts to
configure the sink pad, instead only supporting crop rectangles on
the source, and we use the source crop rectangle to define the
down-scaling.

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

#1604070

On 03/19/2017 08:22 AM, Russell King - ARM Linux wrote:
> On Thu, Mar 09, 2017 at 08:53:18PM -0800, Steve Longerbeam wrote:
>> From: Philipp Zabel <p.zabel@pengutronix.de>
>>
>> The csi_try_crop call in set_fmt should compare the cropping rectangle
>> to the currently set input format, not to the previous input format.
> Are we really sure that the cropping support is implemented correctly?
>
> I came across this while looking at what we're doing with the
> V4L2_SEL_FLAG_KEEP_CONFIG flag.
>
> Documentation/media/uapi/v4l/dev-subdev.rst defines the behaviour of
> the user API, and "Order of configuration and format propagation" says:
>
>    The coordinates to a step always refer to the actual size of the
>    previous step. The exception to this rule is the source compose
>    rectangle, which refers to the sink compose bounds rectangle --- if it
>    is supported by the hardware.
>    
>    1. Sink pad format. The user configures the sink pad format. This format
>       defines the parameters of the image the entity receives through the
>       pad for further processing.
>    
>    2. Sink pad actual crop selection. The sink pad crop defines the crop
>       performed to the sink pad format.
>    
>    3. Sink pad actual compose selection. The size of the sink pad compose
>       rectangle defines the scaling ratio compared to the size of the sink
>       pad crop rectangle. The location of the compose rectangle specifies
>       the location of the actual sink compose rectangle in the sink compose
>       bounds rectangle.
>    
>    4. Source pad actual crop selection. Crop on the source pad defines crop
>       performed to the image in the sink compose bounds rectangle.
>    
>    5. Source pad format. The source pad format defines the output pixel
>       format of the subdev, as well as the other parameters with the
>       exception of the image width and height. Width and height are defined
>       by the size of the source pad actual crop selection.
>    
>    Accessing any of the above rectangles not supported by the subdev will
>    return ``EINVAL``. Any rectangle referring to a previous unsupported
>    rectangle coordinates will instead refer to the previous supported
>    rectangle. For example, if sink crop is not supported, the compose
>    selection will refer to the sink pad format dimensions instead.
>
> Note step 3 above: scaling is defined by the ratio of the _sink_ crop
> rectangle to the _sink_ compose rectangle.
>
> So, lets say that the camera produces a 1280x720 image, and the sink
> pad format is configured with 1280x720.  That's step 1.
>
> The sink crop operates within that rectangle, cropping it to an area.
> Let's say we're only interested in its centre, so we'd chose 640x360
> with the top-left as 320,180.  This is step 2.
>
> Then, if we want to down-scale by a factor of two, we'd set the sink
> compose selection to 320x180.
>
> This seems to be at odds with how the scaling is done in CSI at
> present: the selection implementations all reject attempts to
> configure the sink pad, instead only supporting crop rectangles on
> the source,

Correct. Currently cropping is only supported at the source pad
(step 4).

Initially the CSI didn't support down-scaling, so step 3 is not supported,
so the sink pad format/crop selection rectangle/crop compose rectangle
are collapsed into the same sink pad format rectangle.

Philipp later added support for /2 downscaling, but we didn't put this in
the correct API, looks like this needs to move into the selection API at
step 3 (sink pad compose rectangle).


>   and we use the source crop rectangle to define the
> down-scaling.

Yes. And maybe there is nothing wrong with that, because scaling is also
defined by the source/sink _format_ ratios (if I'm not mistaken), so looking
at this another way, we're just defining scaling in the CSI via another
legal API.


Steve

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

#1604456

On Sun, 2017-03-19 at 12:08 -0700, Steve Longerbeam wrote:
> 
> On 03/19/2017 08:22 AM, Russell King - ARM Linux wrote:
> > On Thu, Mar 09, 2017 at 08:53:18PM -0800, Steve Longerbeam wrote:
> >> From: Philipp Zabel <p.zabel@pengutronix.de>
> >>
> >> The csi_try_crop call in set_fmt should compare the cropping rectangle
> >> to the currently set input format, not to the previous input format.
> > Are we really sure that the cropping support is implemented correctly?
> >
> > I came across this while looking at what we're doing with the
> > V4L2_SEL_FLAG_KEEP_CONFIG flag.
> >
> > Documentation/media/uapi/v4l/dev-subdev.rst defines the behaviour of
> > the user API, and "Order of configuration and format propagation" says:
> >
> >    The coordinates to a step always refer to the actual size of the
> >    previous step. The exception to this rule is the source compose
> >    rectangle, which refers to the sink compose bounds rectangle --- if it
> >    is supported by the hardware.
> >    
> >    1. Sink pad format. The user configures the sink pad format. This format
> >       defines the parameters of the image the entity receives through the
> >       pad for further processing.
> >    
> >    2. Sink pad actual crop selection. The sink pad crop defines the crop
> >       performed to the sink pad format.
> >    
> >    3. Sink pad actual compose selection. The size of the sink pad compose
> >       rectangle defines the scaling ratio compared to the size of the sink
> >       pad crop rectangle. The location of the compose rectangle specifies
> >       the location of the actual sink compose rectangle in the sink compose
> >       bounds rectangle.
> >    
> >    4. Source pad actual crop selection. Crop on the source pad defines crop
> >       performed to the image in the sink compose bounds rectangle.
> >    
> >    5. Source pad format. The source pad format defines the output pixel
> >       format of the subdev, as well as the other parameters with the
> >       exception of the image width and height. Width and height are defined
> >       by the size of the source pad actual crop selection.
> >    
> >    Accessing any of the above rectangles not supported by the subdev will
> >    return ``EINVAL``. Any rectangle referring to a previous unsupported
> >    rectangle coordinates will instead refer to the previous supported
> >    rectangle. For example, if sink crop is not supported, the compose
> >    selection will refer to the sink pad format dimensions instead.
> >
> > Note step 3 above: scaling is defined by the ratio of the _sink_ crop
> > rectangle to the _sink_ compose rectangle.

The above paragraph suggests we skip any rectangles that are not
supported. In our case that would be 3. and 4., since the CSI can't
compose into a larger frame. I hadn't realised that the crop selection
currently happens on the source pad.
The hardware actually only supports cropping of the input (the crop
rectangle we write into the window registers are before downscaling). So
the crop rectangle should be moved to the sink pad.

> > So, lets say that the camera produces a 1280x720 image, and the sink
> > pad format is configured with 1280x720.  That's step 1.
> >
> > The sink crop operates within that rectangle, cropping it to an area.
> > Let's say we're only interested in its centre, so we'd chose 640x360
> > with the top-left as 320,180.  This is step 2.
>>
> > Then, if we want to down-scale by a factor of two, we'd set the sink
> > compose selection to 320x180.

Except when composing is not supported. If the sink compose and source
crop rectangles are not supported, the source pad format takes their
place in determining the scaling output resolution. At least that's how
I read the documentation.

> > This seems to be at odds with how the scaling is done in CSI at
> > present: the selection implementations all reject attempts to
> > configure the sink pad, instead only supporting crop rectangles on
> > the source,
> 
> Correct. Currently cropping is only supported at the source pad
> (step 4).
> 
> Initially the CSI didn't support down-scaling, so step 3 is not supported,
> so the sink pad format/crop selection rectangle/crop compose rectangle
> are collapsed into the same sink pad format rectangle.
> 
> Philipp later added support for /2 downscaling, but we didn't put this in
> the correct API, looks like this needs to move into the selection API at
> step 3 (sink pad compose rectangle).

I am not sure about this. Wouldn't moving the input crop to the sink pad
be enough? If we added support for the sink pad compose rectangle, that
wouldn't actually allow to compose the CSI output into a larger frame.
Since the subdevice can't compose, I'd leave the sink compose rectangle
disabled.

> >   and we use the source crop rectangle to define the
> > down-scaling.

We use the source pad format to define the downscaling relative to the
source crop rectangle (which is wrong, it should be relative to the sink
crop rectangle).

> Yes. And maybe there is nothing wrong with that, because scaling is also
> defined by the source/sink _format_ ratios (if I'm not mistaken), so looking
> at this another way, we're just defining scaling in the CSI via another
> legal API.

I didn't touch the crop rectangle at all, just setting the input
resolution on the sink pad and the desired output resolution on the
source pad should work.

regards
Philipp

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

#1604482

On Mon, Mar 20, 2017 at 12:55:26PM +0100, Philipp Zabel wrote:
> The above paragraph suggests we skip any rectangles that are not
> supported. In our case that would be 3. and 4., since the CSI can't
> compose into a larger frame. I hadn't realised that the crop selection
> currently happens on the source pad.

I'd recommend viewing the documentation in its post-processed version,
because then you get the examples as pictures, and they say that a
picture is worth 1000 words.  See

  https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html

There is almost an exact example of what we're trying to do - it's
figure 4.6.  Here, we have a sink pad with a cropping rectangle on
the input, which is then scaled to a composition rectangle (there's
no bounds rectangle, and it's specified that in such a case the
top,left of the composition rectangle will always be 0,0 - see quote
below).

Where it differs is that the example also supports source cropping
for two source pads.  We don't support that.

The same document says:

  Scaling support is optional. When supported by a subdev, the crop
  rectangle on the subdev's sink pad is scaled to the size configured
  using the
  :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
  using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
  subdev supports scaling but not composing, the top and left values are
  not used and must always be set to zero.

which in itself describes _exactly_ our hardware here as far as the
cropping and scaling the hardware supports.

> Except when composing is not supported. If the sink compose and source
> crop rectangles are not supported, the source pad format takes their
> place in determining the scaling output resolution. At least that's how
> I read the documentation.

This isn't how I read it.  Scaling is the relationship between the size
of the sink crop and sink compose rectangle.  Composition requires that
there be a composition bounds rectangle to define the composition space,
and the top,left of the composition rectangle be adjustable to place
the composition rectangle within that space.

The above quoted paragraph from the documentation backs up my view in
its final sentence - it doesn't say "if the subdev supports scaling
but not composing, there is no composition rectangle" but says that
there _is_ one but its top,left coordinates are always zero.

-- 
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]

#1604631

On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> > The same document says:
> > 
> >   Scaling support is optional. When supported by a subdev, the crop
> >   rectangle on the subdev's sink pad is scaled to the size configured
> >   using the
> >   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
> >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> >   subdev supports scaling but not composing, the top and left values are
> >   not used and must always be set to zero.
> 
> Right, this sentence does imply that when scaling is supported, there
> must be a sink compose rectangle, even when composing is not.
> 
> I have previously set up scaling like this:
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> 
> Does this mean, it should work like this instead?
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> 
> I suppose setting the source pad format should not be allowed to modify
> the sink compose rectangle.

That is what I believe having read these documents several times, but
we need v4l2 people to confirm.

Note that setting the format on 'ipu1_csi0':0 should already be done by
the previous media-ctl command, so it should be possible to simplify
that to:

media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]"
media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

I have tripped over a bug in media-ctl when specifying both a crop and
compose rectangle - the --help output suggests that "," should be used
to separate them.  media-ctl rejects that, telling me the character at
the "," should be "]".  Replacing the "," with " " allows media-ctl to
accept it and set both rectangles, so it sounds like a parser bug - I've
not looked into this any further yet.

-- 
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]

#1604822

On Mon, Mar 20, 2017 at 02:17:05PM +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> > > The same document says:
> > > 
> > >   Scaling support is optional. When supported by a subdev, the crop
> > >   rectangle on the subdev's sink pad is scaled to the size configured
> > >   using the
> > >   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
> > >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> > >   subdev supports scaling but not composing, the top and left values are
> > >   not used and must always be set to zero.
> > 
> > Right, this sentence does imply that when scaling is supported, there
> > must be a sink compose rectangle, even when composing is not.
> > 
> > I have previously set up scaling like this:
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > Does this mean, it should work like this instead?
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > I suppose setting the source pad format should not be allowed to modify
> > the sink compose rectangle.
> 
> That is what I believe having read these documents several times, but
> we need v4l2 people to confirm.
> 
> Note that setting the format on 'ipu1_csi0':0 should already be done by
> the previous media-ctl command, so it should be possible to simplify
> that to:
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

There is a slightly puzzling bit in the documentation.  If we consider
the CSI, and the sink compose rectangle size has to always match the
the source output pad format size (since in hardware they are one of
the same), then:

  Inside subdevs, the order of image processing steps will always be from
  the sink pad towards the source pad. This is also reflected in the order
  in which the configuration must be performed by the user: the changes
  made will be propagated to any subsequent stages. If this behaviour is
  not desired, the user must set ``V4L2_SEL_FLAG_KEEP_CONFIG`` flag. This
                                                                     ^^^^
  flag causes no propagation of the changes are allowed in any
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  circumstances. This may also cause the accessed rectangle to be adjusted
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  by the driver, depending on the properties of the underlying hardware.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

presumably, this means if we get a request to change the source compose
rectangle with V4L2_SEL_FLAG_KEEP_CONFIG set, we need to force its size
to be the current output format size.  I don't think we can do anything
else - because the above makes it very clear that any following stages
shall not be updated.  The last sentence appears to give permission to
do that.

This also has implications when changing the sink crop - the sink crop
(eg) must not be smaller than the sink compose, as we don't support
scaling up in CSI.

It seems to me that V4L2_SEL_FLAG_KEEP_CONFIG in practice changes the
way validation of the request works.  So, rather than validating the
request against the upstream rectangle and propagating downstream, it
needs to be validated against both the upstream and downstream
rectangles instead.

It seems there's many subtleties to this...

-- 
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]

#1604827

Hi Steve, Russell,

On Mon, 2017-03-20 at 14:17 +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> > > The same document says:
> > > 
> > >   Scaling support is optional. When supported by a subdev, the crop
> > >   rectangle on the subdev's sink pad is scaled to the size configured
> > >   using the
> > >   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
> > >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> > >   subdev supports scaling but not composing, the top and left values are
> > >   not used and must always be set to zero.
> > 
> > Right, this sentence does imply that when scaling is supported, there
> > must be a sink compose rectangle, even when composing is not.
> > 
> > I have previously set up scaling like this:
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > Does this mean, it should work like this instead?
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > I suppose setting the source pad format should not be allowed to modify
> > the sink compose rectangle.
> 
> That is what I believe having read these documents several times, but
> we need v4l2 people to confirm.

What do you think of this:

----------8<----------
From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Mon, 20 Mar 2017 17:10:21 +0100
Subject: [PATCH] media: imx: csi: add sink selection rectangles

Move the crop rectangle to the sink pad and add a sink compose rectangle
to configure scaling. Also propagate rectangles from sink pad to crop
rectangle, to compose rectangle, and to the source pads both in ACTIVE
and TRY variants of set_fmt/selection, and initialize the default crop
and compose rectangles.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-csi.c | 216 +++++++++++++++++++++---------
 1 file changed, 152 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 560da3abdd70b..b026a5d602ddf 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -79,6 +79,7 @@ struct csi_priv {
 	const struct imx_media_pixfmt *cc[CSI_NUM_PADS];
 	struct v4l2_fract frame_interval;
 	struct v4l2_rect crop;
+	struct v4l2_rect compose;
 	const struct csi_skip_desc *skip[CSI_NUM_PADS - 1];
 
 	/* active vb2 buffers to send to video dev sink */
@@ -574,8 +575,8 @@ static int csi_setup(struct csi_priv *priv)
 	ipu_csi_set_window(priv->csi, &priv->crop);
 
 	ipu_csi_set_downsize(priv->csi,
-			     priv->crop.width == 2 * outfmt->width,
-			     priv->crop.height == 2 * outfmt->height);
+			     priv->crop.width == 2 * priv->compose.width,
+			     priv->crop.height == 2 * priv->compose.height);
 
 	ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
 
@@ -1001,6 +1002,27 @@ __csi_get_fmt(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
 		return &priv->format_mbus[pad];
 }
 
+static struct v4l2_rect *
+__csi_get_crop(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
+	       enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_crop(&priv->sd, cfg, CSI_SINK_PAD);
+	else
+		return &priv->crop;
+}
+
+static struct v4l2_rect *
+__csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
+		  enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_compose(&priv->sd, cfg,
+						   CSI_SINK_PAD);
+	else
+		return &priv->compose;
+}
+
 static void csi_try_crop(struct csi_priv *priv,
 			 struct v4l2_rect *crop,
 			 struct v4l2_subdev_pad_config *cfg,
@@ -1159,6 +1181,7 @@ static void csi_try_fmt(struct csi_priv *priv,
 			struct v4l2_subdev_pad_config *cfg,
 			struct v4l2_subdev_format *sdformat,
 			struct v4l2_rect *crop,
+			struct v4l2_rect *compose,
 			const struct imx_media_pixfmt **cc)
 {
 	const struct imx_media_pixfmt *incc;
@@ -1173,15 +1196,8 @@ static void csi_try_fmt(struct csi_priv *priv,
 		incc = imx_media_find_mbus_format(infmt->code,
 						  CS_SEL_ANY, true);
 
-		if (sdformat->format.width < priv->crop.width * 3 / 4)
-			sdformat->format.width = priv->crop.width / 2;
-		else
-			sdformat->format.width = priv->crop.width;
-
-		if (sdformat->format.height < priv->crop.height * 3 / 4)
-			sdformat->format.height = priv->crop.height / 2;
-		else
-			sdformat->format.height = priv->crop.height;
+		sdformat->format.width = compose->width;
+		sdformat->format.height = compose->height;
 
 		if (incc->bayer) {
 			sdformat->format.code = infmt->code;
@@ -1217,11 +1233,17 @@ static void csi_try_fmt(struct csi_priv *priv,
 		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
 				      W_ALIGN, &sdformat->format.height,
 				      MIN_H, MAX_H, H_ALIGN, S_ALIGN);
+
+		/* Reset crop and compose rectangles */
 		crop->left = 0;
 		crop->top = 0;
 		crop->width = sdformat->format.width;
 		crop->height = sdformat->format.height;
 		csi_try_crop(priv, crop, cfg, &sdformat->format, sensor);
+		compose->left = 0;
+		compose->top = 0;
+		compose->width = crop->width;
+		compose->height = crop->height;
 
 		*cc = imx_media_find_mbus_format(sdformat->format.code,
 						 CS_SEL_ANY, true);
@@ -1245,7 +1267,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
 	const struct imx_media_pixfmt *cc;
 	struct imx_media_subdev *sensor;
 	struct v4l2_pix_format vdev_fmt;
-	struct v4l2_rect crop;
+	struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_rect *crop, *compose;
 	int ret = 0;
 
 	if (sdformat->pad >= CSI_NUM_PADS)
@@ -1264,37 +1287,42 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
 		goto out;
 	}
 
-	csi_try_fmt(priv, sensor, cfg, sdformat, &crop, &cc);
+	crop = __csi_get_crop(priv, cfg, sdformat->which);
+	compose = __csi_get_compose(priv, cfg, sdformat->which);
 
-	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
-		cfg->try_fmt = sdformat->format;
-		goto out;
-	}
+	csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc);
 
-	priv->format_mbus[sdformat->pad] = sdformat->format;
-	priv->cc[sdformat->pad] = cc;
+	fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
+	*fmt = sdformat->format;
 
 	if (sdformat->pad == CSI_SINK_PAD) {
 		int pad;
 
-		/* reset the crop window */
-		priv->crop = crop;
-
 		/* propagate format to source pads */
 		for (pad = CSI_SINK_PAD + 1; pad < CSI_NUM_PADS; pad++) {
 			const struct imx_media_pixfmt *outcc;
 			struct v4l2_subdev_format format;
+			struct v4l2_mbus_framefmt *outfmt;
 
 			format.pad = pad;
-			format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+			format.which = sdformat->which;
 			format.format = sdformat->format;
-			csi_try_fmt(priv, sensor, cfg, &format, &crop, &outcc);
+			csi_try_fmt(priv, sensor, cfg, &format, NULL, compose,
+				    &outcc);
 
-			priv->format_mbus[pad] = format.format;
-			priv->cc[pad] = outcc;
+			outfmt = __csi_get_fmt(priv, cfg, pad, sdformat->which);
+			*outfmt = format.format;
+
+			if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+				priv->cc[pad] = outcc;
 		}
 	}
 
+	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY)
+		goto out;
+
+	priv->cc[sdformat->pad] = cc;
+
 	/* propagate IDMAC output pad format to capture device */
 	imx_media_mbus_fmt_to_pix_fmt(&vdev_fmt,
 				      &priv->format_mbus[CSI_SRC_PAD_IDMAC],
@@ -1314,18 +1342,17 @@ static int csi_get_selection(struct v4l2_subdev *sd,
 {
 	struct csi_priv *priv = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *infmt;
+	struct v4l2_rect *crop, *compose;
 	int ret = 0;
 
-	if (sel->pad >= CSI_NUM_PADS || sel->pad == CSI_SINK_PAD)
+	if (sel->pad != CSI_SINK_PAD)
 		return -EINVAL;
 
 	mutex_lock(&priv->lock);
 
 	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
-	if (!infmt) {
-		ret = -EINVAL;
-		goto out;
-	}
+	crop = __csi_get_crop(priv, cfg, sel->which);
+	compose = __csi_get_compose(priv, cfg, sel->which);
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
@@ -1335,36 +1362,54 @@ static int csi_get_selection(struct v4l2_subdev *sd,
 		sel->r.height = infmt->height;
 		break;
 	case V4L2_SEL_TGT_CROP:
-		if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
-			struct v4l2_rect *try_crop =
-				v4l2_subdev_get_try_crop(&priv->sd,
-							 cfg, sel->pad);
-			sel->r = *try_crop;
-		} else {
-			sel->r = priv->crop;
-		}
+		sel->r = *crop;
+		break;
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = crop->width;
+		sel->r.height = crop->height;
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		sel->r = *compose;
 		break;
 	default:
 		ret = -EINVAL;
 	}
 
-out:
 	mutex_unlock(&priv->lock);
 	return ret;
 }
 
+static int csi_set_scale(u32 *compose, u32 crop, u32 flags)
+{
+	if ((flags & (V4L2_SEL_FLAG_LE | V4L2_SEL_FLAG_GE)) ==
+		     (V4L2_SEL_FLAG_LE | V4L2_SEL_FLAG_GE) &&
+	    *compose != crop && *compose != crop / 2)
+		return -ERANGE;
+
+	if (*compose <= crop / 2 ||
+	    (*compose < crop * 3 / 4 && !(flags & V4L2_SEL_FLAG_GE)) ||
+	    (*compose < crop && (flags & V4L2_SEL_FLAG_LE)))
+		*compose = crop / 2;
+	else
+		*compose = crop;
+
+	return 0;
+}
+
 static int csi_set_selection(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_pad_config *cfg,
 			     struct v4l2_subdev_selection *sel)
 {
 	struct csi_priv *priv = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *infmt;
+	struct v4l2_rect *crop, *compose;
 	struct imx_media_subdev *sensor;
+	int pad;
 	int ret = 0;
 
-	if (sel->pad >= CSI_NUM_PADS ||
-	    sel->pad == CSI_SINK_PAD ||
-	    sel->target != V4L2_SEL_TGT_CROP)
+	if (sel->pad != CSI_SINK_PAD)
 		return -EINVAL;
 
 	sensor = imx_media_find_sensor(priv->md, &priv->sd.entity);
@@ -1380,31 +1425,68 @@ static int csi_set_selection(struct v4l2_subdev *sd,
 		goto out;
 	}
 
-	/*
-	 * Modifying the crop rectangle always changes the format on the source
-	 * pad. If the KEEP_CONFIG flag is set, just return the current crop
-	 * rectangle.
-	 */
-	if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
-		sel->r = priv->crop;
-		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
-			cfg->try_crop = sel->r;
+	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
+	crop = __csi_get_crop(priv, cfg, sel->which);
+	compose = __csi_get_compose(priv, cfg, sel->which);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		/*
+		 * Modifying the crop rectangle always changes the format on
+		 * the source pads. If the KEEP_CONFIG flag is set, just return
+		 * the current crop rectangle.
+		 */
+		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
+			sel->r = priv->crop;
+			if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+				*crop = sel->r;
+			goto out;
+		}
+
+		csi_try_crop(priv, &sel->r, cfg, infmt, sensor);
+
+		*crop = sel->r;
+
+		/* Reset scaling to 1:1 */
+		compose->width = crop->width;
+		compose->height = crop->height;
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		/*
+		 * Modifying the compose rectangle always changes the format on
+		 * the source pads. If the KEEP_CONFIG flag is set, just return
+		 * the current compose rectangle.
+		 */
+		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
+			sel->r = priv->compose;
+			if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+				*compose = sel->r;
+			goto out;
+		}
+
+		sel->r.left = 0;
+		sel->r.top = 0;
+		ret = csi_set_scale(&sel->r.width, crop->width, sel->flags);
+		if (ret)
+			goto out;
+		ret = csi_set_scale(&sel->r.height, crop->height, sel->flags);
+		if (ret)
+			goto out;
+
+		*compose = sel->r;
+		break;
+	default:
+		ret = -EINVAL;
 		goto out;
 	}
 
-	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
-	csi_try_crop(priv, &sel->r, cfg, infmt, sensor);
-
-	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
-		cfg->try_crop = sel->r;
-	} else {
-		struct v4l2_mbus_framefmt *outfmt =
-			&priv->format_mbus[sel->pad];
+	/* Reset source pads to sink compose rectangle */
+	for (pad = CSI_SINK_PAD + 1; pad < CSI_NUM_PADS; pad++) {
+		struct v4l2_mbus_framefmt *outfmt;
 
-		priv->crop = sel->r;
-		/* Update the source format */
-		outfmt->width = sel->r.width;
-		outfmt->height = sel->r.height;
+		outfmt = __csi_get_fmt(priv, cfg, pad, sel->which);
+		outfmt->width = compose->width;
+		outfmt->height = compose->height;
 	}
 
 out:
@@ -1467,6 +1549,12 @@ static int csi_registered(struct v4l2_subdev *sd)
 			priv->skip[i - 1] = &csi_skip[0];
 	}
 
+	/* init default crop and compose rectangle sizes */
+	priv->crop.width = 640;
+	priv->crop.height = 480;
+	priv->compose.width = 640;
+	priv->compose.height = 480;
+
 	/* init default frame interval */
 	priv->frame_interval.numerator = 1;
 	priv->frame_interval.denominator = 30;
-- 
2.11.0
---------->8----------

regards
Philipp

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

#1605130

On Mon, Mar 20, 2017 at 06:23:24PM +0100, Philipp Zabel wrote:
> ----------8<----------
> >From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Mon, 20 Mar 2017 17:10:21 +0100
> Subject: [PATCH] media: imx: csi: add sink selection rectangles
> 
> Move the crop rectangle to the sink pad and add a sink compose rectangle
> to configure scaling. Also propagate rectangles from sink pad to crop
> rectangle, to compose rectangle, and to the source pads both in ACTIVE
> and TRY variants of set_fmt/selection, and initialize the default crop
> and compose rectangles.

Looks fine for the most part.

> -	/*
> -	 * Modifying the crop rectangle always changes the format on the source
> -	 * pad. If the KEEP_CONFIG flag is set, just return the current crop
> -	 * rectangle.
> -	 */
> -	if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> -		sel->r = priv->crop;
> -		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> -			cfg->try_crop = sel->r;
> +	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
> +	crop = __csi_get_crop(priv, cfg, sel->which);
> +	compose = __csi_get_compose(priv, cfg, sel->which);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		/*
> +		 * Modifying the crop rectangle always changes the format on
> +		 * the source pads. If the KEEP_CONFIG flag is set, just return
> +		 * the current crop rectangle.
> +		 */
> +		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> +			sel->r = priv->crop;

My understanding of KEEP_CONFIG is that the only thing we're not
allowed to do is to propagate the change downstream.

Since downstream of the crop is the compose, that means the only
restriction here is that the width and height of the crop window must
be either equal to the compose width/height, or double the compose
width/height.  (Anything else would necessitate the compose window
changing.)

However, the crop window can move position within the crop bounds,
provided it's entirely contained within those crop bounds.

The problem is that this becomes rather more complex it deal with
(as I'm finding out in my imx219 camera driver) and I'm thinking
that some of this complexity should probably be in a helper in
generic v4l2 code.

I don't know whether this applies (I hope it doesn't) but there's a
pile of guidelines in Documentation/media/uapi/v4l/vidioc-g-selection.rst
which describe how a crop/compose rectangle should be adjusted.  As
I say, I hope they don't apply, because if they do, we would _really_
need helpers for this stuff, as I don't think having each driver
implement all these rules would be too successful!

> +			if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +				*crop = sel->r;
> +			goto out;
> +		}
> +
> +		csi_try_crop(priv, &sel->r, cfg, infmt, sensor);
> +
> +		*crop = sel->r;
> +
> +		/* Reset scaling to 1:1 */
> +		compose->width = crop->width;
> +		compose->height = crop->height;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		/*
> +		 * Modifying the compose rectangle always changes the format on
> +		 * the source pads. If the KEEP_CONFIG flag is set, just return
> +		 * the current compose rectangle.
> +		 */
> +		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> +			sel->r = priv->compose;

I think, with my understanding of how the KEEP_CONFIG flag works, this
should be:
			sel->r = *compose;

because if we change the compose rectangle width/height, we would need
to propagate this to the source pad, and the KEEP_CONFIG description
says we're not allowed to do that.

-- 
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]

#1605290

On 03/20/2017 10:23 AM, Philipp Zabel wrote:
> Hi Steve, Russell,
>
> On Mon, 2017-03-20 at 14:17 +0000, Russell King - ARM Linux wrote:
>> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
>>> On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
>>>> The same document says:
>>>>
>>>>   Scaling support is optional. When supported by a subdev, the crop
>>>>   rectangle on the subdev's sink pad is scaled to the size configured
>>>>   using the
>>>>   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
>>>>   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
>>>>   subdev supports scaling but not composing, the top and left values are
>>>>   not used and must always be set to zero.
>>>
>>> Right, this sentence does imply that when scaling is supported, there
>>> must be a sink compose rectangle, even when composing is not.
>>>
>>> I have previously set up scaling like this:
>>>
>>> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
>>> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
>>>
>>> Does this mean, it should work like this instead?
>>>
>>> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
>>> media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
>>> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
>>>
>>> I suppose setting the source pad format should not be allowed to modify
>>> the sink compose rectangle.
>>
>> That is what I believe having read these documents several times, but
>> we need v4l2 people to confirm.
>
> What do you think of this:
>
> ----------8<----------
> From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Mon, 20 Mar 2017 17:10:21 +0100
> Subject: [PATCH] media: imx: csi: add sink selection rectangles
>
> Move the crop rectangle to the sink pad and add a sink compose rectangle
> to configure scaling. Also propagate rectangles from sink pad to crop
> rectangle, to compose rectangle, and to the source pads both in ACTIVE
> and TRY variants of set_fmt/selection, and initialize the default crop
> and compose rectangles.
>

I haven't had a chance to look at this patch in detail yet, but on
first glance it looks good to me. I'll try to find the time tomorrow
to incorporate it and then fixup Russell's subsequent patches for
the enum frame sizes/intervals.

Steve

> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 216 +++++++++++++++++++++---------
>  1 file changed, 152 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 560da3abdd70b..b026a5d602ddf 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -79,6 +79,7 @@ struct csi_priv {
>  	const struct imx_media_pixfmt *cc[CSI_NUM_PADS];
>  	struct v4l2_fract frame_interval;
>  	struct v4l2_rect crop;
> +	struct v4l2_rect compose;
>  	const struct csi_skip_desc *skip[CSI_NUM_PADS - 1];
>
>  	/* active vb2 buffers to send to video dev sink */
> @@ -574,8 +575,8 @@ static int csi_setup(struct csi_priv *priv)
>  	ipu_csi_set_window(priv->csi, &priv->crop);
>
>  	ipu_csi_set_downsize(priv->csi,
> -			     priv->crop.width == 2 * outfmt->width,
> -			     priv->crop.height == 2 * outfmt->height);
> +			     priv->crop.width == 2 * priv->compose.width,
> +			     priv->crop.height == 2 * priv->compose.height);
>
>  	ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
>
> @@ -1001,6 +1002,27 @@ __csi_get_fmt(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
>  		return &priv->format_mbus[pad];
>  }
>
> +static struct v4l2_rect *
> +__csi_get_crop(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
> +	       enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_crop(&priv->sd, cfg, CSI_SINK_PAD);
> +	else
> +		return &priv->crop;
> +}
> +
> +static struct v4l2_rect *
> +__csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
> +		  enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_compose(&priv->sd, cfg,
> +						   CSI_SINK_PAD);
> +	else
> +		return &priv->compose;
> +}
> +
>  static void csi_try_crop(struct csi_priv *priv,
>  			 struct v4l2_rect *crop,
>  			 struct v4l2_subdev_pad_config *cfg,
> @@ -1159,6 +1181,7 @@ static void csi_try_fmt(struct csi_priv *priv,
>  			struct v4l2_subdev_pad_config *cfg,
>  			struct v4l2_subdev_format *sdformat,
>  			struct v4l2_rect *crop,
> +			struct v4l2_rect *compose,
>  			const struct imx_media_pixfmt **cc)
>  {
>  	const struct imx_media_pixfmt *incc;
> @@ -1173,15 +1196,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		incc = imx_media_find_mbus_format(infmt->code,
>  						  CS_SEL_ANY, true);
>
> -		if (sdformat->format.width < priv->crop.width * 3 / 4)
> -			sdformat->format.width = priv->crop.width / 2;
> -		else
> -			sdformat->format.width = priv->crop.width;
> -
> -		if (sdformat->format.height < priv->crop.height * 3 / 4)
> -			sdformat->format.height = priv->crop.height / 2;
> -		else
> -			sdformat->format.height = priv->crop.height;
> +		sdformat->format.width = compose->width;
> +		sdformat->format.height = compose->height;
>
>  		if (incc->bayer) {
>  			sdformat->format.code = infmt->code;
> @@ -1217,11 +1233,17 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
>  				      W_ALIGN, &sdformat->format.height,
>  				      MIN_H, MAX_H, H_ALIGN, S_ALIGN);
> +
> +		/* Reset crop and compose rectangles */
>  		crop->left = 0;
>  		crop->top = 0;
>  		crop->width = sdformat->format.width;
>  		crop->height = sdformat->format.height;
>  		csi_try_crop(priv, crop, cfg, &sdformat->format, sensor);
> +		compose->left = 0;
> +		compose->top = 0;
> +		compose->width = crop->width;
> +		compose->height = crop->height;
>
>  		*cc = imx_media_find_mbus_format(sdformat->format.code,
>  						 CS_SEL_ANY, true);
> @@ -1245,7 +1267,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  	const struct imx_media_pixfmt *cc;
>  	struct imx_media_subdev *sensor;
>  	struct v4l2_pix_format vdev_fmt;
> -	struct v4l2_rect crop;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_rect *crop, *compose;
>  	int ret = 0;
>
>  	if (sdformat->pad >= CSI_NUM_PADS)
> @@ -1264,37 +1287,42 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>  		goto out;
>  	}
>
> -	csi_try_fmt(priv, sensor, cfg, sdformat, &crop, &cc);
> +	crop = __csi_get_crop(priv, cfg, sdformat->which);
> +	compose = __csi_get_compose(priv, cfg, sdformat->which);
>
> -	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		cfg->try_fmt = sdformat->format;
> -		goto out;
> -	}
> +	csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc);
>
> -	priv->format_mbus[sdformat->pad] = sdformat->format;
> -	priv->cc[sdformat->pad] = cc;
> +	fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
> +	*fmt = sdformat->format;
>
>  	if (sdformat->pad == CSI_SINK_PAD) {
>  		int pad;
>
> -		/* reset the crop window */
> -		priv->crop = crop;
> -
>  		/* propagate format to source pads */
>  		for (pad = CSI_SINK_PAD + 1; pad < CSI_NUM_PADS; pad++) {
>  			const struct imx_media_pixfmt *outcc;
>  			struct v4l2_subdev_format format;
> +			struct v4l2_mbus_framefmt *outfmt;
>
>  			format.pad = pad;
> -			format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +			format.which = sdformat->which;
>  			format.format = sdformat->format;
> -			csi_try_fmt(priv, sensor, cfg, &format, &crop, &outcc);
> +			csi_try_fmt(priv, sensor, cfg, &format, NULL, compose,
> +				    &outcc);
>
> -			priv->format_mbus[pad] = format.format;
> -			priv->cc[pad] = outcc;
> +			outfmt = __csi_get_fmt(priv, cfg, pad, sdformat->which);
> +			*outfmt = format.format;
> +
> +			if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +				priv->cc[pad] = outcc;
>  		}
>  	}
>
> +	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY)
> +		goto out;
> +
> +	priv->cc[sdformat->pad] = cc;
> +
>  	/* propagate IDMAC output pad format to capture device */
>  	imx_media_mbus_fmt_to_pix_fmt(&vdev_fmt,
>  				      &priv->format_mbus[CSI_SRC_PAD_IDMAC],
> @@ -1314,18 +1342,17 @@ static int csi_get_selection(struct v4l2_subdev *sd,
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
>  	struct v4l2_mbus_framefmt *infmt;
> +	struct v4l2_rect *crop, *compose;
>  	int ret = 0;
>
> -	if (sel->pad >= CSI_NUM_PADS || sel->pad == CSI_SINK_PAD)
> +	if (sel->pad != CSI_SINK_PAD)
>  		return -EINVAL;
>
>  	mutex_lock(&priv->lock);
>
>  	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
> -	if (!infmt) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	crop = __csi_get_crop(priv, cfg, sel->which);
> +	compose = __csi_get_compose(priv, cfg, sel->which);
>
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
> @@ -1335,36 +1362,54 @@ static int csi_get_selection(struct v4l2_subdev *sd,
>  		sel->r.height = infmt->height;
>  		break;
>  	case V4L2_SEL_TGT_CROP:
> -		if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> -			struct v4l2_rect *try_crop =
> -				v4l2_subdev_get_try_crop(&priv->sd,
> -							 cfg, sel->pad);
> -			sel->r = *try_crop;
> -		} else {
> -			sel->r = priv->crop;
> -		}
> +		sel->r = *crop;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +		sel->r.left = 0;
> +		sel->r.top = 0;
> +		sel->r.width = crop->width;
> +		sel->r.height = crop->height;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		sel->r = *compose;
>  		break;
>  	default:
>  		ret = -EINVAL;
>  	}
>
> -out:
>  	mutex_unlock(&priv->lock);
>  	return ret;
>  }
>
> +static int csi_set_scale(u32 *compose, u32 crop, u32 flags)
> +{
> +	if ((flags & (V4L2_SEL_FLAG_LE | V4L2_SEL_FLAG_GE)) ==
> +		     (V4L2_SEL_FLAG_LE | V4L2_SEL_FLAG_GE) &&
> +	    *compose != crop && *compose != crop / 2)
> +		return -ERANGE;
> +
> +	if (*compose <= crop / 2 ||
> +	    (*compose < crop * 3 / 4 && !(flags & V4L2_SEL_FLAG_GE)) ||
> +	    (*compose < crop && (flags & V4L2_SEL_FLAG_LE)))
> +		*compose = crop / 2;
> +	else
> +		*compose = crop;
> +
> +	return 0;
> +}
> +
>  static int csi_set_selection(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_pad_config *cfg,
>  			     struct v4l2_subdev_selection *sel)
>  {
>  	struct csi_priv *priv = v4l2_get_subdevdata(sd);
>  	struct v4l2_mbus_framefmt *infmt;
> +	struct v4l2_rect *crop, *compose;
>  	struct imx_media_subdev *sensor;
> +	int pad;
>  	int ret = 0;
>
> -	if (sel->pad >= CSI_NUM_PADS ||
> -	    sel->pad == CSI_SINK_PAD ||
> -	    sel->target != V4L2_SEL_TGT_CROP)
> +	if (sel->pad != CSI_SINK_PAD)
>  		return -EINVAL;
>
>  	sensor = imx_media_find_sensor(priv->md, &priv->sd.entity);
> @@ -1380,31 +1425,68 @@ static int csi_set_selection(struct v4l2_subdev *sd,
>  		goto out;
>  	}
>
> -	/*
> -	 * Modifying the crop rectangle always changes the format on the source
> -	 * pad. If the KEEP_CONFIG flag is set, just return the current crop
> -	 * rectangle.
> -	 */
> -	if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> -		sel->r = priv->crop;
> -		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> -			cfg->try_crop = sel->r;
> +	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
> +	crop = __csi_get_crop(priv, cfg, sel->which);
> +	compose = __csi_get_compose(priv, cfg, sel->which);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		/*
> +		 * Modifying the crop rectangle always changes the format on
> +		 * the source pads. If the KEEP_CONFIG flag is set, just return
> +		 * the current crop rectangle.
> +		 */
> +		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> +			sel->r = priv->crop;
> +			if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +				*crop = sel->r;
> +			goto out;
> +		}
> +
> +		csi_try_crop(priv, &sel->r, cfg, infmt, sensor);
> +
> +		*crop = sel->r;
> +
> +		/* Reset scaling to 1:1 */
> +		compose->width = crop->width;
> +		compose->height = crop->height;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		/*
> +		 * Modifying the compose rectangle always changes the format on
> +		 * the source pads. If the KEEP_CONFIG flag is set, just return
> +		 * the current compose rectangle.
> +		 */
> +		if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
> +			sel->r = priv->compose;
> +			if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +				*compose = sel->r;
> +			goto out;
> +		}
> +
> +		sel->r.left = 0;
> +		sel->r.top = 0;
> +		ret = csi_set_scale(&sel->r.width, crop->width, sel->flags);
> +		if (ret)
> +			goto out;
> +		ret = csi_set_scale(&sel->r.height, crop->height, sel->flags);
> +		if (ret)
> +			goto out;
> +
> +		*compose = sel->r;
> +		break;
> +	default:
> +		ret = -EINVAL;
>  		goto out;
>  	}
>
> -	infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, sel->which);
> -	csi_try_crop(priv, &sel->r, cfg, infmt, sensor);
> -
> -	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		cfg->try_crop = sel->r;
> -	} else {
> -		struct v4l2_mbus_framefmt *outfmt =
> -			&priv->format_mbus[sel->pad];
> +	/* Reset source pads to sink compose rectangle */
> +	for (pad = CSI_SINK_PAD + 1; pad < CSI_NUM_PADS; pad++) {
> +		struct v4l2_mbus_framefmt *outfmt;
>
> -		priv->crop = sel->r;
> -		/* Update the source format */
> -		outfmt->width = sel->r.width;
> -		outfmt->height = sel->r.height;
> +		outfmt = __csi_get_fmt(priv, cfg, pad, sel->which);
> +		outfmt->width = compose->width;
> +		outfmt->height = compose->height;
>  	}
>
>  out:
> @@ -1467,6 +1549,12 @@ static int csi_registered(struct v4l2_subdev *sd)
>  			priv->skip[i - 1] = &csi_skip[0];
>  	}
>
> +	/* init default crop and compose rectangle sizes */
> +	priv->crop.width = 640;
> +	priv->crop.height = 480;
> +	priv->compose.width = 640;
> +	priv->compose.height = 480;
> +
>  	/* init default frame interval */
>  	priv->frame_interval.numerator = 1;
>  	priv->frame_interval.denominator = 30;
>

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

#1605511

On Mon, Mar 20, 2017 at 06:23:24PM +0100, Philipp Zabel wrote:
> @@ -1173,15 +1196,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>  		incc = imx_media_find_mbus_format(infmt->code,
>  						  CS_SEL_ANY, true);
>  
> -		if (sdformat->format.width < priv->crop.width * 3 / 4)
> -			sdformat->format.width = priv->crop.width / 2;
> -		else
> -			sdformat->format.width = priv->crop.width;
> -
> -		if (sdformat->format.height < priv->crop.height * 3 / 4)
> -			sdformat->format.height = priv->crop.height / 2;
> -		else
> -			sdformat->format.height = priv->crop.height;
> +		sdformat->format.width = compose->width;
> +		sdformat->format.height = compose->height;
>  
>  		if (incc->bayer) {
>  			sdformat->format.code = infmt->code;

We need to do more in here, because right now setting the source pads
overwrites the colorimetry etc information.  Maybe something like the
below?  I'm wondering if it would be a saner approach to copy the
sink format and update the parameters that can be changed, rather than
trying to list all the parameters that shouldn't be changed.  What if
the format structure gains a new member?

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 1492b92e1970..756204ced53c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1221,6 +1221,12 @@ static void csi_try_fmt(struct csi_priv *priv,
 			sdformat->format.field =  (infmt->height == 480) ?
 				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		}
+
+		/* copy settings we can't change */
+		sdformat->format.colorspace = infmt->colorspace;
+		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
+		sdformat->format.quantization = infmt->quantization;
+		sdformat->format.xfer_func = infmt->xfer_func;
 		break;
 	case CSI_SINK_PAD:
 		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,



-- 
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]

#1606103

On 03/21/2017 04:27 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 06:23:24PM +0100, Philipp Zabel wrote:
>> @@ -1173,15 +1196,8 @@ static void csi_try_fmt(struct csi_priv *priv,
>>  		incc = imx_media_find_mbus_format(infmt->code,
>>  						  CS_SEL_ANY, true);
>>
>> -		if (sdformat->format.width < priv->crop.width * 3 / 4)
>> -			sdformat->format.width = priv->crop.width / 2;
>> -		else
>> -			sdformat->format.width = priv->crop.width;
>> -
>> -		if (sdformat->format.height < priv->crop.height * 3 / 4)
>> -			sdformat->format.height = priv->crop.height / 2;
>> -		else
>> -			sdformat->format.height = priv->crop.height;
>> +		sdformat->format.width = compose->width;
>> +		sdformat->format.height = compose->height;
>>
>>  		if (incc->bayer) {
>>  			sdformat->format.code = infmt->code;
>
> We need to do more in here, because right now setting the source pads
> overwrites the colorimetry etc information.  Maybe something like the
> below?

I'm thinking, to support propagating the colorimetry params, there
should be a util function

void imx_media_copy_colorimetry(struct v4l2_mbus_framefmt *out,
				struct v4l2_mbus_framefmt *in);

that can be used throughout the pipeline, that does exactly what
you add below.

>  I'm wondering if it would be a saner approach to copy the
> sink format and update the parameters that can be changed, rather than
> trying to list all the parameters that shouldn't be changed.

For CSI that is a bit difficult, because the source formats are
hardly related to the sink formats, so so much would have to be
modified after copying the sink format that it would be rather
pointless, except to forward the colorimetry params.

Steve

>  What if the format structure gains a new member?
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 1492b92e1970..756204ced53c 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1221,6 +1221,12 @@ static void csi_try_fmt(struct csi_priv *priv,
>  			sdformat->format.field =  (infmt->height == 480) ?
>  				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
>  		}
> +
> +		/* copy settings we can't change */
> +		sdformat->format.colorspace = infmt->colorspace;
> +		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
> +		sdformat->format.quantization = infmt->quantization;
> +		sdformat->format.xfer_func = infmt->xfer_func;
>  		break;
>  	case CSI_SINK_PAD:
>  		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
>
>
>

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

#1606081

Hi Philipp, Russell,


On 03/20/2017 10:23 AM, Philipp Zabel wrote:
> Hi Steve, Russell,
>
> <snip>
>
> What do you think of this:
>
> ----------8<----------
> From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Mon, 20 Mar 2017 17:10:21 +0100
> Subject: [PATCH] media: imx: csi: add sink selection rectangles
>
> Move the crop rectangle to the sink pad and add a sink compose rectangle
> to configure scaling. Also propagate rectangles from sink pad to crop
> rectangle, to compose rectangle, and to the source pads both in ACTIVE
> and TRY variants of set_fmt/selection, and initialize the default crop
> and compose rectangles.
>


I applied this patch.

I noticed Philipp fixed a bug in the sink->source pad format
propagation. I was only propagating the active runs, and not
the trial runs. I fixed this everywhere in the pipeline, applied
to the "propagate sink pad formats to source pads" patch. Try
runs should now work correctly across the whole pipeline, but
I haven't tested this.

Steve

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

#1604849

On Mon, 2017-03-20 at 14:17 +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> > > The same document says:
> > > 
> > >   Scaling support is optional. When supported by a subdev, the crop
> > >   rectangle on the subdev's sink pad is scaled to the size configured
> > >   using the
> > >   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
> > >   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> > >   subdev supports scaling but not composing, the top and left values are
> > >   not used and must always be set to zero.
> > 
> > Right, this sentence does imply that when scaling is supported, there
> > must be a sink compose rectangle, even when composing is not.
> > 
> > I have previously set up scaling like this:
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > Does this mean, it should work like this instead?
> > 
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> > 
> > I suppose setting the source pad format should not be allowed to modify
> > the sink compose rectangle.
> 
> That is what I believe having read these documents several times, but
> we need v4l2 people to confirm.
> 
> Note that setting the format on 'ipu1_csi0':0 should already be done by
> the previous media-ctl command, so it should be possible to simplify
> that to:
> 
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

Thanks, that works, too.

> I have tripped over a bug in media-ctl when specifying both a crop and
> compose rectangle - the --help output suggests that "," should be used
> to separate them.  media-ctl rejects that, telling me the character at
> the "," should be "]".  Replacing the "," with " " allows media-ctl to
> accept it and set both rectangles, so it sounds like a parser bug - I've
> not looked into this any further yet.

I can confirm this. I don't see any place in
v4l2_subdev_parse_pad_format that handles the "," separator. There's
just whitespace skipping between the v4l2-properties.

regards
Philipp

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

#1604943

On Mon, Mar 20, 2017 at 06:40:21PM +0100, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 14:17 +0000, Russell King - ARM Linux wrote:
> > I have tripped over a bug in media-ctl when specifying both a crop and
> > compose rectangle - the --help output suggests that "," should be used
> > to separate them.  media-ctl rejects that, telling me the character at
> > the "," should be "]".  Replacing the "," with " " allows media-ctl to
> > accept it and set both rectangles, so it sounds like a parser bug - I've
> > not looked into this any further yet.
> 
> I can confirm this. I don't see any place in
> v4l2_subdev_parse_pad_format that handles the "," separator. There's
> just whitespace skipping between the v4l2-properties.

Maybe this is the easiest solution:

 utils/media-ctl/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
index 83ca1ca..8b97874 100644
--- a/utils/media-ctl/options.c
+++ b/utils/media-ctl/options.c
@@ -65,7 +65,7 @@ static void usage(const char *argv0)
 	printf("\tentity          = entity-number | ( '\"' entity-name '\"' ) ;\n");
 	printf("\n");
 	printf("\tv4l2            = pad '[' v4l2-properties ']' ;\n");
-	printf("\tv4l2-properties = v4l2-property { ',' v4l2-property } ;\n");
+	printf("\tv4l2-properties = v4l2-property { ' '* v4l2-property } ;\n");
 	printf("\tv4l2-property   = v4l2-mbusfmt | v4l2-crop | v4l2-interval\n");
 	printf("\t                | v4l2-compose | v4l2-interval ;\n");
 	printf("\tv4l2-mbusfmt    = 'fmt:' fcc '/' size ; { 'field:' v4l2-field ; } { 'colorspace:' v4l2-colorspace ; }\n");

;)

-- 
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]

#1604635

On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 12:55:26PM +0100, Philipp Zabel wrote:
> > The above paragraph suggests we skip any rectangles that are not
> > supported. In our case that would be 3. and 4., since the CSI can't
> > compose into a larger frame. I hadn't realised that the crop selection
> > currently happens on the source pad.
> 
> I'd recommend viewing the documentation in its post-processed version,
> because then you get the examples as pictures, and they say that a
> picture is worth 1000 words.  See
> 
>   https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html
> 
> There is almost an exact example of what we're trying to do - it's
> figure 4.6.  Here, we have a sink pad with a cropping rectangle on
> the input, which is then scaled to a composition rectangle (there's
> no bounds rectangle, and it's specified that in such a case the
> top,left of the composition rectangle will always be 0,0 - see quote
> below).
> 
> Where it differs is that the example also supports source cropping
> for two source pads.  We don't support that.
>
> The same document says:
> 
>   Scaling support is optional. When supported by a subdev, the crop
>   rectangle on the subdev's sink pad is scaled to the size configured
>   using the
>   :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
>   using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
>   subdev supports scaling but not composing, the top and left values are
>   not used and must always be set to zero.

Right, this sentence does imply that when scaling is supported, there
must be a sink compose rectangle, even when composing is not.

I have previously set up scaling like this:

media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

Does this mean, it should work like this instead?

media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"

I suppose setting the source pad format should not be allowed to modify
the sink compose rectangle.

regards
Philipp

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

#1605106

On 03/20/2017 07:00 AM, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 12:08 +0000, Russell King - ARM Linux wrote:
>> On Mon, Mar 20, 2017 at 12:55:26PM +0100, Philipp Zabel wrote:
>>> The above paragraph suggests we skip any rectangles that are not
>>> supported. In our case that would be 3. and 4., since the CSI can't
>>> compose into a larger frame. I hadn't realised that the crop selection
>>> currently happens on the source pad.
>> I'd recommend viewing the documentation in its post-processed version,
>> because then you get the examples as pictures, and they say that a
>> picture is worth 1000 words.  See
>>
>>    https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html
>>
>> There is almost an exact example of what we're trying to do - it's
>> figure 4.6.  Here, we have a sink pad with a cropping rectangle on
>> the input, which is then scaled to a composition rectangle (there's
>> no bounds rectangle, and it's specified that in such a case the
>> top,left of the composition rectangle will always be 0,0 - see quote
>> below).
>>
>> Where it differs is that the example also supports source cropping
>> for two source pads.  We don't support that.
>>
>> The same document says:
>>
>>    Scaling support is optional. When supported by a subdev, the crop
>>    rectangle on the subdev's sink pad is scaled to the size configured
>>    using the
>>    :ref:`VIDIOC_SUBDEV_S_SELECTION <VIDIOC_SUBDEV_G_SELECTION>` IOCTL
>>    using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
>>    subdev supports scaling but not composing, the top and left values are
>>    not used and must always be set to zero.
> Right, this sentence does imply that when scaling is supported, there
> must be a sink compose rectangle, even when composing is not.

Ok, this all makes consistent sense to me too. So:

- the CSI hardware cropping rectangle should be specified via the
   sink pad crop selection.

- the CSI hardware /2 downscaler should be specified via the
   sink pad compose selection.

- the final source pad rectangle is the same as the sink pad
   compose rectangle.

So that leaves only step 4 (source pad crop selection) as
unsupported.

Steve


> I have previously set up scaling like this:
>
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
>
> Does this mean, it should work like this instead?
>
> media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
>
> I suppose setting the source pad format should not be allowed to modify
> the sink compose rectangle.
>
> regards
> Philipp
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web