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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > --- > > ... > > > + 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
Attachment:
signature.asc
Description: PGP signature
