Hi,
On 7/3/23 10:50, Dan Scally wrote:
> Hi Hans
>
> On 27/06/2023 15:18, Hans de Goede wrote:
>> Instead of using a long fixed register settings list for each resolution,
>> calculate the register settings based on the requested width + height.
>>
>> This is based on atomisp-ov2680 commit 0611888592df ("media: atomisp:
>> ov2680: Make setting the modes algorithm based").
>>
>> This will allow future enhancements like adding hblank and vblank controls
>> and adding selection support.
>>
>> This also adds properly prgramming the ISP window and setting
>
>
> s/prgramming/programming
>
>
> This looks mostly good but I have one query down below.
<snip>
>> @@ -609,15 +580,20 @@ static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
>> struct v4l2_subdev_state *sd_state,
>> struct v4l2_subdev_frame_size_enum *fse)
>> {
>> - int index = fse->index;
>> + static const struct v4l2_frmsize_discrete ov2680_frame_sizes[] = {
>> + { 1600, 1200 },
>> + { 1280, 720 },
>> + { 800, 600 },
>> + };
>> + u32 index = fse->index;
>> - if (index >= OV2680_MODE_MAX || index < 0)
>> + if (index >= ARRAY_SIZE(ov2680_frame_sizes))
>> return -EINVAL;
>> - fse->min_width = ov2680_mode_data[index].width;
>> - fse->min_height = ov2680_mode_data[index].height;
>> - fse->max_width = ov2680_mode_data[index].width;
>> - fse->max_height = ov2680_mode_data[index].height;
>> + fse->min_width = ov2680_frame_sizes[index].width;
>> + fse->min_height = ov2680_frame_sizes[index].height;
>> + fse->max_width = ov2680_frame_sizes[index].width;
>> + fse->max_height = ov2680_frame_sizes[index].height;
>> return 0;
>> }
>
>
> Unless I'm missing something, .set_fmt() will let you set any arbitrary frame size you like within the bounds of the ov2680's pixel array. That's good, but why then should this callback only report the three discrete sizes?
The later added selection (crop-target) support makes it possible for consumers of this driver to find out the real resolution + explicitly set a crop, rather then doing this through s_fmt.
Just like how the ov5693 driver is doing. The ov5693 driver just lists the full + binned size in ov2680_enum_frame_size().
I was a bit reluctant to do this here to break any potential existing users of the drivers, but :
1. Given the poor state of the driver before I doubt that there are any actual users
2. Jacopo has asked me to change the gain control CID which is also an API break
I think we might just as well go ahead here and only list the full + binned sizes like the ov5693 code does. So I'll go an update that for v4.
Regards,
Hans
