Hi Niklas, Laurent,
On Tue, Oct 15, 2019 at 01:11:07AM +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Mon, Oct 14, 2019 at 02:16:15AM +0200, Niklas Söderlund wrote:
> > Most Gen3 boards can output frames in NV12 format, add support for this
> > with a runtime check that the running hardware supports it.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 5 ++-
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 39 +++++++++++++++++----
> > 2 files changed, 37 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index af4f774149f08597..cf9029efeb0450cb 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -118,6 +118,7 @@
> > #define VNDMR_ABIT (1 << 2)
> > #define VNDMR_DTMD_YCSEP (1 << 1)
>
> While at it, I would define this as (2 << 0).
>
> > #define VNDMR_DTMD_ARGB (1 << 0)
> > +#define VNDMR_DTMD_YCSEP_420 (3 << 0)
> >
> > /* Video n Data Mode Register 2 bits */
> > #define VNDMR2_VPS (1 << 30)
> > @@ -701,11 +702,13 @@ static int rvin_setup(struct rvin_dev *vin)
> > * Output format
> > */
> > switch (vin->format.pixelformat) {
> > + case V4L2_PIX_FMT_NV12:
> > case V4L2_PIX_FMT_NV16:
> > rvin_write(vin,
> > ALIGN(vin->format.bytesperline * vin->format.height,
> > 0x80), VNUVAOF_REG);
> > - dmr = VNDMR_DTMD_YCSEP;
> > + dmr = vin->format.pixelformat == V4L2_PIX_FMT_NV12 ?
> > + VNDMR_DTMD_YCSEP_420 : VNDMR_DTMD_YCSEP;
>
> According to the datasheet, VNDMR_DTMD_YCSEP_420 is only valid for some
> of the channels (see footnote of the VnDMR register documentation).
>
It took me a while to interpret this footnote:
3. Do not make this setting for video channels 2, 3, 6, 7, 10(V3H), 11(V3H), 14(V3H) and 15(V3H).
But if the note in table 26.15 is considered too:
Setting the DTMD[1:0] bits to B'11 is only allowed for video channels 0, 1, 4, and 5.
Can we refuse to set the fromat here or is it too late and should be
catched at rvin_format_from_pixel() time (where, if I'm not mistaken,
we have not channel id information available though).
Thanks
j
> > output_is_yuv = true;
> > break;
> > case V4L2_PIX_FMT_YUYV:
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 13b7cd5d2e40415a..9e2e63ffcc47acad 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -30,6 +30,10 @@
> > */
> >
> > static const struct rvin_video_format rvin_formats[] = {
> > + {
> > + .fourcc = V4L2_PIX_FMT_NV12,
> > + .bpp = 1,
> > + },
> > {
> > .fourcc = V4L2_PIX_FMT_NV16,
> > .bpp = 1,
> > @@ -72,6 +76,9 @@ const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> > if (vin->info->model == RCAR_M1 && pixelformat == V4L2_PIX_FMT_XBGR32)
> > return NULL;
> >
> > + if (pixelformat == V4L2_PIX_FMT_NV12 && !vin->info->nv12)
> > + return NULL;
> > +
> > for (i = 0; i < ARRAY_SIZE(rvin_formats); i++)
> > if (rvin_formats[i].fourcc == pixelformat)
> > return rvin_formats + i;
> > @@ -90,17 +97,29 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
> > if (WARN_ON(!fmt))
> > return -EINVAL;
> >
> > - align = pix->pixelformat == V4L2_PIX_FMT_NV16 ? 0x20 : 0x10;
> > + switch (pix->pixelformat) {
> > + case V4L2_PIX_FMT_NV12:
> > + case V4L2_PIX_FMT_NV16:
> > + align = 0x20;
> > + break;
> > + default:
> > + align = 0x10;
> > + break;
> > + }
> >
> > return ALIGN(pix->width, align) * fmt->bpp;
> > }
> >
> > static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > {
> > - if (pix->pixelformat == V4L2_PIX_FMT_NV16)
> > + switch (pix->pixelformat) {
> > + case V4L2_PIX_FMT_NV12:
> > + return pix->bytesperline * pix->height * 3 / 2;
> > + case V4L2_PIX_FMT_NV16:
> > return pix->bytesperline * pix->height * 2;
> > -
> > - return pix->bytesperline * pix->height;
> > + default:
> > + return pix->bytesperline * pix->height;
> > + }
> > }
> >
> > static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
> > @@ -124,8 +143,16 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
> > break;
> > }
> >
> > - /* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
> > - walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
> > + /* HW limit width to a multiple of 32 (2^5) for NV12/16 else 2 (2^1) */
> > + switch (vin->format.pixelformat) {
> > + case V4L2_PIX_FMT_NV12:
> > + case V4L2_PIX_FMT_NV16:
> > + walign = 5;
> > + break;
> > + default:
> > + walign = 1;
> > + break;
> > + }
> >
> > /* Limit to VIN capabilities */
> > v4l_bound_align_image(&pix->width, 2, vin->info->max_width, walign,
>
> --
> Regards,
>
> Laurent Pinchart
Attachment:
signature.asc
Description: PGP signature
