Hi Laurent,
On 2018-02-13 18:41:33 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Monday, 29 January 2018 18:34:18 EET Niklas Söderlund wrote:
> > On Gen3 the CSI-2 routing is controlled by the VnCSI_IFMD register. One
> > feature of this register is that it's only present in the VIN0 and VIN4
> > instances. The register in VIN0 controls the routing for VIN0-3 and the
> > register in VIN4 controls routing for VIN4-7.
> >
> > To be able to control routing from a media device this function is need
> > to control runtime PM for the subgroup master (VIN0 and VIN4). The
> > subgroup master must be switched on before the register is manipulated,
> > once the operation is complete it's safe to switch the master off and
> > the new routing will still be in effect.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 28 +++++++++++++++++++++++++++
> > drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > 2f9ad1bec1c8a92f..ae286742f15a3ab5 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -16,6 +16,7 @@
> >
> > #include <linux/delay.h>
> > #include <linux/interrupt.h>
> > +#include <linux/pm_runtime.h>
> >
> > #include <media/videobuf2-dma-contig.h>
> >
> > @@ -1228,3 +1229,30 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >
> > return ret;
> > }
> > +
> > +/* ------------------------------------------------------------------------
> > + * Gen3 CHSEL manipulation
> > + */
> > +
> > +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
> > +{
> > + u32 ifmd, vnmc;
> > +
> > + pm_runtime_get_sync(vin->dev);
>
> No need to check for errors ?
You asked the samething for v9 so I will copy paste the same reply :-)
Sakari asked the same thing in v4 :-)
In short no its not needed please see Geert's response [1]. If I
recall correctly this was also discussed in more detail in another
thread for some other driver whit a bit longer answer saying that it
pm_runtime_get_sync() fails you have big problems but I can't find
that thread now :-(
1. https://www.spinics.net/lists/linux-media/msg115241.html
>
> > +
> > + /* Make register writes take effect immediately */
> > + vnmc = rvin_read(vin, VNMC_REG);
> > + rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
> > +
> > + ifmd = VNCSI_IFMD_DES2 | VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 |
> > + VNCSI_IFMD_CSI_CHSEL(chsel);
> > +
> > + rvin_write(vin, ifmd, VNCSI_IFMD_REG);
> > +
> > + vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> > +
> > + /* Restore VNMC */
> > + rvin_write(vin, vnmc, VNMC_REG);
>
> No need for locking around all this ? What happens if this VIN instance
> decides to write to another VIN register (for instance due to a userpace
> call)
> when this function has disabled VNMC_VUP ?
You also asked a related question to this in v9 as a start I will copy
in that reply.
Media link changes are not allowed when any VIN in the group are
streaming so this should not be an issue.
And to compliment that. This function is only valid for a VIN which has
the CHSEL register which currently is VIN0 and VIN4. It can only be
modified when a media link is enabled. Catching media links are only
allowed when all VIN in the system are _not_ streaming. And VNMC_VUP is
only enabled when a VIN is streaming so there is no need for locking
here.
>
> > + pm_runtime_put(vin->dev);
> > +}
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h index
> > 146683142e6533fa..a5dae5b5e9cb704b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -165,4 +165,6 @@ const struct rvin_video_format
> > *rvin_format_from_pixel(u32 pixelformat); /* Cropping, composing and
> > scaling */
> > void rvin_crop_scale_comp(struct rvin_dev *vin);
> >
> > +void rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> > +
> > #endif
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
