Re: [PATCH] ARM: Build secure_cntvoff.S unconditionally to fix shmobile !SMP build
On Wed, Jun 06, 2018 at 10:52:34AM +0200, Geert Uytterhoeven wrote:
> If CONFIG_SMP=n, building a kernel for R-Car Gen2 fails with:
>
> arch/arm/mach-shmobile/setup-rcar-gen2.o: In function
> `rcar_gen2_timer_init':
> setup-rcar-gen2.c:(.init.text+0x30): undefined reference to
> `secure_cntvoff_init'
>
> Indeed, on R-Car Gen2 SoCs, secure_cntvoff_init() is not only needed for
> secondary CPUs, but also for the boot CPU. This is most visible on SoCs
> with Cortex A7 cores (e.g. R-Car E2, cfr. commit 9ce3fa6816c2fb59 ("ARM:
> shmobile: rcar-gen2: Add CA7 arch_timer initialization for r8a7794")),
> but Cortex A15 is affected, too.
>
> Fix this by always providing secure_cntvoff_init().
>
> Reported-by: Arnd Bergmann
> Fixes: 7c607944bc657616 ("ARM: smp: Add initialization of CNTVOFF")
> Fixes: cad160ed0a94927e ("ARM: shmobile: Convert file to use cntvoff")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Maxime Ripard
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v15 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
On Sun, May 13, 2018 at 09:19:17PM +0200, Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se> Reviewed-by: Maxime Ripard <maxime.rip...@bootlin.com> Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v9 00/12] Sunxi: Add SMP support on A83T
Hi! On Tue, May 08, 2018 at 10:11:07AM -0700, Florian Fainelli wrote: > On 05/08/2018 06:19 AM, Maxime Ripard wrote: > > Hi, > > > > On Fri, May 04, 2018 at 09:05:33PM +0200, Mylène Josserand wrote: > >> Hello everyone, > >> > >> This is a V9 of my series that adds SMP support for Allwinner sun8i-a83t. > >> Based on sunxi's tree, sunxi/for-next branch. > >> Depends on a patch from Doug Berger that allows to include the "cpu-type" > >> header on assembly files that I included in my series (patch 01). > > > > I applied the patches, with the remarks done by Russell on v8 fixed, > > and the function sun8i_a83t_cntvoff_init made static. > > Did you push those patches to sunxi/linux.git yet? I would like to make > sure I have the same copy of patch 1 since that is necessary for some of > our Broadcom ARM SoCs for 4.18. I forgot, I just did. I tested to merge the latest next tag though, and it went fine without any conflicts, so I guess we're fine? Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v9 00/12] Sunxi: Add SMP support on A83T
Hi, On Fri, May 04, 2018 at 09:05:33PM +0200, Mylène Josserand wrote: > Hello everyone, > > This is a V9 of my series that adds SMP support for Allwinner sun8i-a83t. > Based on sunxi's tree, sunxi/for-next branch. > Depends on a patch from Doug Berger that allows to include the "cpu-type" > header on assembly files that I included in my series (patch 01). I applied the patches, with the remarks done by Russell on v8 fixed, and the function sun8i_a83t_cntvoff_init made static. > The difference with the v8 is just that the machine is renamed in sun8i-a83t > (see patch 07), according to Maxime's review. The machine name hasn't changed, and the name sun8i-a83t still doesn't make much sense. I've fixed it as well. Thanks, Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v8 02/12] ARM: sunxi: smp: Move assembly code into a file
On Tue, May 01, 2018 at 02:31:21PM +0200, Mylène Josserand wrote: > Move the assembly code for cluster cache enabling and resuming > into an assembly file instead of having it directly in C code. > > Remove the CFLAGS because we are using the ARM directive "arch" > instead. > > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> Acked-by: Maxime Ripard <maxime.rip...@bootlin.com> Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v8 12/12] ARM: shmobile: Convert file to use cntvoff
Hi Simon, On Wed, May 02, 2018 at 08:36:39AM +0200, Simon Horman wrote: > On Tue, May 01, 2018 at 02:31:31PM +0200, Mylène Josserand wrote: > > Now that a common function is available for CNTVOFF's > > initialization, let's convert shmobile-apmu code to use > > this function. > > > > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> > > Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be> > > Tested-by: Geert Uytterhoeven <geert+rene...@glider.be> > > Acked-by: Simon Horman <horms+rene...@verge.net.au> For the sake of bisectability (and to ease the merge), is it ok if we take it through the sunxi (and then arm-soc) tree? Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v8 10/12] ARM: sun8i: smp: Add support for A83T
On Tue, May 01, 2018 at 02:31:29PM +0200, Mylène Josserand wrote: > Add the support for A83T. > > A83T SoC has an additional register than A80 to handle CPU configurations: > R_CPUS_CFG. Information about the register comes from Allwinner's BSP > driver. > An important difference is the Power Off Gating register for clusters > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. > There is also a bit swap between sun8i-a83t and sun9i-a80 that must be > handled. > > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> Acked-by: Maxime Ripard <maxime.rip...@bootlin.com> Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v8 09/12] ARM: sun9i: smp: Add is_a83t field
On Tue, May 01, 2018 at 02:31:28PM +0200, Mylène Josserand wrote: > To prepare the support of sun8i-a83t, add a field in the smp_data > structure to know if we are on sun9i-a80 or sun8i-a83t. > > Add also a global variable to retrieve which architecture we are > having. > > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> Acked-by: Maxime Ripard <maxime.rip...@bootlin.com> Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v8 07/12] ARM: sunxi: Add initialization of CNTVOFF
On Tue, May 01, 2018 at 02:31:26PM +0200, Mylène Josserand wrote:
> Add the initialization of CNTVOFF for sun8i-a83t.
>
> For boot CPU, create a new machine that handles this
> function's call in an "init_early" callback. We need to initialize
> CNTVOFF before the arch timer's initialization otherwise, it will
> not be taken into account and fails to boot correctly.
> Because of that, this function can't be called in SMP's early_initcall
> function which is called after timer's init.
>
> For secondary CPUs, add this function into secondary_startup
> assembly entry.
>
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> Acked-by: Maxime Ripard <maxime.rip...@bootlin.com>
> ---
> arch/arm/mach-sunxi/headsmp.S | 1 +
> arch/arm/mach-sunxi/sunxi.c | 20 +++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index 37dc772701f3..32d76be98541 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -71,6 +71,7 @@ ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>
> ENTRY(sunxi_mc_smp_secondary_startup)
> bl sunxi_mc_smp_cluster_cache_enable
> + bl secure_cntvoff_init
> b secondary_startup
> ENDPROC(sunxi_mc_smp_secondary_startup)
>
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..2770a31a9278 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -16,6 +16,7 @@
> #include
>
> #include
> +#include
>
> static const char * const sunxi_board_dt_compat[] = {
> "allwinner,sun4i-a10",
> @@ -62,7 +63,6 @@ MACHINE_END
> static const char * const sun8i_board_dt_compat[] = {
> "allwinner,sun8i-a23",
> "allwinner,sun8i-a33",
> - "allwinner,sun8i-a83t",
> "allwinner,sun8i-h2-plus",
> "allwinner,sun8i-h3",
> "allwinner,sun8i-r40",
> @@ -75,6 +75,24 @@ DT_MACHINE_START(SUN8I_DT, "Allwinner sun8i Family")
> .dt_compat = sun8i_board_dt_compat,
> MACHINE_END
>
> +void __init sun8i_a83t_cntvoff_init(void)
> +{
> +#ifdef CONFIG_SMP
> + secure_cntvoff_init();
> +#endif
> +}
> +
> +static const char * const sun8i_a83t_cntvoff_board_dt_compat[] = {
> + "allwinner,sun8i-a83t",
> + NULL,
> +};
> +
> +DT_MACHINE_START(SUN8I_CNTVOFF_DT, "Allwinner sun8i-a83t board")
I put my Acked-by on the condition that you would fix this. And you
still didn't.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v7 06/11] ARM: sunxi: Add initialization of CNTVOFF
On Fri, Apr 20, 2018 at 11:10:17PM +0200, Mylène Josserand wrote:
> Add the initialization of CNTVOFF for sun8i-a83t.
>
> For boot CPU, create a new machine that handles this
> function's call in an "init_early" callback. We need to initialize
> CNTVOFF before the arch timer's initialization otherwise, it will
> not be taken into account and fails to boot correctly.
> Because of that, this function can't be called in SMP's early_initcall
> function which is called after timer's init.
>
> For secondary CPUs, add this function into secondary_startup
> assembly entry.
>
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> ---
> arch/arm/mach-sunxi/headsmp.S | 1 +
> arch/arm/mach-sunxi/sunxi.c | 20 +++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index 37dc772701f3..32d76be98541 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -71,6 +71,7 @@ ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>
> ENTRY(sunxi_mc_smp_secondary_startup)
> bl sunxi_mc_smp_cluster_cache_enable
> + bl secure_cntvoff_init
> b secondary_startup
> ENDPROC(sunxi_mc_smp_secondary_startup)
>
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..ddc439f6269b 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -16,6 +16,7 @@
> #include
>
> #include
> +#include
>
> static const char * const sunxi_board_dt_compat[] = {
> "allwinner,sun4i-a10",
> @@ -62,7 +63,6 @@ MACHINE_END
> static const char * const sun8i_board_dt_compat[] = {
> "allwinner,sun8i-a23",
> "allwinner,sun8i-a33",
> - "allwinner,sun8i-a83t",
> "allwinner,sun8i-h2-plus",
> "allwinner,sun8i-h3",
> "allwinner,sun8i-r40",
> @@ -75,6 +75,24 @@ DT_MACHINE_START(SUN8I_DT, "Allwinner sun8i Family")
> .dt_compat = sun8i_board_dt_compat,
> MACHINE_END
>
> +void __init sun8i_cntvoff_init(void)
> +{
> +#ifdef CONFIG_SMP
> + secure_cntvoff_init();
> +#endif
> +}
> +
> +static const char * const sun8i_cntvoff_board_dt_compat[] = {
> + "allwinner,sun8i-a83t",
> + NULL,
> +};
> +
> +DT_MACHINE_START(SUN8I_CNTVOFF_DT, "Allwinner sun8i-a83t board")
This name still doesn't really make much sense. It's an A83t, that's
it.
Apart from the other minor comment I had, and once that name has been
fixed:
Acked-by: Maxime Ripard <maxime.rip...@bootlin.com>
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v7 08/11] ARM: sun9i: smp: Add is_a83t field
On Fri, Apr 20, 2018 at 11:10:19PM +0200, Mylène Josserand wrote:
> To prepare the support of sun8i-a83t, add a field in the smp_data
> structure to know if we are on sun9i-a80 or sun8i-a83t.
>
> Add also a global variable to retrieve which architecture we are
> having.
>
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> ---
> arch/arm/mach-sunxi/mc_smp.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index 03f021d0c73e..48e5f4db64b6 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -74,6 +74,7 @@ static void __iomem *sram_b_smp_base;
>
> extern void sunxi_mc_smp_secondary_startup(void);
> extern void sunxi_mc_smp_resume(void);
> +static int is_a83t;
>
> static bool sunxi_core_is_cortex_a15(unsigned int core, unsigned int cluster)
> {
> @@ -624,6 +625,7 @@ struct sunxi_mc_smp_nodes {
> struct sunxi_mc_smp_data {
> const char *enable_method;
> int (*get_smp_nodes)(struct sunxi_mc_smp_nodes *nodes);
> + int is_a83t;
This can be made a boolean.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v6 01/11] ARM: sunxi: smp: Move assembly code into a file
On Tue, Apr 17, 2018 at 07:25:15PM +0800, Chen-Yu Tsai wrote: > On Tue, Apr 17, 2018 at 7:17 PM, Maxime Ripard > <maxime.rip...@bootlin.com> wrote: > > On Tue, Apr 17, 2018 at 11:12:41AM +0800, Chen-Yu Tsai wrote: > >> On Tue, Apr 17, 2018 at 5:50 AM, Mylène Josserand > >> <mylene.josser...@bootlin.com> wrote: > >> > Move the assembly code for cluster cache enabling and resuming > >> > into an assembly file instead of having it directly in C code. > >> > > >> > Remove the CFLAGS because we are using the ARM directive "arch" > >> > instead. > >> > > >> > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> > >> > --- > >> > arch/arm/mach-sunxi/Makefile | 4 +-- > >> > arch/arm/mach-sunxi/headsmp.S | 80 > >> > + > >> > arch/arm/mach-sunxi/mc_smp.c | 82 > >> > +++ > >> > 3 files changed, 85 insertions(+), 81 deletions(-) > >> > create mode 100644 arch/arm/mach-sunxi/headsmp.S > >> > >> I'm still not convinced about this whole "move ASM to separate > >> file" thing, especially now that you aren't actually adding any > >> sunxi-specific ASM code beyond a simple function call. > >> > >> Could you drop this for now? > > > > I'd really like to have this merged actually. There's a significant > > readibility improvement, so even if there's no particular functional > > improvement, I'd still call it a win. > > What parts do you consider hard to read? The extra quotes? Trailing > newline? Or perhaps the __stringify bits? All of this, plus the clobbers and operands. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v6 09/11] ARM: sun8i: smp: Add support for A83T
On Mon, Apr 16, 2018 at 11:50:30PM +0200, Mylène Josserand wrote:
> @@ -535,8 +599,12 @@ static int sunxi_mc_smp_cpu_kill(unsigned int l_cpu)
> return !ret;
> }
>
> -static bool sunxi_mc_smp_cpu_can_disable(unsigned int __unused)
> +static bool sunxi_mc_smp_cpu_can_disable(unsigned int cpu)
> {
> + /* CPU0 hotplug not handled for sun8i-a83t */
> + if (is_sun8i)
> + if (cpu == 0)
> + return false;
> return true;
I think Chen-Yu told you how to implement the hotplug in the previous
iteration, did you have time to test it?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v6 08/11] ARM: sun9i: smp: Add is_sun8i field
On Tue, Apr 17, 2018 at 03:57:07PM +0800, Chen-Yu Tsai wrote: > >> @@ -697,6 +700,8 @@ static int __init sunxi_mc_smp_init(void) > >> break; > >> } > >> > >> + is_sun8i = sunxi_mc_smp_data[i].is_sun8i; > >> + > > > > Do we really need to cache it? Can't we just have a pointer to the SMP > > data structure and use that instead? > > I recommended that. We don't need any of the other fields in the SMP > data structure once we're past the init phase. This saves a dereference > or two. Fair enough. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v6 01/11] ARM: sunxi: smp: Move assembly code into a file
On Tue, Apr 17, 2018 at 11:12:41AM +0800, Chen-Yu Tsai wrote: > On Tue, Apr 17, 2018 at 5:50 AM, Mylène Josserand > <mylene.josser...@bootlin.com> wrote: > > Move the assembly code for cluster cache enabling and resuming > > into an assembly file instead of having it directly in C code. > > > > Remove the CFLAGS because we are using the ARM directive "arch" > > instead. > > > > Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com> > > --- > > arch/arm/mach-sunxi/Makefile | 4 +-- > > arch/arm/mach-sunxi/headsmp.S | 80 > > + > > arch/arm/mach-sunxi/mc_smp.c | 82 > > +++ > > 3 files changed, 85 insertions(+), 81 deletions(-) > > create mode 100644 arch/arm/mach-sunxi/headsmp.S > > I'm still not convinced about this whole "move ASM to separate > file" thing, especially now that you aren't actually adding any > sunxi-specific ASM code beyond a simple function call. > > Could you drop this for now? I'd really like to have this merged actually. There's a significant readibility improvement, so even if there's no particular functional improvement, I'd still call it a win. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v6 08/11] ARM: sun9i: smp: Add is_sun8i field
Hi,
On Mon, Apr 16, 2018 at 11:50:29PM +0200, Mylène Josserand wrote:
> To prepare the support of sun8i-a83t, add a field in the smp_data
> structure to know if we are on sun9i-a80 or sun8i-a83t.
>
> Add also a global variable to retrieve which architecture we are
> having.
>
> Signed-off-by: Mylène Josserand <mylene.josser...@bootlin.com>
> ---
> arch/arm/mach-sunxi/mc_smp.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index 03f021d0c73e..9d57ea27dacc 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -74,6 +74,7 @@ static void __iomem *sram_b_smp_base;
>
> extern void sunxi_mc_smp_secondary_startup(void);
> extern void sunxi_mc_smp_resume(void);
> +static int is_sun8i;
>
> static bool sunxi_core_is_cortex_a15(unsigned int core, unsigned int cluster)
> {
> @@ -624,6 +625,7 @@ struct sunxi_mc_smp_nodes {
> struct sunxi_mc_smp_data {
> const char *enable_method;
> int (*get_smp_nodes)(struct sunxi_mc_smp_nodes *nodes);
> + int is_sun8i;
> };
>
> static void __init sunxi_mc_smp_put_nodes(struct sunxi_mc_smp_nodes *nodes)
> @@ -664,6 +666,7 @@ static const struct sunxi_mc_smp_data sunxi_mc_smp_data[]
> __initconst = {
> {
> .enable_method = "allwinner,sun9i-a80-smp",
> .get_smp_nodes = sun9i_a80_get_smp_nodes,
> + .is_sun8i = false,
I'm still not convinced about the name of that flag. sun8i doesn't
mean anything, really. What you want to discriminate against it what
you're writing in your commit log: if it is an A80 or an A83t. The
A33, H3, A23, you name it are also part of the sun8i family, yet they
are completely irrelevant to this file.
Also, false is the default value, you can leave it out.
> },
> };
>
> @@ -697,6 +700,8 @@ static int __init sunxi_mc_smp_init(void)
> break;
> }
>
> + is_sun8i = sunxi_mc_smp_data[i].is_sun8i;
> +
Do we really need to cache it? Can't we just have a pointer to the SMP
data structure and use that instead?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
Hi Niklas,
On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> + switch (priv->lanes) {
> + case 1:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> + break;
> + case 2:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + case 4:
> + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
> + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> + break;
> + default:
> + return -EINVAL;
> + }
I guess you could have a simpler construct here using this:
phycnt = PHYCNT_ENABLECLK;
switch (priv->lanes) {
case 4:
phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
case 2:
phycnt |= PHYCNT_ENABLE_1;
case 1:
phycnt |= PHYCNT_ENABLE_0;
break;
default:
return -EINVAL;
}
But that's really up to you.
> +static int rcar_csi2_probe(struct platform_device *pdev)
> +{
> + const struct soc_device_attribute *attr;
> + struct rcar_csi2 *priv;
> + unsigned int i;
> + int ret;
> +
> + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->info = of_device_get_match_data(>dev);
> +
> + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> + attr = soc_device_match(r8a7795es1);
> + if (attr)
> + priv->info = attr->data;
> +
> + priv->dev = >dev;
> +
> + mutex_init(>lock);
> + priv->stream_count = 0;
> +
> + ret = rcar_csi2_probe_resources(priv, pdev);
> + if (ret) {
> + dev_err(priv->dev, "Failed to get resources\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + ret = rcar_csi2_parse_dt(priv);
> + if (ret)
> + return ret;
> +
> + priv->subdev.owner = THIS_MODULE;
> + priv->subdev.dev = >dev;
> + v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> + v4l2_set_subdevdata(>subdev, >dev);
> + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> + KBUILD_MODNAME, dev_name(>dev));
> + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + priv->subdev.entity.ops = _csi2_entity_ops;
> +
> + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> + priv->pads);
> + if (ret)
> + goto error;
> +
> + pm_runtime_enable(>dev);
Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the
device uninitialised at probe, and pm_runtime_get_sync will not
initialise it either if CONFIG_PM is not enabled. I guess you could
call your runtime_resume function unconditionally, and mark the device
as active in runtime_pm using pm_runtime_set_active.
Looks good otherwise, once fixed (and if relevant):
Reviewed-by: Maxime Ripard <maxime.rip...@bootlin.com>
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [RFC][PATCH 04/11] drm: Split the display info into static and dynamic parts
On Tue, Feb 27, 2018 at 02:56:53PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > Currently we have a mix of static and dynamic information stored in > the display info structure. That makes it rather difficult to repopulate > the dynamic parts when a new EDID appears. Let's make life easier by > splitting the structure up into static and dynamic parts. > > The static part will consist of subpixel_order, panel_orientation, > and bus_formats. > > Actually I'm not sure where bus_formats & co. fit in all this. For some > drivers those seem to be static, even though they might fill them out > from .get_modes(). For other drivers this stuff even gets frobbed at > runtime, making it more some kind of a bastard encoder/connector state. > I'll just stick it into the static side so that the behaviour doesn't > change when I start clear out the entire dynamic state with memset(). > > Cc: Keith Packard <kei...@keithp.com> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > Cc: Hans de Goede <hdego...@redhat.com> > Cc: Shashank Sharma <shashank.sha...@intel.com> > Cc: Stefan Agner <ste...@agner.ch> > Cc: Thierry Reding <thierry.red...@gmail.com> > Cc: Boris Brezillon <boris.brezil...@bootlin.com> > Cc: Philipp Zabel <p.za...@pengutronix.de> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Manfred Schlaegl <manfred.schla...@gmx.at> > Cc: Marek Vasut <ma...@denx.de> > Cc: Archit Taneja <arch...@codeaurora.org> > Cc: Andrzej Hajda <a.ha...@samsung.com> > Cc: Alison Wang <alison.w...@freescale.com> > Cc: Eric Anholt <e...@anholt.net> > Cc: Linus Walleij <linus.wall...@linaro.org> > Cc: linux-renesas-soc@vger.kernel.org > Cc: Maxime Ripard <maxime.rip...@bootlin.com> For sun4i, Acked-by: Maxime Ripard <maxime.rip...@bootlin.com> Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH/RFC 4/4] drm: rcar-du: Add support for color keying on Gen3
Hi,
On Sun, Dec 17, 2017 at 02:17:24AM +0200, Laurent Pinchart wrote:
> +static const struct drm_prop_enum_list colorkey_modes[] = {
> + { 0, "disabled" },
> + { 1, "source" },
> +};
> +
> int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
>unsigned int crtcs)
> {
> @@ -441,6 +453,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct
> device_node *np,
> rcdu->props.alpha, 255);
> drm_plane_create_zpos_property(>plane, 1, 1,
> vsp->num_planes - 1);
> + drm_plane_create_colorkey_properties(>plane,
> + colorkey_modes,
> + ARRAY_SIZE(colorkey_modes),
> + true);
You seem to define the same list in your enumeration between your
patch 2 and this one. Can this be something made generic too?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
Re: [PATCH/RFC 1/4] drm: Add colorkey properties
On Sun, Dec 17, 2017 at 02:17:21AM +0200, Laurent Pinchart wrote: > Color keying is the action of replacing pixels matching a given color > (or range of colors) with transparent pixels in an overlay when > performing blitting. Depending on the hardware capabilities, the > matching pixel can either become fully transparent, or gain a > programmable alpha value. > > Color keying is found in a large number of devices whose capabilities > often differ, but they still have enough common features in range to > standardize color key properties. This commit adds four properties > related to color keying named colorkey.min, colorkey.max, colorkey.alpha > and colorkey.mode. Additional properties can be defined by drivers to > expose device-specific features. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> Sorry for the delay, Reviewed-by: Maxime Ripard <maxime.rip...@free-electrons.com> Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH 00/20] Add multiplexed media pads to support CSI-2 virtual channels
Hi Niklas, On Fri, Aug 11, 2017 at 11:56:43AM +0200, Niklas Söderlund wrote: > This series is a RFC for how I think one could add CSI-2 virtual channel > support to the V4L2 framework. The problem is that there is no way to in > the media framework describe and control links between subdevices which > carry more then one video stream, for example a CSI-2 bus which can have > 4 virtual channels carrying different video streams. > > This series adds a new pad flag which would indicate that a pad carries > multiplexed streams, adds a new s_stream() operation to the pad > operations structure which takes a new argument 'stream'. This new > s_stream() operation then is both pad and stream aware. It also extends > struct v4l2_mbus_frame_desc_entry with a new sub-struct to describe how > a CSI-2 link multiplexes virtual channels. I also include one > implementation based on Renesas R-Car which makes use of these patches > as I think they help with understanding but they have no impact on the > RFC feature itself. > > The idea is that on both sides of the multiplexed media link there are > one multiplexer subdevice and one demultiplexer subdevice. These two > subdevices can't do any format conversions, there sole purpose is to > (de)multiplex the CSI-2 link. If there is hardware which can do both > CSI-2 multiplexing and format conversions they can be modeled as two > subdevices from the same device driver and using the still pending > incremental async mechanism to connect the external pads. The reason > there is no format conversion is important as the multiplexed pads can't > have a format in the current V4L2 model, get/set_fmt are not aware of > streams. > > +--+ +--+ > +---+ subdev 1 | | subdev 2 +---+ > +--+ Pad 1 | | | | Pad 3 +---+ > +--++ +-+---+ +---+-+ ++--+ > || Muxed pad A +--+ Muxed pad B || > +--++ +-+---+ +---+-+ ++--+ > +--+ Pad 2 | | | | Pad 4 +---+ > +---+ | | +---+ > +--+ +--+ > > In the simple example above Pad 1 is routed to Pad 3 and Pad 2 to Pad 4, > and the video data for both of them travels the link between pad A and > B. One shortcoming of this RFC is that there currently are no way to > express to user-space which pad is routed to which stream of the > multiplexed link. But inside the kernel this is known and format > validation is done by comparing the format of Pad 1 to Pad 3 and Pad 2 > to Pad 4 by the V4L2 framework. But it would be nice for the user to > also be able to get this information while setting up the MC graph in > user-space. Thanks for your patches. I guess I already have a different use-case for this, one you might not have envisionned (on top of the Cadence CSI2-RX driver I've been posting and that would need it at some point). I have a CSI2-TX controller that I'm currently writing a driver for. That controller takes 4 video streams in input, and aggregates them on a single CSI-2 link. Those video streams use some kind of parallel interfaces. So far, so good. However, those parallel interfaces come with additional signals that set the virtual channel and data types of the associated video signal. Those signals are under control of the upstream video output device, and can change at runtime. The virtual channel are direcly mapped to the CSI-2 Virtual Channels, so that part is completely transparent to the transmitter. However, the video input datatype is a 0-7 value. Each data type value has a separate set of registers where you need to set up the width and height of the video data, and the corresponding CSI-2 Data Type. You can use this to do some interleaving of either the virtual channels or the data types, or both. To make things (slightly) more complicated, the FIFO registers are per-video input. Which means that while most of the (input) configuration will be done per-datatype, we still have to know which input stream is generating it, for example to optimise the FIFO fill levels to the resolution... Since the stream is multiplexed both using the virtual channels and the data-types, I'm not sure representing it using only muxed pads like you did would work. And I don't really know what a good stop gap measure would be either. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending
Hi Daniel, On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote: > On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard > <maxime.rip...@free-electrons.com> wrote: > > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: > >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard > >> <maxime.rip...@free-electrons.com> wrote: > >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > >> >> Hi, > >> >> > >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > >> >> <maxime.rip...@free-electrons.com> wrote: > >> >> > In the earlier display engine designs, any register access while a > >> >> > commit > >> >> > is pending is forbidden. > >> >> > > >> >> > One of the symptoms is that reading a register will return another, > >> >> > random, > >> >> > register value which can lead to register corruptions if we ever do a > >> >> > read/modify/write cycle. > >> >> > >> >> Alternatively, if changes to the backend (layers) are guaranteed to > >> >> happen > >> >> while the CRTC is disabled (which seems to be the case after looking at > >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > >> >> could just turn on register auto-commit all the time and not deal with > >> >> this. > >> > > >> > As far as I understand, it will only be the case if we need a new > >> > modeset or we changed the active CRTC or connectors. But if you change > >> > only the format, buffers or properties it won't be the case, and we'll > >> > need to commit. > >> > >> So in other words, if someone were to use it for actual compositing and > >> moved the upper composited layer around, we would need commit support to be > >> safe. > >> > >> Sounds more or less like something a video player would do. > > > > Not only that. A change of buffer will happen every frame or so, and > > we can change the format whenever we want too (even if it's usually > > going to be in sync with a new buffer). Changing a property can happen > > any time too (like zpos for example). > > You can upgrade any property change to an atomic modeset by e.g. > setting connector->mode_changed (and then making sure to call > check_modeset() helper again perhaps). This is for cases where your hw > can't handle a property change within 1 vblank. The default is just > the solution for most common hw. > > The other way round works too, you can clear these flags in your > atomic_check callbacks. But that requires a bit more care (to make > sure you never clear it when there's something else also changing that > still needs a full modeset sequence to commit to hw). Hmm, that's good to know, but that would imply disabling the CRTC each time we change even a small property, with all the visual artifacts it might imply, right? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices
Hi Niklas, On Wed, Jul 19, 2017 at 12:49:42PM +0200, Niklas Söderlund wrote: > Hi, > > I know Sakari have posted a series '[RFC 00/19] Async sub-notifiers and > how to use them' which address similar problems as this series. This is > not intended to compete whit his work and Sakari includes one of my v3 > patches in his series. Never the less I post this updated series since > it fixes some issues in my v3 implementation and contains some other > fixes for the v4l2-async framework which are not addressed in Sakaris > patches. I think the correct solution to the problems we both try to fix > is a mix of our two series, would you agree Sakari? > > This series enables incremental async find and bind of subdevices, > please se patch 4/4 for a more detailed description. This is a rewrite > of the feature since v3, see changelog in this cover letter for the > differences to v3. The two primary reasons for a new implementation > where: > > 1. Hans expressed an interest having the async complete() callbacks to >happen only once all notifiers in the pipeline where complete. To do >this a stronger connection between the notifiers where needed, hence >the subnotifier is now embedded in struct v4l2_subdev. > >Whit this change it is possible to check all notifiers in a pipeline >is complete before calling any of them. > > 2. There where concerns that the v3 solution was a bit to complex and >hard to refactor in the future if other issues in the v4l2-async >framework where to be addressed. By hiding the notifier in the struct >v4l2_subdev and adding a new function to set that structure the >interface towards drivers are minimized while everything else happens >in the v4l2-async framework. This leaves the interface in a good >position for possible changes in v4l2-async. > > This is tested on Renesas H3 and M3-W together with the Renesas CSI-2 > and VIN Gen3 driver (posted separately). It is based on top of the media-tree. Thanks for the new iteration, you can add my Tested-by: Maxime Ripard <maxime.rip...@free-electrons.com> On the cadence CSI2 RX driver I sent earlier. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH] gpu: Convert to using %pOF instead of full_name
On Tue, Jul 18, 2017 at 04:43:04PM -0500, Rob Herring wrote: > Now that we have a custom printf format specifier, convert users of > full_name to use %pOF instead. This is preparation to remove storing > of the full path string for each node. > > Signed-off-by: Rob Herring <r...@kernel.org> > Cc: Russell King <li...@armlinux.org.uk> > Cc: David Airlie <airl...@linux.ie> > Cc: Daniel Vetter <daniel.vet...@intel.com> > Cc: Jani Nikula <jani.nik...@linux.intel.com> > Cc: Sean Paul <seanp...@chromium.org> > Cc: Inki Dae <inki@samsung.com> > Cc: Joonyoung Shim <jy0922.s...@samsung.com> > Cc: Seung-Woo Kim <sw0312@samsung.com> > Cc: Kyungmin Park <kyungmin.p...@samsung.com> > Cc: Kukjin Kim <kg...@kernel.org> > Cc: Krzysztof Kozlowski <k...@kernel.org> > Cc: Javier Martinez Canillas <jav...@osg.samsung.com> > Cc: Xinliang Liu <z.liuxinli...@hisilicon.com> > Cc: Rongrong Zou <zourongr...@gmail.com> > Cc: Xinwei Kong <kong.kongxin...@hisilicon.com> > Cc: Chen Feng <puck.c...@hisilicon.com> > Cc: CK Hu <ck...@mediatek.com> > Cc: Philipp Zabel <p.za...@pengutronix.de> > Cc: Matthias Brugger <matthias@gmail.com> > Cc: Neil Armstrong <narmstr...@baylibre.com> > Cc: Carlo Caione <ca...@caione.org> > Cc: Kevin Hilman <khil...@baylibre.com> > Cc: Thierry Reding <thierry.red...@gmail.com> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Mark Yao <mark@rock-chips.com> > Cc: Heiko Stuebner <he...@sntech.de> > Cc: Maxime Ripard <maxime.rip...@free-electrons.com> > Cc: Chen-Yu Tsai <w...@csie.org> > Cc: Jyri Sarha <jsa...@ti.com> > Cc: Tomi Valkeinen <tomi.valkei...@ti.com> > Cc: dri-de...@lists.freedesktop.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-samsung-...@vger.kernel.org > Cc: linux-media...@lists.infradead.org > Cc: linux-amlo...@lists.infradead.org > Cc: linux-renesas-soc@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > --- > drivers/gpu/drm/armada/armada_crtc.c| 3 +-- > drivers/gpu/drm/armada/armada_drv.c | 4 ++-- > drivers/gpu/drm/drm_mipi_dsi.c | 6 +++--- > drivers/gpu/drm/drm_modes.c | 4 ++-- > drivers/gpu/drm/drm_of.c| 4 ++-- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 +-- > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 3 +-- > drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 ++-- > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 4 ++-- > drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 4 ++-- > drivers/gpu/drm/mediatek/mtk_dpi.c | 6 +++--- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 7 +++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 6 ++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 16 > drivers/gpu/drm/mediatek/mtk_dsi.c | 4 ++-- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 > drivers/gpu/drm/meson/meson_drv.c | 5 ++--- > drivers/gpu/drm/panel/panel-lvds.c | 16 > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++-- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 16 > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 4 ++-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++-- > drivers/gpu/drm/stm/ltdc.c | 3 +-- > drivers/gpu/drm/sun4i/sun4i_drv.c | 9 - For sun4i-drm: Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com> Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH] clk: Convert to using %pOF instead of full_name
On Tue, Jul 18, 2017 at 04:42:52PM -0500, Rob Herring wrote: > Now that we have a custom printf format specifier, convert users of > full_name to use %pOF instead. This is preparation to remove storing > of the full path string for each node. > > Signed-off-by: Rob Herring <r...@kernel.org> > Cc: Michael Turquette <mturque...@baylibre.com> > Cc: Stephen Boyd <sb...@codeaurora.org> > Cc: Maxime Coquelin <mcoquelin.st...@gmail.com> > Cc: Alexandre Torgue <alexandre.tor...@st.com> > Cc: Russell King <li...@armlinux.org.uk> > Cc: Matthias Brugger <matthias....@gmail.com> > Cc: Geert Uytterhoeven <geert+rene...@glider.be> > Cc: Maxime Ripard <maxime.rip...@free-electrons.com> > Cc: Chen-Yu Tsai <w...@csie.org> > Cc: "Emilio López" <emi...@elopez.com.ar> > Cc: Peter De Schrijver <pdeschrij...@nvidia.com> > Cc: Prashant Gaikwad <pgaik...@nvidia.com> > Cc: Thierry Reding <thierry.red...@gmail.com> > Cc: Jonathan Hunter <jonath...@nvidia.com> > Cc: Tero Kristo <t-kri...@ti.com> > Cc: linux-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-media...@lists.infradead.org > Cc: linux-renesas-soc@vger.kernel.org > Cc: linux-te...@vger.kernel.org > Cc: linux-o...@vger.kernel.org > --- > drivers/clk/berlin/bg2.c | 3 +-- > drivers/clk/berlin/bg2q.c| 7 +++ > drivers/clk/clk-asm9260.c| 4 ++-- > drivers/clk/clk-conf.c | 16 > drivers/clk/clk-moxart.c | 12 ++-- > drivers/clk/clk-qoriq.c | 7 +++ > drivers/clk/clk-stm32f4.c| 4 ++-- > drivers/clk/clk-xgene.c | 15 ++- > drivers/clk/clk.c| 4 ++-- > drivers/clk/clkdev.c | 4 ++-- > drivers/clk/mediatek/clk-cpumux.c| 2 +- > drivers/clk/mediatek/clk-mtk.c | 2 +- > drivers/clk/mediatek/reset.c | 2 +- > drivers/clk/renesas/clk-mstp.c | 2 +- > drivers/clk/renesas/clk-rcar-gen2.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun5i.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun6i-a31.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun8i-r.c | 3 +-- > drivers/clk/sunxi-ng/ccu-sun8i-v3s.c | 3 +-- > drivers/clk/sunxi/clk-sunxi.c| 17 +++-- For the sunxi and sunxi-ng clocks: Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com> Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending
On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: > On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard > <maxime.rip...@free-electrons.com> wrote: > > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > >> Hi, > >> > >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > >> <maxime.rip...@free-electrons.com> wrote: > >> > In the earlier display engine designs, any register access while a commit > >> > is pending is forbidden. > >> > > >> > One of the symptoms is that reading a register will return another, > >> > random, > >> > register value which can lead to register corruptions if we ever do a > >> > read/modify/write cycle. > >> > >> Alternatively, if changes to the backend (layers) are guaranteed to happen > >> while the CRTC is disabled (which seems to be the case after looking at > >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > >> could just turn on register auto-commit all the time and not deal with > >> this. > > > > As far as I understand, it will only be the case if we need a new > > modeset or we changed the active CRTC or connectors. But if you change > > only the format, buffers or properties it won't be the case, and we'll > > need to commit. > > So in other words, if someone were to use it for actual compositing and > moved the upper composited layer around, we would need commit support to be > safe. > > Sounds more or less like something a video player would do. Not only that. A change of buffer will happen every frame or so, and we can change the format whenever we want too (even if it's usually going to be in sync with a new buffer). Changing a property can happen any time too (like zpos for example). Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users
Hi Laurent,
On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> Thank you for the patch.
>
> On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> > The current drm_atomic_helper_commit_tail helper works only if the CRTC is
> > accessible, and documents an alternative implementation that is supposed to
> > be used if that happens.
> >
> > That implementation is then duplicated by some drivers. Instead of
> > documenting it, let's implement an helper that all the relevant users can
> > use directly.
> >
> > Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c| 47 +++
> > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +-
>
> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to
> avoid flicker" that changes the rcar-du implementation to the standard
> disable/update planes/enable order, so I'd appreciate if you could drop the
> rcar-du part of this patch to avoid conflicts.
I will.
> This being said, the reason why I switched back from the "runtime PM" to the
> "standard" order is probably of interest to you. Quoting the commit message,
>
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> >
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
>
> I believe that the "runtime PM" order is problematic in most drivers. The
> problem usually goes unnoticed as most monitors will not even display the
> first frame, and I assume many devices will just output it black, but it's an
> issue nonetheless.
>
> Note that my driver hasn't lost the "runtime PM" requirements, so I had to
> support them with the "standard" order. The best way I've found was to
> runtime
> resume in the one of .atomic_begin() and .enable() that is run first. Not
> very
> neat, as similar code would be needed in most drivers. I wonder whether it
> wouldn't be useful to add resume/suspend helper callbacks for the CRTC.
I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
but in order for the commits to happen, we need to have the CRTC
active, but it will remain powered up the whole time. I'm not sure if
we'll ever see such a frame.
But since this seems to be a pretty generic, maybe we should address
it in the helper itself?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending
On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > Hi, > > On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > <maxime.rip...@free-electrons.com> wrote: > > In the earlier display engine designs, any register access while a commit > > is pending is forbidden. > > > > One of the symptoms is that reading a register will return another, random, > > register value which can lead to register corruptions if we ever do a > > read/modify/write cycle. > > Alternatively, if changes to the backend (layers) are guaranteed to happen > while the CRTC is disabled (which seems to be the case after looking at > drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > could just turn on register auto-commit all the time and not deal with > this. As far as I understand, it will only be the case if we need a new modeset or we changed the active CRTC or connectors. But if you change only the format, buffers or properties it won't be the case, and we'll need to commit. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker
On Thu, Jul 13, 2017 at 05:25:08PM +0100, Kieran Bingham wrote: > >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> index 82b978a5dae6..c2f382feca07 100644 > >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > >>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct > >>> drm_atomic_state *old_state) > >>> > >>> /* Apply the atomic update. */ > >>> drm_atomic_helper_commit_modeset_disables(dev, old_state); > >>> - drm_atomic_helper_commit_modeset_enables(dev, old_state); > >>> drm_atomic_helper_commit_planes(dev, old_state, > >>> DRM_PLANE_COMMIT_ACTIVE_ONLY); > >> > >> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much > >> like > >> the default drm_atomic_helper_commit_tail() code. > >> > >> Reading around other uses /variants of commit_tail() style functions in > >> other > >> drivers has left me confused as to how the ordering affects things here. > >> > >> Could be worth adding a comment at least to describe why we can't use the > >> default helper... > > > > Or better still ... Use Maxime's new : > > > > [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for > > runtime_pm users > > Never mind - I've just looked again, and seen that this new helper function is > the ordering previous to *this* patch, and therefore isn't the same. > > However - it's worth noting that Maxime's patch converts this function to the > new helper - which contradicts this patch's motivations. So I guess I should drop the renesas modifications in my serie then? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
[PATCH 3/4] drm/sun4i: engine: Add commit_poll function
On the earlier Allwinner chips, with the first iteration of the display
engine, the backend commit bit needs to be polled before making any
register access to the backend.
Add an operation for that, that will be called in atomic_begin in order to
be sure to have that bit cleared before we do any modifications.
Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
drivers/gpu/drm/sun4i/sun4i_crtc.c | 2 ++
drivers/gpu/drm/sun4i/sunxi_engine.h | 14 ++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c
b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index f8c70439d1e2..2eba1d8639d8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -45,6 +45,8 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
spin_unlock_irqrestore(>event_lock, flags);
crtc->state->event = NULL;
}
+
+ WARN_ON(sunxi_engine_commit_poll(engine));
}
static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h
b/drivers/gpu/drm/sun4i/sunxi_engine.h
index 4cb70ae65c79..6618e182613c 100644
--- a/drivers/gpu/drm/sun4i/sunxi_engine.h
+++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
@@ -17,6 +17,7 @@ struct sunxi_engine;
struct sunxi_engine_ops {
void (*commit)(struct sunxi_engine *engine);
+ int (*commit_poll)(struct sunxi_engine *engine);
struct drm_plane **(*layers_init)(struct drm_device *drm,
struct sunxi_engine *engine);
@@ -55,6 +56,19 @@ sunxi_engine_commit(struct sunxi_engine *engine)
}
/**
+ * sunxi_engine_commit_poll() - wait for all changes to be committed
+ * @engine:pointer to the engine
+ */
+static inline int
+sunxi_engine_commit_poll(struct sunxi_engine *engine)
+{
+ if (engine->ops && engine->ops->commit_poll)
+ return engine->ops->commit_poll(engine);
+
+ return 0;
+}
+
+/**
* sunxi_engine_layers_init() - Create planes (layers) for the engine
* @drm: pointer to the drm_device for which planes will be created
* @engine:pointer to the engine
--
git-series 0.9.1
[PATCH 2/4] drm/sun4i: Use the runtime_pm commit_tail variant
The backend (planes) commit can only happen when the TCON (CRTC) is
enabled, which is not guaranteed with the default commit_tail helper.
Let's use the runtime_pm version that is designed specifically to deal with
that case.
Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
index 9872e0fc03b0..189048d33a1d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
@@ -10,6 +10,7 @@
* the License, or (at your option) any later version.
*/
+#include
#include
#include
#include
@@ -31,6 +32,10 @@ static const struct drm_mode_config_funcs
sun4i_de_mode_config_funcs = {
.fb_create = drm_fb_cma_create,
};
+static struct drm_mode_config_helper_funcs sun4i_de_mode_config_helpers = {
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
+};
+
struct drm_fbdev_cma *sun4i_framebuffer_init(struct drm_device *drm)
{
drm_mode_config_reset(drm);
@@ -39,6 +44,7 @@ struct drm_fbdev_cma *sun4i_framebuffer_init(struct
drm_device *drm)
drm->mode_config.max_height = 8192;
drm->mode_config.funcs = _de_mode_config_funcs;
+ drm->mode_config.helper_private = _de_mode_config_helpers;
return drm_fbdev_cma_init(drm, 32, drm->mode_config.num_connector);
}
--
git-series 0.9.1
[PATCH 4/4] drm/sun4i: make sure we don't have a commit pending
In the earlier display engine designs, any register access while a commit
is pending is forbidden.
One of the symptoms is that reading a register will return another, random,
register value which can lead to register corruptions if we ever do a
read/modify/write cycle.
Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++
drivers/gpu/drm/sun4i/sun4i_crtc.c| 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
b/drivers/gpu/drm/sun4i/sun4i_backend.c
index cf480218daa5..ce1f40f7511e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine *engine)
SUN4I_BACKEND_REGBUFFCTL_LOADCTL);
}
+static int sun4i_backend_commit_poll(struct sunxi_engine *engine)
+{
+ u32 val;
+
+ DRM_DEBUG_DRIVER("Polling for the commit to end\n");
+
+ return regmap_read_poll_timeout(engine->regs,
+ SUN4I_BACKEND_REGBUFFCTL_REG,
+ val,
+ !(val &
SUN4I_BACKEND_REGBUFFCTL_LOADCTL),
+ 100, 5);
+}
+
void sun4i_backend_layer_enable(struct sun4i_backend *backend,
int layer, bool enable)
{
@@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node *node)
static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
.commit = sun4i_backend_commit,
+ .commit_poll= sun4i_backend_commit_poll,
.layers_init= sun4i_layers_init,
.apply_color_correction = sun4i_backend_apply_color_correction,
.disable_color_correction =
sun4i_backend_disable_color_correction,
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c
b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 2eba1d8639d8..31550493fa1d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_crtc_state *old_state)
{
struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
+ struct sunxi_engine *engine = scrtc->engine;
struct drm_device *dev = crtc->dev;
unsigned long flags;
--
git-series 0.9.1
[PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users
The current drm_atomic_helper_commit_tail helper works only if the CRTC is
accessible, and documents an alternative implementation that is supposed to
be used if that happens.
That implementation is then duplicated by some drivers. Instead of
documenting it, let's implement an helper that all the relevant users can
use directly.
Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
drivers/gpu/drm/drm_atomic_helper.c| 47 +++
drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +-
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +--
include/drm/drm_atomic_helper.h| 1 +-
5 files changed, 36 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 86d3093c6c9b..a288805078f9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1245,23 +1245,11 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
* @old_state: atomic state object with old state structures
*
* This is the default implementation for the
- * _mode_config_helper_funcs.atomic_commit_tail hook.
+ * _mode_config_helper_funcs.atomic_commit_tail hook, for drivers
+ * that do not support runtime_pm.
*
* Note that the default ordering of how the various stages are called is to
- * match the legacy modeset helper library closest. One peculiarity of that is
- * that it doesn't mesh well with runtime PM at all.
- *
- * For drivers supporting runtime PM the recommended sequence is instead ::
- *
- * drm_atomic_helper_commit_modeset_disables(dev, old_state);
- *
- * drm_atomic_helper_commit_modeset_enables(dev, old_state);
- *
- * drm_atomic_helper_commit_planes(dev, old_state,
- * DRM_PLANE_COMMIT_ACTIVE_ONLY);
- *
- * for committing the atomic update to hardware. See the kerneldoc entries for
- * these three functions for more details.
+ * match the legacy modeset helper library closest.
*/
void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
{
@@ -1281,6 +1269,35 @@ void drm_atomic_helper_commit_tail(struct
drm_atomic_state *old_state)
}
EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
+/**
+ * drm_atomic_helper_commit_tail_runtime_pm - commit atomic update to hardware
+ * @old_state: new modeset state to be committed
+ *
+ * This is a variant of drm_atomic_helper_commit_tail suited for
+ * drivers that implement runtime_pm.
+ *
+ * Note that the default ordering of how the various stages are called is to
+ * match the legacy modeset helper library closest.
+ */
+void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state
*old_state)
+{
+ struct drm_device *dev = old_state->dev;
+
+ drm_atomic_helper_commit_modeset_disables(dev, old_state);
+
+ drm_atomic_helper_commit_modeset_enables(dev, old_state);
+
+ drm_atomic_helper_commit_planes(dev, old_state,
+ DRM_PLANE_COMMIT_ACTIVE_ONLY);
+
+ drm_atomic_helper_commit_hw_done(old_state);
+
+ drm_atomic_helper_wait_for_vblanks(dev, old_state);
+
+ drm_atomic_helper_cleanup_planes(dev, old_state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_tail_runtime_pm);
+
static void commit_tail(struct drm_atomic_state *old_state)
{
struct drm_device *dev = old_state->dev;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index d48fd7c918f8..71f6873255f1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer
*fb, int index)
return exynos_fb->dma_addr[index];
}
-static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
-{
- struct drm_device *dev = state->dev;
-
- drm_atomic_helper_commit_modeset_disables(dev, state);
-
- drm_atomic_helper_commit_modeset_enables(dev, state);
-
- /*
-* Exynos can't update planes with CRTCs and encoders disabled,
-* its updates routines, specially for FIMD, requires the clocks
-* to be enabled. So it is necessary to handle the modeset operations
-* *before* the commit_planes() step, this way it will always
-* have the relevant clocks enabled to perform the update.
-*/
- drm_atomic_helper_commit_planes(dev, state,
- DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
- drm_atomic_helper_commit_hw_done(state);
-
- drm_atomic_helper_wait_for_vblanks(dev, state);
-
- drm_atomic_helper_cleanup_planes(dev, state);
-}
-
static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
- .atomic_commit_tail = exynos_drm_atomic_commit_tail,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
};
static const struct drm
Re: [PATCH 07/12] usb: sunxi: Uses the resource-managed extcon API when registering extcon notifier
On Wed, Nov 30, 2016 at 02:57:35PM +0900, Chanwoo Choi wrote: > This patch just uses the resource-managed extcon API when registering > the extcon notifier. > > Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com> Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com> Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH v3 07/13] drm: bridge: vga-dac: Add adi,adv7123 compatible string
On Tue, Nov 29, 2016 at 11:04:37AM +0200, Laurent Pinchart wrote: > The ADV7123 is a transparent VGA DAC. Unlike dumb VGA DACs it can be > controlled through a power save pin, and requires a power supply. > However, on most boards where the device is used neither the power save > signal nor the power supply are controllable. > > To avoid developing a separate device-specific driver add an > "adi,adv7123" compatible entry to the dumb-vga-dac driver. This will > allow supporting most ADV7123-based boards easily, while allowing future > development of an adv7123 driver when needed without breaking backward > compatibility. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com> Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
