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

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?

Regards,

	Hans

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel