Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Jerry-ch Chen <Jerry-ch.Chen@mediatek.com>
Cc: "hans.verkuil@cisco.com" <hans.verkuil@cisco.com>,
	HansVerkuil <hverkuil@xs4all.nl>,
	"laurent.pinchart+renesas@ideasonboard.com"
	<laurent.pinchart+renesas@ideasonboard.com>,
	"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"pihsun@chromium.org" <pihsun@chromium.org>,
	"yuzhao@chromium.org" <yuzhao@chromium.org>,
	"zwisler@chromium.org" <zwisler@chromium.org>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"Sean Cheng (鄭昇弘)" <Sean.Cheng@mediatek.com>,
	"Sj Huang (黃信璋)" <sj.huang@mediatek.com>,
	"Christie Yu (游雅惠)" <christie.yu@mediatek.com>,
	"Frederic Chen (陳俊元)" <Frederic.Chen@mediatek.com>,
	"Jungo Lin (林明俊)" <jungo.lin@mediatek.com>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [RFC PATCH V4 0/4] media: platform: Add support for Face Detection (FD) on mt8183 SoC
Date: Tue, 30 Jun 2020 17:19:12 +0000
Message-ID: <20200630171912.GE1212092@chromium.org> (raw)
In-Reply-To: <1593526253.29676.28.camel@mtksdccf07>

Hi Jerry,

On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:
> Hi Tomasz,
> 
> On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:
> > Hi Jerry,
> > 
> > On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> > > Hi Jerry,
> > > 
> > > On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > >
> > > > Hi Laurent, Tomasz, Matthias,
> > > >
> > > > gentle ping for this patch set,
> > > > If no new comments, I would like to send a newer version.
> > > >
> > > 
> > > Sorry, I still haven't had a chance to look at the series, so feel
> > > free to send a new version and I will take a look at the new one.
> > > 
> > 
> > Finally found some time to review the series. Again sorry for the delay
> > and thanks for your patience.
> > 
> > Some general comments:
> > 1) The metadata format FourCC should be added in a separate patch,
> > together with documentation for it.
> > 2) Control IDs, structs used by the userspace, etc. should be defined in
> > a header under include/uapi/linux.
> > 
> > Please also check my replies to particular patches for further comments.
> > 
> > Best regards,
> > Tomasz
> 
> Appreciate for your reply,
> 
> So far, I've locally created an uapi header:
> include/uapi/linux/mtk_fd_40.h
> which provides some values, control ids, and the definitions of
> structures that would be needed by user of mtk_fd_40 driver.
> In addition, I also provide a MACRO as example in comments that can
> extract the struct member with bit length and offset
> definitions(eliminate the bit-fields).
> 
> Also, I would like to rename struct fd_user_output with struct
> mtk_fd_hw_result. I worry fd_user_output would be a confusing name.

The change sounds good to me.

> I will add them in a separate patch in next version.
> 

Okay.

> I am still working on the documentation, which might be
> Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
> Refering the other pixfmt-*.rst files, I will try to provide the
> flat-table of the metadata with the structure of the mtk_fd_hw_result.
> 

Sounds good to me.

> I am confusing that should I remain the name with -40 in the tail of rst
> file?

The header and documentation file names should match the driver name.  I
just noticed there is some inconsistency in the naming, though. The
driver seems to be located under drivers/media/platform/mtk-isp/fd, but
the driver name in the platform driver struct and as reported by
VIDIOC_QUERYCAP seems to be "mtk-fd-4.0". 

Since we have many mtk-* drivers in the tree currently, I think it might
make sense to consolidate them under drivers/media/platform/mediatek,
similarly to drivers/media/platform/qcom or /rockchip. But it could be
done later, as a follow-up.

My suggestion would be to place the driver under
drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
symbol to include the _40 suffix.

What do you think?

> Since I think the layout of metadata might be different in the future
> mtk fd drivers. Maybe they create a new one or should they update the
> rst file when they are upstreaming?
> 

I think we'll decide what to do when that happens. Depending on whether
there is more face detection hardware available, we might be able to
migrate to standard controls and metadata buffer formats at that time.

> Other comments are almost fixed, I will inform you by this mail thread
> as soon as I finish the documentation of fd metadata format.

Okay, thanks.

Best regards,
Tomasz

      reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-04 12:47 Jerry-ch Chen
2019-12-04 12:47 ` [RFC PATCH V4 1/4] media: v4l2-mem2mem: add v4l2_m2m_suspend, v4l2_m2m_resume Jerry-ch Chen
2020-05-21 17:11   ` Tomasz Figa
2020-05-22  6:01     ` Jerry-ch Chen
2020-06-10 10:28     ` Hans Verkuil
2020-06-10 10:32       ` Tomasz Figa
2020-06-10 18:52         ` Ezequiel Garcia
2020-06-10 19:03           ` Tomasz Figa
2020-06-10 19:14             ` Ezequiel Garcia
2020-06-10 19:26               ` Tomasz Figa
2020-06-14 22:43                 ` Ezequiel Garcia
2019-12-04 12:47 ` [RFC PATCH V4 2/4] dt-bindings: mt8183: Added FD dt-bindings Jerry-ch Chen
2019-12-04 18:58   ` Rob Herring
2020-05-06  8:41     ` Jerry-ch Chen
2019-12-04 12:47 ` [RFC PATCH V4 3/4] dts: arm64: mt8183: Add FD nodes Jerry-ch Chen
2019-12-04 12:47 ` [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver Jerry-ch Chen
2020-05-21 18:28   ` Tomasz Figa
2020-05-22 14:10     ` Jerry-ch Chen
2020-05-25 12:24       ` Tomasz Figa
2020-05-29 12:26         ` Jerry-ch Chen
2020-05-29 12:59           ` Tomasz Figa
2020-06-01 10:37             ` Jerry-ch Chen
2020-05-08  2:02 ` [RFC PATCH V4 0/4] media: platform: Add support for Face Detection (FD) on mt8183 SoC Jerry-ch Chen
2020-05-13 21:45   ` Tomasz Figa
2020-05-21 18:38     ` Tomasz Figa
2020-06-30 14:10       ` Jerry-ch Chen
2020-06-30 17:19         ` Tomasz Figa [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200630171912.GE1212092@chromium.org \
    --to=tfiga@chromium.org \
    --cc=Frederic.Chen@mediatek.com \
    --cc=Jerry-ch.Chen@mediatek.com \
    --cc=Sean.Cheng@mediatek.com \
    --cc=christie.yu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hans.verkuil@cisco.com \
    --cc=hverkuil@xs4all.nl \
    --cc=jungo.lin@mediatek.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=pihsun@chromium.org \
    --cc=sj.huang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    --cc=yuzhao@chromium.org \
    --cc=zwisler@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git