[snip]
>>> +static int isc_scaler_g_sel(struct v4l2_subdev *sd,
>>> + struct v4l2_subdev_state *sd_state,
>>> + struct v4l2_subdev_selection *sel)
>>> +{
>>> + struct isc_device *isc = container_of(sd, struct isc_device, scaler_sd);
>>> +
>>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>>> + return -EINVAL;
>>> +
>>> + if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
>>> + /* bounds are the input format received */
>>> + sel->r.height = isc->scaler_format[ISC_SCALER_PAD_SINK].height;
>>> + sel->r.width = isc->scaler_format[ISC_SCALER_PAD_SINK].width;
>>
>> I'll re-paste our discussion on v4 to make sure we're on the same page
>>
>> -------------------------------------------------------------------------------
>>>>> + if (sel->pad == ISC_SCALER_PAD_SOURCE)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (sel->target != V4L2_SEL_TGT_CROP_BOUNDS &&
>>>>> + sel->target != V4L2_SEL_TGT_CROP)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + sel->r.height = isc->max_height;
>>>>> + sel->r.width = isc->max_width;
>>>>
>>>> The CROP_BOUNDS should be set to the same size as the sink pad image format,
>>>> as it represents the maximum valid crop rectangle.
>>>>
>>>> TGT_CROP should report the configured crop rectangle which can be
>>>> intiialized to the same size as CROP_BOUNDS, if my understanding of
>>>> the spec is correct
>>>
>>> So you would like to have this differentiated, and report the
>>> CROP_BOUNDS to whatever is on the sink pad, and the TGT_CROP to what is
>>> reported now, the maximum size of the ISC frame .
>>> My understanding is correct ?
>>>
>>
>> I didn't know you have an HW limitation, so your _BOUNDS is not the
>> input image size but rather 3264x2464 ( == max_width x max_height).
>>
>> What I meant is that _BOUNDS should report the maximum rectangle size
>> that can be applied to the _CROP target. In you case you have an HW
>> limitation 3264x2464 and that's the largest rectangle you can apply.
>> TGT_CROP can be initialized to the same as _BOUND, but if you
>> implement s_selection it should report what has been there applied.
>> But as you don't implement s_selection yet, I think this is fine for
>> now. Maybe a little comment ?
>> -------------------------------------------------------------------------------
>>
>> I think then that _BOUNDS should be fixed to 3264x2464 and CROP to
>> src_fmt.
>>
>> The rest looks good! Thanks for pushing up to this version and thanks
>> for addressings comments!
>>
>> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>
>> Thanks
>> j
>
> Hello Jacopo,
>
> So BOUNDS goes to isc max H / max V, and CROP goes to format of the
> source pad.
> Got it.
>
> I will update with a v7 as soon as you give me your feedback on the
> patch 8/13 which still required some changes from v6.
>
> Thanks again for reviewing this !
With the incoming v7, media-ctl -p now looks like this:
Device topology
- entity 1: atmel_isc_scaler (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:SRGGB10_1X10/3280x2464 field:none colorspace:srgb
crop.bounds:(0,0)/3264x2464
crop:(0,0)/3264x2464]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/3264x2464 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
Bounds as maximum isc size: 3264x2464, crop as maximum isc size 3264x2464
In case e.g. the sensor is sending a stream at lower res, like
1920x1080, it will keep the same bounds:
Device topology
- entity 1: atmel_isc_scaler (2 pads, 2 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb
crop.bounds:(0,0)/3264x2464
crop:(0,0)/1920x1080]
<- "csi2dc":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:SRGGB10_1X10/1920x1080 field:none colorspace:srgb]
-> "atmel_isc_common":0 [ENABLED,IMMUTABLE]
>
> Eugen
>
[snip]
