linux-kernel - Re: [PATCH v9] media: vsp1: Add VSPX 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]

Message-ID: <20250507140949.GB836326@ragnatech.se>
Date: Wed, 7 May 2025 16:09:49 +0200
From: Niklas Söderlund <niklas.soderlund@...natech.se>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi+renesas@...asonboard.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v9] media: vsp1: Add VSPX support

Hi Jacopo,

On 2025-05-07 14:28:50 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, May 06, 2025 at 07:37:33PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for this new version!
> >
> > I will give this a good testing as soon as I have finished some
> > refactoring of the user of this. I had a comment I wanted to send for
> > v8, but seems I never did. So for now I will just post this incase you
> > need to do a v10 before I had time.
> >
> > On 2025-05-06 18:32:23 +0200, Jacopo Mondi wrote:
> >
> > .. snip ..
> >
> > > +struct vsp1_dl_list;
> > > +struct vsp1_isp_job_desc {
> > > +	struct {
> > > +		unsigned int pairs;
> > > +		dma_addr_t mem;
> > > +	} config;
> > > +	struct {
> > > +		struct v4l2_format fmt;
> >
> > I'm very happy to see this is now the only location for the format, nice
> > work! I wonder if we shall take it one step further and just record the
> > fourcc, width, height and bytesperline here? Or at switch to a
> > v4l2_pix_format struct?
> >
> > The user of this field do not really support multi plane formats, nor do
> > it validate fmt.type field.
> >
> > I recently hit a snag in the ISP driver using this interface where I
> > *think* the solution is to only allow single plane buffers to be used as
> > input to the ISP (V4L2_CAP_VIDEO_OUTPUT_MPLANE vs
> > V4L2_CAP_VIDEO_OUTPUT). While fixing the plumbing for this I ran across
> > this, sorry for not noticing before.
> 
> True that.
> 
> However my understanding is that nowadays the multiplaner API should be used
> for single planar formats too.

Mine too, I'm just grasping for straws debugging things and this was one 
thing I tried to figure things out. It was unrelated to the VSPX, and 
did not solve my instal problem. But working on it was what lead me to 
this design decision in the VSPX driver.

> If you want to avoid passing in the
> whole 'struct v4l2_format' then I would pass in the
> v4l2_pix_format_mplane .pix_mp member (which the VSPX uses
> unconditionally at the moment).
> 
> However, assuming future developments where a different user uses
> V4L2_CAP_VIDEO_OUTPUT, the VSPX driver can be extended and use the
> .type field to select which member of the 'fmt' union to use if we
> pass the whole 'struct v4l2_format' in. This is all very theoretical I
> know. To be honest I would keep the interface as it is and eventually
> improve the VSPX driver to use the 'type' field to select which format
> to use.

I'm fine with that. Maybe add a check on .type field in the VSPX driver 
and fail if the .pix_mp 'variant' is not used to configure the VSPX?  
When playing with V4L2_CAP_VIDEO_OUTPUT the VSPX code still happy 
accepted the struct v4l2_format but it contained bad data when 
interpreted as .pix_mp instead of the .pix that was populated.

> 
> >
> > > +		dma_addr_t mem;
> > > +	} img;
> > > +	struct vsp1_dl_list *dl;
> > > +};
> >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund