Re: [PATCH v10 13/30] rcar-vin: add function to manipulate Gen3 chsel value

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@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 :-)

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.

> >> +
> >> +	/* 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 ?

> >> +	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