linux-kernel - RE: [PATCH v3 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

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: <CY4PR02MB2709000898D5E290B5EE99F4A7640@CY4PR02MB2709.namprd02.prod.outlook.com>
Date:   Mon, 11 Feb 2019 12:43:08 +0000
From:   Vishal Sagar <vsagar@...inx.com>
To:     Luca Ceresoli <luca@...aceresoli.net>,
        Vishal Sagar <vishal.sagar@...inx.com>,
        Hyun Kwon <hyunk@...inx.com>,
        "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        Michal Simek <michals@...inx.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
        "hans.verkuil@...co.com" <hans.verkuil@...co.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Dinesh Kumar <dineshk@...inx.com>,
        Sandip Kothari <sandipk@...inx.com>
Subject: RE: [PATCH v3 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
 Subsystem

Hi Luca,

Thanks for reviewing this. 

> -----Original Message-----
> From: Luca Ceresoli [mailto:luca@...aceresoli.net]
> Sent: Monday, February 11, 2019 4:12 PM
> To: Vishal Sagar <vishal.sagar@...inx.com>; Hyun Kwon <hyunk@...inx.com>;
> laurent.pinchart@...asonboard.com; mchehab@...nel.org;
> robh+dt@...nel.org; mark.rutland@....com; Michal Simek
> <michals@...inx.com>; linux-media@...r.kernel.org;
> devicetree@...r.kernel.org; sakari.ailus@...ux.intel.com;
> hans.verkuil@...co.com; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; Dinesh Kumar <dineshk@...inx.com>; Sandip Kothari
> <sandipk@...inx.com>
> Subject: Re: [PATCH v3 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem
> 
> EXTERNAL EMAIL
> 
> Hi Vishal,
> 
> On 01/02/19 13:56, Vishal Sagar wrote:
> > The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> > from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> > for image processing. Please refer to PG232 for details.
> 
> For those unused to Xilinx documentation I'd use the full document name
> ("MIPI CSI-2 Receiver Subsystem v4.0") or, even better, a stable URL if
> available.
> 

Ok. I will add the full documentation name here and URL.

> > The driver is used to set the number of active lanes, if enabled
> > in hardware. The CSI2 Rx controller filters out all packets except for
> > the packets with data type fixed in hardware. RAW8 packets are always
> > allowed to pass through.
> >
> > It is also used to setup and handle interrupts and enable the core. It
> > logs all the events in respective counters between streaming on and off.
> > The generic short packets received are notified to application via
> > v4l2_events.
> >
> > The driver supports only the video format bridge enabled configuration.
> > Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
> the
> > CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> > the maximum number of virtual channels becomes 16 from 4.
> >
> > Signed-off-by: Vishal Sagar <vishal.sagar@...inx.com>
> 
> ...
> 
> > +/**
> > + * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
> > + * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
> > + *
> > + * Core takes less than 100 video clock cycles to reset.
> > + * So a larger timeout value is chosen for margin.
> > + *
> > + * Return: 0 - on success OR -ETIME if reset times out
> > + */
> > +static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
> > +{
> > +     u32 timeout = XCSI_TIMEOUT_VAL;
> 
> The comment about the timeout is duplicated here and at the #define
> line. Why not removing the define above and just putting
> 
>   u32 timeout = 1000; /* us */
> 
> here? It would make the entire timeout logic appear in a unique place.
> 

Agree. It was kept like that as the timeout value was being used in some other place in earlier patch versions.
I will do as suggested in next version.

> > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> > +{
> > +     struct xcsi2rxss_core *core = &state->core;
> > +     int ret = 0;
> > +
> > +     xcsi2rxss_enable(core);
> > +
> > +     ret = xcsi2rxss_reset(core);
> > +     if (ret < 0) {
> > +             state->streaming = false;
> > +             return ret;
> > +     }
> > +
> > +     xcsi2rxss_intr_enable(core);
> > +     state->streaming = true;
> 
> Shouldn't you propagate s_stream to the upstream subdev here calling
> v4l2_subdev_call(..., ..., s_stream, 1)?
> 

This is done by the xvip_pipeline_start_stop() in xilinx-dma.c for Xilinx Video pipeline.

> > +     return ret;
> > +}
> 
> 
> --
> Luca

Regards
Vishal Sagar