Hi Hans,
Thanks again for your review! I have addressed your comments in v3 which
I just posted but there where a few things I just wanted to answer in
this thread.
On 2016-02-29 10:52:25 +0100, Hans Verkuil wrote:
> Hi Niklas,
>
> Thanks for your patch! Much appreciated.
>
> I have more comments for the v2, but nothing really big :-)
>
> One high-level comment I have is that you should create an rcar-v4l2.c (or video.c)
> source where all the v4l2 ioctls and file ops reside.
>
> Most of what is in rcar-dma has nothing to do with dma. That's only the vb2
> ops and the interrupt handler.
>
> I think that should make the driver code a lot easier to navigate.
>
> On 02/24/2016 03:58 AM, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car VIN driver that do not depend on
> > soc_camera. The driver is heavily based on its predecessor and aims to
> > replace it.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> >
> > The driver is tested on Koelsch and can do streaming using qv4l2 and
> > grab frames using yavta. It passes a v4l2-compliance (git master) run
> > without failures, see bellow for output. Some issues I know about but
> > will have to wait for future work in other patches.
> > - The soc_camera driver provides some pixel formats that do not display
> > properly for me in qv4l2 or yavta. I have ported these formats as is
> > (not working correctly?) to the new driver.
> > - One can not bind/unbind the subdevice and continue using the driver.
> >
> > As stated in commit message the driver is based on its soc_camera
> > version but some features have been drooped (for now?).
> > - The driver no longer try to use the subdev for cropping (using
> > cropcrop/s_crop).
>
> The vin driver now does the cropping, right? Which makes perfect sense
> to me. The feature is still there, just done differently.
>
> > - Do not interrogate the subdev using g_mbus_config.
>
> And that's because we can now rely on what the device tree gives us, right?
Yes only device tree is used now.
<snip>
> > +void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t
> > addr)
> > +{
> > + const struct rvin_video_format *fmt;
> > + int offsetx, offsety;
> > + dma_addr_t offset;
> > +
> > + fmt = rvin_format_from_pixel(vin->format.pixelformat);
> > +
> > + /*
> > + * There is no HW support for composition do the beast we can
> > + * by modifying the buffer offset
> > + */
> > + offsetx = vin->compose.left * fmt->bpp;
> > + offsety = vin->compose.top * vin->format.bytesperline;
>
> Does this work for a planar format like NV16? Just wondering.
On this SoC is dose since the CbCr plane starts from a set offset (we
can control this offset also but there is need to to anything special
with it for this to work). So if we inject a offset here the CbCr plane
will use the same offset.
>
> > + offset = addr + offsetx + offsety;
> > +
> > + /*
> > + * The address needs to be 128 bytes aligned. Driver should never accept
> > + * settings that do not satisfy this in the first place...
> > + */
> > + if (WARN_ON((offsetx | offsety | offset) & HW_BUFFER_MASK))
> > + return;
> > +
> > + rvin_write(vin, offset, VNMB_REG(slot));
> > +}
<snip>
--
Regards,
Niklas Söderlund
