[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Minghsiu Tsai]

From: Daniel Kurtz <djkurtz@chromium.org>

Experiments show that the:
 (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
 (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>

---
Changes in v2:
. Can not use *_MPLANE type in g_/s_selection 
---
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index 13afe48..e18ac626 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -838,10 +838,10 @@ static int mtk_mdp_m2m_g_selection(struct file *file, void *fh,
 	bool valid = false;
 
 	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
-		if (mtk_mdp_is_target_compose(s->target))
+		if (mtk_mdp_is_target_crop(s->target))
 			valid = true;
 	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
-		if (mtk_mdp_is_target_crop(s->target))
+		if (mtk_mdp_is_target_compose(s->target))
 			valid = true;
 	}
 	if (!valid) {
@@ -908,10 +908,10 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
 	bool valid = false;
 
 	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
-		if (s->target == V4L2_SEL_TGT_COMPOSE)
+		if (s->target == V4L2_SEL_TGT_CROP)
 			valid = true;
 	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
-		if (s->target == V4L2_SEL_TGT_CROP)
+		if (s->target == V4L2_SEL_TGT_COMPOSE)
 			valid = true;
 	}
 	if (!valid) {
@@ -925,7 +925,7 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	if (mtk_mdp_is_target_crop(s->target))
+	if (mtk_mdp_is_target_compose(s->target))
 		frame = &ctx->s_frame;
 	else
 		frame = &ctx->d_frame;
-- 
1.9.1

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Minghsiu Tsai]

Hi, Hans,

Would you have time to review this patch v2?
The patch v1 violates v4l2 spec. I have fixed it in v2.


Sincerely,
Ming Hsiu

On Fri, 2017-05-12 at 10:42 +0800, Minghsiu Tsai wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Experiments show that the:
>  (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
>  (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> 
> ---
> Changes in v2:
> . Can not use *_MPLANE type in g_/s_selection 
> ---
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 13afe48..e18ac626 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -838,10 +838,10 @@ static int mtk_mdp_m2m_g_selection(struct file *file, void *fh,
>  	bool valid = false;
>  
>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -		if (mtk_mdp_is_target_compose(s->target))
> +		if (mtk_mdp_is_target_crop(s->target))
>  			valid = true;
>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> -		if (mtk_mdp_is_target_crop(s->target))
> +		if (mtk_mdp_is_target_compose(s->target))
>  			valid = true;
>  	}
>  	if (!valid) {
> @@ -908,10 +908,10 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>  	bool valid = false;
>  
>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -		if (s->target == V4L2_SEL_TGT_COMPOSE)
> +		if (s->target == V4L2_SEL_TGT_CROP)
>  			valid = true;
>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> -		if (s->target == V4L2_SEL_TGT_CROP)
> +		if (s->target == V4L2_SEL_TGT_COMPOSE)
>  			valid = true;
>  	}
>  	if (!valid) {
> @@ -925,7 +925,7 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>  	if (ret)
>  		return ret;
>  
> -	if (mtk_mdp_is_target_crop(s->target))
> +	if (mtk_mdp_is_target_compose(s->target))
>  		frame = &ctx->s_frame;
>  	else
>  		frame = &ctx->d_frame;

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Hans Verkuil]

On 06/15/17 08:29, Minghsiu Tsai wrote:
> Hi, Hans,
> 
> Would you have time to review this patch v2?
> The patch v1 violates v4l2 spec. I have fixed it in v2.

I plan to review it Friday or Monday.

Regards,

	Hans

> 
> 
> Sincerely,
> Ming Hsiu
> 
> On Fri, 2017-05-12 at 10:42 +0800, Minghsiu Tsai wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Experiments show that the:
>>  (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
>>  (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
>> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
>>
>> ---
>> Changes in v2:
>> . Can not use *_MPLANE type in g_/s_selection 
>> ---
>>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
>> index 13afe48..e18ac626 100644
>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
>> @@ -838,10 +838,10 @@ static int mtk_mdp_m2m_g_selection(struct file *file, void *fh,
>>  	bool valid = false;
>>  
>>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>> -		if (mtk_mdp_is_target_compose(s->target))
>> +		if (mtk_mdp_is_target_crop(s->target))
>>  			valid = true;
>>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>> -		if (mtk_mdp_is_target_crop(s->target))
>> +		if (mtk_mdp_is_target_compose(s->target))
>>  			valid = true;
>>  	}
>>  	if (!valid) {
>> @@ -908,10 +908,10 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>>  	bool valid = false;
>>  
>>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>> -		if (s->target == V4L2_SEL_TGT_COMPOSE)
>> +		if (s->target == V4L2_SEL_TGT_CROP)
>>  			valid = true;
>>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>> -		if (s->target == V4L2_SEL_TGT_CROP)
>> +		if (s->target == V4L2_SEL_TGT_COMPOSE)
>>  			valid = true;
>>  	}
>>  	if (!valid) {
>> @@ -925,7 +925,7 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (mtk_mdp_is_target_crop(s->target))
>> +	if (mtk_mdp_is_target_compose(s->target))
>>  		frame = &ctx->s_frame;
>>  	else
>>  		frame = &ctx->d_frame;
> 
> 

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Hans Verkuil]

On 05/12/17 04:42, Minghsiu Tsai wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Experiments show that the:
>  (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT

Please drop this, since this no longer applies to this patch.

>  (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets

Are you really certain about this?

For m2m devices the output (i.e. memory to hardware) typically crops from memory
and the capture side (hardware to memory) composes into memory.

I.e.: for the output side you crop the part of the memory buffer that you want
to process and on the capture side you compose the result into a memory buffer:
i.e. the memory buffer might be 1920x1080, but you compose the decoder output
into a rectangle of 640x480 at offset 128x128 within that buffer (just an example).

CAPTURE using crop would be if, before the data is DMAed, the hardware decoder
output is cropped. E.g. if the stream fed to the decoder is 1920x1080, but you
want to only DMA a subselection of that, then that would be cropping, and it
would go to a memory buffer of the size of the crop selection.

OUTPUT using compose is highly unlikely: that means that the frame you give
is composed in a larger internal buffer with generated border data around it.
Very rare and really only something that a compositor of some sort would do.

What exactly does the hardware do? Both for the encoder and for the decoder
case. Perhaps if I knew exactly what that is, then I can advise.

Regards,

	Hans

> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> 
> ---
> Changes in v2:
> . Can not use *_MPLANE type in g_/s_selection 
> ---
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 13afe48..e18ac626 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -838,10 +838,10 @@ static int mtk_mdp_m2m_g_selection(struct file *file, void *fh,
>  	bool valid = false;
>  
>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -		if (mtk_mdp_is_target_compose(s->target))
> +		if (mtk_mdp_is_target_crop(s->target))
>  			valid = true;
>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> -		if (mtk_mdp_is_target_crop(s->target))
> +		if (mtk_mdp_is_target_compose(s->target))
>  			valid = true;
>  	}
>  	if (!valid) {
> @@ -908,10 +908,10 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>  	bool valid = false;
>  
>  	if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -		if (s->target == V4L2_SEL_TGT_COMPOSE)
> +		if (s->target == V4L2_SEL_TGT_CROP)
>  			valid = true;
>  	} else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> -		if (s->target == V4L2_SEL_TGT_CROP)
> +		if (s->target == V4L2_SEL_TGT_COMPOSE)
>  			valid = true;
>  	}
>  	if (!valid) {
> @@ -925,7 +925,7 @@ static int mtk_mdp_m2m_s_selection(struct file *file, void *fh,
>  	if (ret)
>  		return ret;
>  
> -	if (mtk_mdp_is_target_crop(s->target))
> +	if (mtk_mdp_is_target_compose(s->target))
>  		frame = &ctx->s_frame;
>  	else
>  		frame = &ctx->d_frame;
> 

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Minghsiu Tsai]

Hi, Hans,

On Fri, 2017-06-16 at 12:42 +0200, Hans Verkuil wrote:
> On 05/12/17 04:42, Minghsiu Tsai wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > Experiments show that the:
> >  (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
> 
> Please drop this, since this no longer applies to this patch.
> 

I will remove it next version


> >  (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
> 
> Are you really certain about this?
> 
> For m2m devices the output (i.e. memory to hardware) typically crops from memory
> and the capture side (hardware to memory) composes into memory.
> 
> I.e.: for the output side you crop the part of the memory buffer that you want
> to process and on the capture side you compose the result into a memory buffer:
> i.e. the memory buffer might be 1920x1080, but you compose the decoder output
> into a rectangle of 640x480 at offset 128x128 within that buffer (just an example).
> 
> CAPTURE using crop would be if, before the data is DMAed, the hardware decoder
> output is cropped. E.g. if the stream fed to the decoder is 1920x1080, but you
> want to only DMA a subselection of that, then that would be cropping, and it
> would go to a memory buffer of the size of the crop selection.
> 
> OUTPUT using compose is highly unlikely: that means that the frame you give
> is composed in a larger internal buffer with generated border data around it.
> Very rare and really only something that a compositor of some sort would do.
> 

That's strange. In v4l2-ioctl.c, v4l_g_crop()
OUTPUT is using COMPOSE
CAPTURE is using CROP

static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
				struct file *file, void *fh, void *arg)
...
	/* crop means compose for output devices */
	if (V4L2_TYPE_IS_OUTPUT(p->type))
		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
	else
		s.target = V4L2_SEL_TGT_CROP_ACTIVE;

	ret = ops->vidioc_g_selection(file, fh, &s);


> What exactly does the hardware do? Both for the encoder and for the decoder
> case. Perhaps if I knew exactly what that is, then I can advise.
> 

NV12M/YUV420M/MT21 -> MDP -> NV12M/YUV420M

The usage would be like this:
For decoder:
decoder -> MT21 -> MDP -> NV12M/YUV420M

For encoder:
NV12M/YUV420M -> MDP -> NV12M/YUV420M -> encoder



> Regards,
> 
> 	Hans
> 

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Hans Verkuil]

On 06/19/2017 12:03 PM, Minghsiu Tsai wrote:
> Hi, Hans,
> 
> On Fri, 2017-06-16 at 12:42 +0200, Hans Verkuil wrote:
>> On 05/12/17 04:42, Minghsiu Tsai wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> Experiments show that the:
>>>   (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
>>
>> Please drop this, since this no longer applies to this patch.
>>
> 
> I will remove it next version
> 
> 
>>>   (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
>>
>> Are you really certain about this?
>>
>> For m2m devices the output (i.e. memory to hardware) typically crops from memory
>> and the capture side (hardware to memory) composes into memory.
>>
>> I.e.: for the output side you crop the part of the memory buffer that you want
>> to process and on the capture side you compose the result into a memory buffer:
>> i.e. the memory buffer might be 1920x1080, but you compose the decoder output
>> into a rectangle of 640x480 at offset 128x128 within that buffer (just an example).
>>
>> CAPTURE using crop would be if, before the data is DMAed, the hardware decoder
>> output is cropped. E.g. if the stream fed to the decoder is 1920x1080, but you
>> want to only DMA a subselection of that, then that would be cropping, and it
>> would go to a memory buffer of the size of the crop selection.
>>
>> OUTPUT using compose is highly unlikely: that means that the frame you give
>> is composed in a larger internal buffer with generated border data around it.
>> Very rare and really only something that a compositor of some sort would do.
>>
> 
> That's strange. In v4l2-ioctl.c, v4l_g_crop()
> OUTPUT is using COMPOSE
> CAPTURE is using CROP

The old crop API can't be used with most (all?) codec drivers. Codec drivers do
exactly the opposite of what the old crop API did.

It is the reason the selection API was created.

The old crop API was written for SDTV receivers where the hardware would crop
the received video. For output (rarely used) the hardware would compose the
image into a larger (PAL or NTSC sizes) frame.

Applications have to use the selection API to work with codec drivers. Just
ignore the old crop ioctls, they are not suitable for such hardware.

> 
> static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> 				struct file *file, void *fh, void *arg)
> ...
> 	/* crop means compose for output devices */
> 	if (V4L2_TYPE_IS_OUTPUT(p->type))
> 		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> 	else
> 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> 
> 	ret = ops->vidioc_g_selection(file, fh, &s);
> 
> 
>> What exactly does the hardware do? Both for the encoder and for the decoder
>> case. Perhaps if I knew exactly what that is, then I can advise.
>>
> 
> NV12M/YUV420M/MT21 -> MDP -> NV12M/YUV420M
> 
> The usage would be like this:
> For decoder:
> decoder -> MT21 -> MDP -> NV12M/YUV420M
> 
> For encoder:
> NV12M/YUV420M -> MDP -> NV12M/YUV420M -> encoder

That doesn't help. Where in these two chains does the cropping (encoder) or
composing (decoder) take place? I assume it is the DMA engine that read from
a rectangle in the source buffer (encoder) or writes to a rectangle in the
destination buffer (decoder).

And in that case the current driver is correct.

I wonder if the g/s_crop description in the V4L2 Specification should get a
big fat warning that it doesn't apply to codec drivers and that the selection
API should be used instead. A patch for that is welcome!

Regards,

	Hans

[PATCH v2] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[Minghsiu Tsai]

On Mon, 2017-06-19 at 12:10 +0200, Hans Verkuil wrote:
> On 06/19/2017 12:03 PM, Minghsiu Tsai wrote:
> > Hi, Hans,
> > 
> > On Fri, 2017-06-16 at 12:42 +0200, Hans Verkuil wrote:
> >> On 05/12/17 04:42, Minghsiu Tsai wrote:
> >>> From: Daniel Kurtz <djkurtz@chromium.org>
> >>>
> >>> Experiments show that the:
> >>>   (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
> >>
> >> Please drop this, since this no longer applies to this patch.
> >>
> > 
> > I will remove it next version
> > 
> > 
> >>>   (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
> >>
> >> Are you really certain about this?
> >>
> >> For m2m devices the output (i.e. memory to hardware) typically crops from memory
> >> and the capture side (hardware to memory) composes into memory.
> >>
> >> I.e.: for the output side you crop the part of the memory buffer that you want
> >> to process and on the capture side you compose the result into a memory buffer:
> >> i.e. the memory buffer might be 1920x1080, but you compose the decoder output
> >> into a rectangle of 640x480 at offset 128x128 within that buffer (just an example).
> >>
> >> CAPTURE using crop would be if, before the data is DMAed, the hardware decoder
> >> output is cropped. E.g. if the stream fed to the decoder is 1920x1080, but you
> >> want to only DMA a subselection of that, then that would be cropping, and it
> >> would go to a memory buffer of the size of the crop selection.
> >>
> >> OUTPUT using compose is highly unlikely: that means that the frame you give
> >> is composed in a larger internal buffer with generated border data around it.
> >> Very rare and really only something that a compositor of some sort would do.
> >>
> > 
> > That's strange. In v4l2-ioctl.c, v4l_g_crop()
> > OUTPUT is using COMPOSE
> > CAPTURE is using CROP
> 
> The old crop API can't be used with most (all?) codec drivers. Codec drivers do
> exactly the opposite of what the old crop API did.
> 
> It is the reason the selection API was created.
> 
> The old crop API was written for SDTV receivers where the hardware would crop
> the received video. For output (rarely used) the hardware would compose the
> image into a larger (PAL or NTSC sizes) frame.
> 
> Applications have to use the selection API to work with codec drivers. Just
> ignore the old crop ioctls, they are not suitable for such hardware.
> 

Hi, Hans,
  Thanks for your review and comment. 
  Conclusion is application uses wrong ioctls, g/s_crop.

> > 
> > static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> > 				struct file *file, void *fh, void *arg)
> > ...
> > 	/* crop means compose for output devices */
> > 	if (V4L2_TYPE_IS_OUTPUT(p->type))
> > 		s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> > 	else
> > 		s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> > 
> > 	ret = ops->vidioc_g_selection(file, fh, &s);
> > 
> > 
> >> What exactly does the hardware do? Both for the encoder and for the decoder
> >> case. Perhaps if I knew exactly what that is, then I can advise.
> >>
> > 
> > NV12M/YUV420M/MT21 -> MDP -> NV12M/YUV420M
> > 
> > The usage would be like this:
> > For decoder:
> > decoder -> MT21 -> MDP -> NV12M/YUV420M
> > 
> > For encoder:
> > NV12M/YUV420M -> MDP -> NV12M/YUV420M -> encoder
> 
> That doesn't help. Where in these two chains does the cropping (encoder) or
> composing (decoder) take place? I assume it is the DMA engine that read from
> a rectangle in the source buffer (encoder) or writes to a rectangle in the
> destination buffer (decoder).
> 

 Yes. they are only buffer transfer between decoder, mdp and encoder.


> And in that case the current driver is correct.

 Next step, we will try to refine framework in userspace code.


> I wonder if the g/s_crop description in the V4L2 Specification should get a
> big fat warning that it doesn't apply to codec drivers and that the selection
> API should be used instead. A patch for that is welcome!
>
> Regards,
> 
> 	Hans