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=-3.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 C9439CA9EAF for ; Thu, 24 Oct 2019 09:24:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8BABA205F4 for ; Thu, 24 Oct 2019 09:24:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="LErg4eJH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2393062AbfJXJYA (ORCPT ); Thu, 24 Oct 2019 05:24:00 -0400 Received: from mail-ed1-f68.google.com ([209.85.208.68]:38008 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2390225AbfJXJX7 (ORCPT ); Thu, 24 Oct 2019 05:23:59 -0400 Received: by mail-ed1-f68.google.com with SMTP id y8so5807586edu.5 for ; Thu, 24 Oct 2019 02:23:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=2Fg4OORdS56GYlEdlxxKyHlR08HjtyPW5mXDJkHAU3c=; b=LErg4eJHixvsMHo3T4UdsdOcjyo5lbO9XudSeZq6rpgNnkVpGr/b0G90t4XOSP66/E yPiEP9VtvxDCkUlH7d1janZhA4XnWhbkjvm/cOE2ToCJXuDoxd+uba3V89MU7eceWGgo 3dPD97YPgwTyy5neIrYaAT8SNNB8krsO2ClP8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=2Fg4OORdS56GYlEdlxxKyHlR08HjtyPW5mXDJkHAU3c=; b=qt1n/9U6Um5z1CkNaZRDWc7U2SsW5ldA8BRuvLA2av32lCSI+sO2WPUVT+9SE9N8P5 Akfmn4Q1lRNyltfwjlaHunyrAAJSEBUaC9okbcwtjqfSM/8ndfiJbVwMPvwtii8FSia9 m5XiN9OZGA0DBNZzHm1YW8Z6+eGMj9n72K5yiIInReKh/8yeAR+lHv+vplvgYqNlnJrX 9A1EzaTrtReHsrF7XGJq2QpsZ2nNaq7ZslIH9awa7PLFVcqMrAk8PoO7cGt5URGBASbE 9u+oXbZ70jLVPqpZ4KZsjPyxrUzWk6oDgae2vn3uchnSAXFKdam2rYWLwNFQB5u9I2WN sOPg== X-Gm-Message-State: APjAAAVdkd3Z8c5MMOPzWMq9b94Pc5bpfnacDDN7VfahCTkg8UnQTQkZ y9cLuQvWYDF85JNeFwWf5NfB16rDqOhQNg== X-Google-Smtp-Source: APXvYqwrVxTOXBhFsZzGueJ0vGWrsElEzfqDAJ7R2u4cXj0RYY0mI2YJvOdJdTqAE67TBbdGQwlxyg== X-Received: by 2002:a05:6402:128c:: with SMTP id w12mr42908034edv.284.1571909037677; Thu, 24 Oct 2019 02:23:57 -0700 (PDT) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com. [209.85.221.44]) by smtp.gmail.com with ESMTPSA id g43sm473645edb.14.2019.10.24.02.23.55 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 24 Oct 2019 02:23:56 -0700 (PDT) Received: by mail-wr1-f44.google.com with SMTP id z11so1845021wro.11 for ; Thu, 24 Oct 2019 02:23:55 -0700 (PDT) X-Received: by 2002:a5d:6a42:: with SMTP id t2mr2809593wrw.155.1571909035112; Thu, 24 Oct 2019 02:23:55 -0700 (PDT) MIME-Version: 1.0 References: <20191017084033.28299-1-xia.jiang@mediatek.com> <20191017084033.28299-6-xia.jiang@mediatek.com> <20191023103945.GA41089@chromium.org> <1571906317.6254.64.camel@mhfsdcap03> In-Reply-To: <1571906317.6254.64.camel@mhfsdcap03> From: Tomasz Figa Date: Thu, 24 Oct 2019 18:23:43 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 5/5] media: platform: Add jpeg dec/enc feature To: Xia Jiang Cc: Hans Verkuil , Mauro Carvalho Chehab , Rob Herring , Matthias Brugger , Rick Chang , Linux Media Mailing List , linux-devicetree , Linux Kernel Mailing List , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , "moderated list:ARM/Mediatek SoC support" , Marek Szyprowski , srv_heupstream Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu, Oct 24, 2019 at 5:38 PM Xia Jiang wrote: > > On Wed, 2019-10-23 at 19:39 +0900, Tomasz Figa wrote: > > Hi Xia, > > > > On Thu, Oct 17, 2019 at 04:40:38PM +0800, Xia Jiang wrote: > > > Add mtk jpeg encode v4l2 driver based on jpeg decode, because that jp= eg > > > decode and encode have great similarities with function operation. > > > > > > Signed-off-by: Xia Jiang > > > --- > > > 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 b= y > > > 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_HEIGH= T > > > and MTK_JPEG_MIN_WIDTH/MTK_JPEG_MAX_WIDTH fro= m > > > '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 | 731 +++++++++++++++-= -- > > > .../media/platform/mtk-jpeg/mtk_jpeg_core.h | 123 ++- > > > .../media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h | 7 +- > > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 175 +++++ > > > .../media/platform/mtk-jpeg/mtk_jpeg_enc_hw.h | 60 ++ > > > .../platform/mtk-jpeg/mtk_jpeg_enc_reg.h | 49 ++ > > > 7 files changed, 1004 insertions(+), 146 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 > > > create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_reg.= h > > > > > > > First of all, thanks for the patch! > > > > Please check my comments below. > > > > My general feeling about this code is that the encoder hardware block i= s > > completely orthogonal from the decoder block and there is very little c= ode > > reuse from the original decoder driver. > > > > Moreover, a lot of existing code now needs if (decoder) { ... } else {.= .. } > > segments, which complicates the code. > > > > Would it perhaps make sense to instead create a separate mtk-jpeg-enc > > driver? > > > Dear Tomasz, > > Thanks for your comments. > > My reasons about the architecture of jpeg enc driver are as follows: > > The first internal design and realization of jpeg enc driver was a > separate driver, but found that mtk_jpeg_core.c and mtk_jpeg_enc_core.c > have lots of reuse.Because that the core.c mainly contains realization > of v4L2 ioctl functions and some logic which are high similarity between > encoder and decoder. > > The jpeg encoder and decoder are two independent hardwares exactly, so > the code about hardware specification(register setting) are > separated(mtk_jpeg_enc_hw.c and mtk_jpeg_dec_hw.c). > > As for 17 existing code segments contain if(decoder){} else {}, they are > not complicated IMHO.The complicated(multilayer nested) functions are > separated in V4 version as Hans recommendation. > > By the way,the upstreamed module s5p-jpeg > (https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5= p-jpeg/jpeg-core.c#L1998) also use encoder and decoder mode in the common c= ore.c, but their encoder and decoder are the same hardware.Maybe our jpeg e= nc and dec are designed into one hardware in the future.In that case the cu= rrent architecture is more compatible. > > So I prefer the current design. > Would you be able to give some numbers to show the code reuse to justify using the same driver? From my observation, a new driver would result in a significantly cleaner code. If there is a further hardware architecture change, that would likely require another driver, because it wouldn't be compatible with existing programming model anyway. Regardless of that, if we end up with reusing the same driver, I'd like you to fix the issues existing in the current base before adding the encoder functionality. Best regards, Tomasz