Re: [PATCH] [media] imx: csi: retain current field order and colorimetry setting as default

[Hans Verkuil]

On 04/06/2017 06:01 PM, Philipp Zabel wrote:
> On Thu, 2017-04-06 at 17:43 +0200, Hans Verkuil wrote:
>> On 04/06/2017 04:54 PM, Philipp Zabel wrote:
>>> On Thu, 2017-04-06 at 16:20 +0200, Hans Verkuil wrote:
>>>> On 04/06/2017 03:55 PM, Philipp Zabel wrote:
>>>>> If the the field order is set to ANY in set_fmt, choose the currently
>>>>> set field order. If the colorspace is set to DEFAULT, choose the current
>>>>> colorspace.  If any of xfer_func, ycbcr_enc or quantization are set to
>>>>> DEFAULT, either choose the current setting, or the default setting for the
>>>>> new colorspace, if non-DEFAULT colorspace was given.
>>>>>
>>>>> This allows to let field order and colorimetry settings be propagated
>>>>> from upstream by calling media-ctl on the upstream entity source pad,
>>>>> and then call media-ctl on the sink pad to manually set the input frame
>>>>> interval, without changing the already set field order and colorimetry
>>>>> information.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>>>> ---
>>>>> This is based on imx-media-staging-md-v14, and it is supposed to allow
>>>>> configuring the pipeline with media-ctl like this:
>>>>>
>>>>> 1) media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY8_1X16/1920x1080]"
>>>>> 2) media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY8_1X16/1920x108]"
>>>>> 3) media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY8_1X16/1920x1080]"
>>>>> 4) media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY8_1X16/1920x1080@1/60]"
>>>>> 5) media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080@1/30]"
>>>>>
>>>>> Without having step 4) overwrite the colorspace and field order set on
>>>>> 'ipu1_csi0':0 by the propagation in step 3).
>>>>> ---
>>>>>  drivers/staging/media/imx/imx-media-csi.c | 34 +++++++++++++++++++++++++++++++
>>>>>  1 file changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>>>> index 64dc454f6b371..d94ce1de2bf05 100644
>>>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>>>> @@ -1325,6 +1325,40 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
>>>>>  	csi_try_fmt(priv, sensor, cfg, sdformat, crop, compose, &cc);
>>>>>  
>>>>>  	fmt = __csi_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
>>>>> +
>>>>> +	/* Retain current field setting as default */
>>>>> +	if (sdformat->format.field == V4L2_FIELD_ANY)
>>>>> +		sdformat->format.field = fmt->field;
>>>>
>>>> sdformat->format.field should never be FIELD_ANY. If it is, then that's a
>>>> subdev bug and I'm pretty sure FIELD_NONE was intended.
>>>
>>> This is the subdev. sdformat is passed in from userspace, so we have to
>>> deal with it being set to ANY. I'm trying hard right now not to return
>>> ANY though. The values in sdformat->format are applied to fmt down
>>> below.
>>
>> Do you have a git tree with this patch? It is really hard to review without
>> having the full imx-media-csi.c source.
> 
> The patch applies on top of
> 
>   https://github.com/slongerbeam/mediatree.git imx-media-staging-md-v14
> 
> I have uploaded a branch
> 
>   git://git.pengutronix.de/git/pza/linux imx-media-staging-md-v14+color
> 
> with the patch applied on top.
> 
>> I think one problem is that it is not clearly defined how subdevs and colorspace
>> information should work.

Ah, having the full source helped.

Ignore my previous review, it was incorrect.

I'll have to think about this some more. I'll get back to this, but it may take some
time since my vacation starts tomorrow. The spec is simply unclear about how to handle
this so we have to come up with some guidelines.

Regards,

	Hans