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=-1.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 AEFACC433E0 for ; Tue, 30 Jun 2020 16:54:41 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 7DE3B20768 for ; Tue, 30 Jun 2020 16:54:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="pAo63ak8"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="CVgtFTy2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7DE3B20768 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+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=merlin.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=d2kj1unUzdveTgedccnYwFwKMd1526xp0REiNeRtPEA=; b=pAo63ak8ffzCIcL4rHGjvbKBw N249QPtD5SnUgZQ3klSlw3M8lV+epmT1CHZG1dRt3NNO4zVJmFdMrxnreAvy7iBQz0WUnyp1jGgfy F2a0ga+F29FHLy4fo7O/1J2w48P5EeZKfTbjCclmxAPVrD1aMppHyLz6oMlvcwR1H71glmLkgCrX1 VsojKzEmzRFw1smcZHBfB+i91Z2LJtcbQEGUECyX8jbW5CxfHE97h3Gyj2QeOrtYVeYCT7jNRE4w0 yGuoJQ+aiKmpWQQGXv64m6ktMZhH6kUnghSnusS+RVB3z1KxGydG1RtsDOKAz1RpLAWHchK+PK8Q2 ntJ9XpLng==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqJVS-0004JF-QJ; Tue, 30 Jun 2020 16:53:14 +0000 Received: from mail-wm1-x344.google.com ([2a00:1450:4864:20::344]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqJVJ-0004Hg-6P for linux-arm-kernel@lists.infradead.org; Tue, 30 Jun 2020 16:53:11 +0000 Received: by mail-wm1-x344.google.com with SMTP id q15so19478143wmj.2 for ; Tue, 30 Jun 2020 09:53:04 -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=PL3gqYes7408Y9j0oN+9/8tWxq6suSL5CFMjvoIOGD8=; b=CVgtFTy2QTfqoViXoqnmGJ/iZi91tUj2Bpdip43Pq0diQqzgGNBkGq5N9G2XQWvogo 68V3n2TQHlygtYUjKpStr7tg1ROHAbvN648GHZPuqq49G5p7U3AlcHrOQaxXfINfdefd Gk9ELgymUifA8xcKS3fnlBXshAs1nJ7VDRI+M= 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=PL3gqYes7408Y9j0oN+9/8tWxq6suSL5CFMjvoIOGD8=; b=DzbTsnOb9OgJFNfBxC6Q4paXPE6/+oqW071rMnwKoU0HLjrFgYef5J/o6Tb8N707rx ApoRoNHPDFVyZvJUsRbolzAy1wG1qx4/zQu79cS88uUUarCE3OAMJ4lqcOukpOfx3762 vFwbbv8EVg3ec/xJQjhf0wzLBR5WX2CB8V1dMRDAI8HggshFI7e12BD5cGykGGzkY/PW su7OfobmnfcF38spNgDvhSXuP2Rwk3Bq1A5KBS7O2+VjfkIcFtD1Y556h8zxe0BfTfGH fWi4Q9XDdNMEe1xaSXAS22ZwnLOF3aslgKNeZiqnByCtaGaA7r/tfxIsByYD/cBRdbCE aLng== X-Gm-Message-State: AOAM532SLxnwfQk9ci4ZIrmsKs1+H7WFJHjNQSVtDOTv5wjldqAmRXLP CFomQACBH0/YwC4nNp94wXp6ZQ== X-Google-Smtp-Source: ABdhPJxKifqOAzYOp4J9Bly4NiQnEMA2gDFLYP7zxPsqUSediKZtqJghVuv6mAfW2Fa0i+QexvGymA== X-Received: by 2002:a1c:bcd4:: with SMTP id m203mr22111391wmf.124.1593535983680; Tue, 30 Jun 2020 09:53:03 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id f186sm4010614wmf.29.2020.06.30.09.53.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jun 2020 09:53:03 -0700 (PDT) Date: Tue, 30 Jun 2020 16:53:01 +0000 From: Tomasz Figa To: Xia Jiang Subject: Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature Message-ID: <20200630165301.GA1212092@chromium.org> References: <20200604090553.10861-1-xia.jiang@mediatek.com> <20200604090553.10861-20-xia.jiang@mediatek.com> <20200611184640.GC8694@chromium.org> <1593485781.20112.43.camel@mhfsdcap03> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1593485781.20112.43.camel@mhfsdcap03> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200630_125305_382797_8ABA96FD X-CRM114-Status: GOOD ( 36.90 ) 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: drinkcat@chromium.org, devicetree@vger.kernel.org, mojahsu@chromium.org, srv_heupstream@mediatek.com, Rick Chang , senozhatsky@chromium.org, linux-kernel@vger.kernel.org, maoguang.meng@mediatek.com, Mauro Carvalho Chehab , sj.huang@mediatek.com, Rob Herring , Matthias Brugger , Hans Verkuil , linux-mediatek@lists.infradead.org, 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+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Xia, On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote: > On Thu, 2020-06-11 at 18:46 +0000, Tomasz Figa wrote: > > Hi Xia, > > > > On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote: [snip] > > > +static void mtk_jpeg_enc_device_run(void *priv) > > > +{ > > > + struct mtk_jpeg_ctx *ctx = priv; > > > + struct mtk_jpeg_dev *jpeg = ctx->jpeg; > > > + struct vb2_v4l2_buffer *src_buf, *dst_buf; > > > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > > > + unsigned long flags; > > > + struct mtk_jpeg_src_buf *jpeg_src_buf; > > > + struct mtk_jpeg_enc_bs enc_bs; > > > + int ret; > > > + > > > + src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > > > + dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > > + jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf); > > > + > > > + ret = pm_runtime_get_sync(jpeg->dev); > > > + if (ret < 0) > > > + goto enc_end; > > > + > > > + spin_lock_irqsave(&jpeg->hw_lock, flags); > > > + > > > + /* > > > + * Resetting the hardware every frame is to ensure that all the > > > + * registers are cleared. This is a hardware requirement. > > > + */ > > > + mtk_jpeg_enc_reset(jpeg->reg_base); > > > + > > > + mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf, &enc_bs); > > > + mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf); > > > + mtk_jpeg_enc_set_config(jpeg->reg_base, ctx->out_q.fmt->hw_format, > > > + ctx->enable_exif, ctx->enc_quality, > > > + ctx->restart_interval); > > > + mtk_jpeg_enc_start(jpeg->reg_base); > > > > Could we just move the above 5 functions into one function inside > > mtk_jpeg_enc_hw.c that takes mtk_jpeg_dev pointer as its argument, let's > > say mtk_jpeg_enc_hw_run() and simply program all the data to the registers > > directly, without the extra level of abstractions? > I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but > this function will be very long, because it contains computation code > such as setting dst addr, blk_num, quality. > In v4, you have adviced the following architecture: > How about the following model, as used by many other drivers: > > mtk_jpeg_enc_set_src() > { > // Set any registers related to source format and buffer > } > > mtk_jpeg_enc_set_dst() > { > // Set any registers related to destination format and buffer > } > > mtk_jpeg_enc_set_params() > { > // Set any registers related to additional encoding parameters > } > > mtk_jpeg_enc_device_run(enc, ctx) > { > mtk_jpeg_enc_set_src(enc, src_buf, src_fmt); > mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt); > mtk_jpeg_enc_set_params(enc, ctx); > // Trigger the hardware run > } > I think that this architecture is more clear(mtk_jpeg_enc_set_config is > equivalent to mtk_jpeg_enc_set_params). > Should I keep the original architecture or move 5 functions into > mtk_jpeg_enc_hw_run? Sounds good to me. My biggest issue with the code that it ends up introducing one more level of abstraction, but with the approach you suggested, the arguments just accept standard structs, which avoids that problem. [snip] > > > + > > > + ctx->fh.ctrl_handler = &ctx->ctrl_hdl; > > > + ctx->colorspace = V4L2_COLORSPACE_JPEG, > > > + ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > + ctx->quantization = V4L2_QUANTIZATION_DEFAULT; > > > + ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > Since we already have a v4l2_pix_format_mplane struct which has fields for > > the above 4 values, could we just store them there? > > > > Also, I don't see this driver handling the colorspaces in any way, but it > > seems to allow changing them from the userspace. This is incorrect, because > > the userspace has no way to know that the colorspace is not handled. > > Instead, the try_fmt implementation should always override the > > userspace-provided colorspace configuration with the ones that the driver > > assumes. > > > > > + pix_mp->width = MTK_JPEG_MIN_WIDTH; > > > + pix_mp->height = MTK_JPEG_MIN_HEIGHT; > > > + > > > + q->fmt = mtk_jpeg_find_format(V4L2_PIX_FMT_YUYV, > > > + MTK_JPEG_FMT_FLAG_ENC_OUTPUT); > > > + vidioc_try_fmt(container_of(pix_mp, struct v4l2_format, > > > + fmt.pix_mp), q->fmt); > > > + q->w = pix_mp->width; > > > + q->h = pix_mp->height; > > > + q->crop_rect.width = pix_mp->width; > > > + q->crop_rect.height = pix_mp->height; > > > + q->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage; > > > + q->bytesperline[0] = pix_mp->plane_fmt[0].bytesperline; > > > > Actually, do we need this custom mtk_jpeg_q_data struct? Why couldn't we > > just keep the same values inside the standard v4l2_pix_format_mplane > > struct? > I think that we need mtk_jpeg_q_data struct.If we delete it, how can we > know these values(w, h, sizeimage, bytesperline, mtk_jpeg_fmt) belong to > output or capture(output and capture's sizeimages are different, width > and height are differnt too for jpeg dec )?We have > s_fmt_vid_out_mplane/cap_mplane function to set these values. > > But we can use standard v4l2_pix_format_mplane struct replacing the w, h > bytesperline, sizeimage in mtk_jpeg_q_data struct like this: > struct mtk_jpeg_q_data{ > struct mtk_jpeg_fmt *fmt; > struct v4l2_pix_format_mplane pix_mp; > struct v4l2_rect enc_crop_rect > } > Then delete ctx->colorspace ctx->ycbcr_enc ctx->quantization > ctx->xfer_func, becuase v4l2_pix_format_mplane in q_data has contained > them and assign them for out_q and cap_q separately. > > WDYT? Sounds good to me. I was considering just making it like struct mtk_jpeg_ctx { struct mtk_jpeg_fmt *src_fmt; struct v4l2_pix_format_mplane src_pix_mp; struct v4l2_rect src_crop; struct mtk_jpeg_fmt *dst_fmt; struct v4l2_pix_format_mplane dst_pix_mp; struct v4l2_rect dst_crop; }; but I like your suggestion as well, as long as custom data structures are not used to store standard information. [snip] > > > @@ -1042,8 +1619,12 @@ static int mtk_jpeg_probe(struct platform_device *pdev) > > > return jpeg_irq; > > > } > > > > > > - ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_dec_irq, 0, > > > - pdev->name, jpeg); > > > + if (jpeg->variant->is_encoder) > > > + ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_enc_irq, > > > + 0, pdev->name, jpeg); > > > + else > > > + ret = devm_request_irq(&pdev->dev, jpeg_irq, mtk_jpeg_dec_irq, > > > + 0, pdev->name, jpeg); > > > > Rather than having "is_encoder" in the variant struct, would it make more > > sense to have "irq_handler" instead? That would avoid the explicit if. > Do you mean to delete "is_encoder"? It is used 8 times in the > driver.Should I move them all to the match data? Yes. It would make the code linear and the varability between the decoder and encoder would be self-contained in the variant struct. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel