| Message ID | 20190613000439.28746-4-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Add support for RGB formats with alpha component | expand |
Hi Niklas, Thank you for the patch. On Thu, Jun 13, 2019 at 02:04:39AM +0200, Niklas Söderlund wrote: > The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and > V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and > allow the alpha component to be changed while streaming using the > V4L2_CID_ALPHA_COMPONENT control. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++++++++ > drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++++++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -111,8 +111,11 @@ > #define VNIE_EFE (1 << 1) > > /* Video n Data Mode Register bits */ > +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) > +#define VNDMR_A8BIT_MASK (0xff << 24) > #define VNDMR_EXRGB (1 << 8) > #define VNDMR_BPSM (1 << 4) > +#define VNDMR_ABIT (1 << 2) > #define VNDMR_DTMD_YCSEP (1 << 1) > #define VNDMR_DTMD_ARGB (1 << 0) > > @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) > /* Note: not supported on M1 */ > dmr = VNDMR_EXRGB; > break; > + case V4L2_PIX_FMT_ARGB555: > + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; > + break; > default: > vin_err(vin, "Invalid pixelformat (0x%x)\n", > vin->format.pixelformat); > @@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > { > + u32 dmr; > + > vin->alpha = alpha; > + > + if (vin->state == STOPPED) The state is protected by the vin->qlock spinlock. Is it safe to check it here without holding the spinlock ? The answer may be yes if you can guarantee that no code patch will race except for the IRQ handler, and guarantee that the race with the IRQ handler isn't an issue. Additionally, what happens if the control is set and streaming is then started ? I don't see in call to v4l2_ctrl_handler_setup() in 2/3 or 3/3. > + return; > + > + switch (vin->format.pixelformat) { > + case V4L2_PIX_FMT_ARGB555: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; > + if (vin->alpha) > + dmr |= VNDMR_ABIT; > + break; > + case V4L2_PIX_FMT_ABGR32: > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; > + dmr |= VNDMR_A8BIT(vin->alpha); > + break; > + default: > + return; > + } > + > + rvin_write(vin, dmr, VNDMR_REG); > } > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { > .fourcc = V4L2_PIX_FMT_XBGR32, > .bpp = 4, > }, > + { > + .fourcc = V4L2_PIX_FMT_ARGB555, > + .bpp = 2, > + }, > + { > + .fourcc = V4L2_PIX_FMT_ABGR32, > + .bpp = 4, > + }, > }; > > const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat)
Hi Laurent, Thanks for your feedback. On 2019-06-17 17:33:41 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Jun 13, 2019 at 02:04:39AM +0200, Niklas Söderlund wrote: > > The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and > > V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and > > allow the alpha component to be changed while streaming using the > > V4L2_CID_ALPHA_COMPONENT control. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++++++++ > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -111,8 +111,11 @@ > > #define VNIE_EFE (1 << 1) > > > > /* Video n Data Mode Register bits */ > > +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) > > +#define VNDMR_A8BIT_MASK (0xff << 24) > > #define VNDMR_EXRGB (1 << 8) > > #define VNDMR_BPSM (1 << 4) > > +#define VNDMR_ABIT (1 << 2) > > #define VNDMR_DTMD_YCSEP (1 << 1) > > #define VNDMR_DTMD_ARGB (1 << 0) > > > > @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) > > /* Note: not supported on M1 */ > > dmr = VNDMR_EXRGB; > > break; > > + case V4L2_PIX_FMT_ARGB555: > > + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; > > + break; > > + case V4L2_PIX_FMT_ABGR32: > > + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; > > + break; > > default: > > vin_err(vin, "Invalid pixelformat (0x%x)\n", > > vin->format.pixelformat); > > @@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > > > void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) > > { > > + u32 dmr; > > + > > vin->alpha = alpha; > > + > > + if (vin->state == STOPPED) > > The state is protected by the vin->qlock spinlock. Is it safe to check > it here without holding the spinlock ? The answer may be yes if you can > guarantee that no code patch will race except for the IRQ handler, and > guarantee that the race with the IRQ handler isn't an issue. This is just a optimization to not try and write to the hardware if it's stopped and switched off. I assume this could race and a lock of vin->qlock could be added, if races worst case it writes the alpha value to HW when it don't need to. I will add the lock in the next version. > > Additionally, what happens if the control is set and streaming is then > started ? I don't see in call to v4l2_ctrl_handler_setup() in 2/3 or > 3/3. This is a good point, I have recently reworked part of the driver for gen2 which already had controls without considering gen3 will gain controls with this series. I will fix this and send a new version. > > > + return; > > + > > + switch (vin->format.pixelformat) { > > + case V4L2_PIX_FMT_ARGB555: > > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; > > + if (vin->alpha) > > + dmr |= VNDMR_ABIT; > > + break; > > + case V4L2_PIX_FMT_ABGR32: > > + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; > > + dmr |= VNDMR_A8BIT(vin->alpha); > > + break; > > + default: > > + return; > > + } > > + > > + rvin_write(vin, dmr, VNDMR_REG); > > } > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { > > .fourcc = V4L2_PIX_FMT_XBGR32, > > .bpp = 4, > > }, > > + { > > + .fourcc = V4L2_PIX_FMT_ARGB555, > > + .bpp = 2, > > + }, > > + { > > + .fourcc = V4L2_PIX_FMT_ABGR32, > > + .bpp = 4, > > + }, > > }; > > > > const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat) > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 4e991cce5fb56a90..5c0ed27c5d05dd45 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -111,8 +111,11 @@ #define VNIE_EFE (1 << 1) /* Video n Data Mode Register bits */ +#define VNDMR_A8BIT(n) ((n & 0xff) << 24) +#define VNDMR_A8BIT_MASK (0xff << 24) #define VNDMR_EXRGB (1 << 8) #define VNDMR_BPSM (1 << 4) +#define VNDMR_ABIT (1 << 2) #define VNDMR_DTMD_YCSEP (1 << 1) #define VNDMR_DTMD_ARGB (1 << 0) @@ -730,6 +733,12 @@ static int rvin_setup(struct rvin_dev *vin) /* Note: not supported on M1 */ dmr = VNDMR_EXRGB; break; + case V4L2_PIX_FMT_ARGB555: + dmr = (vin->alpha ? VNDMR_ABIT : 0) | VNDMR_DTMD_ARGB; + break; + case V4L2_PIX_FMT_ABGR32: + dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB; + break; default: vin_err(vin, "Invalid pixelformat (0x%x)\n", vin->format.pixelformat); @@ -1346,5 +1355,26 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha) { + u32 dmr; + vin->alpha = alpha; + + if (vin->state == STOPPED) + return; + + switch (vin->format.pixelformat) { + case V4L2_PIX_FMT_ARGB555: + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_ABIT; + if (vin->alpha) + dmr |= VNDMR_ABIT; + break; + case V4L2_PIX_FMT_ABGR32: + dmr = rvin_read(vin, VNDMR_REG) & ~VNDMR_A8BIT_MASK; + dmr |= VNDMR_A8BIT(vin->alpha); + break; + default: + return; + } + + rvin_write(vin, dmr, VNDMR_REG); } diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 7cbdcbf9b090c638..bb2900f5d000f9a6 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -54,6 +54,14 @@ static const struct rvin_video_format rvin_formats[] = { .fourcc = V4L2_PIX_FMT_XBGR32, .bpp = 4, }, + { + .fourcc = V4L2_PIX_FMT_ARGB555, + .bpp = 2, + }, + { + .fourcc = V4L2_PIX_FMT_ABGR32, + .bpp = 4, + }, }; const struct rvin_video_format *rvin_format_from_pixel(u32 pixelformat)
The R-Car VIN module supports V4L2_PIX_FMT_ARGB555 and V4L2_PIX_FMT_ABGR32 pixel formats. Add the hardware register setup and allow the alpha component to be changed while streaming using the V4L2_CID_ALPHA_COMPONENT control. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++++++++ drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++++++ 2 files changed, 38 insertions(+)