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

[Tomasz Figa]

On Wed, Oct 7, 2020 at 9:33 PM <vgarodia@xxxxxxxxxxxxxx> wrote:
>
> Hi Tomasz,
>
> On 2020-10-01 20:47, Tomasz Figa wrote:
> > On Thu, Oct 1, 2020 at 3:32 AM Stanimir Varbanov
> > <stanimir.varbanov@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:
> >> >>
> >> >> From: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> >> >>
> >> >> - 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@xxxxxxxxxxxxxx>
> >> >> ---
> >> >>  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
>
> Assuming the bitstream height and width would be configured with capture
> plane
> setting (s_fmt), configuring the crop as height/width would indicate to
> venus
> hardware as scaling. To distinguish scaling with crop, firmware needs to
> be
> configured separately indicating crop rectangle.

The V4L2 encoder API does _not_ configure the bitstream width and
height currently. Scaling is not defined in the API at the moment. As
per the spec [1], the CAPTURE width and height fields are
ignored/read-only.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-encoder.html#initialization

Currently there are following parameters configured by the V4L2 API:

OUTPUT format width: the number of pixels per line of the input
buffer, including any padding pixels, i.e. stride in pixels,
OUTPUT format height: the total number of lines of the input buffer.
including or not, any padding lines (for NV12 non-M format any padding
lines must be included, as plane offsets are calculated based on
this),
CROP left, width: horizontal position of valid pixel data in the
buffer; left is typically 0 and width can be less than OUTPUT format
width,
CROP top, height: vertical position of valid pixel data in the buffer:
top is typically 0 and height can be less than OUTPUT format 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?
>
> Yes, the hardware is used and also supported in android. The interface
> to configure
> crop rectangle to firmware is via extradata. This extradata info is
> passed from v4l2
> clients via a separate plane in v4l2 buffer. The extradata payload is
> passed to
> firmware as is and the firmware parses it to know if crop, roi, etc.

Okay, so do I get it correctly that without extradata, the firmware
can only handle the case where width == stride?

If so, it sounds like this extradata should be generated by the driver
internally based on the selection CROP rectangle. In fact, the driver
already seems to have a definition of struct hfi_extradata_input_crop
[2]. So perhaps it wouldn't require much effort to implement the crop
properly?

[2] https://elixir.bootlin.com/linux/v5.9-rc8/source/drivers/media/platform/qcom/venus/hfi_helper.h#L817

Best regards,
Tomasz