by Tommaso Merciai

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH v4 2/4] media: amphion: tell and handle contiguous and non contiguous format

Hi Ming,

On Mon, Aug 22, 2022 at 11:56:11AM +0000, Ming Qian wrote:
> >Hi Ming,
> >Sorry for delay. I'm on vacation last week :)
> >
> >On Tue, Aug 09, 2022 at 02:50:39PM +0800, Ming Qian wrote:
> >> Driver should tell the number of memory planes and component planes.
> >> the amphion vpu support non contiguous planes, but for compatibility
> >> with other device that only support contiguous planes.
> >> driver can add support for contiguous planes in the same time.
> >> Then the mem_planes can be different from the comp_planes.
> >> driver need to handle buffer according mem_planes and comp_planes.
> >>
> >> So driver can support NV12 and NV12M.
> >>
> >> Signed-off-by: Ming Qian <[email protected]>
> >> ---
> >> drivers/media/platform/amphion/vdec.c | 187 ++++++++++---------
> >> drivers/media/platform/amphion/venc.c | 33 ++--
> >> drivers/media/platform/amphion/vpu.h | 4 +-
> >> drivers/media/platform/amphion/vpu_dbg.c | 8 +-
> >> drivers/media/platform/amphion/vpu_helpers.c | 48 ++++-
> >> drivers/media/platform/amphion/vpu_helpers.h | 2 +
> >> drivers/media/platform/amphion/vpu_malone.c | 3 +-
> >> drivers/media/platform/amphion/vpu_v4l2.c | 168 ++++++++++++-----
> >> drivers/media/platform/amphion/vpu_v4l2.h | 3 +-
> >> drivers/media/platform/amphion/vpu_windsor.c | 8 +-
> >> 10 files changed, 299 insertions(+), 165 deletions(-)
> >>
>
> [snip]
>
> >> +const struct vpu_format *vpu_helper_find_sibling(struct vpu_inst
> >> +*inst, u32 type, u32 pixelfmt) {
> >> + const struct vpu_format *fmt;
> >> + const struct vpu_format *sibling;
> >> +
> >> + fmt = vpu_helper_find_format(inst, type, pixelfmt);
> >> + if (!fmt)
> >> + return NULL;
> >> + if (!fmt->sibling)
> >> + return NULL;
> >> + sibling = vpu_helper_find_format(inst, type, fmt->sibling);
> >> + if (!sibling)
> >> + return NULL;
> >> + if (sibling->sibling != fmt->pixfmt)
> >> + return NULL;
> >> + if (sibling->comp_planes != fmt->comp_planes)
> >> + return NULL;
> >> + return sibling;
> >> +}
> >
> >I think we can limit the use of "if" statement here. What about this?
> >
> >const struct vpu_format *vpu_helper_find_sibling(struct vpu_inst *inst, u32
> >type, u32 pixelfmt) {
> > const struct vpu_format *fmt;
> > const struct vpu_format *sibling;
> >
> > fmt = vpu_helper_find_format(inst, type, pixelfmt);
> > if (!fmt || !fmt->sibling)
> > return NULL;
> >
> > sibling = vpu_helper_find_format(inst, type, fmt->sibling);
> > if (!sibling || (sibling->sibling != fmt->pixfmt) ||
> > (sibling->comp_planes != fmt->comp_planes))
> > return NULL;
> >
> > return sibling;
> >}
> >
>
> OK, I'll apply this change in v5
>
> >> +
> >> +bool vpu_helper_match_format(struct vpu_inst *inst, u32 type, u32
> >> +fmta, u32 fmtb) {
> >> + const struct vpu_format *sibling;
> >> +
> >> + if (fmta == fmtb)
> >> + return true;
> >> +
> >> + sibling = vpu_helper_find_sibling(inst, type, fmta);
> >> + if (sibling && sibling->pixfmt == fmtb)
> >> + return true;
> >> + return false;
> >> +}
> >> +
>
> [snip]
>
> >> --- a/drivers/media/platform/amphion/vpu_malone.c
> >> +++ b/drivers/media/platform/amphion/vpu_malone.c
> >> @@ -583,7 +583,8 @@ bool vpu_malone_check_fmt(enum vpu_core_type
> >type, u32 pixelfmt)
> >> if (!vpu_imx8q_check_fmt(type, pixelfmt))
> >> return false;
> >>
> >> - if (pixelfmt == V4L2_PIX_FMT_NV12M_8L128 || pixelfmt ==
> >V4L2_PIX_FMT_NV12M_10BE_8L128)
> >> + if (pixelfmt == V4L2_PIX_FMT_NV12_8L128 || pixelfmt ==
> >V4L2_PIX_FMT_NV12_10BE_8L128 ||
> >> + pixelfmt == V4L2_PIX_FMT_NV12M_8L128 || pixelfmt ==
> >> + V4L2_PIX_FMT_NV12M_10BE_8L128)
> >
> >^Here are we using spaces instead of tab or I'm wrong?
> >
>
> It's following the rule of checkpatch.pl
>
> >> return true;
> >> if (vpu_malone_format_remap(pixelfmt) == MALONE_FMT_NULL)
> >> return false;
>
> [snip]
>
> >> +static int vpu_calc_fmt_sizeimage(struct vpu_inst *inst, struct
> >> +vpu_format *fmt) {
> >> u32 stride = 1;
> >> - u32 bytesperline;
> >> - u32 sizeimage;
> >> - const struct vpu_format *fmt;
> >> - const struct vpu_core_resources *res;
> >> int i;
> >>
> >> - fmt = vpu_helper_find_format(inst, type, pixmp->pixelformat);
> >> - if (!fmt) {
> >> - fmt = vpu_helper_enum_format(inst, type, 0);
> >> - if (!fmt)
> >> - return NULL;
> >> - pixmp->pixelformat = fmt->pixfmt;
> >> + if (!(fmt->flags & V4L2_FMT_FLAG_COMPRESSED)) {
> >> + const struct vpu_core_resources *res =
> >> + vpu_get_resource(inst);
> >> +
> >> + if (res)
> >> + stride = res->stride;
> >
> >If res=NULL stride=1 it is ok? Or we need to return some error?
> >
>
> If res is NULL, it means there is no additional alignment constraints
> So it's ok to set stride to 1 in this case.
>
> >> }
> >>
> >> - res = vpu_get_resource(inst);
> >> - if (res)
> >> - stride = res->stride;
> >> - if (pixmp->width)
> >> - pixmp->width = vpu_helper_valid_frame_width(inst,
> >pixmp->width);
> >> - if (pixmp->height)
> >> - pixmp->height = vpu_helper_valid_frame_height(inst,
> >pixmp->height);
> >> + for (i = 0; i < fmt->comp_planes; i++) {
> >> + fmt->sizeimage[i] = vpu_helper_get_plane_size(fmt->pixfmt,
> >> +
> >fmt->width,
> >> +
> >fmt->height,
> >> + i,
> >> + stride,
> >> +
> >fmt->field != V4L2_FIELD_NONE ? 1 : 0,
> >> +
> >&fmt->bytesperline[i]);
> >> + fmt->sizeimage[i] = max_t(u32, fmt->sizeimage[i], PAGE_SIZE);
> >> + if (fmt->flags & V4L2_FMT_FLAG_COMPRESSED) {
> >> + fmt->sizeimage[i] = clamp_val(fmt->sizeimage[i],
> >SZ_128K, SZ_8M);
> >> + fmt->bytesperline[i] = 0;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +u32 vpu_get_fmt_plane_size(struct vpu_format *fmt, u32 plane_no) {
> >> + u32 size;
> >> + int i;
> >> +
> >> + if (plane_no >= fmt->mem_planes)
> >> + return 0;
> >> +
> >> + if (fmt->comp_planes == fmt->mem_planes)
> >> + return fmt->sizeimage[plane_no];
> >> + if (plane_no < fmt->mem_planes - 1)
> >> + return fmt->sizeimage[plane_no];
> >
> >I like a space here but is my personal opinion :)
> >
>
> OK, I'll add a space line here in v5
>
> [snip]

Thanks for clarifications.

Regards,
Tommaso

--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com