From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 33668C433E1 for ; Thu, 21 May 2020 16:09:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ED18A207D8 for ; Thu, 21 May 2020 16:09:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="bCXDsCgY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728512AbgEUQJD (ORCPT ); Thu, 21 May 2020 12:09:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42322 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726282AbgEUQJC (ORCPT ); Thu, 21 May 2020 12:09:02 -0400 Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 78DBAC061A0E for ; Thu, 21 May 2020 09:09:02 -0700 (PDT) Received: by mail-wr1-x441.google.com with SMTP id l18so7188117wrn.6 for ; Thu, 21 May 2020 09:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=bCXDsCgYV0CSrRRnuf5+7lTo0NBYDtuebWPY6hj3INHvJK561Jp7QFDckmYmGUfS/t DhG4y89ElZE34sqQC9yQ78tUvgI7ZIpCs1MBy0bgzwhLAr3obg12vh/jJjPL9xpNK7n3 pJwSKDTCDokUZ+6mGVdJ2EnB4VspWLFJ4gotY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=W4/8O8hs1T+FeeBlJhqdMzCEtuX/Keh4baMXzUeduJH5/mHg7em+xfjfTCsw1x0Zy9 /6hIzwE1f36VWBTB0YsRBd0BKRs+WgyO8JrV0ZlYrBeiMBuIwnv4IFh/Nqjly+hFPgxh GeyDrbV3X/Y4Ml7P7wlIB99ZSmKG9L7KUSk4pfhBQssWn+evBGBd5HZTdxN+HQwzbxKp 0DdgTTw86aNA+fdgsGezo74SalalPmxLn2iWQolmv5gDum9P9liV6SoMnzQCtSmCcNK7 pz+bakNS7P839yvxzC7/UnGjPMjSWWKmdA5OfmMx/jrwCbA+pDrey/lFdQtL+5rhR8mT ymeg== X-Gm-Message-State: AOAM533cSJzIxvFstw5LiGaEww2N0zPNIOO95zYDJFAfgVngLjy0dLYO l6gzHDkY2GUocGX1ZNnxUiDUzQ== X-Google-Smtp-Source: ABdhPJxofDwgei5nJILFlmeUVYT5sPEg4Co9C6L/QHzh+MWc0ciLfVXyWKXT+NreFg41NQoX52BzBQ== X-Received: by 2002:adf:d4c6:: with SMTP id w6mr9583901wrk.92.1590077340874; Thu, 21 May 2020 09:09:00 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id c143sm7134776wmd.43.2020.05.21.09.09.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 May 2020 09:09:00 -0700 (PDT) Date: Thu, 21 May 2020 16:08:58 +0000 From: Tomasz Figa To: Hans Verkuil , Xia Jiang Cc: Xia Jiang , Mauro Carvalho Chehab , Rob Herring , Matthias Brugger , Rick Chang , linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, Marek Szyprowski , srv_heupstream@mediatek.com, senozhatsky@chromium.org, mojahsu@chromium.org, drinkcat@chromium.org, maoguang.meng@mediatek.com, sj.huang@mediatek.com Subject: Re: [PATCH v8 14/14] media: platform: Add jpeg dec/enc feature Message-ID: <20200521160858.GL209565@chromium.org> References: <20200403094033.8288-1-xia.jiang@mediatek.com> <20200403094033.8288-15-xia.jiang@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 11, 2020 at 11:04:19AM +0200, Hans Verkuil wrote: > On 03/04/2020 11:40, Xia Jiang wrote: > > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg > > decode and encode have great similarities with function operation. > > > > Signed-off-by: Xia Jiang > > --- > > v8:jpeg encoder and decoder use separate callbacks instead of repeating > > the if/else in every callback. > > improve vidioc_try_fmt() implementation that can be shared by jpeg > > encoder and decoder. > > fix the bug of jpeg encoder s_selection implementation. > > cancel the state of the jpeg encoder. > > improve jpeg encoder and decoder set default params flow. > > put the clock names and other datas in a match_data struct. > > fix the bug of geting correctly quality value. > > do the all the bits' settings of one register in one function. > > move the code of mtk_jpeg_enc_reg.h to mtk_jpeg_enc_hw.h and delete > > mtk_jpeg_enc_reg.h. > > > > v7: reverse spin lock and unlock operation in device run function for > > multi-instance. > > > > v6: add space to arounding '+'. > > alignment 'struct mtk_jpeg_fmt *fmt' match open parenthesis. > > change 'mtk_jpeg_enc_set_encFormat' to 'mtk_jpeg_enc_set_enc_format'. > > make 'mtk_jpeg_ctrls_setup' to static prototype. > > delete unused variables 'jpeg'/'align_h'/'align_w'/'flags'. > > initialize 'yuv_format'/'enc_quality' variables. > > > > v5: support crop for encoder and compose for decoder in s_selection and > > g_selection function. > > use clamp() to replace mtk_jpeg_bound_align_image() and round_up() > > to replace mtk_jpeg_align(). > > delete jpeg_enc_param/mtk_jpeg_enc_param structure and > > mtk_jpeg_set_param(), program the registers directly based on > > the original V4L2 values. > > move macro definition about hw to mtk_jpeg_enc_reg.h. > > delete unnecessary V4L2 logs in driver. > > cancel spin lock and unlock operation in deviec run function. > > change jpeg enc register offset hex numberals from upercase to lowercase. > > > > v4: split mtk_jpeg_try_fmt_mplane() to two functions, one for encoder, > > one for decoder. > > split mtk_jpeg_set_default_params() to two functions, one for > > encoder, one for decoder. > > add cropping support for encoder in g/s_selection ioctls. > > change exif mode support by using V4L2_JPEG_ACTIVE_MARKER_APP1. > > change MTK_JPEG_MAX_WIDTH/MTK_JPEG_MAX_HEIGH from 8192 to 65535 by > > specification. > > move width shifting operation behind aligning operation in > > mtk_jpeg_try_enc_fmt_mplane() for bug fix. > > fix user abuseing data_offset issue for DMABUF in > > mtk_jpeg_set_enc_src(). > > fix kbuild warings: change MTK_JPEG_MIN_HEIGHT/MTK_JPEG_MAX_HEIGHT > > and MTK_JPEG_MIN_WIDTH/MTK_JPEG_MAX_WIDTH from > > 'int' type to 'unsigned int' type. > > fix msleadingly indented of 'else'. > > v3: delete Change-Id. > > only test once handler->error after the last v4l2_ctrl_new_std(). > > seperate changes of v4l2-ctrls.c and v4l2-controls.h to new patch. > > v2: fix compliance test fail, check created buffer size in driver. > > --- > > drivers/media/platform/mtk-jpeg/Makefile | 5 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 1038 +++++++++++++---- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 51 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h | 7 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193 +++ > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 ++ > > 6 files changed, 1188 insertions(+), 229 deletions(-) > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h > > > > diff --git a/drivers/media/platform/mtk-jpeg/Makefile b/drivers/media/platform/mtk-jpeg/Makefile > > index 48516dcf96e6..76c33aad0f3f 100644 > > --- a/drivers/media/platform/mtk-jpeg/Makefile > > +++ b/drivers/media/platform/mtk-jpeg/Makefile > > @@ -1,3 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > -mtk_jpeg-objs := mtk_jpeg_core.o mtk_jpeg_dec_hw.o mtk_jpeg_dec_parse.o > > +mtk_jpeg-objs := mtk_jpeg_core.o \ > > + mtk_jpeg_dec_hw.o \ > > + mtk_jpeg_dec_parse.o \ > > + mtk_jpeg_enc_hw.o > > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk_jpeg.o > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > index 77a95185584c..18a759ce2c46 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > @@ -3,6 +3,7 @@ > > * Copyright (c) 2016 MediaTek Inc. > > * Author: Ming Hsiu Tsai > > * Rick Chang > > + * Xia Jiang > > */ > > > > #include > > @@ -23,11 +24,60 @@ > > #include > > #include > > > > +#include "mtk_jpeg_enc_hw.h" > > #include "mtk_jpeg_dec_hw.h" > > #include "mtk_jpeg_core.h" > > #include "mtk_jpeg_dec_parse.h" > > > > -static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > +static struct mtk_jpeg_fmt mtk_jpeg_enc_formats[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_JPEG, > > + .colplanes = 1, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_CAPTURE, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV12M, > > + .hw_format = JPEG_ENC_YUV_FORMAT_NV12, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV21M, > > + .hw_format = JEPG_ENC_YUV_FORMAT_NV21, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YUYV, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YVYU, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YVYU, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > +}; > > + > > +static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = { > > { > > .fourcc = V4L2_PIX_FMT_JPEG, > > .colplanes = 1, > > @@ -53,7 +103,8 @@ static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > }, > > }; > > > > -#define MTK_JPEG_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_formats) > > +#define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats) > > +#define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats) > > > > enum { > > MTK_JPEG_BUF_FLAGS_INIT = 0, > > @@ -70,6 +121,11 @@ struct mtk_jpeg_src_buf { > > static int debug; > > module_param(debug, int, 0644); > > > > +static inline struct mtk_jpeg_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl) > > +{ > > + return container_of(ctrl->handler, struct mtk_jpeg_ctx, ctrl_hdl); > > +} > > + > > static inline struct mtk_jpeg_ctx *mtk_jpeg_fh_to_ctx(struct v4l2_fh *fh) > > { > > return container_of(fh, struct mtk_jpeg_ctx, fh); > > @@ -81,12 +137,25 @@ static inline struct mtk_jpeg_src_buf *mtk_jpeg_vb2_to_srcbuf( > > return container_of(to_vb2_v4l2_buffer(vb), struct mtk_jpeg_src_buf, b); > > } > > > > -static int mtk_jpeg_querycap(struct file *file, void *priv, > > - struct v4l2_capability *cap) > > +static int mtk_jpeg_enc_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > +{ > > + struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > + > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > + strscpy(cap->card, MTK_JPEG_NAME " encoder", sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > + dev_name(jpeg->dev)); > > + > > + return 0; > > +} > > + > > +static int mtk_jpeg_dec_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > { > > struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > > > - strscpy(cap->driver, MTK_JPEG_NAME " decoder", sizeof(cap->driver)); > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > strscpy(cap->card, MTK_JPEG_NAME " decoder", sizeof(cap->card)); > > snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > dev_name(jpeg->dev)); > > @@ -94,6 +163,54 @@ static int mtk_jpeg_querycap(struct file *file, void *priv, > > return 0; > > } > > > > +static int vidioc_jpeg_enc_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct mtk_jpeg_ctx *ctx = ctrl_to_ctx(ctrl); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_JPEG_RESTART_INTERVAL: > > + ctx->restart_interval = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_COMPRESSION_QUALITY: > > + ctx->enc_quality = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_ACTIVE_MARKER: > > + ctx->enable_exif = ctrl->val & V4L2_JPEG_ACTIVE_MARKER_APP1 ? > > + true : false; > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops mtk_jpeg_enc_ctrl_ops = { > > + .s_ctrl = vidioc_jpeg_enc_s_ctrl, > > +}; > > + > > +static int mtk_jpeg_enc_ctrls_setup(struct mtk_jpeg_ctx *ctx) > > +{ > > + const struct v4l2_ctrl_ops *ops = &mtk_jpeg_enc_ctrl_ops; > > + struct v4l2_ctrl_handler *handler = &ctx->ctrl_hdl; > > + > > + v4l2_ctrl_handler_init(handler, 3); > > + > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_RESTART_INTERVAL, 0, 100, > > + 1, 0); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_COMPRESSION_QUALITY, 48, > > + 100, 1, 90); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_ACTIVE_MARKER, 0, > > + V4L2_JPEG_ACTIVE_MARKER_APP1, 0, 0); > > + > > + if (handler->error) { > > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl); > > + return handler->error; > > + } > > + > > + v4l2_ctrl_handler_setup(&ctx->ctrl_hdl); > > + > > + return 0; > > +} > > + > > static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > struct v4l2_fmtdesc *f, u32 type) > > { > > @@ -115,117 +232,105 @@ static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > return 0; > > } > > > > -static int mtk_jpeg_enum_fmt_vid_cap(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, > > + MTK_JPEG_DEC_NUM_FORMATS, f, > > MTK_JPEG_FMT_FLAG_DEC_CAPTURE); > > } > > > > -static int mtk_jpeg_enum_fmt_vid_out(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > - MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, MTK_JPEG_DEC_NUM_FORMATS, > > + f, MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > } > > OK, so this patch is very hard to read because there are two independent changes > taking place: > > 1) rename existing functions/defines/variables with a _dec prefix to prepare > for the addition of the encoder feature. > > 2) add the encoder feature. > > Please split up this patch into two parts: one that does the rename and as much of > the preparation to support both decoder and encoder without changing the > functionality, and a second one that actually adds the new encoder feature. > > In fact, once that's done it is likely that most of this patch series can be > merged, even if there are still things that need to be changed for the last > patch adding the encoder support. I see nothing objectionable in patches 1-10 > and 13. So merging those together with a new rename patch wouldn't be an issue, > I think. > > In any case, the diffs should be a lot cleaner and easier to review by splitting > it up like that. Agreed with Hans. The split will make it much easier to review this one for me, so I'll wait for it. Thanks in advance. Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AA7AEC433DF for ; Thu, 21 May 2020 16:09:13 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 56A8420671 for ; Thu, 21 May 2020 16:09:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="b4pjSqRE"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="bCXDsCgY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 56A8420671 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OCFoKannRAdpesuS8J1usm0Tq0a+NdL5GGMo8K4cQd0=; b=b4pjSqRE6T597q FazCwp37fTgToFuXpIcWuDdO1JfcVIcz51K7I2mbCWzeC8/5z1jKJuvqmrBzwaph6SMC3JH4Li3Cv HpIDcnMvA798+W/M3HVbjNmfMgy5NA3dxUIODt26yvwxk6br9Pto5MZF6iwqrluKIzm9G5SUbvO0S 8l78115f+dq3LMbtBvajmnevLIcQZMcNWHXufkv8hj9ONi4DZ/fjAA1fLog3NCzXV/chEsoltrzIv vdUnezOLEa9aeIWhppga58bbrazBsemUlHco/MIeW6itdbmgyVd30kx0dDAh+VeXC3CBfarvxqmjt THg/P6Eyejb8OmJfmGXw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jbnkn-0001Wv-F7; Thu, 21 May 2020 16:09:05 +0000 Received: from mail-wr1-x444.google.com ([2a00:1450:4864:20::444]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jbnkk-0001Vp-FL for linux-mediatek@lists.infradead.org; Thu, 21 May 2020 16:09:04 +0000 Received: by mail-wr1-x444.google.com with SMTP id s8so7167215wrt.9 for ; Thu, 21 May 2020 09:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=bCXDsCgYV0CSrRRnuf5+7lTo0NBYDtuebWPY6hj3INHvJK561Jp7QFDckmYmGUfS/t DhG4y89ElZE34sqQC9yQ78tUvgI7ZIpCs1MBy0bgzwhLAr3obg12vh/jJjPL9xpNK7n3 pJwSKDTCDokUZ+6mGVdJ2EnB4VspWLFJ4gotY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=ZYRJI7EBZoieo6vuzGMoznO923JhyGbyKuzY6Ndn5VrLrpXPO6hTiRF3vp84hKq7pB h3OFaYyAr9xXHl52QEuZxmiKYf8BGzajgcdYERlgIY8i0FU2O+0CSwbovNqZVvHatUpO w0JbyTGVX8tQMp702aIkhoBZITGVlfmYKqfePsi8MGnk8SCbPG7y8SpJJqcrsxDktHHP cWOSZ20XGy0Ym64ehsv0EXEQD5NRLLvsNLLA84Kqbj9+UWrZJz/oUs6eyHRuQmGTMmgv hDnl4x3nLol+ZJ88s49+2HjqLAH201UT1h+MfjWBRD/WehEiAKEFQRfm/7lghtUhJs+U s+Fg== X-Gm-Message-State: AOAM533+6tX09WaxlD0NKm/GBYU2FIHpVgHuA70++SlL7uiDN2GmoX17 /uQNh8XqWma9M48fR4EjQAhjHA== X-Google-Smtp-Source: ABdhPJxofDwgei5nJILFlmeUVYT5sPEg4Co9C6L/QHzh+MWc0ciLfVXyWKXT+NreFg41NQoX52BzBQ== X-Received: by 2002:adf:d4c6:: with SMTP id w6mr9583901wrk.92.1590077340874; Thu, 21 May 2020 09:09:00 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id c143sm7134776wmd.43.2020.05.21.09.09.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 May 2020 09:09:00 -0700 (PDT) Date: Thu, 21 May 2020 16:08:58 +0000 From: Tomasz Figa To: Hans Verkuil , Xia Jiang Subject: Re: [PATCH v8 14/14] media: platform: Add jpeg dec/enc feature Message-ID: <20200521160858.GL209565@chromium.org> References: <20200403094033.8288-1-xia.jiang@mediatek.com> <20200403094033.8288-15-xia.jiang@mediatek.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200521_090902_578906_FC7E921D X-CRM114-Status: GOOD ( 25.18 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Xia Jiang , devicetree@vger.kernel.org, mojahsu@chromium.org, srv_heupstream@mediatek.com, Rick Chang , senozhatsky@chromium.org, drinkcat@chromium.org, linux-kernel@vger.kernel.org, maoguang.meng@mediatek.com, Matthias Brugger , sj.huang@mediatek.com, Rob Herring , linux-mediatek@lists.infradead.org, Mauro Carvalho Chehab , Marek Szyprowski , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Mon, May 11, 2020 at 11:04:19AM +0200, Hans Verkuil wrote: > On 03/04/2020 11:40, Xia Jiang wrote: > > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg > > decode and encode have great similarities with function operation. > > > > Signed-off-by: Xia Jiang > > --- > > v8:jpeg encoder and decoder use separate callbacks instead of repeating > > the if/else in every callback. > > improve vidioc_try_fmt() implementation that can be shared by jpeg > > encoder and decoder. > > fix the bug of jpeg encoder s_selection implementation. > > cancel the state of the jpeg encoder. > > improve jpeg encoder and decoder set default params flow. > > put the clock names and other datas in a match_data struct. > > fix the bug of geting correctly quality value. > > do the all the bits' settings of one register in one function. > > move the code of mtk_jpeg_enc_reg.h to mtk_jpeg_enc_hw.h and delete > > mtk_jpeg_enc_reg.h. > > > > v7: reverse spin lock and unlock operation in device run function for > > multi-instance. > > > > v6: add space to arounding '+'. > > alignment 'struct mtk_jpeg_fmt *fmt' match open parenthesis. > > change 'mtk_jpeg_enc_set_encFormat' to 'mtk_jpeg_enc_set_enc_format'. > > make 'mtk_jpeg_ctrls_setup' to static prototype. > > delete unused variables 'jpeg'/'align_h'/'align_w'/'flags'. > > initialize 'yuv_format'/'enc_quality' variables. > > > > v5: support crop for encoder and compose for decoder in s_selection and > > g_selection function. > > use clamp() to replace mtk_jpeg_bound_align_image() and round_up() > > to replace mtk_jpeg_align(). > > delete jpeg_enc_param/mtk_jpeg_enc_param structure and > > mtk_jpeg_set_param(), program the registers directly based on > > the original V4L2 values. > > move macro definition about hw to mtk_jpeg_enc_reg.h. > > delete unnecessary V4L2 logs in driver. > > cancel spin lock and unlock operation in deviec run function. > > change jpeg enc register offset hex numberals from upercase to lowercase. > > > > v4: split mtk_jpeg_try_fmt_mplane() to two functions, one for encoder, > > one for decoder. > > split mtk_jpeg_set_default_params() to two functions, one for > > encoder, one for decoder. > > add cropping support for encoder in g/s_selection ioctls. > > change exif mode support by using V4L2_JPEG_ACTIVE_MARKER_APP1. > > change MTK_JPEG_MAX_WIDTH/MTK_JPEG_MAX_HEIGH from 8192 to 65535 by > > specification. > > move width shifting operation behind aligning operation in > > mtk_jpeg_try_enc_fmt_mplane() for bug fix. > > fix user abuseing data_offset issue for DMABUF in > > mtk_jpeg_set_enc_src(). > > fix kbuild warings: change MTK_JPEG_MIN_HEIGHT/MTK_JPEG_MAX_HEIGHT > > and MTK_JPEG_MIN_WIDTH/MTK_JPEG_MAX_WIDTH from > > 'int' type to 'unsigned int' type. > > fix msleadingly indented of 'else'. > > v3: delete Change-Id. > > only test once handler->error after the last v4l2_ctrl_new_std(). > > seperate changes of v4l2-ctrls.c and v4l2-controls.h to new patch. > > v2: fix compliance test fail, check created buffer size in driver. > > --- > > drivers/media/platform/mtk-jpeg/Makefile | 5 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 1038 +++++++++++++---- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 51 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h | 7 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193 +++ > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 ++ > > 6 files changed, 1188 insertions(+), 229 deletions(-) > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h > > > > diff --git a/drivers/media/platform/mtk-jpeg/Makefile b/drivers/media/platform/mtk-jpeg/Makefile > > index 48516dcf96e6..76c33aad0f3f 100644 > > --- a/drivers/media/platform/mtk-jpeg/Makefile > > +++ b/drivers/media/platform/mtk-jpeg/Makefile > > @@ -1,3 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > -mtk_jpeg-objs := mtk_jpeg_core.o mtk_jpeg_dec_hw.o mtk_jpeg_dec_parse.o > > +mtk_jpeg-objs := mtk_jpeg_core.o \ > > + mtk_jpeg_dec_hw.o \ > > + mtk_jpeg_dec_parse.o \ > > + mtk_jpeg_enc_hw.o > > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk_jpeg.o > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > index 77a95185584c..18a759ce2c46 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > @@ -3,6 +3,7 @@ > > * Copyright (c) 2016 MediaTek Inc. > > * Author: Ming Hsiu Tsai > > * Rick Chang > > + * Xia Jiang > > */ > > > > #include > > @@ -23,11 +24,60 @@ > > #include > > #include > > > > +#include "mtk_jpeg_enc_hw.h" > > #include "mtk_jpeg_dec_hw.h" > > #include "mtk_jpeg_core.h" > > #include "mtk_jpeg_dec_parse.h" > > > > -static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > +static struct mtk_jpeg_fmt mtk_jpeg_enc_formats[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_JPEG, > > + .colplanes = 1, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_CAPTURE, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV12M, > > + .hw_format = JPEG_ENC_YUV_FORMAT_NV12, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV21M, > > + .hw_format = JEPG_ENC_YUV_FORMAT_NV21, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YUYV, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YVYU, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YVYU, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > +}; > > + > > +static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = { > > { > > .fourcc = V4L2_PIX_FMT_JPEG, > > .colplanes = 1, > > @@ -53,7 +103,8 @@ static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > }, > > }; > > > > -#define MTK_JPEG_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_formats) > > +#define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats) > > +#define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats) > > > > enum { > > MTK_JPEG_BUF_FLAGS_INIT = 0, > > @@ -70,6 +121,11 @@ struct mtk_jpeg_src_buf { > > static int debug; > > module_param(debug, int, 0644); > > > > +static inline struct mtk_jpeg_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl) > > +{ > > + return container_of(ctrl->handler, struct mtk_jpeg_ctx, ctrl_hdl); > > +} > > + > > static inline struct mtk_jpeg_ctx *mtk_jpeg_fh_to_ctx(struct v4l2_fh *fh) > > { > > return container_of(fh, struct mtk_jpeg_ctx, fh); > > @@ -81,12 +137,25 @@ static inline struct mtk_jpeg_src_buf *mtk_jpeg_vb2_to_srcbuf( > > return container_of(to_vb2_v4l2_buffer(vb), struct mtk_jpeg_src_buf, b); > > } > > > > -static int mtk_jpeg_querycap(struct file *file, void *priv, > > - struct v4l2_capability *cap) > > +static int mtk_jpeg_enc_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > +{ > > + struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > + > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > + strscpy(cap->card, MTK_JPEG_NAME " encoder", sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > + dev_name(jpeg->dev)); > > + > > + return 0; > > +} > > + > > +static int mtk_jpeg_dec_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > { > > struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > > > - strscpy(cap->driver, MTK_JPEG_NAME " decoder", sizeof(cap->driver)); > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > strscpy(cap->card, MTK_JPEG_NAME " decoder", sizeof(cap->card)); > > snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > dev_name(jpeg->dev)); > > @@ -94,6 +163,54 @@ static int mtk_jpeg_querycap(struct file *file, void *priv, > > return 0; > > } > > > > +static int vidioc_jpeg_enc_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct mtk_jpeg_ctx *ctx = ctrl_to_ctx(ctrl); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_JPEG_RESTART_INTERVAL: > > + ctx->restart_interval = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_COMPRESSION_QUALITY: > > + ctx->enc_quality = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_ACTIVE_MARKER: > > + ctx->enable_exif = ctrl->val & V4L2_JPEG_ACTIVE_MARKER_APP1 ? > > + true : false; > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops mtk_jpeg_enc_ctrl_ops = { > > + .s_ctrl = vidioc_jpeg_enc_s_ctrl, > > +}; > > + > > +static int mtk_jpeg_enc_ctrls_setup(struct mtk_jpeg_ctx *ctx) > > +{ > > + const struct v4l2_ctrl_ops *ops = &mtk_jpeg_enc_ctrl_ops; > > + struct v4l2_ctrl_handler *handler = &ctx->ctrl_hdl; > > + > > + v4l2_ctrl_handler_init(handler, 3); > > + > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_RESTART_INTERVAL, 0, 100, > > + 1, 0); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_COMPRESSION_QUALITY, 48, > > + 100, 1, 90); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_ACTIVE_MARKER, 0, > > + V4L2_JPEG_ACTIVE_MARKER_APP1, 0, 0); > > + > > + if (handler->error) { > > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl); > > + return handler->error; > > + } > > + > > + v4l2_ctrl_handler_setup(&ctx->ctrl_hdl); > > + > > + return 0; > > +} > > + > > static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > struct v4l2_fmtdesc *f, u32 type) > > { > > @@ -115,117 +232,105 @@ static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > return 0; > > } > > > > -static int mtk_jpeg_enum_fmt_vid_cap(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, > > + MTK_JPEG_DEC_NUM_FORMATS, f, > > MTK_JPEG_FMT_FLAG_DEC_CAPTURE); > > } > > > > -static int mtk_jpeg_enum_fmt_vid_out(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > - MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, MTK_JPEG_DEC_NUM_FORMATS, > > + f, MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > } > > OK, so this patch is very hard to read because there are two independent changes > taking place: > > 1) rename existing functions/defines/variables with a _dec prefix to prepare > for the addition of the encoder feature. > > 2) add the encoder feature. > > Please split up this patch into two parts: one that does the rename and as much of > the preparation to support both decoder and encoder without changing the > functionality, and a second one that actually adds the new encoder feature. > > In fact, once that's done it is likely that most of this patch series can be > merged, even if there are still things that need to be changed for the last > patch adding the encoder support. I see nothing objectionable in patches 1-10 > and 13. So merging those together with a new rename patch wouldn't be an issue, > I think. > > In any case, the diffs should be a lot cleaner and easier to review by splitting > it up like that. Agreed with Hans. The split will make it much easier to review this one for me, so I'll wait for it. Thanks in advance. Best regards, Tomasz _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4DCDC433E2 for ; Thu, 21 May 2020 16:09:08 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7CC7720671 for ; Thu, 21 May 2020 16:09:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="fqdzoaIk"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="bCXDsCgY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7CC7720671 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=uvWhUc/dXvFMvF4Yh5BIPYEhyuQ3nwRrF6S3vacovwY=; b=fqdzoaIkoacmq+ B76o1gxVL5sXdCV6/Xy+wKvZhvfP9xT2P/oXpDbpfFEGpDUXHh+bkz+c3NLob62dQGoJOmdY0mgBE FmTIUo8JUEs6hKeThLMNK6f9TZp+hSXMKvtaIutSnQtJvN8+7uECb1hxiNAkwboKv4ugpGsFKPwB7 Cvf80oAl3uquGsXlL7GnVLdu13i1rt19kHmS4uPkv+XXWG0lDOjA8Fd5l8fb9wMvoYDyHi9wP1EmT KQbhEQYtO50AhImxNIurS/3tMmX/lNPxBpwGmk9Q4GbmjraB3IoJhw3GfClEKFyRNjuZ4y5U+IiWI JivaKrI+BkihkDSim9UA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jbnko-0001Xf-Ba; Thu, 21 May 2020 16:09:06 +0000 Received: from mail-wr1-x442.google.com ([2a00:1450:4864:20::442]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jbnkk-0001Vq-FP for linux-arm-kernel@lists.infradead.org; Thu, 21 May 2020 16:09:04 +0000 Received: by mail-wr1-x442.google.com with SMTP id w7so7141866wre.13 for ; Thu, 21 May 2020 09:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=bCXDsCgYV0CSrRRnuf5+7lTo0NBYDtuebWPY6hj3INHvJK561Jp7QFDckmYmGUfS/t DhG4y89ElZE34sqQC9yQ78tUvgI7ZIpCs1MBy0bgzwhLAr3obg12vh/jJjPL9xpNK7n3 pJwSKDTCDokUZ+6mGVdJ2EnB4VspWLFJ4gotY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=RaC8loFLlcSNg14GKqnFZxfWstB46ZrSBfPhwjQfG8c=; b=hyI9dXpka0d0DCEkceRitxkgTwbKaPK7xqyVZDz+fevEdOEJCJqBh+FRzFT2Q5RdZf lsKT8wgZizLXFb6g/XCKEZE24nr3b8APVZOSXpx1vdb27ov769v9o0yfg5f7I35kxtL0 aiXkZgYH5D1ku11y+Qny1wVga4JcsObceZwfVS8zkNcZ+AXz0S2dmRuRM7lBw0bXBFz8 9qc91G9DEhBny0Ie/FDEVNMSxi0SGBhb1qXdZfudVmaTz7CnDB6AXcs+dAU9P+2kq6hD AqDK5HKz8jWK6UbaFLT3RbMt2fJdSWrKQihg4lWSTXF35z7asfbungneEPkXiZAEtW3I vSLg== X-Gm-Message-State: AOAM530XxFMBw8vt7x7P1QyfbG+3b1NyfF+RHF1Ke25dakw14KmkgMa1 Pitg7z3I2ahJhn1R3m5+dP80QoQ2FijCjg== X-Google-Smtp-Source: ABdhPJxofDwgei5nJILFlmeUVYT5sPEg4Co9C6L/QHzh+MWc0ciLfVXyWKXT+NreFg41NQoX52BzBQ== X-Received: by 2002:adf:d4c6:: with SMTP id w6mr9583901wrk.92.1590077340874; Thu, 21 May 2020 09:09:00 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id c143sm7134776wmd.43.2020.05.21.09.09.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 May 2020 09:09:00 -0700 (PDT) Date: Thu, 21 May 2020 16:08:58 +0000 From: Tomasz Figa To: Hans Verkuil , Xia Jiang Subject: Re: [PATCH v8 14/14] media: platform: Add jpeg dec/enc feature Message-ID: <20200521160858.GL209565@chromium.org> References: <20200403094033.8288-1-xia.jiang@mediatek.com> <20200403094033.8288-15-xia.jiang@mediatek.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200521_090902_578623_ECAE49FE X-CRM114-Status: GOOD ( 26.74 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Xia Jiang , devicetree@vger.kernel.org, mojahsu@chromium.org, srv_heupstream@mediatek.com, Rick Chang , senozhatsky@chromium.org, drinkcat@chromium.org, linux-kernel@vger.kernel.org, maoguang.meng@mediatek.com, Matthias Brugger , sj.huang@mediatek.com, Rob Herring , linux-mediatek@lists.infradead.org, Mauro Carvalho Chehab , Marek Szyprowski , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, May 11, 2020 at 11:04:19AM +0200, Hans Verkuil wrote: > On 03/04/2020 11:40, Xia Jiang wrote: > > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jpeg > > decode and encode have great similarities with function operation. > > > > Signed-off-by: Xia Jiang > > --- > > v8:jpeg encoder and decoder use separate callbacks instead of repeating > > the if/else in every callback. > > improve vidioc_try_fmt() implementation that can be shared by jpeg > > encoder and decoder. > > fix the bug of jpeg encoder s_selection implementation. > > cancel the state of the jpeg encoder. > > improve jpeg encoder and decoder set default params flow. > > put the clock names and other datas in a match_data struct. > > fix the bug of geting correctly quality value. > > do the all the bits' settings of one register in one function. > > move the code of mtk_jpeg_enc_reg.h to mtk_jpeg_enc_hw.h and delete > > mtk_jpeg_enc_reg.h. > > > > v7: reverse spin lock and unlock operation in device run function for > > multi-instance. > > > > v6: add space to arounding '+'. > > alignment 'struct mtk_jpeg_fmt *fmt' match open parenthesis. > > change 'mtk_jpeg_enc_set_encFormat' to 'mtk_jpeg_enc_set_enc_format'. > > make 'mtk_jpeg_ctrls_setup' to static prototype. > > delete unused variables 'jpeg'/'align_h'/'align_w'/'flags'. > > initialize 'yuv_format'/'enc_quality' variables. > > > > v5: support crop for encoder and compose for decoder in s_selection and > > g_selection function. > > use clamp() to replace mtk_jpeg_bound_align_image() and round_up() > > to replace mtk_jpeg_align(). > > delete jpeg_enc_param/mtk_jpeg_enc_param structure and > > mtk_jpeg_set_param(), program the registers directly based on > > the original V4L2 values. > > move macro definition about hw to mtk_jpeg_enc_reg.h. > > delete unnecessary V4L2 logs in driver. > > cancel spin lock and unlock operation in deviec run function. > > change jpeg enc register offset hex numberals from upercase to lowercase. > > > > v4: split mtk_jpeg_try_fmt_mplane() to two functions, one for encoder, > > one for decoder. > > split mtk_jpeg_set_default_params() to two functions, one for > > encoder, one for decoder. > > add cropping support for encoder in g/s_selection ioctls. > > change exif mode support by using V4L2_JPEG_ACTIVE_MARKER_APP1. > > change MTK_JPEG_MAX_WIDTH/MTK_JPEG_MAX_HEIGH from 8192 to 65535 by > > specification. > > move width shifting operation behind aligning operation in > > mtk_jpeg_try_enc_fmt_mplane() for bug fix. > > fix user abuseing data_offset issue for DMABUF in > > mtk_jpeg_set_enc_src(). > > fix kbuild warings: change MTK_JPEG_MIN_HEIGHT/MTK_JPEG_MAX_HEIGHT > > and MTK_JPEG_MIN_WIDTH/MTK_JPEG_MAX_WIDTH from > > 'int' type to 'unsigned int' type. > > fix msleadingly indented of 'else'. > > v3: delete Change-Id. > > only test once handler->error after the last v4l2_ctrl_new_std(). > > seperate changes of v4l2-ctrls.c and v4l2-controls.h to new patch. > > v2: fix compliance test fail, check created buffer size in driver. > > --- > > drivers/media/platform/mtk-jpeg/Makefile | 5 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 1038 +++++++++++++---- > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 51 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h | 7 +- > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 193 +++ > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 123 ++ > > 6 files changed, 1188 insertions(+), 229 deletions(-) > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h > > > > diff --git a/drivers/media/platform/mtk-jpeg/Makefile b/drivers/media/platform/mtk-jpeg/Makefile > > index 48516dcf96e6..76c33aad0f3f 100644 > > --- a/drivers/media/platform/mtk-jpeg/Makefile > > +++ b/drivers/media/platform/mtk-jpeg/Makefile > > @@ -1,3 +1,6 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > -mtk_jpeg-objs := mtk_jpeg_core.o mtk_jpeg_dec_hw.o mtk_jpeg_dec_parse.o > > +mtk_jpeg-objs := mtk_jpeg_core.o \ > > + mtk_jpeg_dec_hw.o \ > > + mtk_jpeg_dec_parse.o \ > > + mtk_jpeg_enc_hw.o > > obj-$(CONFIG_VIDEO_MEDIATEK_JPEG) += mtk_jpeg.o > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > index 77a95185584c..18a759ce2c46 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > > @@ -3,6 +3,7 @@ > > * Copyright (c) 2016 MediaTek Inc. > > * Author: Ming Hsiu Tsai > > * Rick Chang > > + * Xia Jiang > > */ > > > > #include > > @@ -23,11 +24,60 @@ > > #include > > #include > > > > +#include "mtk_jpeg_enc_hw.h" > > #include "mtk_jpeg_dec_hw.h" > > #include "mtk_jpeg_core.h" > > #include "mtk_jpeg_dec_parse.h" > > > > -static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > +static struct mtk_jpeg_fmt mtk_jpeg_enc_formats[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_JPEG, > > + .colplanes = 1, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_CAPTURE, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV12M, > > + .hw_format = JPEG_ENC_YUV_FORMAT_NV12, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_NV21M, > > + .hw_format = JEPG_ENC_YUV_FORMAT_NV21, > > + .h_sample = {4, 4}, > > + .v_sample = {4, 2}, > > + .colplanes = 2, > > + .h_align = 4, > > + .v_align = 4, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YUYV, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_YVYU, > > + .hw_format = JPEG_ENC_YUV_FORMAT_YVYU, > > + .h_sample = {8}, > > + .v_sample = {4}, > > + .colplanes = 1, > > + .h_align = 5, > > + .v_align = 3, > > + .flags = MTK_JPEG_FMT_FLAG_ENC_OUTPUT, > > + }, > > +}; > > + > > +static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = { > > { > > .fourcc = V4L2_PIX_FMT_JPEG, > > .colplanes = 1, > > @@ -53,7 +103,8 @@ static struct mtk_jpeg_fmt mtk_jpeg_formats[] = { > > }, > > }; > > > > -#define MTK_JPEG_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_formats) > > +#define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats) > > +#define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats) > > > > enum { > > MTK_JPEG_BUF_FLAGS_INIT = 0, > > @@ -70,6 +121,11 @@ struct mtk_jpeg_src_buf { > > static int debug; > > module_param(debug, int, 0644); > > > > +static inline struct mtk_jpeg_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl) > > +{ > > + return container_of(ctrl->handler, struct mtk_jpeg_ctx, ctrl_hdl); > > +} > > + > > static inline struct mtk_jpeg_ctx *mtk_jpeg_fh_to_ctx(struct v4l2_fh *fh) > > { > > return container_of(fh, struct mtk_jpeg_ctx, fh); > > @@ -81,12 +137,25 @@ static inline struct mtk_jpeg_src_buf *mtk_jpeg_vb2_to_srcbuf( > > return container_of(to_vb2_v4l2_buffer(vb), struct mtk_jpeg_src_buf, b); > > } > > > > -static int mtk_jpeg_querycap(struct file *file, void *priv, > > - struct v4l2_capability *cap) > > +static int mtk_jpeg_enc_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > +{ > > + struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > + > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > + strscpy(cap->card, MTK_JPEG_NAME " encoder", sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > + dev_name(jpeg->dev)); > > + > > + return 0; > > +} > > + > > +static int mtk_jpeg_dec_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > { > > struct mtk_jpeg_dev *jpeg = video_drvdata(file); > > > > - strscpy(cap->driver, MTK_JPEG_NAME " decoder", sizeof(cap->driver)); > > + strscpy(cap->driver, MTK_JPEG_NAME, sizeof(cap->driver)); > > strscpy(cap->card, MTK_JPEG_NAME " decoder", sizeof(cap->card)); > > snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > dev_name(jpeg->dev)); > > @@ -94,6 +163,54 @@ static int mtk_jpeg_querycap(struct file *file, void *priv, > > return 0; > > } > > > > +static int vidioc_jpeg_enc_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct mtk_jpeg_ctx *ctx = ctrl_to_ctx(ctrl); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_JPEG_RESTART_INTERVAL: > > + ctx->restart_interval = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_COMPRESSION_QUALITY: > > + ctx->enc_quality = ctrl->val; > > + break; > > + case V4L2_CID_JPEG_ACTIVE_MARKER: > > + ctx->enable_exif = ctrl->val & V4L2_JPEG_ACTIVE_MARKER_APP1 ? > > + true : false; > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static const struct v4l2_ctrl_ops mtk_jpeg_enc_ctrl_ops = { > > + .s_ctrl = vidioc_jpeg_enc_s_ctrl, > > +}; > > + > > +static int mtk_jpeg_enc_ctrls_setup(struct mtk_jpeg_ctx *ctx) > > +{ > > + const struct v4l2_ctrl_ops *ops = &mtk_jpeg_enc_ctrl_ops; > > + struct v4l2_ctrl_handler *handler = &ctx->ctrl_hdl; > > + > > + v4l2_ctrl_handler_init(handler, 3); > > + > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_RESTART_INTERVAL, 0, 100, > > + 1, 0); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_COMPRESSION_QUALITY, 48, > > + 100, 1, 90); > > + v4l2_ctrl_new_std(handler, ops, V4L2_CID_JPEG_ACTIVE_MARKER, 0, > > + V4L2_JPEG_ACTIVE_MARKER_APP1, 0, 0); > > + > > + if (handler->error) { > > + v4l2_ctrl_handler_free(&ctx->ctrl_hdl); > > + return handler->error; > > + } > > + > > + v4l2_ctrl_handler_setup(&ctx->ctrl_hdl); > > + > > + return 0; > > +} > > + > > static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > struct v4l2_fmtdesc *f, u32 type) > > { > > @@ -115,117 +232,105 @@ static int mtk_jpeg_enum_fmt(struct mtk_jpeg_fmt *mtk_jpeg_formats, int n, > > return 0; > > } > > > > -static int mtk_jpeg_enum_fmt_vid_cap(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_CAPTURE); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_cap(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, > > + MTK_JPEG_DEC_NUM_FORMATS, f, > > MTK_JPEG_FMT_FLAG_DEC_CAPTURE); > > } > > > > -static int mtk_jpeg_enum_fmt_vid_out(struct file *file, void *priv, > > - struct v4l2_fmtdesc *f) > > +static int mtk_jpeg_enc_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + return mtk_jpeg_enum_fmt(mtk_jpeg_enc_formats, > > + MTK_JPEG_ENC_NUM_FORMATS, f, > > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT); > > +} > > + > > +static int mtk_jpeg_dec_enum_fmt_vid_out(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > { > > - return mtk_jpeg_enum_fmt(mtk_jpeg_formats, MTK_JPEG_NUM_FORMATS, f, > > - MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > + return mtk_jpeg_enum_fmt(mtk_jpeg_dec_formats, MTK_JPEG_DEC_NUM_FORMATS, > > + f, MTK_JPEG_FMT_FLAG_DEC_OUTPUT); > > } > > OK, so this patch is very hard to read because there are two independent changes > taking place: > > 1) rename existing functions/defines/variables with a _dec prefix to prepare > for the addition of the encoder feature. > > 2) add the encoder feature. > > Please split up this patch into two parts: one that does the rename and as much of > the preparation to support both decoder and encoder without changing the > functionality, and a second one that actually adds the new encoder feature. > > In fact, once that's done it is likely that most of this patch series can be > merged, even if there are still things that need to be changed for the last > patch adding the encoder support. I see nothing objectionable in patches 1-10 > and 13. So merging those together with a new rename patch wouldn't be an issue, > I think. > > In any case, the diffs should be a lot cleaner and easier to review by splitting > it up like that. Agreed with Hans. The split will make it much easier to review this one for me, so I'll wait for it. Thanks in advance. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel