linux-kernel - Re: [PATCH 2/2] media: cedrus: Add HEVC/H.265 decoding support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Aug 2018 17:06:57 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devel@...verdev.osuosl.org, linux-arm-kernel@...ts.infradead.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        linux-sunxi@...glegroups.com, Randy Li <ayaka@...lik.info>,
        Hans Verkuil <hverkuil@...all.nl>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Alexandre Courbot <acourbot@...omium.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>
Subject: Re: [PATCH 2/2] media: cedrus: Add HEVC/H.265 decoding support

Hi,

On Tue, Aug 28, 2018 at 10:02:40AM +0200, Paul Kocialkowski wrote:
> @@ -165,7 +182,8 @@ static inline void cedrus_write(struct cedrus_dev *dev, u32 reg, u32 val)
>  
>  static inline u32 cedrus_read(struct cedrus_dev *dev, u32 reg)
>  {
> -	return readl(dev->base + reg);
> +	u32 val = readl(dev->base + reg);
> +	return val;

I'm not sure that's needed :)

> +static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
> +					     const struct v4l2_hevc_dpb_entry *dpb,
> +					     u8 num_active_dpb_entries)
> +{
> +	struct cedrus_dev *dev = ctx->dev;
> +	dma_addr_t dst_luma_addr, dst_chroma_addr;
> +	dma_addr_t mv_col_buf_addr[2];
> +	u32 pic_order_cnt[2];
> +	unsigned int i;
> +
> +	for (i = 0; i < num_active_dpb_entries; i++) {
> +		dst_luma_addr = cedrus_dst_buf_addr(ctx, dpb[i].buffer_index,
> +						    0); // FIXME - PHYS_OFFSET ?
> +		dst_chroma_addr = cedrus_dst_buf_addr(ctx, dpb[i].buffer_index,
> +						      1); // FIXME - PHYS_OFFSET ?
> +		mv_col_buf_addr[0] = cedrus_h265_frame_info_mv_col_buf_addr(ctx,
> +			dpb[i].buffer_index, 0);
> +		pic_order_cnt[0] = dpb[i].pic_order_cnt[0];
> +
> +		if (dpb[i].field_pic) {
> +			mv_col_buf_addr[1] =
> +				cedrus_h265_frame_info_mv_col_buf_addr(ctx,
> +				dpb[i].buffer_index, 1);
> +			pic_order_cnt[1] = dpb[i].pic_order_cnt[1];
> +		}
> +
> +		cedrus_h265_frame_info_write_single(dev, i, dpb[i].field_pic,
> +						    pic_order_cnt,
> +						    mv_col_buf_addr,
> +						    dst_luma_addr,
> +						    dst_chroma_addr);
> +	}
> +}

You have this a number of times, but you can reduce the range of most
of the variables (basically all of them but dev and i) to the loop
itself. Declaring them for the whole function like you did suggests
that you're going to use the value from one iteration to the other,
which isn't the case here.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)