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

From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
To: Jeffrey Kardatzke <jkardatzke@google.com>, linux-media@vger.kernel.org
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Malathi Gottam <mgottam@codeaurora.org>
Subject: Re: [PATCH v3] media: venus: add support for selection rectangles
Date: Mon, 24 Feb 2020 13:02:30 +0200	[thread overview]
Message-ID: <4e9cad3b-3e01-3b02-1050-b0428a632744@linaro.org> (raw)
In-Reply-To: <CA+ddPcOMpiHU=2bumCe93NzLe-uO04TnhCS+5CDDU4GyLXYCNA@mail.gmail.com>

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