Linux-Rockchip Archive on lore.kernel.org
 help / color / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Ezequiel Garcia <ezequiel@collabora.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 14:15:10 +0000
Message-ID: <0dd9fb9d-3f33-b9b0-a7a8-6d3111e92d64@arm.com> (raw)
In-Reply-To: <d4d080658113ad97816eda9262736e5fcc7b017c.camel@collabora.com>

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 :)

Robin.

> 
> PS: Another option is to just fork RK3399 to its
> own driver and call the day, given how different it is :)
> 
> Thanks!
> Ezequiel
> 
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

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

  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 [this message]
2020-10-29 14:48     ` Mark Brown
2020-10-29 16:27     ` Ezequiel Garcia
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=0dd9fb9d-3f33-b9b0-a7a8-6d3111e92d64@arm.com \
    --to=robin.murphy@arm.com \
    --cc=Peter.Fruehberger@de.bosch.com \
    --cc=adrian.ratiu@collabora.com \
    --cc=broonie@kernel.org \
    --cc=daniel@ffwll.ch \
    --cc=ezequiel@collabora.com \
    --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 \
    /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