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

[Tommaso Merciai <tommaso.merciai@amarulasolutions.com>]

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 <ming.qian@nxp.com>
> >> ---
> >>  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
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel