linux-kernel - Re: [PATCH v8 3/3] media: cedrus: Add HEVC/H.265 decoding support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191022124012.GD2651@aptenodytes>
Date:   Tue, 22 Oct 2019 14:40:12 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...glegroups.com,
        Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <mripard@...tlin.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Jonas Karlman <jonas@...boo.se>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v8 3/3] media: cedrus: Add HEVC/H.265 decoding support

Hi Mauro and thanks for the review,

On Thu 17 Oct 19, 09:57, Mauro Carvalho Chehab wrote:
> Em Fri, 27 Sep 2019 16:34:11 +0200
> Paul Kocialkowski <paul.kocialkowski@...tlin.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@...tlin.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.

I wasn't aware that it was the case, thanks for bringing this to light!
I'll make it 1UL then.

> > +#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.

Sounds good! I find that the reverse wording (SHIFT_AND_MASK_BITS) would be
a bit more explicit since the shift happens prior to the mask.

Also we probably need to have parenthesis around "low", right?

> 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.

Understood, I'll include it in that header for now and send a separate patch
for inclusion in linux/bits.h (apparently the preprocessor doesn't care about
redefinitions so we can just remove the cedrus fashion once the common one is
in).

What do you think?

Cheers,

Paul

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)