| Message ID | 20180129163435.24936-14-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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+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 ? > + > + /* 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 ? > + 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
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+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 :-) 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 >
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
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@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. 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 >
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); + + /* 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); + + 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
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(+)