Messages in this thread |  | | Date | Thu, 17 Oct 2019 09:57:51 -0300 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH v8 3/3] media: cedrus: Add HEVC/H.265 decoding support |
| |
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
|  |