Hi Niklas,
On Sun, Jun 15, 2025 at 02:47:52PM +0200, Niklas Söderlund wrote:
> On 2025-06-15 00:32:13 +0300, Laurent Pinchart wrote:
> > On Sat, Jun 14, 2025 at 04:15:44PM +0200, Niklas Söderlund wrote:
> > > Depending on if the capture session deals with fields or whole frames
> > > interrupts can be generated at an end of field, or end of frame event.
> > > The interrupt mask is setup to generate an interrupt on one of the two
> > > events depending on what is needed when the VIN is started. The end of
> > > field bit is set in both cases so controlling the mask that generates an
> > > interrupt have been enough to control the two use-cases.
> > >
> > > Before extending the interrupt handler to deal with other types of
> > > interrupt events it is needs to extended to "capture complete" check for
> > > correct the use-case in operation. Without this the simplification in
> > > the handler can result in corrupted frames when the mask on what type of
> > > events can generate an interrupt generated can no longer be assumed to
> > > only be an "capture complete" event.
> > >
> > > Which bit is checked matches which bit is enabled at configuration time
> > > as which event can generate an interrupt for "capture complete". There
> > > is no functional change.
> > >
> > > While at it switch to use the BIT() macro to describe the bit positions
> > > for the interrupt functions.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > ---
> > > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +++++++++-----
> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > index 585b8b3dcfd8..85e44a00e0fc 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > @@ -115,11 +115,12 @@
> > > #define VNFC_S_FRAME (1 << 0)
> > >
> > > /* Video n Interrupt Enable Register bits */
> > > -#define VNIE_FIE (1 << 4)
> > > -#define VNIE_EFE (1 << 1)
> > > +#define VNIE_FIE BIT(4)
> > > +#define VNIE_EFE BIT(1)
> > >
> > > /* Video n Interrupt Status Register bits */
> > > -#define VNINTS_FIS (1 << 4)
> > > +#define VNINTS_FIS BIT(4)
> > > +#define VNINTS_EFS BIT(1)
> > >
> > > /* Video n Data Mode Register bits */
> > > #define VNDMR_A8BIT(n) (((n) & 0xff) << 24)
> > > @@ -1034,7 +1035,7 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > > static irqreturn_t rvin_irq(int irq, void *data)
> > > {
> > > struct rvin_dev *vin = data;
> > > - u32 status, vnms;
> > > + u32 capture, status, vnms;
> > > int slot;
> > > unsigned int handled = 0;
> > > unsigned long flags;
> > > @@ -1049,7 +1050,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > > handled = 1;
> > >
> > > /* Nothing to do if nothing was captured. */
> > > - if (!(status & VNINTS_FIS))
> > > + capture = vin->format.field == V4L2_FIELD_NONE ||
> > > + vin->format.field == V4L2_FIELD_ALTERNATE ?
> > > + VNINTS_FIS : VNINTS_EFS;
> >
> > I would find
> >
> > capture = vin->format.field == V4L2_FIELD_NONE ||
> > vin->format.field == V4L2_FIELD_ALTERNATE
> > ? VNINTS_FIS : VNINTS_EFS;
> >
> > easier to read, but it could be just me.
>
> I agree it's easier to read, but my auto style checker for the file will
> not allow it. And if I make one exception from my format rules I fear
> the file will soon turn wild. The whole construct is ugly and I hope to
> be able to remove it all together in follow up work.
Up to you. Fixing the style checker is a big rabbit hole.
> > Do I read it correctly that you use the "Field Interrupt" with
> > V4L2_FIELD_NONE ? That seems a bit weird to me, but maybe I don't get
> > how the hardware operates.
>
> I agree it's odd, and likely can be improved upon. But it mirrors what
> is done in the capture setup routine. The "Field Interrupt" is enabled
> for NONE and ALTERNATE, and the "End Frame Interrupt" is used for the
> other cases. See the 'progressive' variable in rvin_setup().
>
> The historical reason for this oddity is that VIN placed to much logic
> in the kernel driver that should have been up to user-space, specially
> for Gen2. In this case if the video source was providing ALTERNATE then
> VIN would automatically default to INTERLACED the output. Some of this
> have been reworked and fixed over the years, and a lot of it is finally
> fixed in the Gen2 MC series on the list.
>
> That work is however not a dependency of this work, and I feel like a
> broken record, but I have started work to refactor this and make it
> right. That work is however based on the Gen2 MC series as this allows
> me to add proper tests for all of this that covers all the different
> generations.
>
> I did not want this series to depend on the cleanups and potentially
> grew to yet another large series that fix lots of unrelated things. I
> hope it's OK we live with this oddity a little while longer so we can
> address it properly as a separate thing.
Fine with me.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > + if (!(status & capture))
> > > goto done;
> > >
> > > /* Nothing to do if not running. */
--
Regards,
Laurent Pinchart
