Re: [PATCH v3 3/4] staging: media: Add support for the Allwinner A31 ISP

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
	linux-staging@lists.linux.dev,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Samuel Holland <samuel@sholland.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v3 3/4] staging: media: Add support for the Allwinner A31 ISP
Date: Wed, 20 Apr 2022 09:50:42 +0200	[thread overview]
Message-ID: <Yl+7UrQFyLvfKRdG@aptenodytes> (raw)
In-Reply-To: <20220420074249.GB2951@kadam>

[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]

Hi Dan,

On Wed 20 Apr 22, 10:42, Dan Carpenter wrote:
> I ran Smatch on this patch.

Thanks for doing this!

> On Fri, Apr 15, 2022 at 05:37:07PM +0200, Paul Kocialkowski wrote:
> > +void sun6i_isp_capture_configure(struct sun6i_isp_device *isp_dev)
> > +{
> > +	unsigned int width, height;
> > +	unsigned int stride_luma, stride_chroma = 0;
> > +	unsigned int stride_luma_div4, stride_chroma_div4;
> > +	const struct sun6i_isp_capture_format *format;
> > +	const struct v4l2_format_info *info;
> > +	u32 pixelformat;
> > +
> > +	sun6i_isp_capture_dimensions(isp_dev, &width, &height);
> > +	sun6i_isp_capture_format(isp_dev, &pixelformat);
> > +
> > +	format = sun6i_isp_capture_format_find(pixelformat);
> > +	if (WARN_ON(!format))
> > +		return;
> > +
> > +	sun6i_isp_load_write(isp_dev, SUN6I_ISP_MCH_SIZE_CFG_REG,
> > +			     SUN6I_ISP_MCH_SIZE_CFG_WIDTH(width) |
> > +			     SUN6I_ISP_MCH_SIZE_CFG_HEIGHT(height));
> > +
> > +	info = v4l2_format_info(pixelformat);
> > +	if (WARN_ON(!info))
> > +		return;
> > +
> > +	stride_luma = width * info->bpp[0];
> > +	stride_luma_div4 = DIV_ROUND_UP(stride_luma, 4);
> > +
> > +	if (info->comp_planes > 1) {
> > +		stride_chroma = width * info->bpp[1] / info->hdiv;
> > +		stride_chroma_div4 = DIV_ROUND_UP(stride_chroma, 4);
> 
> stride_chroma_div4 is not intialized on the else path.

One could say it's not an issue to put an uninitialized value in this situation
since the hardware won't be taking it into account but I'll initialize the value
early in the next iteration.

> > +	}
> > +
> > +	sun6i_isp_load_write(isp_dev, SUN6I_ISP_MCH_CFG_REG,
> > +			     SUN6I_ISP_MCH_CFG_EN |
> > +			     SUN6I_ISP_MCH_CFG_OUTPUT_FMT(format->output_format) |
> > +			     SUN6I_ISP_MCH_CFG_STRIDE_Y_DIV4(stride_luma_div4) |
> > +			     SUN6I_ISP_MCH_CFG_STRIDE_UV_DIV4(stride_chroma_div4));
> > +}
> 
> [ snip ]
> 
> > +void sun6i_isp_params_configure(struct sun6i_isp_device *isp_dev)
> > +{
> > +	struct sun6i_isp_params_state *state = &isp_dev->params.state;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&state->lock, flags);
> > +
> > +	sun6i_isp_params_configure_base(isp_dev);
> > +
> > +	/* Default config is only applied at the very first stream start. */
> > +	if (state->configured)
> > +		goto complete;
> > +
> > +	 sun6i_isp_params_configure_modules(isp_dev,
>         ^
> There is an extra space character here.

Good catch, thanks!

> > +					    &sun6i_isp_params_config_default);
> > +
> > +	state->configured = true;
> > +
> > +complete:
> > +	spin_unlock_irqrestore(&state->lock, flags);
> > +}

Cheers,

Paul

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]