[media][imx219] selection API

[vvarma]

This change primarily adds the set_selection APIs (crop and compose). While the hardware supports other binning ratios, only the BINNING_NONE, BINNING_2x2 and BINNING_2x2_ANALOG are supported. The rule to use the "special binning" is based on the frame size and whether it is an 8-bit format. At the same time with this change, the driver allows binning on x or y dimensions or both based on the crop and compose fields.

The biggest change in terms of the implementation is to reduce the scope of imx219->mode. Instead, the crop and compose are stored in the imx219 struct. All the usages of imx219->mode have been transitioned to use these fields instead except in the imx219_init_controls to set defaults. The supported_modes continues to be used for S_FMT operations.

One change in the API is that the v_blank value is not set to the default of the mode selected on S_FMT but is based on whatever the user set last. It should be possible to avoid this if not maintaining the v_blanking in the S_FMT op is a required feature.

Note

The commits are separated out in order to help review the code. I plan to squash all of them into 1 or 2 commits if and when it's close to getting approval unless the reviewers prefer I squash it right away.

Links

• imx219: implement v4l2 set selection api #4935

[6by9]

Sorry I'm going to have to direct this one upstream to the linux-media mailing list.

There is already at least one significant patch set on there (https://patchwork.linuxtv.org/project/linux-media/list/?series=11202), and accepting something like this downstream is going to lead to major merge conflicts when that hits (all have a Reviewed-by tag, so should be in the next merge cycle).

The other issue is that adding support for selection API on top of a mode based sensor driver was discussed within the linux-media community last September (https://www.spinics.net/lists/linux-media/msg219442.html). A basic route forward was proposed, but no implementation has been forthcoming since.

We have defined modes. Those can not be dropped as it would be considered a regression in stopping existing applications working.
It is undefined how a set_pad_format call should interact with a controllable selection crop, so if you set the crop to say 2x2 binned 1292x972, and then set the format to 2592x1944 you can't say what the driver should do.

One approach would be that setting the format is allowed to update the selections, thereby allowing you to retain enumeration and selection of the current modes, but also amend them afterwards via the selection API. Implementing something like that will require buy in from the linux-media maintainers though. I haven't followed through the complete details of your patch set to see if that is what you've implemented.

[vvarma]

Thanks for the quick response @6by9, replying inline -

Sorry I'm going to have to direct this one upstream to the linux-media mailing list.

There is already at least one significant patch set on there (https://patchwork.linuxtv.org/project/linux-media/list/?series=11202), and accepting something like this downstream is going to lead to major merge conflicts when that hits (all have a Reviewed-by tag, so should be in the next merge cycle).

Ah too bad! Looking at the changes in the patch set you shared, I think there shouldn't be too many major conflicts. I'll pull in those commits to my tree and try rebasing my changes on top of the same.

The other issue is that adding support for selection API on top of a mode based sensor driver was discussed within the linux-media community last September (https://www.spinics.net/lists/linux-media/msg219442.html). A basic route forward was proposed, but no implementation has been forthcoming since.

"Another alternative is a new API to cover freely configurable and mode-based
drivers. Backwards compatible, and must not break freely configurable drivers
that now work well. This seems to be the way forward, Jacopo will work towards
this."
Is there a document or some specs to refer to what the new APIs will look like? Personally (and with my limited experience), I found the current selection API to be quite capable when dealing with binning except when dealing with the special binning mode of the imx219 which has quite a few cases to handle.

We have defined modes. Those can not be dropped as it would be considered a regression in stopping existing applications working.
It is undefined how a set_pad_format call should interact with a controllable selection crop, so if you set the crop to say 2x2 binned 1292x972, and then set the format to 2592x1944 you can't say what the driver should do.

One approach would be that setting the format is allowed to update the selections, thereby allowing you to retain enumeration and selection of the current modes, but also amend them afterwards via the selection API. Implementing something like that will require buy in from the linux-media maintainers though. I haven't followed through the complete details of your patch set to see if that is what you've implemented.

That is indeed how I have implemented it. The enumerated formats are retained and the S_FMT allows only one of the enumerated modes to be set, maintaining backwards compatibility with existing users. This means, as you mentioned, that the set_pad_fmt is allowed to override the crop and compose targets. The expectation is that users of the selection API first set the format and then change the selection targets as required.

[6by9]

There is already at least one significant patch set on there (https://patchwork.linuxtv.org/project/linux-media/list/?series=11202), and accepting something like this downstream is going to lead to major merge conflicts when that hits (all have a Reviewed-by tag, so should be in the next merge cycle).

Ah too bad! Looking at the changes in the patch set you shared, I think there shouldn't be too many major conflicts. I'll pull in those commits to my tree and try rebasing my changes on top of the same.

The rpi-6.5.y branch should be working if you want to be closer to linux-media. There are a couple of fixups required for imx219 on rpi-6.6.y at present which should be sorted in the next few days.
You may be best taking rpi-6.6.y whilst it hasn't got the fixups, as Laurent's patches are more likely to apply cleanly to that.

The other issue is that adding support for selection API on top of a mode based sensor driver was discussed within the linux-media community last September (https://www.spinics.net/lists/linux-media/msg219442.html). A basic route forward was proposed, but no implementation has been forthcoming since.

"Another alternative is a new API to cover freely configurable and mode-based drivers. Backwards compatible, and must not break freely configurable drivers that now work well. This seems to be the way forward, Jacopo will work towards this." Is there a document or some specs to refer to what the new APIs will look like?

@jmondi You were the one who raised the subject of sensor mode configuration and selection API in Dublin last year. I'm guessing it's not gone anywhere as yet.

Personally (and with my limited experience), I found the current selection API to be quite capable when dealing with binning except when dealing with the special binning mode of the imx219 which has quite a few cases to handle.

It's these holes that mean that the current APIs don't fully work, and why the CCS becomes so much more complex with multiple subdevs (and AFAIK nothing uses it). There's also no way to select between binning and skipping.

[vvarma]

While testing this patch I also came across an issue that I do not yet know how to handle. I added it in this thread since it's related to the PR here, please let me know if I should move this elsewhere.
the v_blank ctrl, when set to its minimum with the special binning mode does not always work. To be more descriptive, I wrote a small test script that captures the issue

#!/bin/bash

function set_selection(){
  height=$1;
  printf "set height to %d " ${height}
  v4l2-ctl --set-selection target=crop,width=1280,height="$((height*2))"
  v4l2-ctl --set-selection target=compose,width=640,height="${height}"
}

function find_vts_limit(){
  vblank=32
  while true ; do
    v4l2-ctl -c vertical_blanking=${vblank}
    if timeout 1s v4l2-ctl --stream-mmap=2 --stream-count 10; then
      printf "Vertical blanking limit: %d\n" ${vblank}
      break
    fi
    vblank=$((vblank+4))
  done
}

# set initial format
v4l2-ctl -v pixelformat=RGGB,height=480,width=640

# reduce height using the selection api and find the vblank level at which the stream doesnt get stuck
height=480;
while [ $height -gt 0 ]; do
  set_selection $height
  find_vts_limit
  height=$((height-4))
done

Running this (results attached), between the compose height 480 and 256, the vblank of 32 works well. From there till 128, the minimum vblank is not working as intended but increases to 156. From 128 then all the way to the height of 4 the minimum vblank reduces till it reaches 32 again.

In the logs, I can see

[ 3145.603602] unicam 3f801000.csi: ISR: ISTA: 0x1, STA: 0xD001, sequence 264, lines done 0
[ 3145.603621] unicam 3f801000.csi: ISR: [0] Dropping frame, buffer not available at FS
[ 3145.603628] unicam 3f801000.csi: Scheduling dummy buffer for node 0

In the unicam driver, in the ISR there is a comment concerning this case

			 * a chance to swap buffers, likely due to having
			 * multiple interrupts occurring simultaneously (like FE
			 * + FS + LS). In this case, we cannot signal the buffer
			 * as complete, as the HW will reuse that buffer.

However, I do not understand these results and how they are affected by vblanking and specifically how there seems to be a point of inflexion at the height of 128.

limit_vts.txt

[6by9]

Welcome to the wonderful world of image sensors. They don't always behave in the logical way one might expect, hence your issue #5493 and subsequent PR #5498.

Sensor vendors generally provide the tables of validated mode registers for specific modes, and that is what they support. There are often niggles when you try and generalise them.

[vvarma]

The above issues were due to a bug in the code. Closing this PR for now as discussed.

[vvarma]

There is already at least one significant patch set on there (https://patchwork.linuxtv.org/project/linux-media/list/?series=11202), and accepting something like this downstream is going to lead to major merge conflicts when that hits (all have a Reviewed-by tag, so should be in the next merge cycle).

Ah too bad! Looking at the changes in the patch set you shared, I think there shouldn't be too many major conflicts. I'll pull in those commits to my tree and try rebasing my changes on top of the same.

The rpi-6.5.y branch should be working if you want to be closer to linux-media. There are a couple of fixups required for imx219 on rpi-6.6.y at present which should be sorted in the next few days. You may be best taking rpi-6.6.y whilst it hasn't got the fixups, as Laurent's patches are more likely to apply cleanly to that.

@6by9 I believe the patchset linked above has been merged into the rpi-6.7.7 branch, hence, I plan to redo the above PR. Based on the newer changes, I think it should be a fairly small PR. Please let me know if you have any suggestions for me going into the change set and whether I should be on the lookout for any other conflicting changes that you may be aware of.