linux-kernel - Re: [PATCH v4 08/11] media: ti: Add CSI2RX support for J721E

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]

Message-ID: <YV4QUU1NxcgEN81V@paasikivi.fi.intel.com>
Date:   Thu, 7 Oct 2021 00:08:33 +0300
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Pratyush Yadav <p.yadav@...com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Nikhil Devshatwar <nikhil.nd@...com>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Benoit Parrot <bparrot@...com>,
        Maxime Ripard <mripard@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v4 08/11] media: ti: Add CSI2RX support for J721E

On Thu, Oct 07, 2021 at 02:31:43AM +0530, Pratyush Yadav wrote:
> On 06/10/21 11:28PM, Sakari Ailus wrote:
> > Hi Pratyush,
> > 
> > On Wed, Sep 15, 2021 at 05:32:37PM +0530, Pratyush Yadav wrote:
> > ...
> > > +/*
> > > + * Find the input format. This is done by finding the first device in the
> > > + * pipeline which can tell us the current format. This could be the sensor, or
> > > + * this could be another device in the middle which is capable of format
> > > + * conversions.
> > > + */
> > > +static int ti_csi2rx_validate_pipeline(struct ti_csi2rx_dev *csi)
> > > +{
> > > +	struct media_pipeline *pipe = &csi->pipe;
> > > +	struct media_entity *entity;
> > > +	struct v4l2_subdev *sd;
> > > +	struct v4l2_subdev_format fmt;
> > > +	struct v4l2_pix_format *pix = &csi->v_fmt.fmt.pix;
> > > +	struct media_device *mdev = &csi->mdev;
> > > +	const struct ti_csi2rx_fmt *ti_fmt;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&mdev->graph_mutex);
> > > +	ret = media_graph_walk_init(&pipe->graph, mdev);
> > > +	if (ret) {
> > > +		mutex_unlock(&mdev->graph_mutex);
> > > +		return ret;
> > > +	}
> > > +
> > > +	media_graph_walk_start(&pipe->graph, &csi->vdev.entity);
> > > +
> > > +	while ((entity = media_graph_walk_next(&pipe->graph))) {
> > > +		if (!is_media_entity_v4l2_subdev(entity))
> > > +			continue;
> > 
> > You shouldn't rely on media_graph_walk_next() to return entities in a
> > particular order.
> 
> Ah, right. Need to drop this.
> 
> > 
> > I'd suggest approach taken in isp_video_check_external_subdevs() (in
> > drivers/media/platform/omap3isp/ispvideo.c).
> > 
> > > +
> > > +		sd = media_entity_to_v4l2_subdev(entity);
> > > +
> > > +		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +		fmt.pad = media_get_pad_index(entity, 0, PAD_SIGNAL_DEFAULT);
> > > +
> > > +		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> > > +		if (ret && ret != -ENOIOCTLCMD) {
> > > +			media_graph_walk_cleanup(&pipe->graph);
> > > +			mutex_unlock(&mdev->graph_mutex);
> > > +			return ret;
> > > +		}
> > > +
> > > +		if (!ret)
> > > +			break;
> > > +	}
> > > +
> > > +	media_graph_walk_cleanup(&pipe->graph);
> > > +	mutex_unlock(&mdev->graph_mutex);
> > > +
> > > +	/* Could not find input format. */
> > > +	if (!entity)
> > > +		return -EPIPE;
> > > +
> > > +	if (fmt.format.width != pix->width)
> > > +		return -EPIPE;
> > > +	if (fmt.format.height != pix->height)
> > > +		return -EPIPE;
> > 
> > Pipeline validation should take place during media_pipeline_start(). Why
> > are you doing it here?
> 
> How would be do that? Via the link_validate callback?

Yes, please. See other drivers for examples --- such as omap3isp.

-- 
Sakari Ailus