On 10/22/19 3:17 PM, Paul Kocialkowski wrote: > 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. Shouldn't that be a (u32) cast? Since this is used with 32 bit registers? Regards, Hans > > 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 > > >
