Re: [PATCH v6 04/13] media: atmel: atmel-isc: implement media controller

[Eugen.Hristev]

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