diff mbox

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

Message ID 1384871228-6648-15-git-send-email-j.anaszewski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jacek Anaszewski Nov. 19, 2013, 2:27 p.m. UTC 
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(+)

Comments

Hans Verkuil Nov. 19, 2013, 2:46 p.m. UTC | #1 
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.

Also, don't use v4l2_s_ctrl, instead use v4l2_ctrl_s_ctrl. The v4l2_s_ctrl
function is for core framework use only, not for use in drivers.

Regards,

	Hans

> +
>  	return vidioc_try_fmt(f, fmt, ctx, FMT_TYPE_OUTPUT);
>  }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacek Anaszewski Nov. 20, 2013, 1:47 p.m. UTC | #2 
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.

Regards,
Jacek Anaszewski


> Also, don't use v4l2_s_ctrl, instead use v4l2_ctrl_s_ctrl. The v4l2_s_ctrl
> function is for core framework use only, not for use in drivers.
>
> Regards,
>
> 	Hans
>
>> +
>>   	return vidioc_try_fmt(f, fmt, ctx, FMT_TYPE_OUTPUT);
>>   }
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Nov. 20, 2013, 2:13 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
+
 	return vidioc_try_fmt(f, fmt, ctx, FMT_TYPE_OUTPUT);
 }