From: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-sunxi@googlegroups.com, Chen-Yu Tsai <wens@csie.org>,
Maxime Ripard <mripard@bootlin.com>,
Hans Verkuil <hverkuil@xs4all.nl>,
Ezequiel Garcia <ezequiel@collabora.com>,
Tomasz Figa <tfiga@chromium.org>,
Nicolas Dufresne <nicolas@ndufresne.ca>,
Jernej Skrabec <jernej.skrabec@siol.net>,
Jonas Karlman <jonas@kwiboo.se>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v8 3/3] media: cedrus: Add HEVC/H.265 decoding support
Date: Thu, 17 Oct 2019 09:57:51 -0300 [thread overview]
Message-ID: <20191017095751.5a229051@coco.lan> (raw)
In-Reply-To: <20190927143411.141526-4-paul.kocialkowski@bootlin.com>
Em Fri, 27 Sep 2019 16:34:11 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> escreveu:
> This introduces support for HEVC/H.265 to the Cedrus VPU driver, with
> both uni-directional and bi-directional prediction modes supported.
>
> Field-coded (interlaced) pictures, custom quantization matrices and
> 10-bit output are not supported at this point.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
...
> + unsigned int ctb_size_luma =
> + 1 << log2_max_luma_coding_block_size;
Shifts like this is a little scary. "1" constant is signed. So, if
log2_max_luma_coding_block_size is 31, the above logic has undefined
behavior. Different archs and C compilers may handle it on different
ways.
> +#define VE_DEC_H265_LOW_ADDR_PRIMARY_CHROMA(a) \
> + (((a) << 24) & GENMASK(31, 24))
Same applies here and on other similar macros. You need to enforce
(a) to be unsigned, as otherwise the behavior is undefined.
Btw, this is a recurrent pattern on this file. I would define a
macro, e. g. something like:
#define MASK_BITS_AND_SHIFT(v, high, low) \
((UL(v) << low) & GENMASK(high, low))
And use it for all similar patterns here.
The best would be to include such macro at linux/bits.h, although some
upstream discussion is required.
So, for now, let's add it at this header file, but work upstream
to have it merged there.
Thanks,
Mauro
next prev parent reply other threads:[~2019-10-17 12:58 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-27 14:34 [PATCH v8 0/3] HEVC/H.265 stateless support for V4L2 and Cedrus Paul Kocialkowski
2019-09-27 14:34 ` [PATCH v8 1/3] media: v4l: Add definitions for HEVC stateless decoding Paul Kocialkowski
2019-09-27 14:34 ` [PATCH v8 2/3] media: pixfmt: Document the HEVC slice pixel format Paul Kocialkowski
2019-09-27 14:34 ` [PATCH v8 3/3] media: cedrus: Add HEVC/H.265 decoding support Paul Kocialkowski
2019-10-17 12:57 ` Mauro Carvalho Chehab [this message]
2019-10-22 12:40 ` Paul Kocialkowski
2019-10-22 13:17 ` Paul Kocialkowski
2019-10-22 13:37 ` Hans Verkuil
2019-10-22 14:01 ` Paul Kocialkowski
2019-10-22 14:02 ` Hans Verkuil
2019-10-08 21:48 ` [PATCH v8 0/3] HEVC/H.265 stateless support for V4L2 and Cedrus Jernej Škrabec
2019-10-09 7:12 ` Paul Kocialkowski
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=20191017095751.5a229051@coco.lan \
--to=mchehab@kernel.org \
--cc=ezequiel@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-sunxi@googlegroups.com \
--cc=mripard@bootlin.com \
--cc=nicolas@ndufresne.ca \
--cc=paul.kocialkowski@bootlin.com \
--cc=tfiga@chromium.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=wens@csie.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).