Re: [PATCH 14/16] s5p-jpeg: Synchronize V4L2_CID_JPEG_CHROMA_SUBSAMPLING control value

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/20/13 14:47, Jacek Anaszewski wrote:
> On 11/19/2013 03:46 PM, Hans Verkuil wrote:
>> On 11/19/2013 03:27 PM, Jacek Anaszewski wrote:
>>> When output queue fourcc is set to any flavour of YUV,
>>> the V4L2_CID_JPEG_CHROMA_SUBSAMPLING control value as
>>> well as its in-driver cached counterpart have to be
>>> updated with the subsampling property of the format
>>> so as to be able to provide correct information to the
>>> user space and preclude setting an illegal subsampling
>>> mode for Exynos4x12 encoder.
>>>
>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>   drivers/media/platform/s5p-jpeg/jpeg-core.c |    5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> index 319be0c..d4db612 100644
>>> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
>>> @@ -1038,6 +1038,7 @@ static int s5p_jpeg_try_fmt_vid_out(struct file *file, void *priv,
>>>   {
>>>       struct s5p_jpeg_ctx *ctx = fh_to_ctx(priv);
>>>       struct s5p_jpeg_fmt *fmt;
>>> +    struct v4l2_control ctrl_subs;
>>>
>>>       fmt = s5p_jpeg_find_format(ctx, f->fmt.pix.pixelformat,
>>>                           FMT_TYPE_OUTPUT);
>>> @@ -1048,6 +1049,10 @@ static int s5p_jpeg_try_fmt_vid_out(struct file *file, void *priv,
>>>           return -EINVAL;
>>>       }
>>>
>>> +    ctrl_subs.id = V4L2_CID_JPEG_CHROMA_SUBSAMPLING;
>>> +    ctrl_subs.value = fmt->subsampling;
>>> +    v4l2_s_ctrl(priv, &ctx->ctrl_handler, &ctrl_subs);
>>
>> TRY_FMT should never have side-effects, so this isn't the correct
>> way of implementing this.
> 
> I am aware of it, but I couldn't have found more suitable place
> for implementing this. Below is the rationale standing behind
> such an implementation:
> 
>   - Exynos4x12 device doesn't generate an eoc interrupt if the
>     subsampling property of an output queue format is lower than the
>     target jpeg subsampling (e.g. V4L2_PIX_FMT_YUYV [4:2:2 subsampling]
>     and JPEG 4:4:4)
>   - It should be possible to inform the user space application that the
>     subsampling it wants to set is not supported with the current output
>     queue fourcc.
>   - It is possible that after calling S_EXT_CTRLS the application
>     will call S_FMT on output queue with different fourcc which will
>     change the allowed scope of JPEG subsampling settings. Let's assume
>     the following flow of ioctls:
>       - S_FMT V4L2_PIX_FMT_YUYV (4:2:2)
>       - S_EXT_CTRLS V4L2_JPEG_CHROMA_SUBSAMPLING_422
>       - S_FMT V4L2_PIX_FMT_YUV420
>     Now the JPEG subsampling set is illegal as 4:2:2 is lower than 4:2:0
>     (lower refers here to the lower number of luma samples assigned
>     to the single chroma sample). It is evident now that the change
>     of output queue fourcc entails change of the allowed scope of JPEG
>     subsampling settings. The way I implemented it reflects this
>     constraint precisely. We could go for adjusting the JPEG subsampling
>     e.g. in the device_run callback but the user space application
>     wouldn't know about it unless it called G_EXT_CTRLS ioctl after end
>     of conversion.
> 
> In view of the above it is clear that calling S_FMT in this case HAS
> side effect no matter whether we take it into account in the driver
> implementation or not. Nevertheless maybe there is some more elegant
> way of handling this problem I am not aware of. I am open to any
> interesting ideas.

I think you misunderstood me. It is OK that S_FMT changes the control,
but TRY_FMT shouldn't. So the code that changes the subsampling control
should be moved to s5p_jpeg_s_fmt_vid_out.

Regards,

	Hans