Hi again, On Tue 22 Oct 19, 14:40, Paul Kocialkowski wrote: > 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. Apparently the UL(v) macro just appends UL to v in preprocessor, so it won't work with anything else than direct integers. I'll replace it with a (unsigned long) cast, that seems to do the job. Cheers, Paul > 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 -- Developer of free digital technology and hardware support. Website: https://www.paulk.fr/ Coding blog: https://code.paulk.fr/ Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Attachment:
signature.asc
Description: PGP signature
