Hi,

On Tue 22 Oct 19, 15:37, Hans Verkuil wrote:

On 10/22/19 3:17 PM, Paul Kocialkowski wrote:

quoted

Hi again,

On Tue 22 Oct 19, 14:40, Paul Kocialkowski wrote:

quoted

Hi Mauro and thanks for the review,

On Thu 17 Oct 19, 09:57, Mauro Carvalho Chehab wrote:

quoted

Em Fri, 27 Sep 2019 16:34:11 +0200
Paul Kocialkowski [off-list ref] escreveu:

quoted

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

...

quoted

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

quoted

quoted

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

This would work for cedrus, but I think that what Mauro had in mind was to
migrate this macro to linux/bits.h, where everthing else (including GENMASK)
is apparently defined in terms of unsigned long and not types with explicit
numbers of bits. So I find it more consistent to go with unsigned long.

In our case, 64-bit platforms that use cedrus would calculate the macro on
64 bits and use it in 32-bit variables. Since we're never masking beyond the
lower 32 bits, I don't see how things could go wrong and the situation looks
fairly similar to the use of GENMASK in similar conditions.

Does that sound right to you or am I missing something here?

Cheers,

Paul

Regards,

	Hans

quoted

Cheers,

Paul

quoted

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

quoted

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

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com