Hi Laurent,
On 2018-02-13 19:02:38 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> On Tuesday, 13 February 2018 18:58:09 EET Niklas Söderlund wrote:
> > On 2018-02-13 18:41:33 +0200, Laurent Pinchart wrote:
> > > 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+renesas@xxxxxxxxxxxx>
> > > > ---
> > >>
> > >> 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 :-)
>
> Oh so you expect me to remember what happened with previous versions ? :-)
:-)
>
> > 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
>
> If kmalloc() fails we also have big problems, but we nonetheless check every
> memory allocation.
I did some quick and dirty statistics for current upstream behavior,
$ git grep pm_runtime_get_sync | wc -l
1044
$ git grep pm_runtime_get_sync | grep = | wc -l
367
It looks like a less then half checks the return value :-) But as it
will take at least one more incarnation of this patch set I will add a
check for it get to the good side of things.
>
> > >> +
> > >> + /* 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.
>
> This seems a bit fragile to me, could you please capture the explanation in a
> comment ?
>
Will do.
> > >> + 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
