Re: [PATCH 2/2] venus: venc: fix handlig of S_SELECTION and G_SELECTION

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Oct 1, 2020 at 3:32 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Tomasz,
>
> On 9/25/20 11:55 PM, Tomasz Figa wrote:
> > Hi Dikshita, Stanimir,
> >
> > On Thu, Sep 24, 2020 at 7:31 PM Dikshita Agarwal
> > <dikshita@codeaurora.org> wrote:
> >>
> >> From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >>
> >> - return correct width and height for G_SELECTION
> >> - if requested rectangle wxh doesn't match with capture port wxh
> >>   adjust the rectangle to supported wxh.
> >>
> >> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/venc.c | 20 ++++++++++++--------
> >>  1 file changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> >> index 7d2aaa8..a2cc12d 100644
> >> --- a/drivers/media/platform/qcom/venus/venc.c
> >> +++ b/drivers/media/platform/qcom/venus/venc.c
> >> @@ -463,13 +463,13 @@ static int venc_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> >>         switch (s->target) {
> >>         case V4L2_SEL_TGT_CROP_DEFAULT:
> >>         case V4L2_SEL_TGT_CROP_BOUNDS:
> >> -               s->r.width = inst->width;
> >> -               s->r.height = inst->height;
> >> -               break;
> >> -       case V4L2_SEL_TGT_CROP:
> >>                 s->r.width = inst->out_width;
> >>                 s->r.height = inst->out_height;
> >>                 break;
> >> +       case V4L2_SEL_TGT_CROP:
> >> +               s->r.width = inst->width;
> >> +               s->r.height = inst->height;
> >> +               break;
> >>         default:
> >>                 return -EINVAL;
> >>         }inter
> >> @@ -490,10 +490,14 @@ static int venc_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
> >>
> >>         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.width != inst->width ||
> >> +                   s->r.height != inst->height ||
> >> +                   s->r.top != 0 || s->r.left != 0) {
> >> +                       s->r.top = 0;
> >> +                       s->r.left = 0;
> >> +                       s->r.width = inst->width;
> >> +                       s->r.height = inst->height;
> >
> > What's the point of exposing the selection API if no selection can
> > actually be done?
>
> If someone can guarantee that dropping of s_selection will not break
> userspace applications I'm fine with removing it.

Indeed the specification could be made more clear about this. The
visible rectangle configuration is described as optional, so I'd
consider the capability to be optional as well.

Of course it doesn't change the fact that something that is optional
in the API may be mandatory for some specific integrations, like
Chrome OS or Android.

>
> I implemented g/s_selection with the idea to add crop functionality
> later because with current firmware interface it needs more work.

I suggested one thing internally, but not sure if it was understood correctly:

Most of the encoders only support partial cropping, with the rectangle
limited to top = 0 and left = 0, in other words, only setting the
visible width and height. This can be easily implemented on most of
the hardware, even those that don't have dedicated cropping
capability, by configuring the hardware as follows:

stride = CAPTURE format width (or bytesperline)
width = CROP width
height = CROP height

I believe Android requires the hardware to support stride and AFAIK
this hardware is also commonly used on Android, so perhaps it's
possible to achieve the above without any firmware changes?

Best regards,
Tomasz