| Message ID | 20200403094033.8288-8-xia.jiang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for mt2701 JPEG ENC support | expand |
Hi Xia, On Fri, Apr 03, 2020 at 05:40:26PM +0800, Xia Jiang wrote: Thank you for the patch. Please see my comments inline. nit: I'd remove "for improving code quality" from the subject, as it's obvious that we don't intend to make the code quality worse. ;) On the contrary, I'd make it more specific, e.g. media: mtk-jpeg: Use generic rounding helpers WDYT? > Use clamp() to replace mtk_jpeg_bound_align_image() and round() to > replace mtk_jpeg_align(). > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > --- > v8: no changes > --- > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 41 +++++-------------- > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- > drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c | 8 ++-- > drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h | 5 --- > 4 files changed, 19 insertions(+), 43 deletions(-) > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > index 2fa3711fdc9b..4e64046a6854 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > @@ -157,25 +157,6 @@ static struct mtk_jpeg_fmt *mtk_jpeg_find_format(struct mtk_jpeg_ctx *ctx, > return NULL; > } > > -static void mtk_jpeg_bound_align_image(u32 *w, unsigned int wmin, > - unsigned int wmax, unsigned int walign, > - u32 *h, unsigned int hmin, > - unsigned int hmax, unsigned int halign) > -{ > - int width, height, w_step, h_step; > - > - width = *w; > - height = *h; > - w_step = 1 << walign; > - h_step = 1 << halign; > - > - v4l_bound_align_image(w, wmin, wmax, walign, h, hmin, hmax, halign, 0); > - if (*w < width && (*w + w_step) <= wmax) > - *w += w_step; > - if (*h < height && (*h + h_step) <= hmax) > - *h += h_step; > -} > - > static void mtk_jpeg_adjust_fmt_mplane(struct mtk_jpeg_ctx *ctx, > struct v4l2_format *f) > { > @@ -218,25 +199,25 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f, > if (q_type == MTK_JPEG_FMT_TYPE_OUTPUT) { > struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[0]; > > - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, > - MTK_JPEG_MAX_WIDTH, 0, > - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, > - MTK_JPEG_MAX_HEIGHT, 0); > + pix_mp->height = clamp(pix_mp->height, MTK_JPEG_MIN_HEIGHT, > + MTK_JPEG_MAX_HEIGHT); > + pix_mp->width = clamp(pix_mp->width, MTK_JPEG_MIN_WIDTH, > + MTK_JPEG_MAX_WIDTH); > > memset(pfmt->reserved, 0, sizeof(pfmt->reserved)); > pfmt->bytesperline = 0; > /* Source size must be aligned to 128 */ > - pfmt->sizeimage = mtk_jpeg_align(pfmt->sizeimage, 128); > + pfmt->sizeimage = round_up(pfmt->sizeimage, 128); > if (pfmt->sizeimage == 0) > pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE; > goto end; > } > > /* type is MTK_JPEG_FMT_TYPE_CAPTURE */ > - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, > - MTK_JPEG_MAX_WIDTH, fmt->h_align, > - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, > - MTK_JPEG_MAX_HEIGHT, fmt->v_align); > + pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align), > + MTK_JPEG_MIN_HEIGHT, MTK_JPEG_MAX_HEIGHT); > + pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align), > + MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH); > > for (i = 0; i < fmt->colplanes; i++) { > struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[i]; > @@ -751,8 +732,8 @@ static void mtk_jpeg_set_dec_src(struct mtk_jpeg_ctx *ctx, > { > bs->str_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); > bs->end_addr = bs->str_addr + > - mtk_jpeg_align(vb2_get_plane_payload(src_buf, 0), 16); > - bs->size = mtk_jpeg_align(vb2_plane_size(src_buf, 0), 128); > + round_up(vb2_get_plane_payload(src_buf, 0), 16); > + bs->size = round_up(vb2_plane_size(src_buf, 0), 128); > } > > static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx, > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > index 999bd1427809..28e9b30ad5c3 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > @@ -21,10 +21,10 @@ > #define MTK_JPEG_FMT_TYPE_OUTPUT 1 > #define MTK_JPEG_FMT_TYPE_CAPTURE 2 > > -#define MTK_JPEG_MIN_WIDTH 32 > -#define MTK_JPEG_MIN_HEIGHT 32 > -#define MTK_JPEG_MAX_WIDTH 8192 > -#define MTK_JPEG_MAX_HEIGHT 8192 > +#define MTK_JPEG_MIN_WIDTH 32U > +#define MTK_JPEG_MIN_HEIGHT 32U > +#define MTK_JPEG_MAX_WIDTH 8192U > +#define MTK_JPEG_MAX_HEIGHT 8192U This change is not mentioned in the commit message. It should go to a separate patch, possibly merged with other really minor stylistic changes like this, e.g. patch 08/14. Otherwise the patch looks good, so after addressing the above minor changes please feel free to add Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
On Thu, 2020-05-21 at 15:41 +0000, Tomasz Figa wrote: > Hi Xia, > > On Fri, Apr 03, 2020 at 05:40:26PM +0800, Xia Jiang wrote: > > Thank you for the patch. Please see my comments inline. > > nit: I'd remove "for improving code quality" from the subject, as it's > obvious that we don't intend to make the code quality worse. ;) > On the contrary, I'd make it more specific, e.g. > > media: mtk-jpeg: Use generic rounding helpers > > WDYT? Done > > > Use clamp() to replace mtk_jpeg_bound_align_image() and round() to > > replace mtk_jpeg_align(). > > > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > > --- > > v8: no changes > > --- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 41 +++++-------------- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- > > drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c | 8 ++-- > > drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h | 5 --- > > 4 files changed, 19 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > index 2fa3711fdc9b..4e64046a6854 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > @@ -157,25 +157,6 @@ static struct mtk_jpeg_fmt *mtk_jpeg_find_format(struct mtk_jpeg_ctx *ctx, > > return NULL; > > } > > > > -static void mtk_jpeg_bound_align_image(u32 *w, unsigned int wmin, > > - unsigned int wmax, unsigned int walign, > > - u32 *h, unsigned int hmin, > > - unsigned int hmax, unsigned int halign) > > -{ > > - int width, height, w_step, h_step; > > - > > - width = *w; > > - height = *h; > > - w_step = 1 << walign; > > - h_step = 1 << halign; > > - > > - v4l_bound_align_image(w, wmin, wmax, walign, h, hmin, hmax, halign, 0); > > - if (*w < width && (*w + w_step) <= wmax) > > - *w += w_step; > > - if (*h < height && (*h + h_step) <= hmax) > > - *h += h_step; > > -} > > - > > static void mtk_jpeg_adjust_fmt_mplane(struct mtk_jpeg_ctx *ctx, > > struct v4l2_format *f) > > { > > @@ -218,25 +199,25 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f, > > if (q_type == MTK_JPEG_FMT_TYPE_OUTPUT) { > > struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[0]; > > > > - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, > > - MTK_JPEG_MAX_WIDTH, 0, > > - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, > > - MTK_JPEG_MAX_HEIGHT, 0); > > + pix_mp->height = clamp(pix_mp->height, MTK_JPEG_MIN_HEIGHT, > > + MTK_JPEG_MAX_HEIGHT); > > + pix_mp->width = clamp(pix_mp->width, MTK_JPEG_MIN_WIDTH, > > + MTK_JPEG_MAX_WIDTH); > > > > memset(pfmt->reserved, 0, sizeof(pfmt->reserved)); > > pfmt->bytesperline = 0; > > /* Source size must be aligned to 128 */ > > - pfmt->sizeimage = mtk_jpeg_align(pfmt->sizeimage, 128); > > + pfmt->sizeimage = round_up(pfmt->sizeimage, 128); > > if (pfmt->sizeimage == 0) > > pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE; > > goto end; > > } > > > > /* type is MTK_JPEG_FMT_TYPE_CAPTURE */ > > - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, > > - MTK_JPEG_MAX_WIDTH, fmt->h_align, > > - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, > > - MTK_JPEG_MAX_HEIGHT, fmt->v_align); > > + pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align), > > + MTK_JPEG_MIN_HEIGHT, MTK_JPEG_MAX_HEIGHT); > > + pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align), > > + MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH); > > > > for (i = 0; i < fmt->colplanes; i++) { > > struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[i]; > > @@ -751,8 +732,8 @@ static void mtk_jpeg_set_dec_src(struct mtk_jpeg_ctx *ctx, > > { > > bs->str_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); > > bs->end_addr = bs->str_addr + > > - mtk_jpeg_align(vb2_get_plane_payload(src_buf, 0), 16); > > - bs->size = mtk_jpeg_align(vb2_plane_size(src_buf, 0), 128); > > + round_up(vb2_get_plane_payload(src_buf, 0), 16); > > + bs->size = round_up(vb2_plane_size(src_buf, 0), 128); > > } > > > > static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx, > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > index 999bd1427809..28e9b30ad5c3 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > @@ -21,10 +21,10 @@ > > #define MTK_JPEG_FMT_TYPE_OUTPUT 1 > > #define MTK_JPEG_FMT_TYPE_CAPTURE 2 > > > > -#define MTK_JPEG_MIN_WIDTH 32 > > -#define MTK_JPEG_MIN_HEIGHT 32 > > -#define MTK_JPEG_MAX_WIDTH 8192 > > -#define MTK_JPEG_MAX_HEIGHT 8192 > > +#define MTK_JPEG_MIN_WIDTH 32U > > +#define MTK_JPEG_MIN_HEIGHT 32U > > +#define MTK_JPEG_MAX_WIDTH 8192U > > +#define MTK_JPEG_MAX_HEIGHT 8192U > > This change is not mentioned in the commit message. It should go to a > separate patch, possibly merged with other really minor stylistic changes > like this, e.g. patch 08/14. Done > > Otherwise the patch looks good, so after addressing the above minor changes > please feel free to add > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > Best regards, > Tomasz >
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c index 2fa3711fdc9b..4e64046a6854 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c @@ -157,25 +157,6 @@ static struct mtk_jpeg_fmt *mtk_jpeg_find_format(struct mtk_jpeg_ctx *ctx, return NULL; } -static void mtk_jpeg_bound_align_image(u32 *w, unsigned int wmin, - unsigned int wmax, unsigned int walign, - u32 *h, unsigned int hmin, - unsigned int hmax, unsigned int halign) -{ - int width, height, w_step, h_step; - - width = *w; - height = *h; - w_step = 1 << walign; - h_step = 1 << halign; - - v4l_bound_align_image(w, wmin, wmax, walign, h, hmin, hmax, halign, 0); - if (*w < width && (*w + w_step) <= wmax) - *w += w_step; - if (*h < height && (*h + h_step) <= hmax) - *h += h_step; -} - static void mtk_jpeg_adjust_fmt_mplane(struct mtk_jpeg_ctx *ctx, struct v4l2_format *f) { @@ -218,25 +199,25 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f, if (q_type == MTK_JPEG_FMT_TYPE_OUTPUT) { struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[0]; - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, - MTK_JPEG_MAX_WIDTH, 0, - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, - MTK_JPEG_MAX_HEIGHT, 0); + pix_mp->height = clamp(pix_mp->height, MTK_JPEG_MIN_HEIGHT, + MTK_JPEG_MAX_HEIGHT); + pix_mp->width = clamp(pix_mp->width, MTK_JPEG_MIN_WIDTH, + MTK_JPEG_MAX_WIDTH); memset(pfmt->reserved, 0, sizeof(pfmt->reserved)); pfmt->bytesperline = 0; /* Source size must be aligned to 128 */ - pfmt->sizeimage = mtk_jpeg_align(pfmt->sizeimage, 128); + pfmt->sizeimage = round_up(pfmt->sizeimage, 128); if (pfmt->sizeimage == 0) pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE; goto end; } /* type is MTK_JPEG_FMT_TYPE_CAPTURE */ - mtk_jpeg_bound_align_image(&pix_mp->width, MTK_JPEG_MIN_WIDTH, - MTK_JPEG_MAX_WIDTH, fmt->h_align, - &pix_mp->height, MTK_JPEG_MIN_HEIGHT, - MTK_JPEG_MAX_HEIGHT, fmt->v_align); + pix_mp->height = clamp(round_up(pix_mp->height, fmt->v_align), + MTK_JPEG_MIN_HEIGHT, MTK_JPEG_MAX_HEIGHT); + pix_mp->width = clamp(round_up(pix_mp->width, fmt->h_align), + MTK_JPEG_MIN_WIDTH, MTK_JPEG_MAX_WIDTH); for (i = 0; i < fmt->colplanes; i++) { struct v4l2_plane_pix_format *pfmt = &pix_mp->plane_fmt[i]; @@ -751,8 +732,8 @@ static void mtk_jpeg_set_dec_src(struct mtk_jpeg_ctx *ctx, { bs->str_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); bs->end_addr = bs->str_addr + - mtk_jpeg_align(vb2_get_plane_payload(src_buf, 0), 16); - bs->size = mtk_jpeg_align(vb2_plane_size(src_buf, 0), 128); + round_up(vb2_get_plane_payload(src_buf, 0), 16); + bs->size = round_up(vb2_plane_size(src_buf, 0), 128); } static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx, diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h index 999bd1427809..28e9b30ad5c3 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h @@ -21,10 +21,10 @@ #define MTK_JPEG_FMT_TYPE_OUTPUT 1 #define MTK_JPEG_FMT_TYPE_CAPTURE 2 -#define MTK_JPEG_MIN_WIDTH 32 -#define MTK_JPEG_MIN_HEIGHT 32 -#define MTK_JPEG_MAX_WIDTH 8192 -#define MTK_JPEG_MAX_HEIGHT 8192 +#define MTK_JPEG_MIN_WIDTH 32U +#define MTK_JPEG_MIN_HEIGHT 32U +#define MTK_JPEG_MAX_WIDTH 8192U +#define MTK_JPEG_MAX_HEIGHT 8192U #define MTK_JPEG_DEFAULT_SIZEIMAGE (1 * 1024 * 1024) diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c index ddf0dfa78e20..68abcfd7494d 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c @@ -153,10 +153,10 @@ static int mtk_jpeg_calc_dst_size(struct mtk_jpeg_dec_param *param) param->sampling_w[i]; /* output format is 420/422 */ param->comp_w[i] = padding_w >> brz_w[i]; - param->comp_w[i] = mtk_jpeg_align(param->comp_w[i], - MTK_JPEG_DCTSIZE); - param->img_stride[i] = i ? mtk_jpeg_align(param->comp_w[i], 16) - : mtk_jpeg_align(param->comp_w[i], 32); + param->comp_w[i] = round_up(param->comp_w[i], + MTK_JPEG_DCTSIZE); + param->img_stride[i] = i ? round_up(param->comp_w[i], 16) + : round_up(param->comp_w[i], 32); ds_row_h[i] = (MTK_JPEG_DCTSIZE * param->sampling_h[i]); } param->dec_w = param->img_stride[0]; diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h index 9c6584eaad99..7b0687f8f4b6 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h @@ -54,11 +54,6 @@ struct mtk_jpeg_dec_param { u8 uv_brz_w; }; -static inline u32 mtk_jpeg_align(u32 val, u32 align) -{ - return (val + align - 1) & ~(align - 1); -} - struct mtk_jpeg_bs { dma_addr_t str_addr; dma_addr_t end_addr;
Use clamp() to replace mtk_jpeg_bound_align_image() and round() to replace mtk_jpeg_align(). Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> --- v8: no changes --- .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 41 +++++-------------- .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++-- drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c | 8 ++-- drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h | 5 --- 4 files changed, 19 insertions(+), 43 deletions(-)