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. 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.
>
> > + dma_addr_t mem;
> > + } img;
> > + struct vsp1_dl_list *dl;
> > +};
>
>
> --
> Kind Regards,
> Niklas Söderlund
