Linux-Rockchip Archive on lore.kernel.org
 help / color / Atom feed
From: Ezequiel Garcia <ezequiel@collabora.com>
To: Robin Murphy <robin.murphy@arm.com>,
	Adrian Ratiu <adrian.ratiu@collabora.com>,
	Philipp Zabel <p.zabel@pengutronix.de>
Cc: Fruehberger Peter <Peter.Fruehberger@de.bosch.com>,
	kernel@collabora.com, linux-kernel@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	Mark Brown <broonie@kernel.org>,
	kuhanh.murugasen.krishnan@intel.com,
	Daniel Vetter <daniel@ffwll.ch>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH 00/18] Add Hantro regmap and VC8000 h264 decode support
Date: Thu, 29 Oct 2020 13:27:08 -0300
Message-ID: <5a653c86f887bf8f3d34c4d620471300b612a4d8.camel@collabora.com> (raw)
In-Reply-To: <0dd9fb9d-3f33-b9b0-a7a8-6d3111e92d64@arm.com>

Hi Robin,

On Thu, 2020-10-29 at 14:15 +0000, Robin Murphy wrote:
> On 2020-10-29 13:07, Ezequiel Garcia wrote:
> > Hello Adrian,
> > 
> > On Mon, 2020-10-12 at 23:59 +0300, Adrian Ratiu wrote:
> > > Dear all,
> > > 
> > > This series introduces a regmap infrastructure for the Hantro driver
> > > which is used to compensate for different HW-revision register layouts.
> > > To justify it h264 decoding capability is added for newer VC8000 chips.
> > > 
> > > This is a gradual conversion to the new infra - a complete conversion
> > > would have been very big and I do not have all the HW yet to test (I'm
> > > expecting a RK3399 shipment next week though ;). I think converting the
> > > h264 decoder provides a nice blueprint for how the other codecs can be
> > > converted and enabled for different HW revisions.
> > > 
> > > The end goal of this is to make the driver more generic and eliminate
> > > entirely custom boilerplate like `struct hantro_reg` or headers with
> > > core-specific bit manipulations like `hantro_g1_regs.h` and instead rely
> > > on the well-tested albeit more verbose regmap subsytem.
> > > 
> > > To give just two examples of bugs which are easily discovered by using
> > > more verbose regmap fields (very easy to compare with the datasheets)
> > > instead of relying on bit-magic tricks: G1_REG_DEC_CTRL3_INIT_QP(x) was
> > > off-by-1 and the wrong .clk_gate bit was set in hantro_postproc.c.
> > > 
> > > Anyway, this series also extends the MMIO regmap API to allow relaxed
> > > writes for the theoretical reason that avoiding unnecessary membarriers
> > > leads to less CPU usage and small improvements to battery life. However,
> > > in practice I could not measure differences between relaxed/non-relaxed
> > > IO, so I'm on the fence whether to keep or remove the relaxed calls.
> > > 
> > > What I could masure is the performance impact of adding more sub-reg
> > > field acesses: a constant ~ 20 microsecond bump per G1 h264 frame. This
> > > is acceptable considering the total time to decode a frame takes three
> > > orders of magnitude longer, i.e. miliseconds ranges, depending on the
> > > frame size and bitstream params, so it is an acceptable trade-off to
> > > have a more generic driver.
> > > 
> > 
> > Before going forward with using regmap, I would like to have a sense
> > of the footprint it adds, and see if we can avoid that 20 us penalty.
> > 
> > I'd also like to try another approach, something that has less
> > memory footprint and less runtime penalty.
> > 
> > How about something like this:
> > 
> > #define G1_PIC_WIDTH 4, 0xff8, 23
> > #define ...
> >                                   
> > struct hantro_swreg {
> >          u32 value[399 /*whatever size goes here*/];
> > };
> >                                                                                   
> > void hantro_reg_write(struct hantro_swreg *r,
> >                        unsigned int swreg, u32 mask, u32 offset, u32 new_val)
> > {
> >          r->value[swreg] = (r->value[swreg] & ~(mask)) |
> >                            ((new_val << offset) & mask);
> > }
> > 
> > Which you can then use in a very similar way as the current proposal:
> > 
> > hantro_reg_write(&swreg, G1_PIC_WIDTH, width);
> > 
> > The first advantage here is that we no longer have any
> > footprint for the fields.
> > 
> > The ugly macros for "4, 0xff8, 23" can be auto-generated from
> > existing vendor headers, when possible, so that shouldn't
> > bother us.
> > 
> > The register set is "flushed" using _relaxed, but it
> > could be still costly.
> > 
> > If that is indeed costly, perhaps we can avoid writing
> > the entire set by having a dirty bit somewhere.
> > 
> > In any case, it's worth exploring our options first, I think.
> 
> Or maybe the regmap API itself deserves extending with a "deferred" 
> operating mode where updates to the cached state can be separated from 
> committing that state to the underlying hardware.
> 
> ...which, after a brief code search out of curiosity, apparently already 
> exists in the form of regcache_cache_only()/regcache_sync(), so there's 
> probably no need to reinvent it :)
> 

To be fair, and despite it could seem an anti-pattern, this particular
wheel is so tiny and trivial, that I'm starting to seriously consider
reinventing it.

I've been thinking long about this but just can't seem to see exactly
what benefit we're getting from using MMIO regmaps here,
as opposed to just a simple macro with an index, a mask, and an offset.

Ezequiel



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12 20:59 Adrian Ratiu
2020-10-12 20:59 ` [PATCH 01/18] media: hantro: document all int reg bits up to vc8000 Adrian Ratiu
2020-10-12 20:59 ` [PATCH 02/18] media: hantro: make consistent use of decimal register notation Adrian Ratiu
2020-10-12 20:59 ` [PATCH 03/18] media: hantro: make G1_REG_SOFT_RESET Rockchip specific Adrian Ratiu
2020-10-12 20:59 ` [PATCH 04/18] media: hantro: add reset controller support Adrian Ratiu
2020-10-13  8:11   ` Philipp Zabel
2020-10-12 20:59 ` [PATCH 05/18] media: hantro: prepare clocks before variant inits are run Adrian Ratiu
2020-10-12 20:59 ` [PATCH 06/18] media: hantro: imx8mq: simplify ctrlblk reset logic Adrian Ratiu
2020-10-12 20:59 ` [PATCH 07/18] regmap: mmio: add config option to allow relaxed MMIO accesses Adrian Ratiu
2020-10-13 10:26   ` Mark Brown
2020-10-14 11:51     ` Adrian Ratiu
2020-10-14 12:12       ` Mark Brown
2020-10-14 13:00         ` Adrian Ratiu
2020-10-12 20:59 ` [PATCH 08/18] media: hantro: add initial MMIO regmap infrastructure Adrian Ratiu
2020-10-12 20:59 ` [PATCH 09/18] media: hantro: default regmap to relaxed MMIO Adrian Ratiu
2020-10-12 20:59 ` [PATCH 10/18] media: hantro: convert G1 h264 decoder to regmap fields Adrian Ratiu
2020-10-12 20:59 ` [PATCH 11/18] media: hantro: convert G1 postproc to regmap Adrian Ratiu
2020-10-12 20:59 ` [PATCH 12/18] media: hantro: add VC8000D h264 decoding Adrian Ratiu
2020-10-12 20:59 ` [PATCH 13/18] media: hantro: add VC8000D postproc support Adrian Ratiu
2020-10-12 20:59 ` [PATCH 14/18] media: hantro: make PP enablement logic a bit smarter Adrian Ratiu
2020-10-12 20:59 ` [PATCH 15/18] media: hantro: add user-selectable, platform-selectable H264 High10 Adrian Ratiu
2020-10-12 20:59 ` [PATCH 16/18] media: hantro: rename h264_dec as it's not G1 specific anymore Adrian Ratiu
2020-10-12 20:59 ` [PATCH 17/18] media: hantro: add dump registers debug option before decode start Adrian Ratiu
2020-10-12 20:59 ` [PATCH 18/18] media: hantro: document encoder reg fields Adrian Ratiu
2020-10-12 23:39 ` [PATCH 00/18] Add Hantro regmap and VC8000 h264 decode support Jonas Karlman
2020-10-13  6:48   ` Adrian Ratiu
2020-10-29 12:38   ` Ezequiel Garcia
2020-10-29 16:21     ` Jonas Karlman
2020-11-03 15:27       ` Ezequiel Garcia
2020-10-29 13:07 ` Ezequiel Garcia
2020-10-29 14:15   ` Robin Murphy
2020-10-29 14:48     ` Mark Brown
2020-10-29 16:27     ` Ezequiel Garcia [this message]
2020-10-29 17:59       ` Mark Brown

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=5a653c86f887bf8f3d34c4d620471300b612a4d8.camel@collabora.com \
    --to=ezequiel@collabora.com \
    --cc=Peter.Fruehberger@de.bosch.com \
    --cc=adrian.ratiu@collabora.com \
    --cc=broonie@kernel.org \
    --cc=daniel@ffwll.ch \
    --cc=kernel@collabora.com \
    --cc=kuhanh.murugasen.krishnan@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=robin.murphy@arm.com \
    /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-Rockchip Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rockchip/0 linux-rockchip/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-rockchip linux-rockchip/ https://lore.kernel.org/linux-rockchip \
		linux-rockchip@lists.infradead.org
	public-inbox-index linux-rockchip

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-rockchip


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