| Message ID | 1539071603-1588-1-git-send-email-mgottam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | media: venus: add support for selection rectangles | expand |
Hi Malathi, On 10/09/2018 10:53 AM, Malathi Gottam wrote: > Handles target type crop by setting the new active rectangle > to hardware. The new rectangle should be within YUV size. > > Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> > --- > drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c > index 3f50cd0..754c19a 100644 > --- a/drivers/media/platform/qcom/venus/venc.c > +++ b/drivers/media/platform/qcom/venus/venc.c > @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void *fh, struct v4l2_format *f) > venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s) > { > struct venus_inst *inst = to_inst(file); > + int ret; > + u32 buftype; > > if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > return -EINVAL; > > switch (s->target) { > case V4L2_SEL_TGT_CROP: > - if (s->r.width != inst->out_width || > - s->r.height != inst->out_height || > + 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) { > + buftype = HFI_BUFFER_OUTPUT; > + ret = venus_helper_set_output_resolution(inst, > + s->r.width, > + s->r.height, > + buftype); I'm afraid that set_output_resolution cannot be called at any time. Do you think we can set it after start_session?
On 2018-10-16 15:11, Stanimir Varbanov wrote: > Hi Malathi, > > On 10/09/2018 10:53 AM, Malathi Gottam wrote: >> Handles target type crop by setting the new active rectangle >> to hardware. The new rectangle should be within YUV size. >> >> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/venc.c >> b/drivers/media/platform/qcom/venus/venc.c >> index 3f50cd0..754c19a 100644 >> --- a/drivers/media/platform/qcom/venus/venc.c >> +++ b/drivers/media/platform/qcom/venus/venc.c >> @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void >> *fh, struct v4l2_format *f) >> venc_s_selection(struct file *file, void *fh, struct v4l2_selection >> *s) >> { >> struct venus_inst *inst = to_inst(file); >> + int ret; >> + u32 buftype; >> >> if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) >> return -EINVAL; >> >> switch (s->target) { >> case V4L2_SEL_TGT_CROP: >> - if (s->r.width != inst->out_width || >> - s->r.height != inst->out_height || >> + 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) { >> + buftype = HFI_BUFFER_OUTPUT; >> + ret = venus_helper_set_output_resolution(inst, >> + s->r.width, >> + s->r.height, >> + buftype); > > I'm afraid that set_output_resolution cannot be called at any time. Do > you think we can set it after start_session? Yes Stan, we can set output_resolution after the session has been initialization. As per the spec, this call s_selection is an optional step under Initialization procedure of encoder even before we request buffers. So I think setting output resolution in this api shouldn't cause any issue once we are confident on the instance state.
Hi Malathi, On 11/1/18 3:10 PM, mgottam@codeaurora.org wrote: > On 2018-10-16 15:11, Stanimir Varbanov wrote: >> Hi Malathi, >> >> On 10/09/2018 10:53 AM, Malathi Gottam wrote: >>> Handles target type crop by setting the new active rectangle >>> to hardware. The new rectangle should be within YUV size. >>> >>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> >>> --- >>> drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- >>> 1 file changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/venc.c >>> b/drivers/media/platform/qcom/venus/venc.c >>> index 3f50cd0..754c19a 100644 >>> --- a/drivers/media/platform/qcom/venus/venc.c >>> +++ b/drivers/media/platform/qcom/venus/venc.c >>> @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void >>> *fh, struct v4l2_format *f) >>> venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s) >>> { >>> struct venus_inst *inst = to_inst(file); >>> + int ret; >>> + u32 buftype; >>> >>> if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) >>> return -EINVAL; >>> >>> switch (s->target) { >>> case V4L2_SEL_TGT_CROP: >>> - if (s->r.width != inst->out_width || >>> - s->r.height != inst->out_height || >>> + 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) { >>> + buftype = HFI_BUFFER_OUTPUT; >>> + ret = venus_helper_set_output_resolution(inst, >>> + s->r.width, >>> + s->r.height, >>> + buftype); >> >> I'm afraid that set_output_resolution cannot be called at any time. Do >> you think we can set it after start_session? > > Yes Stan, we can set output_resolution after the session has been > initialization. > As per the spec, this call s_selection is an optional step under > Initialization > procedure of encoder even before we request buffers. What spec you are referring to? The spec for the encoders [1] or something else. > > So I think setting output resolution in this api shouldn't cause any > issue once > we are confident on the instance state. OK, but I want to test that on v1 and v3 as well (usually the behavior differs between versions).
On Fri, Nov 2, 2018 at 12:02 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Malathi, > > On 11/1/18 3:10 PM, mgottam@codeaurora.org wrote: > > On 2018-10-16 15:11, Stanimir Varbanov wrote: > >> Hi Malathi, > >> > >> On 10/09/2018 10:53 AM, Malathi Gottam wrote: > >>> Handles target type crop by setting the new active rectangle > >>> to hardware. The new rectangle should be within YUV size. > >>> > >>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> > >>> --- > >>> drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- > >>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/qcom/venus/venc.c > >>> b/drivers/media/platform/qcom/venus/venc.c > >>> index 3f50cd0..754c19a 100644 > >>> --- a/drivers/media/platform/qcom/venus/venc.c > >>> +++ b/drivers/media/platform/qcom/venus/venc.c > >>> @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void > >>> *fh, struct v4l2_format *f) > >>> venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s) > >>> { > >>> struct venus_inst *inst = to_inst(file); > >>> + int ret; > >>> + u32 buftype; > >>> > >>> if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > >>> return -EINVAL; > >>> > >>> switch (s->target) { > >>> case V4L2_SEL_TGT_CROP: > >>> - if (s->r.width != inst->out_width || > >>> - s->r.height != inst->out_height || > >>> + if (s->r.width > inst->out_width || > >>> + s->r.height > inst->out_height || > >>> s->r.top != 0 || s->r.left != 0) > >>> return -EINVAL; Note that returning -EINVAL is not what VIDIOC_S_SELECTION should do here. As per the general V4L2 spec [1], VIDIOC_S_SELECTION is expected to adjust the crop rectangle "as close as possible to the requested one". In this case that would likely mean something like this: 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->out_width) s->r.width = inst->out_width; if (s->r.height > inst->out_height) s->r.height = inst->out_height; [1] https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-selection.html > >>> + if (s->r.width != inst->width || > >>> + s->r.height != inst->height) { > >>> + buftype = HFI_BUFFER_OUTPUT; > >>> + ret = venus_helper_set_output_resolution(inst, > >>> + s->r.width, > >>> + s->r.height, > >>> + buftype); > >> > >> I'm afraid that set_output_resolution cannot be called at any time. Do > >> you think we can set it after start_session? > > > > Yes Stan, we can set output_resolution after the session has been > > initialization. > > As per the spec, this call s_selection is an optional step under > > Initialization > > procedure of encoder even before we request buffers. > > What spec you are referring to? The spec for the encoders [1] or > something else. For our convenience, Hans was nice enough to host a compiled version at: https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-encoder.html Best regards, Tomasz
On 2018-11-02 03:16, Tomasz Figa wrote: > On Fri, Nov 2, 2018 at 12:02 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Hi Malathi, >> >> On 11/1/18 3:10 PM, mgottam@codeaurora.org wrote: >> > On 2018-10-16 15:11, Stanimir Varbanov wrote: >> >> Hi Malathi, >> >> >> >> On 10/09/2018 10:53 AM, Malathi Gottam wrote: >> >>> Handles target type crop by setting the new active rectangle >> >>> to hardware. The new rectangle should be within YUV size. >> >>> >> >>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> >> >>> --- >> >>> drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- >> >>> 1 file changed, 17 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/drivers/media/platform/qcom/venus/venc.c >> >>> b/drivers/media/platform/qcom/venus/venc.c >> >>> index 3f50cd0..754c19a 100644 >> >>> --- a/drivers/media/platform/qcom/venus/venc.c >> >>> +++ b/drivers/media/platform/qcom/venus/venc.c >> >>> @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void >> >>> *fh, struct v4l2_format *f) >> >>> venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s) >> >>> { >> >>> struct venus_inst *inst = to_inst(file); >> >>> + int ret; >> >>> + u32 buftype; >> >>> >> >>> if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) >> >>> return -EINVAL; >> >>> >> >>> switch (s->target) { >> >>> case V4L2_SEL_TGT_CROP: >> >>> - if (s->r.width != inst->out_width || >> >>> - s->r.height != inst->out_height || >> >>> + if (s->r.width > inst->out_width || >> >>> + s->r.height > inst->out_height || >> >>> s->r.top != 0 || s->r.left != 0) >> >>> return -EINVAL; > > Note that returning -EINVAL is not what VIDIOC_S_SELECTION should do > here. > > As per the general V4L2 spec [1], VIDIOC_S_SELECTION is expected to > adjust the crop rectangle "as close as possible to the requested one". > In this case that would likely mean something like this: > > 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->out_width) > s->r.width = inst->out_width; > > if (s->r.height > inst->out_height) > s->r.height = inst->out_height; > > [1] > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-selection.html > Yes Tomasz, I overlooked the spec that we cannot return EINVAL in this case. I will make the necessary changes and update the new patch. >> >>> + if (s->r.width != inst->width || >> >>> + s->r.height != inst->height) { >> >>> + buftype = HFI_BUFFER_OUTPUT; >> >>> + ret = venus_helper_set_output_resolution(inst, >> >>> + s->r.width, >> >>> + s->r.height, >> >>> + buftype); >> >> >> >> I'm afraid that set_output_resolution cannot be called at any time. Do >> >> you think we can set it after start_session? >> > >> > Yes Stan, we can set output_resolution after the session has been >> > initialization. >> > As per the spec, this call s_selection is an optional step under >> > Initialization >> > procedure of encoder even before we request buffers. Setting this output resolution here I feel it as a repetition as we have this width and height set to firmware as a part of VIDIOC_REQBUFS(). So updating the selection rectangle would be sufficient in this case. >> >> What spec you are referring to? The spec for the encoders [1] or >> something else. > > For our convenience, Hans was nice enough to host a compiled version > at: > https://hverkuil.home.xs4all.nl/request-api/uapi/v4l/dev-encoder.html > > Best regards, > Tomasz
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c index 3f50cd0..754c19a 100644 --- a/drivers/media/platform/qcom/venus/venc.c +++ b/drivers/media/platform/qcom/venus/venc.c @@ -478,16 +478,31 @@ static int venc_g_fmt(struct file *file, void *fh, struct v4l2_format *f) venc_s_selection(struct file *file, void *fh, struct v4l2_selection *s) { struct venus_inst *inst = to_inst(file); + int ret; + u32 buftype; if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; switch (s->target) { case V4L2_SEL_TGT_CROP: - if (s->r.width != inst->out_width || - s->r.height != inst->out_height || + 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) { + buftype = HFI_BUFFER_OUTPUT; + ret = venus_helper_set_output_resolution(inst, + s->r.width, + s->r.height, + buftype); + if (ret) + return ret; + + inst->width = s->r.width; + inst->height = s->r.height; + } break; default: return -EINVAL;
Handles target type crop by setting the new active rectangle to hardware. The new rectangle should be within YUV size. Signed-off-by: Malathi Gottam <mgottam@codeaurora.org> --- drivers/media/platform/qcom/venus/venc.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)