[PATCH v3] media: venus: add support for selection rectangles

[PATCH v3] media: venus: add support for selection rectangles

[Jeffrey Kardatzke]

From: Malathi Gottam <>

Handles target type crop by setting the new active rectangle
to hardware. The new rectangle should be within YUV size.

This was taken from: https://lkml.org/lkml/2018/11/9/899

Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
---
 drivers/media/platform/qcom/venus/venc.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 453edf966d4f..73b3181eed9a 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -479,10 +479,26 @@ venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
 
 	switch (s->target) {
 	case V4L2_SEL_TGT_CROP:
-		if (s->r.width != inst->out_width ||
-		    s->r.height != inst->out_height ||
-		    s->r.top != 0 || s->r.left != 0)
-			return -EINVAL;
+		if (s->r.left != 0) {
+			s->r.width += s->r.left;
+			s->r.left = 0;
+		}
+
+		if (s->r.top != 0) {
+			s->r.height += s->r.top;
+			s->r.top = 0;
+		}
+
+		if (s->r.width > inst->width)
+			s->r.width = inst->width;
+		else
+			inst->width = s->r.width;
+
+		if (s->r.height > inst->height)
+			s->r.height = inst->height;
+		else
+			inst->height = s->r.height;
+
 		break;
 	default:
 		return -EINVAL;
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH v3] media: venus: add support for selection rectangles

[Jeffrey Kardatzke]

(again, sorry for duplicate to some people, didn't know the plain text
email rule before)
There were a few comments made when this patch was originally posted
that were not addressed.  I left the patch as it last stood, except
for removing the unused variables.

The outstanding comments were:
1. In venc_init_session, it is only using the inst->width/height
rectangles and not the inst->out_width/out_height rectangles. So there
was a question about whether the OUTUPT rectangle should be set to
out_width/out_height. I'm looking for feedback here as I'm not
familiar enough with this driver code yet.
2. We should return EBUSY if the selection rectangles are changed
after we are initialized (I will update it to do this)
3. Support for non-zero top/left parameters. I'm suspicious that the
HFI_INDEX_EXTRADATA_INPUT_CROP property is what should be used for
that, but it's not currently used anywhere. Does anybody have details
on if that's what it's for?


On Tue, Feb 18, 2020 at 10:42 AM Jeffrey Kardatzke
<jkardatzke@google.com> wrote:
>
> From: Malathi Gottam <>
>
> Handles target type crop by setting the new active rectangle
> to hardware. The new rectangle should be within YUV size.
>
> This was taken from: https://lkml.org/lkml/2018/11/9/899
>
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 453edf966d4f..73b3181eed9a 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -479,10 +479,26 @@ venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
>
>         switch (s->target) {
>         case V4L2_SEL_TGT_CROP:
> -               if (s->r.width != inst->out_width ||
> -                   s->r.height != inst->out_height ||
> -                   s->r.top != 0 || s->r.left != 0)
> -                       return -EINVAL;
> +               if (s->r.left != 0) {
> +                       s->r.width += s->r.left;
> +                       s->r.left = 0;
> +               }
> +
> +               if (s->r.top != 0) {
> +                       s->r.height += s->r.top;
> +                       s->r.top = 0;
> +               }
> +
> +               if (s->r.width > inst->width)
> +                       s->r.width = inst->width;
> +               else
> +                       inst->width = s->r.width;
> +
> +               if (s->r.height > inst->height)
> +                       s->r.height = inst->height;
> +               else
> +                       inst->height = s->r.height;
> +
>                 break;
>         default:
>                 return -EINVAL;
> --
> 2.25.0.265.gbab2e86ba0-goog
>

Re: [PATCH v3] media: venus: add support for selection rectangles

[Stanimir Varbanov]

Hi Jeff,

On 2/18/20 9:14 PM, Jeffrey Kardatzke wrote:
> (again, sorry for duplicate to some people, didn't know the plain text
> email rule before)
> There were a few comments made when this patch was originally posted
> that were not addressed.  I left the patch as it last stood, except
> for removing the unused variables.
> 
> The outstanding comments were:
> 1. In venc_init_session, it is only using the inst->width/height
> rectangles and not the inst->out_width/out_height rectangles. So there
> was a question about whether the OUTUPT rectangle should be set to
> out_width/out_height. I'm looking for feedback here as I'm not
> familiar enough with this driver code yet.

Yes, the observant reader will see that out_width/height (the encoder
input resolution) is not changed in set_selection method, and that is
simply because currently the implementation for crop is missing. And it
is missing because the so-called extradata is not implemented and not
used in the driver, yet.

In fact the proposed patch will just fake the userspace application to
think that it sets the crop rectangle but the cropping will not happen
at all.

> 2. We should return EBUSY if the selection rectangles are changed
> after we are initialized (I will update it to do this)

Not sure we need that.

> 3. Support for non-zero top/left parameters. I'm suspicious that the
> HFI_INDEX_EXTRADATA_INPUT_CROP property is what should be used for
> that, but it's not currently used anywhere. Does anybody have details
> on if that's what it's for?

This is the right property to be used to set encoder crop on the input,
but unfortunately it is not that simple.

Actually this property is used to enable the crop extradata handling in
the firmware side, but we need to take care of that extradata population
in the v4l2 driver. For that purpose we need an extradata buffer which
should be filled with extradata header plus the extradata itself.

I'm not sure how to implement that, still. There is a bunch of extradata
types where some of them could be filled by the driver but the others
could be filled by userspace.

For reference you can see how the crop is set on Android [1].

[1]
https://android.googlesource.com/platform/hardware/qcom/sdm845/media/+/refs/heads/master/mm-video-v4l2/vidc/venc/src/video_encoder_device_v4l2.cpp#669

> 
> 
> On Tue, Feb 18, 2020 at 10:42 AM Jeffrey Kardatzke
> <jkardatzke@google.com> wrote:
>>
>> From: Malathi Gottam <>
>>
>> Handles target type crop by setting the new active rectangle
>> to hardware. The new rectangle should be within YUV size.
>>
>> This was taken from: https://lkml.org/lkml/2018/11/9/899
>>
>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> Signed-off-by: Jeffrey Kardatzke <jkardatzke@google.com>
>> ---
>>  drivers/media/platform/qcom/venus/venc.c | 24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
>> index 453edf966d4f..73b3181eed9a 100644
>> --- a/drivers/media/platform/qcom/venus/venc.c
>> +++ b/drivers/media/platform/qcom/venus/venc.c
>> @@ -479,10 +479,26 @@ venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
>>
>>         switch (s->target) {
>>         case V4L2_SEL_TGT_CROP:
>> -               if (s->r.width != inst->out_width ||
>> -                   s->r.height != inst->out_height ||
>> -                   s->r.top != 0 || s->r.left != 0)
>> -                       return -EINVAL;
>> +               if (s->r.left != 0) {
>> +                       s->r.width += s->r.left;
>> +                       s->r.left = 0;
>> +               }
>> +
>> +               if (s->r.top != 0) {
>> +                       s->r.height += s->r.top;
>> +                       s->r.top = 0;
>> +               }
>> +
>> +               if (s->r.width > inst->width)
>> +                       s->r.width = inst->width;
>> +               else
>> +                       inst->width = s->r.width;
>> +
>> +               if (s->r.height > inst->height)
>> +                       s->r.height = inst->height;
>> +               else
>> +                       inst->height = s->r.height;
>> +
>>                 break;
>>         default:
>>                 return -EINVAL;
>> --
>> 2.25.0.265.gbab2e86ba0-goog
>>

-- 
regards,
Stan