RE: [EXT] Re: [PATCH v4 2/4] media: amphion: tell and handle contiguous and non contiguous format
From: Ming Qian
Date: Mon Aug 22 2022 - 07:56:19 EST
>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@xxxxxxx>
>> ---
>> 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]