Re: [linux-next:master] BUILD REGRESSION 8c33787278ca8db73ad7d23f932c8c39b9f6e543

[Marc Zyngier]

On 2023-05-30 19:21, kernel test robot wrote:

tree/branch:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
branch HEAD: 8c33787278ca8db73ad7d23f932c8c39b9f6e543  Add linux-next
specific files for 20230530

Error/Warning reports:

https://lore.kernel.org/oe-kbuild-all/202305070840.x0g3ofjl-...@intel.com

Error/Warning: (recently discovered and may have been fixed)

include/drm/drm_print.h:456:39: error: format '%ld' expects argument
of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned
int'} [-Werror=format=]

Unverified Error/Warning (likely false positive, please contact us if
interested):

arch/arm64/kvm/mmu.c:147:3-9: preceding lock on line 140


This warning looks wrong. The "issue" seems that we acquire the lock
before exiting the function, but the the whole point is that the
lock is supposed to be held all along (it is dropped and then acquired
again).

I guess the coccinelle checker doesn't spot this construct?

M.
--
Who you jivin' with that Cosmik Debris?

Re: [PATCH 2/3] iommu/dma: Move public interfaces to linux/iommu.h

[Marc Zyngier]

On Tue, 16 Aug 2022 18:28:04 +0100,
Robin Murphy  wrote:
> 
> The iommu-dma layer is now mostly encapsulated by iommu_dma_ops, with
> only a couple more public interfaces left pertaining to MSI integration.
> Since these depend on the main IOMMU API header anyway, move their
> declarations there, taking the opportunity to update the half-baked
> comments to proper kerneldoc along the way.
> 
> Signed-off-by: Robin Murphy 
> ---
> 
> Note that iommu_setup_dma_ops() should also become internal in a future
> phase of the great IOMMU API upheaval - for now as the last bit of true
> arch code glue I consider it more "necessarily exposed" than "public".
> 
>  arch/arm64/mm/dma-mapping.c   |  2 +-
>  drivers/iommu/dma-iommu.c | 15 ++--
>  drivers/irqchip/irq-gic-v2m.c |  2 +-
>  drivers/irqchip/irq-gic-v3-its.c  |  2 +-
>  drivers/irqchip/irq-gic-v3-mbi.c  |  2 +-
>  drivers/irqchip/irq-ls-scfg-msi.c |  2 +-
>  drivers/vfio/vfio_iommu_type1.c   |  1 -
>  include/linux/dma-iommu.h | 40 ---
>  include/linux/iommu.h | 36 
>  9 files changed, 54 insertions(+), 48 deletions(-)

For the irqchip side:

Acked-by: Marc Zyngier 

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v16 08/40] gpu: host1x: Add initial runtime PM and OPP support

[Marc Zyngier]

Hi all,

On 2021-12-22 19:31, Dmitry Osipenko wrote:

22.12.2021 22:30, Jon Hunter пишет:


On 22/12/2021 19:01, Dmitry Osipenko wrote:

...

diff --git a/drivers/gpu/host1x/syncpt.c 
b/drivers/gpu/host1x/syncpt.c

index e08e331e46ae..8194826c9ce3 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -137,6 +137,15 @@ void host1x_syncpt_restore(struct host1x *host)
  struct host1x_syncpt *sp_base = host->syncpt;
  unsigned int i;

+    for (i = 0; i < host->info->nb_pts; i++) {
+    /*
+ * Unassign syncpt from channels for purposes of Tegra186
+ * syncpoint protection. This prevents any channel from
+ * accessing it until it is reassigned.
+ */
+    host1x_hw_syncpt_assign_to_channel(host, sp_base + i, NULL);
+    }
+
  for (i = 0; i < host1x_syncpt_nb_pts(host); i++)
  host1x_hw_syncpt_restore(host, sp_base + i);

@@ -352,13 +361,6 @@ int host1x_syncpt_init(struct host1x *host)
  for (i = 0; i < host->info->nb_pts; i++) {
  syncpt[i].id = i;
  syncpt[i].host = host;
-
-    /*
- * Unassign syncpt from channels for purposes of Tegra186
- * syncpoint protection. This prevents any channel from
- * accessing it until it is reassigned.
- */
-    host1x_hw_syncpt_assign_to_channel(host, [i], NULL);
  }

  for (i = 0; i < host->info->nb_bases; i++)




Thanks! This fixed it!


I'll prepare proper patch with yours t-b, thank you.


The fix has been in -next for some time now, but it still hasn't
made it into Linus' tree (at least not in -rc2).

Any hope for this to land -rc3?

Thanks,

 M.
--
Jazz is not dead. It just smells funny...

Re: dt-bindings: treewide: Update @st.com email address to @foss.st.com

[Marc Zyngier]

On Wed, 27 Oct 2021 14:56:35 +0100,
Patrice CHOTARD  wrote:
> 
> Hi Marc
> 
> +Joe Perches
> 
> On 10/27/21 8:11 AM, Patrice CHOTARD wrote:
> > Hi Marc
> > 
> > On 10/20/21 1:39 PM, Marc Zyngier wrote:
> >> On Wed, 20 Oct 2021 08:45:02 +0100,
> >> Krzysztof Kozlowski  wrote:
> >>>
> >>> On 20/10/2021 08:50, patrice.chot...@foss.st.com wrote:
> >>>> From: Patrice Chotard 
> >>>>
> >>>> Not all @st.com email address are concerned, only people who have
> >>>> a specific @foss.st.com email will see their entry updated.
> >>>> For some people, who left the company, remove their email.
> >>>>
> >>>
> >>> Please split simple address change from maintainer updates (removal,
> >>> addition).
> >>>
> >>> Also would be nice to see here explained *why* are you doing this.
> >>
> >> And why this can't be done with a single update to .mailmap, like
> >> anyone else does.
> > 
> > Thanks for the tips, yes, it will be simpler.
> > 
> > Thanks
> > Patrice
> > 
> >>
> >>M.
> >>
> 
> I made a try by updating .mailmap with adding a new entry with my 
> @foss.st.com email :
> 
>  Pali Rohár  
>  Paolo 'Blaisorblade' Giarrusso 
> +Patrice Chotard  
>  Patrick Mochel 
>  Paul Burton  
> 
> But when running ./scripts/get_maintainer.pl 
> Documentation/devicetree/bindings/arm/sti.yaml, by old email is still 
> displayed
> 
> Rob Herring  (maintainer:OPEN FIRMWARE AND FLATTENED 
> DEVICE TREE BINDINGS)
> Patrice Chotard  (in file)
> devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE 
> BINDINGS)
> linux-ker...@vger.kernel.org (open list)
> 
> By default, the get_maintainer.pl script is using .mailmap file
> ($email_use_mailmap = 1).
> 
> It seems there is an issue with get_maintainer.pl and maintainer
> name/e-mail found in yaml file ?

Try this (warning though: my Perl-foo is non-existent).

M.

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 2075db0c08b8..2a84a3fb0130 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -986,6 +986,7 @@ sub get_maintainers {
my ($name, $address) = parse_email($email);
 
my $tmp_email = format_email($name, $address, $email_usename);
+   $tmp_email = mailmap_email($tmp_email);
push_email_address($tmp_email, '');
add_role($tmp_email, 'in file');
}

-- 
Without deviation from the norm, progress is not possible.

Re: dt-bindings: treewide: Update @st.com email address to @foss.st.com

[Marc Zyngier]

On Wed, 20 Oct 2021 08:45:02 +0100,
Krzysztof Kozlowski  wrote:
> 
> On 20/10/2021 08:50, patrice.chot...@foss.st.com wrote:
> > From: Patrice Chotard 
> > 
> > Not all @st.com email address are concerned, only people who have
> > a specific @foss.st.com email will see their entry updated.
> > For some people, who left the company, remove their email.
> > 
> 
> Please split simple address change from maintainer updates (removal,
> addition).
> 
> Also would be nice to see here explained *why* are you doing this.

And why this can't be done with a single update to .mailmap, like
anyone else does.

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/6] KVM: arm64/mmu: avoid struct page in MMU

[Marc Zyngier]

On Thu, 24 Jun 2021 04:57:47 +0100,
David Stevens  wrote:
> 
> From: David Stevens 
> 
> Avoid converting pfns returned by follow_fault_pfn to struct pages to
> transiently take a reference. The reference was originally taken to
> match the reference taken by gup. However, pfns returned by
> follow_fault_pfn may not have a struct page set up for reference
> counting.
> 
> Signed-off-by: David Stevens 
> ---
>  arch/arm64/kvm/mmu.c | 43 +++
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 896b3644b36f..a741972cb75f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c

[...]

> @@ -968,16 +968,16 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
> phys_addr_t fault_ipa,
>*/
>   if (vma_pagesize == PAGE_SIZE && !force_pte)
>   vma_pagesize = transparent_hugepage_adjust(memslot, hva,
> -, _ipa);
> +, _ipa);
>   if (writable)
>   prot |= KVM_PGTABLE_PROT_W;
>  
>   if (fault_status != FSC_PERM && !device)
> - clean_dcache_guest_page(pfn, vma_pagesize);
> + clean_dcache_guest_page(pfnpg.pfn, vma_pagesize);
>  
>   if (exec_fault) {
>   prot |= KVM_PGTABLE_PROT_X;
> - invalidate_icache_guest_page(pfn, vma_pagesize);
> + invalidate_icache_guest_page(pfnpg.pfn, vma_pagesize);

This is going to clash with what is currently in -next, specially with
MTE.

Paolo, if you really want to take this in 5.13, can you please push a
stable branch based on -rc4 or older so that I can merge it in and
test it?

Thanks,

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/6] KVM: x86/mmu: avoid struct page in MMU

[Marc Zyngier]

On Thu, 24 Jun 2021 09:58:00 +0100,
Nicholas Piggin  wrote:
> 
> Excerpts from David Stevens's message of June 24, 2021 1:57 pm:
> > From: David Stevens 
> >  out_unlock:
> > if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
> > read_unlock(>kvm->mmu_lock);
> > else
> > write_unlock(>kvm->mmu_lock);
> > -   kvm_release_pfn_clean(pfn);
> > +   if (pfnpg.page)
> > +   put_page(pfnpg.page);
> > return r;
> >  }
> 
> How about
> 
>   kvm_release_pfn_page_clean(pfnpg);

I'm not sure. I always found kvm_release_pfn_clean() ugly, because it
doesn't mark the page 'clean'. I find put_page() more correct.

Something like 'kvm_put_pfn_page()' would make more sense, but I'm so
bad at naming things that I could just as well call it 'bob()'.

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/6] KVM: mmu: also return page from gfn_to_pfn

[Marc Zyngier]

Hi David,

On Thu, 24 Jun 2021 04:57:45 +0100,
David Stevens  wrote:
> 
> From: David Stevens 
> 
> Return a struct kvm_pfn_page containing both a pfn and an optional
> struct page from the gfn_to_pfn family of functions. This differentiates
> the gup and follow_fault_pfn cases, which allows callers that only need
> a pfn to avoid touching the page struct in the latter case. For callers
> that need a struct page, introduce a helper function that unwraps a
> struct kvm_pfn_page into a struct page. This helper makes the call to
> kvm_get_pfn which had previously been in hva_to_pfn_remapped.
> 
> For now, wrap all calls to gfn_to_pfn functions in the new helper
> function. Callers which don't need the page struct will be updated in
> follow-up patches.
> 
> Signed-off-by: David Stevens 
> ---
>  arch/arm64/kvm/mmu.c   |   5 +-
>  arch/mips/kvm/mmu.c|   3 +-
>  arch/powerpc/kvm/book3s.c  |   3 +-
>  arch/powerpc/kvm/book3s_64_mmu_hv.c|   5 +-
>  arch/powerpc/kvm/book3s_64_mmu_radix.c |   5 +-
>  arch/powerpc/kvm/book3s_hv_uvmem.c |   4 +-
>  arch/powerpc/kvm/e500_mmu_host.c   |   2 +-
>  arch/x86/kvm/mmu/mmu.c |  11 ++-
>  arch/x86/kvm/mmu/mmu_audit.c   |   2 +-
>  arch/x86/kvm/x86.c |   2 +-
>  drivers/gpu/drm/i915/gvt/kvmgt.c   |   2 +-
>  include/linux/kvm_host.h   |  27 --
>  include/linux/kvm_types.h  |   5 +
>  virt/kvm/kvm_main.c| 121 +
>  14 files changed, 109 insertions(+), 88 deletions(-)
> 

[...]

> +kvm_pfn_t kvm_pfn_page_unwrap(struct kvm_pfn_page pfnpg)
> +{
> + if (pfnpg.page)
> + return pfnpg.pfn;
> +
> + kvm_get_pfn(pfnpg.pfn);
> + return pfnpg.pfn;
> +}
> +EXPORT_SYMBOL_GPL(kvm_pfn_page_unwrap);

I'd really like to see a tiny bit of documentation explaining that
calls to kvm_pfn_page_unwrap() are not idempotent.

Otherwise, looks good to me.

Thanks,

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/3] find: micro-optimize for_each_{set,clear}_bit()

[Marc Zyngier]

On Fri, 18 Jun 2021 20:57:34 +0100,
Yury Norov  wrote:
> 
> The macros iterate thru all set/clear bits in a bitmap. They search a
> first bit using find_first_bit(), and the rest bits using find_next_bit().
> 
> Since find_next_bit() is called shortly after find_first_bit(), we can
> save few lines of I-cache by not using find_first_bit().

Really?

> 
> Signed-off-by: Yury Norov 
> ---
>  include/linux/find.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/find.h b/include/linux/find.h
> index 4500e8ab93e2..ae9ed52b52b8 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -280,7 +280,7 @@ unsigned long find_next_bit_le(const void *addr, unsigned
>  #endif
>  
>  #define for_each_set_bit(bit, addr, size) \
> - for ((bit) = find_first_bit((addr), (size));\
> + for ((bit) = find_next_bit((addr), (size), 0);  \

On which architecture do you observe a gain? Only 32bit ARM and m68k
implement their own version of find_first_bit(), and everyone else
uses the canonical implementation:

#ifndef find_first_bit
#define find_first_bit(addr, size) find_next_bit((addr), (size), 0)
#endif

These architectures explicitly have different implementations for
find_first_bit() and find_next_bit() because they can do better
(whether that is true or not is another debate). I don't think you
should remove this optimisation until it has been measured on these
two architectures.

Thanks,

M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller

[Marc Zyngier]

Hi Maxime,

On 2021-02-10 14:40, Maxime Ripard wrote:

Hi Dave,

On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote:

On Mon, 11 Jan 2021 at 14:23, Maxime Ripard  wrote:
>
> The BSC controllers used for the HDMI DDC have an interrupt controller
> shared between both instances. Let's add it to avoid polling.

This seems to have unintended side effects.
GIC interrupt 117 is shared between the standard I2C controllers
(i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts.

Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that
doesn't appear to be an option for l2-intc registering as an interrupt
controller. i2c-bcm2835 therefore loses out and fails to register for
the interrupt.

Is there an equivalent flag that an interrupt controller can add to
say that the parent interrupt is shared? Is that even supported?


Indeed, it looks like setting an equivalent to IRQF_SHARED would be the
solution, but I couldn't find anything that would allow us to in the
irqchip code.

Marc, Thomas, is it something that is allowed?


No, not really. That's because the chained handler is actually an
interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong
level for that.

I can see two possibilities:

- the l2-intc gets turned into a normal handler, and does the demux
  from there. Horrible stuff.

- the i2c controller gets parented to the l2c-int as a fake interrupt,
  and gets called from there. Horrible stuff.

Pick your poison... :-/

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Marc Zyngier]

Hi Maxime,

On 2020-12-14 15:27, Maxime Ripard wrote:

Hi Marc,

On Thu, Dec 10, 2020 at 05:59:09PM +, Marc Zyngier wrote:


[...]


I'm always sceptical of making interrupt controllers user-selectable.
Who is going to know that they need to pick that one?

I'd be much more in favour of directly selecting this symbol
from DRM_VC4_HDMI_CEC, since there is an obvious dependency.


It's a bit weird to me that the HDMI CEC support selects it, since that
interrupt controller is external and here no matter what.


From glancing at the series, I was under the impression that these
controllers were there for the sole benefit of the HDMI controllers.
Is there anything else connected to them?


Would selecting it from the ARCH_* Kconfig option work for you?


Sure. My only ask is that the low level plumbing is selected without
requiring any user guesswork.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch 09/30] ARM: smp: Use irq_desc_kstat_cpu() in show_ipi_list()

[Marc Zyngier]

On Thu, 10 Dec 2020 19:25:45 +,
Thomas Gleixner  wrote:
> 
> The irq descriptor is already there, no need to look it up again.
> 
> Signed-off-by: Thomas Gleixner 
> Cc: Marc Zyngier 
> Cc: Russell King 
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  arch/arm/kernel/smp.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -550,7 +550,7 @@ void show_ipi_list(struct seq_file *p, i
>   seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
>  
>   for_each_online_cpu(cpu)
> - seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
> + seq_printf(p, "%10u ", irq_desc_kstat_cpu(ipi_desc[i], 
> cpu));
>  
>   seq_printf(p, " %s\n", ipi_types[i]);
>   }
> 
> 

Acked-by: Marc Zyngier 

-- 
Without deviation from the norm, progress is not possible.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch 10/30] arm64/smp: Use irq_desc_kstat_cpu() in arch_show_interrupts()

[Marc Zyngier]

On Thu, 10 Dec 2020 19:25:46 +,
Thomas Gleixner  wrote:
> 
> The irq descriptor is already there, no need to look it up again.
> 
> Signed-off-by: Thomas Gleixner 
> Cc: Mark Rutland 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Cc: Marc Zyngier 
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  arch/arm64/kernel/smp.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -809,7 +809,7 @@ int arch_show_interrupts(struct seq_file
>   seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i,
>  prec >= 4 ? " " : "");
>   for_each_online_cpu(cpu)
> - seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
> + seq_printf(p, "%10u ", irq_desc_kstat_cpu(ipi_desc[i], 
> cpu));
>   seq_printf(p, "  %s\n", ipi_types[i]);
>   }
>  
> 
> 

Acked-by: Marc Zyngier 

-- 
Without deviation from the norm, progress is not possible.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] irqchip: Allow to compile bcmstb on other platforms

[Marc Zyngier]

Hi Maxime,

On 2020-12-10 13:46, Maxime Ripard wrote:
The BCM2711 uses a number of instances of the bcmstb-l2 controller in 
its

display engine. Let's allow the driver to be enabled through KConfig.

Signed-off-by: Maxime Ripard 
---
 drivers/irqchip/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c6098eee0c7c..f1e58de117dc 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -131,7 +131,7 @@ config BCM7120_L2_IRQ
select IRQ_DOMAIN

 config BRCMSTB_L2_IRQ
-   bool
+   bool "Broadcom STB L2 Interrupt Controller"
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN


I'm always sceptical of making interrupt controllers user-selectable.
Who is going to know that they need to pick that one?

I'd be much more in favour of directly selecting this symbol
from DRM_VC4_HDMI_CEC, since there is an obvious dependency.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown

[Marc Zyngier]

On 2020-11-23 14:03, Jerome Brunet wrote:

On Fri 20 Nov 2020 at 10:42, Marc Zyngier  wrote:


The HDMI driver request clocks early, but never disable them, leaving
the clocks on even when the driver is removed.

Fix it by slightly refactoring the clock code, and register a devm
action that will eventually disable/unprepare the enabled clocks.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 
++-

 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c

index 7f8eea494147..29623b309cb1 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -145,8 +145,6 @@ struct meson_dw_hdmi {
struct reset_control *hdmitx_apb;
struct reset_control *hdmitx_ctrl;
struct reset_control *hdmitx_phy;
-   struct clk *hdmi_pclk;
-   struct clk *venci_clk;
struct regulator *hdmi_supply;
u32 irq_stat;
struct dw_hdmi *hdmi;
@@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data)
regulator_disable(data);
 }

+static void meson_disable_clk(void *data)
+{
+   clk_disable_unprepare(data);
+}
+
+static int meson_enable_clk(struct device *dev, char *name)
+{
+   struct clk *clk;
+   int ret;
+
+   clk = devm_clk_get(dev, name);
+   if (IS_ERR(clk)) {
+   dev_err(dev, "Unable to get %s pclk\n", name);
+   return PTR_ERR(clk);
+   }
+
+   ret = clk_prepare_enable(clk);
+   if (!ret)
+   ret = devm_add_action_or_reset(dev, meson_disable_clk,
clk);


Thanks for fixing this Marc.

FYI, while it is fine to declare a function to disable the clocks, a 
quick

cast may avoid it

devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, 
clk);


While this works for now, a change to the clk_disable_unprepare()
prototype (such as adding a second argument) would now go completely
unnoticed (after all, you've cast the function, it *must* be correct,
right?), and someone would spend a few hours trying to track down memory
corruption or some other interesting results.

Yes, casting C functions can be hilarious.

I can see a few uses of this hack in the tree, and I have my pop-corn
ready.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/meson: dw-hdmi: Enable the iahb clock early enough

[Marc Zyngier]

On 2020-11-20 10:54, Guillaume Tucker wrote:

On 20/11/2020 09:42, Marc Zyngier wrote:

Instead of moving meson_dw_hdmi_init() around which breaks existing
platform, let's enable the clock meson_dw_hdmi_init() depends on.
This means we don't have to worry about this clock being enabled or
not, depending on the boot-loader features.

Fixes: b33340e33acd ("drm/meson: dw-hdmi: Ensure that clocks are 
enabled before touching the TOP registers")

Reported-by: Guillaume Tucker 


Although I am triaging kernelci bisections, it was initially
found thanks to our friendly bot.  So if you're OK with this, it
would most definitely appreciate a mention:

  Reported-by: "kernelci.org bot" 


Sure. Neil can add this when (and if) he applies these patches.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200

[Marc Zyngier]

On 2020-11-20 09:26, Neil Armstrong wrote:

On 19/11/2020 19:35, Marc Zyngier wrote:
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c

index 7f8eea494147..52af8ba94311 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -146,6 +146,7 @@ struct meson_dw_hdmi {
 struct reset_control *hdmitx_ctrl;
 struct reset_control *hdmitx_phy;
 struct clk *hdmi_pclk;
+    struct clk *iahb_clk;
 struct clk *venci_clk;
 struct regulator *hdmi_supply;
 u32 irq_stat;
@@ -1033,6 +1034,13 @@ static int meson_dw_hdmi_bind(struct device 
*dev, struct device *master,

 }
 clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);

+    meson_dw_hdmi->iahb_clk = devm_clk_get(dev, "iahb");
+    if (IS_ERR(meson_dw_hdmi->iahb_clk)) {
+    dev_err(dev, "Unable to get iahb clk\n");
+    return PTR_ERR(meson_dw_hdmi->iahb_clk);
+    }
+    clk_prepare_enable(meson_dw_hdmi->iahb_clk);



On previous SoCs, iahb was directly the bus clock (clk81), and on 
recent socs

this clock is a gate.

The question is why is it disabled. Maybe a previous failed probe 
disabled it

in the dw-hdmi probe failure code and this clock is needed for
meson_dw_hdmi_init(),
so yeah this is the right fix.

Thanks.

Could you send a revert of b33340e33acdfe5ca6a5aa1244709575ae1e0432
and then proper fix with clk_disable_unprepare added ?


Bah. I missed that email and sent a slightly different resolution.
Hopefully that'll be good enough.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/meson: dw-hdmi: Enable the iahb clock early enough

[Marc Zyngier]

Instead of moving meson_dw_hdmi_init() around which breaks existing
platform, let's enable the clock meson_dw_hdmi_init() depends on.
This means we don't have to worry about this clock being enabled or
not, depending on the boot-loader features.

Fixes: b33340e33acd ("drm/meson: dw-hdmi: Ensure that clocks are enabled before 
touching the TOP registers")
Reported-by: Guillaume Tucker 
Tested-by: Guillaume Tucker 
Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 29623b309cb1..aad75a22dc33 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -1051,6 +1051,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
if (ret)
return ret;
 
+   ret = meson_enable_clk(dev, "iahb");
+   if (ret)
+   return ret;
+
ret = meson_enable_clk(dev, "venci");
if (ret)
return ret;
@@ -1086,6 +1090,8 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
 
encoder->possible_crtcs = BIT(0);
 
+   meson_dw_hdmi_init(meson_dw_hdmi);
+
DRM_DEBUG_DRIVER("encoder initialized\n");
 
/* Bridge / Connector */
@@ -1110,8 +1116,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
if (IS_ERR(meson_dw_hdmi->hdmi))
return PTR_ERR(meson_dw_hdmi->hdmi);
 
-   meson_dw_hdmi_init(meson_dw_hdmi);
-
next_bridge = of_drm_find_bridge(pdev->dev.of_node);
if (next_bridge)
drm_bridge_attach(encoder, next_bridge,
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown

[Marc Zyngier]

The HDMI driver request clocks early, but never disable them, leaving
the clocks on even when the driver is removed.

Fix it by slightly refactoring the clock code, and register a devm
action that will eventually disable/unprepare the enabled clocks.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++-
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 7f8eea494147..29623b309cb1 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -145,8 +145,6 @@ struct meson_dw_hdmi {
struct reset_control *hdmitx_apb;
struct reset_control *hdmitx_ctrl;
struct reset_control *hdmitx_phy;
-   struct clk *hdmi_pclk;
-   struct clk *venci_clk;
struct regulator *hdmi_supply;
u32 irq_stat;
struct dw_hdmi *hdmi;
@@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data)
regulator_disable(data);
 }
 
+static void meson_disable_clk(void *data)
+{
+   clk_disable_unprepare(data);
+}
+
+static int meson_enable_clk(struct device *dev, char *name)
+{
+   struct clk *clk;
+   int ret;
+
+   clk = devm_clk_get(dev, name);
+   if (IS_ERR(clk)) {
+   dev_err(dev, "Unable to get %s pclk\n", name);
+   return PTR_ERR(clk);
+   }
+
+   ret = clk_prepare_enable(clk);
+   if (!ret)
+   ret = devm_add_action_or_reset(dev, meson_disable_clk, clk);
+
+   return ret;
+}
+
 static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
void *data)
 {
@@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev, 
struct device *master,
if (IS_ERR(meson_dw_hdmi->hdmitx))
return PTR_ERR(meson_dw_hdmi->hdmitx);
 
-   meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr");
-   if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) {
-   dev_err(dev, "Unable to get HDMI pclk\n");
-   return PTR_ERR(meson_dw_hdmi->hdmi_pclk);
-   }
-   clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);
+   ret = meson_enable_clk(dev, "isfr");
+   if (ret)
+   return ret;
 
-   meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
-   if (IS_ERR(meson_dw_hdmi->venci_clk)) {
-   dev_err(dev, "Unable to get venci clk\n");
-   return PTR_ERR(meson_dw_hdmi->venci_clk);
-   }
-   clk_prepare_enable(meson_dw_hdmi->venci_clk);
+   ret = meson_enable_clk(dev, "venci");
+   if (ret)
+   return ret;
 
dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
  _dw_hdmi_regmap_config);
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] More meson HDMI fixes

[Marc Zyngier]

Guillaume reported that my earlier fixes for the meson HDMI driver
broke another set of machines which are now exploding (clock not
enabled).

I have thus reconsidered the approach and came up with an alternative
fix (enable a missing clock), which Guillaume confirmed to be working.
Jerome pointed out that this driver is leaking clock references like a
sieve, so that needed addressing too.

The first patch start by fixing the clock leakage using a devm
facility.

The second patch addresses the earlier crash by reusing the
infrastructure put together in the first patch.

Tested on VIM3l.

Marc Zyngier (2):
  drm/meson: dw-hdmi: Disable clocks on driver teardown
  drm/meson: dw-hdmi: Enable the iahb clock early enough

 drivers/gpu/drm/meson/meson_dw_hdmi.c | 51 ++-
 1 file changed, 35 insertions(+), 16 deletions(-)

-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200

[Marc Zyngier]

On 2020-11-19 18:13, Jerome Brunet wrote:

On Thu 19 Nov 2020 at 19:04, Guillaume Tucker
 wrote:


Hi Marc,

On 19/11/2020 11:58, Marc Zyngier wrote:

On 2020-11-19 10:26, Neil Armstrong wrote:

On 19/11/2020 11:20, Marc Zyngier wrote:

On 2020-11-19 08:50, Guillaume Tucker wrote:

Please see the automated bisection report below about some kernel
errors on meson-gxbb-p200.

Reports aren't automatically sent to the public while we're
trialing new bisection features on kernelci.org, however this one
looks valid.

The bisection started with next-20201118 but the errors are still
present in next-20201119.  Details for this regression:

  https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/

The first error is:

  [   14.757489] Internal error: synchronous external abort: 
96000210

[#1] PREEMPT SMP


Looks like yet another clock ordering setup. I guess different 
Amlogic

platforms have slightly different ordering requirements.

Neil, do you have any idea of which platform requires which 
ordering?
The variability in DT and platforms is pretty difficult to follow 
(and

I don't think I have such board around).


The requirements should be the same, here the init was done before 
calling
dw_hdmi_probe to be sure the clocks and internals resets were 
deasserted.
But since you boot from u-boot already enabling these, it's already 
active.


The solution would be to revert and do some check in 
meson_dw_hdmi_init() to

check if already enabled and do nothing.


A better fix seems to be this, which makes it explicit that there is
a dependency between some of the registers accessed from 
meson_dw_hdmi_init()

and the iahb clock.

Guillaume, can you give this a go on your failing box?


I confirm it solves the problem.  Please add this to your fix
patch if it's OK with you:

  Reported-by: "kernelci.org bot" 
  Tested-by: Guillaume Tucker 


For the record, it passed all the tests when applied on top of
the "bad" revision found by the bisection:

  
http://lava.baylibre.com:10080/scheduler/alljobs?search=v5.10-rc3-1021-gb8668a2e5ea1


and the exact same test on the "bad" revision without the fix
consistently showed the error:

  http://lava.baylibre.com:10080/scheduler/job/374176


Thanks,
Guillaume


diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c

index 7f8eea494147..52af8ba94311 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -146,6 +146,7 @@ struct meson_dw_hdmi {
 struct reset_control *hdmitx_ctrl;
 struct reset_control *hdmitx_phy;
 struct clk *hdmi_pclk;
+struct clk *iahb_clk;
 struct clk *venci_clk;
 struct regulator *hdmi_supply;
 u32 irq_stat;
@@ -1033,6 +1034,13 @@ static int meson_dw_hdmi_bind(struct device 
*dev, struct device *master,

 }
 clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);

+meson_dw_hdmi->iahb_clk = devm_clk_get(dev, "iahb");
+if (IS_ERR(meson_dw_hdmi->iahb_clk)) {
+dev_err(dev, "Unable to get iahb clk\n");
+return PTR_ERR(meson_dw_hdmi->iahb_clk);
+}
+clk_prepare_enable(meson_dw_hdmi->iahb_clk);


If you guys are going ahead with this fix, this call to
clk_prepare_enable() needs to be balanced with clk_disable_unprepare() 
somehow


Yup, good point.

Although this driver *never* disables any clock it enables, and leaves 
it

to the main DW driver, which I guess makes it leak references.

So all 3 clocks need fixing.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200

[Marc Zyngier]

On 2020-11-19 10:26, Neil Armstrong wrote:

On 19/11/2020 11:20, Marc Zyngier wrote:

On 2020-11-19 08:50, Guillaume Tucker wrote:

Please see the automated bisection report below about some kernel
errors on meson-gxbb-p200.

Reports aren't automatically sent to the public while we're
trialing new bisection features on kernelci.org, however this one
looks valid.

The bisection started with next-20201118 but the errors are still
present in next-20201119.  Details for this regression:

  https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/

The first error is:

  [   14.757489] Internal error: synchronous external abort: 96000210
[#1] PREEMPT SMP


Looks like yet another clock ordering setup. I guess different Amlogic
platforms have slightly different ordering requirements.

Neil, do you have any idea of which platform requires which ordering?
The variability in DT and platforms is pretty difficult to follow (and
I don't think I have such board around).


The requirements should be the same, here the init was done before 
calling
dw_hdmi_probe to be sure the clocks and internals resets were 
deasserted.
But since you boot from u-boot already enabling these, it's already 
active.


The solution would be to revert and do some check in 
meson_dw_hdmi_init() to

check if already enabled and do nothing.


A better fix seems to be this, which makes it explicit that there is
a dependency between some of the registers accessed from 
meson_dw_hdmi_init()

and the iahb clock.

Guillaume, can you give this a go on your failing box?

Thanks,

M.

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c

index 7f8eea494147..52af8ba94311 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -146,6 +146,7 @@ struct meson_dw_hdmi {
struct reset_control *hdmitx_ctrl;
struct reset_control *hdmitx_phy;
struct clk *hdmi_pclk;
+   struct clk *iahb_clk;
struct clk *venci_clk;
struct regulator *hdmi_supply;
u32 irq_stat;
@@ -1033,6 +1034,13 @@ static int meson_dw_hdmi_bind(struct device *dev, 
struct device *master,

}
clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);

+   meson_dw_hdmi->iahb_clk = devm_clk_get(dev, "iahb");
+   if (IS_ERR(meson_dw_hdmi->iahb_clk)) {
+   dev_err(dev, "Unable to get iahb clk\n");
+   return PTR_ERR(meson_dw_hdmi->iahb_clk);
+   }
+   clk_prepare_enable(meson_dw_hdmi->iahb_clk);
+
meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
if (IS_ERR(meson_dw_hdmi->venci_clk)) {
dev_err(dev, "Unable to get venci clk\n");
@@ -1071,6 +1079,8 @@ static int meson_dw_hdmi_bind(struct device *dev, 
struct device *master,


encoder->possible_crtcs = BIT(0);

+   meson_dw_hdmi_init(meson_dw_hdmi);
+
DRM_DEBUG_DRIVER("encoder initialized\n");

/* Bridge / Connector */
@@ -1095,8 +1105,6 @@ static int meson_dw_hdmi_bind(struct device *dev, 
struct device *master,

if (IS_ERR(meson_dw_hdmi->hdmi))
return PTR_ERR(meson_dw_hdmi->hdmi);

-   meson_dw_hdmi_init(meson_dw_hdmi);
-
next_bridge = of_drm_find_bridge(pdev->dev.of_node);
if (next_bridge)
drm_bridge_attach(encoder, next_bridge,


--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200

[Marc Zyngier]

On 2020-11-19 10:26, Neil Armstrong wrote:

On 19/11/2020 11:20, Marc Zyngier wrote:

On 2020-11-19 08:50, Guillaume Tucker wrote:

Please see the automated bisection report below about some kernel
errors on meson-gxbb-p200.

Reports aren't automatically sent to the public while we're
trialing new bisection features on kernelci.org, however this one
looks valid.

The bisection started with next-20201118 but the errors are still
present in next-20201119.  Details for this regression:

  https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/

The first error is:

  [   14.757489] Internal error: synchronous external abort: 96000210
[#1] PREEMPT SMP


Looks like yet another clock ordering setup. I guess different Amlogic
platforms have slightly different ordering requirements.

Neil, do you have any idea of which platform requires which ordering?
The variability in DT and platforms is pretty difficult to follow (and
I don't think I have such board around).


The requirements should be the same, here the init was done before 
calling
dw_hdmi_probe to be sure the clocks and internals resets were 
deasserted.
But since you boot from u-boot already enabling these, it's already 
active.


The crashing platform also boots with u-boot. What is the difference?
No HDMI support?

The solution would be to revert and do some check in 
meson_dw_hdmi_init() to

check if already enabled and do nothing.


It looks more subtle than that, as it also depends on which DT is 
provided
(an early meson_dw_hdmi_init() works with the kernel DT, and breaks with 
the

u-boot DT). So whatever difference is between the two DTs causes havoc.
u-boot obviously only uses its own DT, so we are looking at a kernel bug 
here.


Not having this patch also breaks module reinsertion (HDMI clocks are 
disabled

on unbind), so *something* has to be done late.

So here are my questions:
- What breaks in my config when I boot using u-boot's DT?
- How to detect early that the registers are clocked or not?

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200

[Marc Zyngier]

On 2020-11-19 08:50, Guillaume Tucker wrote:

Please see the automated bisection report below about some kernel
errors on meson-gxbb-p200.

Reports aren't automatically sent to the public while we're
trialing new bisection features on kernelci.org, however this one
looks valid.

The bisection started with next-20201118 but the errors are still
present in next-20201119.  Details for this regression:

  https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/

The first error is:

  [   14.757489] Internal error: synchronous external abort: 96000210
[#1] PREEMPT SMP


Looks like yet another clock ordering setup. I guess different Amlogic
platforms have slightly different ordering requirements.

Neil, do you have any idea of which platform requires which ordering?
The variability in DT and platforms is pretty difficult to follow (and
I don't think I have such board around).

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] drm/meson: Module removal fixes

[Marc Zyngier]

Hi Neil,

On 2020-11-17 08:49, Neil Armstrong wrote:

Hi Marc,

On 16/11/2020 21:07, Marc Zyngier wrote:

Hi all,

Having recently moved over to a top-of-the-tree u-boot on one of my
VIM3L systems in order to benefit from unrelated improvements
(automatic PCIe detection, EFI...), I faced the issue that my kernel
would hang like this:

[  OK  ] Finished Helper to synchronize boot up for ifupdown.
[  OK  ] Started Rule-based Manager for Device Events and Files.
[7.114516] VDDCPU: supplied by regulator-dummy
[  OK  ] Found device /dev/ttyAML0.
[7.146862] meson-drm ff90.vpu: Queued 2 outputs on vpu
[7.169630] fb0: switching to meson-drm-fb from simple
[7.169944] Console: switching to colour dummy device 80x25
[7.179250] meson-drm ff90.vpu: CVBS Output connector not 
available


and that's it.

After some poking around, I figured out that it is in the
meson-dw-hdmi module that the CPU was hanging...


I'll be interested in having your kernel config, I never had such 
report

since I enabled HDMI support in U-Boot a few years ago.


Yeah, I was pretty surprised too. I have a hunch that this is caused
by u-boot DT exposing an extra MMIO region (dubbed "hhi") that gets
picked up by the kernel driver. *Not* having the region in the DT
(as in the kernel's version of the same DT) makes the driver work
exactly once:

Decompiled u-boot DT:

hdmi-tx@0 {
compatible = "amlogic,meson-g12a-dw-hdmi";
reg = <0x00 0x00 0x00 0x1 0x00 0x3c000 0x00 0x1000>;
[...]
reg-names = "hdmitx\0hhi";

Decompiled kernel DT:

hdmi-tx@0 {
compatible = "amlogic,meson-g12a-dw-hdmi";
reg = <0x00 0x00 0x00 0x1>;

There seem to be some complex interactions between the HDMI driver
and the DRM driver, both using this MMIO region at any given time.
But I admit not having tried very hard to follow the DRM maze of
intricate callbacks. All I needed was this box to reliably boot with
the firmware-provided DT.

You can find a reasonably recent version of my config at [1].

M.

[1] http://www.loen.fr/tmp/Config.full-arm64
--
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/meson: Unbind all connectors on module removal

[Marc Zyngier]

Removing the meson DRM module results in the following splats:

[   42.689228] WARNING: CPU: 0 PID: 572 at drivers/gpu/drm/drm_irq.c:192 
drm_irq_uninstall+0x130/0x160 [drm]
[...]
[   42.812820] Hardware name:  , BIOS 2021.01-rc2-00012-gde865f7ee1 11/16/2020
[   42.819723] pstate: 80400089 (Nzcv daIf +PAN -UAO -TCO BTYPE=--)
[   42.825737] pc : drm_irq_uninstall+0x130/0x160 [drm]
[   42.830647] lr : drm_irq_uninstall+0xc4/0x160 [drm]
[...]
[   42.917614] Call trace:
[   42.920086]  drm_irq_uninstall+0x130/0x160 [drm]
[   42.924612]  meson_drv_unbind+0x68/0xa4 [meson_drm]
[   42.929436]  component_del+0xc0/0x180
[   42.933058]  meson_dw_hdmi_remove+0x28/0x40 [meson_dw_hdmi]
[   42.938576]  platform_drv_remove+0x38/0x60
[   42.942628]  __device_release_driver+0x190/0x23c
[   42.947198]  driver_detach+0xcc/0x160
[   42.950822]  bus_remove_driver+0x68/0xe0
[   42.954702]  driver_unregister+0x3c/0x6c
[   42.958583]  platform_driver_unregister+0x20/0x2c
[   42.963243]  meson_dw_hdmi_platform_driver_exit+0x18/0x4a8 [meson_dw_hdmi]
[   42.970057]  __arm64_sys_delete_module+0x1bc/0x294
[   42.974801]  el0_svc_common.constprop.0+0x80/0x240
[   42.979542]  do_el0_svc+0x30/0xa0
[   42.982821]  el0_svc+0x18/0x50
[   42.985839]  el0_sync_handler+0x198/0x404
[   42.989806]  el0_sync+0x158/0x180

immediatelly followed by

[   43.002296] WARNING: CPU: 0 PID: 572 at 
drivers/gpu/drm/drm_mode_config.c:504 drm_mode_config_cleanup+0x2a8/0x304 [drm]
[...]
[   43.128150] Hardware name:  , BIOS 2021.01-rc2-00012-gde865f7ee1 11/16/2020
[   43.135052] pstate: 8049 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
[   43.141062] pc : drm_mode_config_cleanup+0x2a8/0x304 [drm]
[   43.146492] lr : drm_mode_config_cleanup+0xac/0x304 [drm]
[...]
[   43.233979] Call trace:
[   43.236451]  drm_mode_config_cleanup+0x2a8/0x304 [drm]
[   43.241538]  drm_mode_config_init_release+0x1c/0x2c [drm]
[   43.246886]  drm_managed_release+0xa8/0x120 [drm]
[   43.251543]  drm_dev_put+0x94/0xc0 [drm]
[   43.255380]  meson_drv_unbind+0x78/0xa4 [meson_drm]
[   43.260204]  component_del+0xc0/0x180
[   43.263829]  meson_dw_hdmi_remove+0x28/0x40 [meson_dw_hdmi]
[   43.269344]  platform_drv_remove+0x38/0x60
[   43.273398]  __device_release_driver+0x190/0x23c
[   43.277967]  driver_detach+0xcc/0x160
[   43.281590]  bus_remove_driver+0x68/0xe0
[   43.285471]  driver_unregister+0x3c/0x6c
[   43.289352]  platform_driver_unregister+0x20/0x2c
[   43.294011]  meson_dw_hdmi_platform_driver_exit+0x18/0x4a8 [meson_dw_hdmi]
[   43.300826]  __arm64_sys_delete_module+0x1bc/0x294
[   43.305570]  el0_svc_common.constprop.0+0x80/0x240
[   43.310312]  do_el0_svc+0x30/0xa0
[   43.313590]  el0_svc+0x18/0x50
[   43.316608]  el0_sync_handler+0x198/0x404
[   43.320574]  el0_sync+0x158/0x180
[   43.323852] ---[ end trace d796a3072dab01da ]---
[   43.328561] [drm:drm_mode_config_cleanup [drm]] *ERROR* connector HDMI-A-1 
leaked!

both triggered by the fact that the HDMI subsystem is still active,
and the DRM removal doesn't result in the connectors being torn down.

Call drm_atomic_helper_shutdown() and component_unbind_all() to safely
tear the module down.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index 324fa489f1c4..3d1de9cbb1c8 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -390,8 +390,10 @@ static void meson_drv_unbind(struct device *dev)
}
 
drm_dev_unregister(drm);
-   drm_irq_uninstall(drm);
drm_kms_helper_poll_fini(drm);
+   drm_atomic_helper_shutdown(drm);
+   component_unbind_all(dev, drm);
+   drm_irq_uninstall(drm);
drm_dev_put(drm);
 
if (priv->afbcd.ops) {
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/meson: dw-hdmi: Register a callback to disable the regulator

[Marc Zyngier]

Removing the meson-dw-hdmi module results in the following splat:

i[   43.340509] WARNING: CPU: 0 PID: 572 at drivers/regulator/core.c:2125 
_regulator_put.part.0+0x16c/0x174
[...]
[   43.454870] CPU: 0 PID: 572 Comm: modprobe Tainted: GW   E 
5.10.0-rc4-00049-gd274813a4de3-dirty #2147
[   43.465042] Hardware name:  , BIOS 2021.01-rc2-00012-gde865f7ee1 11/16/2020
[   43.471945] pstate: 8049 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
[   43.477896] pc : _regulator_put.part.0+0x16c/0x174
[   43.482638] lr : regulator_put+0x44/0x60
[...]
[   43.568715] Call trace:
[   43.571132]  _regulator_put.part.0+0x16c/0x174
[   43.575529]  regulator_put+0x44/0x60
[   43.579067]  devm_regulator_release+0x20/0x2c
[   43.583380]  release_nodes+0x1c8/0x2b4
[   43.587087]  devres_release_all+0x44/0x6c
[   43.591056]  __device_release_driver+0x1a0/0x23c
[   43.595626]  driver_detach+0xcc/0x160
[   43.599249]  bus_remove_driver+0x68/0xe0
[   43.603130]  driver_unregister+0x3c/0x6c
[   43.607011]  platform_driver_unregister+0x20/0x2c
[   43.611678]  meson_dw_hdmi_platform_driver_exit+0x18/0x4a8 [meson_dw_hdmi]
[   43.618485]  __arm64_sys_delete_module+0x1bc/0x294

as the HDMI regulator is still enabled on release.

In order to address this, register a callback that will deal with
the disabling when the driver is unbound, solving the problem.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 29a8ff41595d..68826cf9993f 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -941,6 +941,11 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi 
*meson_dw_hdmi)
 
 }
 
+static void meson_disable_regulator(void *data)
+{
+   regulator_disable(data);
+}
+
 static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
void *data)
 {
@@ -989,6 +994,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
ret = regulator_enable(meson_dw_hdmi->hdmi_supply);
if (ret)
return ret;
+   ret = devm_add_action_or_reset(dev, meson_disable_regulator,
+  meson_dw_hdmi->hdmi_supply);
+   if (ret)
+   return ret;
}
 
meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/4] drm/meson: Module removal fixes

[Marc Zyngier]

Hi all,

Having recently moved over to a top-of-the-tree u-boot on one of my
VIM3L systems in order to benefit from unrelated improvements
(automatic PCIe detection, EFI...), I faced the issue that my kernel
would hang like this:

[  OK  ] Finished Helper to synchronize boot up for ifupdown.
[  OK  ] Started Rule-based Manager for Device Events and Files.
[7.114516] VDDCPU: supplied by regulator-dummy
[  OK  ] Found device /dev/ttyAML0.
[7.146862] meson-drm ff90.vpu: Queued 2 outputs on vpu
[7.169630] fb0: switching to meson-drm-fb from simple
[7.169944] Console: switching to colour dummy device 80x25
[7.179250] meson-drm ff90.vpu: CVBS Output connector not available

and that's it.

After some poking around, I figured out that it is in the
meson-dw-hdmi module that the CPU was hanging...

Reverting to the kernel DT instead of u-boot's papered over it
somehow, but it turned out that removing the module (modprobe -r)
resulted in a firework. And for every issue I was fixing, another
followed. Much fun for a rainy Monday in the basement!

I ended up with the following 4 patches, which solve all my problems:
I can now boot with the u-boot provided DT, and the hdmi and DRM
drivers can be removed and re-inserted at will.

The first patch is a straightforward use-after-free, causing a NULL
pointer dereference. Moving things around fixes it.

The second patch shows that I have no clue about the DRM subsystem
whatsoever. I mimicked what my Rockchip systems are doing, and the two
warnings disappeared. It can't completely be wrong (famous last
words...).

The third patch fixes a *very* common issue with regulators (I've
fixed at least 3 drivers with a similar issue). I guess the devm
subsystem needs to grow a new helper at some point.

The last patch finally fixes the issue I was seeing: the HDMI driver
hangs when accessing a register with clocks disabled, which they are
on module removal. It also fixes my u-boot booting for similar
reasons, I guess.

I went as far as reaching out for a HDMI cable and verifying that I
was getting a working display. Total dedication.

Feedback much appreciated.

M.

Marc Zyngier (4):
  drm/meson: Free RDMA resources after tearing down DRM
  drm/meson: Unbind all connectors on module removal
  drm/meson: dw-hdmi: Register a callback to disable the regulator
  drm/meson: dw-hdmi: Ensure that clocks are enabled before touching the
TOP registers

 drivers/gpu/drm/meson/meson_drv.c | 12 +++-
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 13 +++--
 2 files changed, 18 insertions(+), 7 deletions(-)

-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/meson: dw-hdmi: Ensure that clocks are enabled before touching the TOP registers

[Marc Zyngier]

Removing the meson-dw-hdmi module and re-inserting it results in a hang
as the driver writes to HDMITX_TOP_SW_RESET. Similar effects can be seen
when booting with mainline u-boot and using the u-boot provided DT (which
is highly desirable).

The reason for the hang seem to be that the clocks are not always
enabled by the time we enter meson_dw_hdmi_init(). Moving this call
*after* dw_hdmi_probe() ensures that the clocks are enabled.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_dw_hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 68826cf9993f..7f8eea494147 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -1073,8 +1073,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
 
DRM_DEBUG_DRIVER("encoder initialized\n");
 
-   meson_dw_hdmi_init(meson_dw_hdmi);
-
/* Bridge / Connector */
 
dw_plat_data->priv_data = meson_dw_hdmi;
@@ -1097,6 +1095,8 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
if (IS_ERR(meson_dw_hdmi->hdmi))
return PTR_ERR(meson_dw_hdmi->hdmi);
 
+   meson_dw_hdmi_init(meson_dw_hdmi);
+
next_bridge = of_drm_find_bridge(pdev->dev.of_node);
if (next_bridge)
drm_bridge_attach(encoder, next_bridge,
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/meson: Free RDMA resources after tearing down DRM

[Marc Zyngier]

Removing the meson DRM module results in the following splat:

[ 2179.451346] Hardware name:  , BIOS 2021.01-rc2-00012-gde865f7ee1 11/16/2020
[ 2179.458316] Workqueue: events drm_mode_rmfb_work_fn [drm]
[ 2179.463597] pstate: 80c9 (Nzcv daif +PAN +UAO -TCO BTYPE=--)
[ 2179.469558] pc : meson_rdma_writel_sync+0x44/0xb0 [meson_drm]
[ 2179.475243] lr : meson_g12a_afbcd_reset+0x34/0x60 [meson_drm]
[ 2179.480930] sp : ffc01212bb70
[ 2179.484207] x29: ffc01212bb70 x28: ff8044f66f00
[ 2179.489469] x27: ff8045b13800 x26: 0001
[ 2179.494730] x25:  x24: 0001
[ 2179.41] x23:  x22: 
[ 2179.505252] x21: 0028 x20: 1a01
[ 2179.510513] x19: ff8046029480 x18: 
[ 2179.515775] x17:  x16: 
[ 2179.521036] x15:  x14: 
[ 2179.526297] x13: 00400326 x12: 0309030303260300
[ 2179.531558] x11: 0300054004a0 x10: 0418054004000400
[ 2179.536820] x9 : ffc008fe4914 x8 : ff8040a1adc0
[ 2179.542081] x7 :  x6 : ff8042aa0080
[ 2179.547342] x5 : ff8044f66f00 x4 : ffc008fe5bc8
[ 2179.552603] x3 : 00010101 x2 : 0001
[ 2179.557865] x1 :  x0 : 
[ 2179.563127] Call trace:
[ 2179.565548]  meson_rdma_writel_sync+0x44/0xb0 [meson_drm]
[ 2179.570894]  meson_g12a_afbcd_reset+0x34/0x60 [meson_drm]
[ 2179.576241]  meson_plane_atomic_disable+0x38/0xb0 [meson_drm]
[ 2179.581966]  drm_atomic_helper_commit_planes+0x1e0/0x21c [drm_kms_helper]
[ 2179.588684]  drm_atomic_helper_commit_tail_rpm+0x68/0xb0 [drm_kms_helper]
[ 2179.595410]  commit_tail+0xac/0x190 [drm_kms_helper]
[ 2179.600326]  drm_atomic_helper_commit+0x16c/0x390 [drm_kms_helper]
[ 2179.606484]  drm_atomic_commit+0x58/0x70 [drm]
[ 2179.610880]  drm_framebuffer_remove+0x398/0x434 [drm]
[ 2179.615881]  drm_mode_rmfb_work_fn+0x68/0x8c [drm]
[ 2179.620575]  process_one_work+0x1cc/0x49c
[ 2179.624538]  worker_thread+0x200/0x444
[ 2179.628246]  kthread+0x14c/0x160
[ 2179.631439]  ret_from_fork+0x10/0x38

caused by the fact that the RDMA buffer has already been freed,
resulting in meson_rdma_writel_sync() getting a NULL pointer.

Move the afbcd reset and meson_rdma_free calls after the DRM
unregistration is complete so that the teardown can safely complete.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/meson/meson_drv.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index 8b9c8dd788c4..324fa489f1c4 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -389,15 +389,15 @@ static void meson_drv_unbind(struct device *dev)
meson_canvas_free(priv->canvas, priv->canvas_id_vd1_2);
}
 
-   if (priv->afbcd.ops) {
-   priv->afbcd.ops->reset(priv);
-   meson_rdma_free(priv);
-   }
-
drm_dev_unregister(drm);
drm_irq_uninstall(drm);
drm_kms_helper_poll_fini(drm);
drm_dev_put(drm);
+
+   if (priv->afbcd.ops) {
+   priv->afbcd.ops->reset(priv);
+   meson_rdma_free(priv);
+   }
 }
 
 static const struct component_master_ops meson_drv_master_ops = {
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/tegra: sor: Ensure regulators are disabled on teardown

[Marc Zyngier]

The Tegra SOR driver uses the devm infrastructure to request regulators,
but enables them without registering them with the infrastructure.

This results in the following splat if probing fails for any odd resaon
(such as dependencies not being available):

[8.974187] tegra-sor 1558.sor: cannot get HDMI supply: -517
[9.414403] tegra-sor 1558.sor: failed to probe HDMI: -517
[9.421240] [ cut here ]
[9.425879] WARNING: CPU: 1 PID: 164 at drivers/regulator/core.c:2089 
_regulator_put.part.0+0x16c/0x174
[9.435259] Modules linked in: tegra_drm(E+) cec(E) ahci_tegra(E) 
drm_kms_helper(E) drm(E) libahci_platform(E) libahci(E) max77620_regulator(E) 
xhci_tegra(E+) sdhci_tegra(E) xhci_hcd(E) libata(E) sdhci_pltfm(E) cqhci(E) 
fixed(E) usbcore(E) scsi_mod(E) sdhci(E) host1x(E)
[9.459211] CPU: 1 PID: 164 Comm: systemd-udevd Tainted: G SD W   E 
5.9.0-rc7-00298-gf6337624c4fe #1980
[9.469285] Hardware name: NVIDIA Jetson TX2 Developer Kit (DT)
[9.475202] pstate: 8005 (Nzcv daif -PAN -UAO BTYPE=--)
[9.480784] pc : _regulator_put.part.0+0x16c/0x174
[9.485581] lr : regulator_put+0x44/0x60
[9.489501] sp : ffc011d837b0
[9.492814] x29: ffc011d837b0 x28: ff81dd085900
[9.498141] x27: ff81de1c8ec0 x26: ff81de1c8c10
[9.503464] x25: ff81dd085800 x24: ffc008f2c6b0
[9.508790] x23: ffc0117373f0 x22: 0005
[9.514101] x21: ff81dd085900 x20: ffc01172b098
[9.515822] ata1: SATA link down (SStatus 0 SControl 300)
[9.519426] x19: ff81dd085100 x18: 0030
[9.530122] x17:  x16: 
[9.535453] x15:  x14: 038f
[9.540777] x13: 0003 x12: 0040
[9.546105] x11: ff81eb80 x10: 0ae0
[9.551417] x9 : ffc0106fea24 x8 : ff81de83e6c0
[9.556728] x7 : 0018 x6 : 03c3
[9.562064] x5 : 5660 x4 : 
[9.567392] x3 : ffc01172b388 x2 : ff81de83db80
[9.572702] x1 :  x0 : 0001
[9.578034] Call trace:
[9.580494]  _regulator_put.part.0+0x16c/0x174
[9.584940]  regulator_put+0x44/0x60
[9.588522]  devm_regulator_release+0x20/0x2c
[9.592885]  release_nodes+0x1c8/0x2c0
[9.596636]  devres_release_all+0x44/0x6c
[9.600649]  really_probe+0x1ec/0x504
[9.604316]  driver_probe_device+0x100/0x170
[9.608589]  device_driver_attach+0xcc/0xd4
[9.612774]  __driver_attach+0xb0/0x17c
[9.616614]  bus_for_each_dev+0x7c/0xd4
[9.620450]  driver_attach+0x30/0x3c
[9.624027]  bus_add_driver+0x154/0x250
[9.627867]  driver_register+0x84/0x140
[9.631719]  __platform_register_drivers+0xa0/0x180
[9.636660]  host1x_drm_init+0x60/0x1000 [tegra_drm]
[9.641629]  do_one_initcall+0x54/0x2d0
[9.645490]  do_init_module+0x68/0x29c
[9.649244]  load_module+0x2178/0x26c0
[9.652997]  __do_sys_finit_module+0xb0/0x120
[9.657356]  __arm64_sys_finit_module+0x2c/0x40
[9.661902]  el0_svc_common.constprop.0+0x80/0x240
[9.666701]  do_el0_svc+0x30/0xa0
[9.670022]  el0_svc+0x18/0x50
[9.673081]  el0_sync_handler+0x90/0x318
[9.677006]  el0_sync+0x158/0x180
[9.680324] ---[ end trace 90f6c89d62d85ff6 ]---

Instead, let's register a callback that will disable the regulators
on teardown. This allows for the removal of the .remove callbacks,
which are not needed anymore.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/tegra/sor.c | 59 +++--
 1 file changed, 24 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 45b5258c77a2..39e6b32f6c10 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -397,7 +397,6 @@ struct tegra_sor;
 struct tegra_sor_ops {
const char *name;
int (*probe)(struct tegra_sor *sor);
-   int (*remove)(struct tegra_sor *sor);
void (*audio_enable)(struct tegra_sor *sor);
void (*audio_disable)(struct tegra_sor *sor);
 };
@@ -2942,6 +2941,24 @@ static const struct drm_encoder_helper_funcs 
tegra_sor_dp_helpers = {
.atomic_check = tegra_sor_encoder_atomic_check,
 };
 
+static void tegra_sor_disable_regulator(void *data)
+{
+   struct regulator *reg = data;
+
+   regulator_disable(reg);
+}
+
+static int tegra_sor_enable_regulator(struct tegra_sor *sor, struct regulator 
*reg)
+{
+   int err;
+
+   err = regulator_enable(reg);
+   if (err)
+   return err;
+
+   return devm_add_action_or_reset(sor->dev, tegra_sor_disable_regulator, 
reg);
+}
+
 static int tegra_sor_hdmi_probe(struct tegra_sor *sor)
 {
int err;
@@ -2953,7 +2970,7 @@ static int tegra_sor_hdmi_probe(struct tegra_sor *sor)
return PTR_ERR(sor->avdd_io_supply);
}
 
-   err = regulator_enable(sor->avdd_

Re: [PATCH 03/35] docs: fix broken references to text files

[Marc Zyngier]

On Wed,  8 Apr 2020 17:45:55 +0200
Mauro Carvalho Chehab  wrote:

> Several references got broken due to txt to ReST conversion.
> 
> Several of them can be automatically fixed with:
> 
>   scripts/documentation-file-ref-check --fix
> 
> Reviewed-by: Mathieu Poirier  # 
> hwtracing/coresight/Kconfig
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  Documentation/virt/kvm/arm/pvtime.rst|  2 +-
>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  2 +-
>  virt/kvm/arm/vgic/vgic.h         |  4 ++--

Acked-by: Marc Zyngier  # kvm/arm64

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/rockchip: shutdown drm subsystem on shutdown

[Marc Zyngier]

Hi Heiko,

On 02/04/2019 12:37, Heiko Stuebner wrote:
> From: Vicente Bergas 
> 
> As explained by Robin Murphy:
>> the IOMMU shutdown disables paging, so if the VOP is still
>> scanning out then that will result in whatever IOVAs it was using now going
>> straight out onto the bus as physical addresses.
> 
> We had a more radical approach before in commit
> 7f3ef5dedb14 ("drm/rockchip: Allow driver to be shutdown on reboot/kexec")
> but that resulted in new warnings and oopses on shutdown on rk3399
> chromeos devices.
> 
> So second try is resurrecting Vicentes shutdown change which should
> achieve the same result but in a less drastic way.
> 
> Fixes: 63238173b2fa ("Revert drm/rockchip: Allow driver to be shutdown on 
> reboot/kexec")
> Cc: Jeffy Chen 
> Cc: Robin Murphy 
> Cc: Marc Zyngier 
> Cc: Brian Norris 
> Cc: Doug Anderson 
> Cc: sta...@vger.kernel.org
> Suggested-by: JeffyChen 
> Suggested-by: Robin Murphy 
> Signed-off-by: Vicente Bergas 
> [adapted commit message to explain the history]
> Signed-off-by: Heiko Stuebner 

Sorry it took so long to test this. I've just given it a go on kevin,
and managed to kexec into a secondary kernel. So FWIW:

Acked-by: Marc Zyngier 

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: Allow driver to be shutdown on reboot/kexec

[Marc Zyngier]

Hi all,

On 05/12/2018 14:11, Heiko Stübner wrote:
> Hi Brian,
> 
> Am Mittwoch, 5. Dezember 2018, 04:01:34 CET schrieb Brian Norris:
>> + others
>>
>> Hi,
>>
>> On Sun, Aug 05, 2018 at 01:48:07PM +0100, Marc Zyngier wrote:
>>> Leaving the DRM driver enabled on reboot or kexec has the annoying
>>> effect of leaving the display generating transactions whilst the
>>> IOMMU has been shut down.
>>>
>>> In turn, the IOMMU driver (which shares its interrupt line with
>>> the VOP) starts warning either on shutdown or when entering the
>>> secondary kernel in the kexec case (nothing is expected on that
>>> front).
>>>
>>> A cheap way of ensuring that things are nicely shut down is to
>>> register a shutdown callback in the platform driver.
>>>
>>> Signed-off-by: Marc Zyngier 
>>> ---
>>
>> This patch made it into 4.20-rc1 as well as -stable, and it has caused
>> regressions for me, on the Kevin and Scarlet [1] RK3399 platforms.
>>
>> On
>> shutdown/reboot, I see this:
>>
>> [   94.742559] WARNING: CPU: 4 PID: 2035 at
>> drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x1c4/0x294
>> ...
>> [   94.775904] CPU: 4 PID: 2035 Comm: reboot Tainted: GW
>> 4.20.0-rc5+ #83 [   94.784651] Hardware name: Google Scarlet (DT)
>> [   94.789611] pstate: 2005 (nzCv daif -PAN -UAO)
>> [   94.794959] pc : drm_mode_config_cleanup+0x1c4/0x294
>> [   94.800500] lr : drm_mode_config_cleanup+0x108/0x294
>> ...
>> [   94.898683] Call trace:
>> [   94.901410]  drm_mode_config_cleanup+0x1c4/0x294
>> [   94.906565]  rockchip_drm_unbind+0x4c/0x8c
>> [   94.911138]  component_master_del+0x88/0xb8
>> [   94.915807]  rockchip_drm_platform_remove+0x2c/0x44
>> [   94.921243]  rockchip_drm_platform_shutdown+0x20/0x2c
>> [   94.926881]  platform_drv_shutdown+0x2c/0x38
>> [   94.931647]  device_shutdown+0x164/0x1b8
>> [   94.936016]  kernel_restart_prepare+0x40/0x48
>> [   94.940878]  kernel_restart+0x20/0x68
>> [   94.944964]  __se_sys_reboot+0x1ac/0x204
>> [   94.949331]  __arm64_sys_reboot+0x2c/0x38
>> [   94.953806]  el0_svc_common+0xa4/0xec
>> [   94.957891]  el0_svc_compat_handler+0x30/0x3c
>> [   94.962753]  el0_svc_compat+0x8/0x18
>> [   94.966740] ---[ end trace b9ba2e701f4fb233 ]---
>> [   95.255169] Memory manager not clean during takedown.
>> [   95.260824] WARNING: CPU: 4 PID: 2035 at drivers/gpu/drm/drm_mm.c:950
>> drm_mm_takedown+0x34/0x44 ...
>> [   95.292314] CPU: 4 PID: 2035 Comm: reboot Tainted: GW
>> 4.20.0-rc5+ #83 [   95.301061] Hardware name: Google Scarlet (DT)
>> [   95.306020] pstate: 6005 (nZCv daif -PAN -UAO)
>> [   95.311369] pc : drm_mm_takedown+0x34/0x44
>> [   95.315940] lr : drm_mm_takedown+0x34/0x44
>> ...
>> [   95.415857]  drm_mm_takedown+0x34/0x44
>> [   95.420042]  rockchip_drm_unbind+0x64/0x8c
>> [   95.424613]  component_master_del+0x88/0xb8
>> [   95.429283]  rockchip_drm_platform_remove+0x2c/0x44
>> [   95.434728]  rockchip_drm_platform_shutdown+0x20/0x2c
>> [   95.440360]  platform_drv_shutdown+0x2c/0x38
>> [   95.445127]  device_shutdown+0x164/0x1b8
>> [   95.449504]  kernel_restart_prepare+0x40/0x48
>> [   95.454358]  kernel_restart+0x20/0x68
>> [   95.458436]  __se_sys_reboot+0x1ac/0x204
>> [   95.462812]  __arm64_sys_reboot+0x2c/0x38
>> [   95.467287]  el0_svc_common+0xa4/0xec
>> [   95.471373]  el0_svc_compat_handler+0x30/0x3c
>> [   95.476235]  el0_svc_compat+0x8/0x18
>> [   95.480215] ---[ end trace b9ba2e701f4fb234 ]---
>>
>> It's especially bad on -stable kernels, where I believe the remove()
>> paths were even worse. This triggers a variety of OOPSes, and it's not
>> clear if those are simply because of backports (e.g., RK3399 did not
>> have support in 4.4.y, but our downstream has merged all sorts of
>> backports to make it work).
>>
>> Anyway, the above warnings occur on v4.20-rc, which I think is
>> justification enough for a revert.
> 
> That's strange. I remember testing quite a number of shutdown/reboot
> cycles before applying that patch. And for good measure did the same
> again right now.
> 
> - Kevin, with netboot firmware, booting into Debian+console only
> - Bob, with stock firmware, booting into Debian+KDE Plasma
> - Scarlet, with stock firmware, booting into Debian+KDE Plasma
> 
> With some random number of reboot and shutdowns on each I didn't
> see any warnings at all.

And I've been using this very patch for quite a while now.

Before suggesting a 

[PATCH] drm/bridge: analogix_dp: Downgrade "Link Training" messages to dev_dbg

[Marc Zyngier]

The Analogix DP bridge driver is pretty verbose, and outputs
things like

[  619.414067] rockchip-dp ff97.edp: Link Training Clock Recovery success
[  619.429233] rockchip-dp ff97.edp: Link Training success!

each time the display gets unblanked. While it is good to know
that the device is behaving correctly, users already know that
because they can see some video output.

Let's keep these messages for cases where we need to actually
debug the driver (we have dynamic debug to enable them at runtime
if need be), and let's keep the kernel quiet otherwise.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 2bcbfadb6ac5..0d715a375ff9 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -554,7 +554,7 @@ static int analogix_dp_process_clock_recovery(struct 
analogix_dp_device *dp)
if (retval < 0)
return retval;
 
-   dev_info(dp->dev, "Link Training Clock Recovery success\n");
+   dev_dbg(dp->dev, "Link Training Clock Recovery success\n");
dp->link_train.lt_state = EQUALIZER_TRAINING;
} else {
for (lane = 0; lane < lane_count; lane++) {
@@ -634,7 +634,7 @@ static int analogix_dp_process_equalizer_training(struct 
analogix_dp_device *dp)
if (retval < 0)
return retval;
 
-   dev_info(dp->dev, "Link Training success!\n");
+   dev_dbg(dp->dev, "Link Training success!\n");
analogix_dp_get_link_bandwidth(dp, );
dp->link_train.link_rate = reg;
dev_dbg(dp->dev, "final bandwidth = %.2x\n",
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: Allow driver to be shutdown on reboot/kexec

[Marc Zyngier]

Leaving the DRM driver enabled on reboot or kexec has the annoying
effect of leaving the display generating transactions whilst the
IOMMU has been shut down.

In turn, the IOMMU driver (which shares its interrupt line with
the VOP) starts warning either on shutdown or when entering the
secondary kernel in the kexec case (nothing is expected on that
front).

A cheap way of ensuring that things are nicely shut down is to
register a shutdown callback in the platform driver.

Signed-off-by: Marc Zyngier 
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f814d37b1db2..05368fa4f956 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -442,6 +442,11 @@ static int rockchip_drm_platform_remove(struct 
platform_device *pdev)
return 0;
 }
 
+static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
+{
+   rockchip_drm_platform_remove(pdev);
+}
+
 static const struct of_device_id rockchip_drm_dt_ids[] = {
{ .compatible = "rockchip,display-subsystem", },
{ /* sentinel */ },
@@ -451,6 +456,7 @@ MODULE_DEVICE_TABLE(of, rockchip_drm_dt_ids);
 static struct platform_driver rockchip_drm_platform_driver = {
.probe = rockchip_drm_platform_probe,
.remove = rockchip_drm_platform_remove,
+   .shutdown = rockchip_drm_platform_shutdown,
.driver = {
.name = "rockchip-drm",
.of_match_table = rockchip_drm_dt_ids,
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

On 18/06/18 09:19, Heiko Stübner wrote:
> Hi Marc,
> 
> Am Mittwoch, 13. Juni 2018, 15:01:27 CEST schrieb Marc Zyngier:
>> On 12/06/18 14:20, Heiko Stuebner wrote:
>>> From: Sandy Huang 
>>>
>>> The vop irq is shared between vop and iommu and irq probing in the
>>> iommu driver moved to the probe function recently. This can in some
>>> cases lead to a stall if the irq is triggered while the vop driver
>>> still has it disabled, but the vop irq handler gets called.
>>>
>>> But there is no real need to disable the irq, as the vop can simply
>>> also track its enabled state and ignore irqs in that case.
>>> For this we can simply check the power-domain state of the vop,
>>> similar to how the iommu driver does it.
>>>
>>> So remove the enable/disable handling and add appropriate condition
>>> to the irq handler.
>>>
>>> changes in v2:
>>> - move to just check the power-domain state
>>> - add clock handling
>>> changes in v3:
>>> - clarify comment to speak of runtime-pm not power-domain
>>> changes in v4:
>>> - address Marc's comments (clk-enable WARN_ON and style improvement)
>>>
>>> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Sandy Huang 
>>> Signed-off-by: Heiko Stuebner 
>>> Tested-by: Ezequiel Garcia 
>>
>> Reviewed-by: Marc Zyngier 
> 
> could I ask you to also look at patch1 of this series, to give it an
> Ack or Review? drm-misc documentation very strongly suggests [0]
> to have at least another set of eyes on a patch and so far noone
> came forward ;-)
> 
> This of course also applies to everybody else in the Cc list :-D

Please feel free to apply my

Acked-by: Marc Zyngier 

to that one.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

On 12/06/18 14:20, Heiko Stuebner wrote:
> From: Sandy Huang 
> 
> The vop irq is shared between vop and iommu and irq probing in the
> iommu driver moved to the probe function recently. This can in some
> cases lead to a stall if the irq is triggered while the vop driver
> still has it disabled, but the vop irq handler gets called.
> 
> But there is no real need to disable the irq, as the vop can simply
> also track its enabled state and ignore irqs in that case.
> For this we can simply check the power-domain state of the vop,
> similar to how the iommu driver does it.
> 
> So remove the enable/disable handling and add appropriate condition
> to the irq handler.
> 
> changes in v2:
> - move to just check the power-domain state
> - add clock handling
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
> changes in v4:
> - address Marc's comments (clk-enable WARN_ON and style improvement)
> 
> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sandy Huang 
> Signed-off-by: Heiko Stuebner 
> Tested-by: Ezequiel Garcia 

Reviewed-by: Marc Zyngier 

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

Hi Heiko,

On 12/06/18 13:15, Heiko Stuebner wrote:
> From: Sandy Huang 
> 
> The vop irq is shared between vop and iommu and irq probing in the
> iommu driver moved to the probe function recently. This can in some
> cases lead to a stall if the irq is triggered while the vop driver
> still has it disabled, but the vop irq handler gets called.
> 
> But there is no real need to disable the irq, as the vop can simply
> also track its enabled state and ignore irqs in that case.
> For this we can simply check the power-domain state of the vop,
> similar to how the iommu driver does it.
> 
> So remove the enable/disable handling and add appropriate condition
> to the irq handler.
> 
> changes in v2:
> - move to just check the power-domain state
> - add clock handling
> changes in v3:
> - clarify comment to speak of runtime-pm not power-domain
> 
> Fixes: d0b912bd4c23 ("iommu/rockchip: Request irqs in rk_iommu_probe()")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sandy Huang 
> Signed-off-by: Heiko Stuebner 
> Tested-by: Ezequiel Garcia 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++---
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 9a1f272e41c7..ae8a69793aed 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
>  
>   spin_unlock(>reg_lock);
>  
> - enable_irq(vop->irq);
> -
>   drm_crtc_vblank_on(crtc);
>  
>   return 0;
> @@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>   vop_dsp_hold_valid_irq_disable(vop);
>  
> - disable_irq(vop->irq);
> -
>   vop->is_enabled = false;
>  
>   /*
> @@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
>   uint32_t active_irqs;
>   int ret = IRQ_NONE;
>  
> + /*
> +  * The irq is shared with the iommu. If the runtime-pm state of the
> +  * vop-device is disabled the irq has to be targetted at the iommu.
> +  */
> + if (!pm_runtime_get_if_in_use(vop->dev))
> + return IRQ_NONE;
> +
> + if (WARN_ON(vop_core_clks_enable(vop)))
> + goto out;

As I mentioned before, a WARN_ON() in an interrupt handler is a good way
to make a bad problem even worse, and will give information (full
register and stack dump) that is mostly useless to the context at hand.
Turning it to a dev_warn_ratelimited() (or DRM_ERROR_RATELIMITED if you
want to be DRM compliant) would be a better approach, IMHO.

> +
>   /*
>* interrupt register has interrupt status, enable and clear bits, we
>* must hold irq_lock to avoid a race with enable/disable_vblank().
> @@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
>   spin_unlock(>irq_lock);
>  
>   /* This is expected for vop iommu irqs, since the irq is shared */
> - if (!active_irqs)
> - return IRQ_NONE;
> + if (!active_irqs) {
> + ret = IRQ_NONE;
> + vop_core_clks_disable(vop);
> + goto out;
> + }

A couple of nits: ret is already set to IRQ_NONE at this stage, and you
could simply rewrite it as:

if (!active_irq)
goto out_disable;

>  
>   if (active_irqs & DSP_HOLD_VALID_INTR) {
>   complete(>dsp_hold_completion);
> @@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
>   DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
> active_irqs);
>  

with the "out_disable" label placed here.

> + vop_core_clks_disable(vop);
> +
> +out:
> + pm_runtime_put(vop->dev);
>   return ret;
>  }
>  
> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct device 
> *master, void *data)
>   if (ret)
>   goto err_disable_pm_runtime;
>  
> - /* IRQ is initially disabled; it gets enabled in power_on */
> - disable_irq(vop->irq);
> -
>   return 0;
>  
>  err_disable_pm_runtime:
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

Hi Heiko,

On 28/05/18 14:20, Heiko Stuebner wrote:
> From: Sandy Huang 
> 
> The vop irq is shared between vop and iommu and irq probing in the
> iommu driver moved to the probe function recently. This can in some
> cases lead to a stall if the irq is triggered while the vop driver
> still has it disabled, but the vop irq handler gets called.
> 
> But there is no real need to disable the irq, as the vop can simply
> also track its enabled state and ignore irqs in that case.
> For this we can simply check the power-domain state of the vop,
> similar to how the iommu driver does it.
> 
> So remove the enable/disable handling and add appropriate condition
> to the irq handler.
> 
> changes in v2:
> - move to just check the power-domain state
> - add clock handling
> 
> Signed-off-by: Sandy Huang 
> [add commit message, moved to pm_runtime_get_if_in_use]
> Signed-off-by: Heiko Stuebner 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++---
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index b55156b8ba3b..615a5b44bfe9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -573,8 +573,6 @@ static int vop_enable(struct drm_crtc *crtc)
>  
>   spin_unlock(>reg_lock);
>  
> - enable_irq(vop->irq);
> -
>   drm_crtc_vblank_on(crtc);
>  
>   return 0;
> @@ -618,8 +616,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>   vop_dsp_hold_valid_irq_disable(vop);
>  
> - disable_irq(vop->irq);
> -
>   vop->is_enabled = false;
>  
>   /*
> @@ -1195,6 +1191,16 @@ static irqreturn_t vop_isr(int irq, void *data)
>   uint32_t active_irqs;
>   int ret = IRQ_NONE;
>  
> + /*
> +  * The irq is shared with the iommu. If the power-domain is off
> +  * the irq has to be targetted at the iommu.
> +  */
> + if (!pm_runtime_get_if_in_use(vop->dev))
> + return IRQ_NONE;
> +
> + if (WARN_ON(vop_core_clks_enable(vop)))

WARN_ON() in an interrupt handler can be quite dangerous. Could you tone
it down a bit, and at least make it only fire once? Something like a
pr_warn_once should be enough (the stack trace is not very relevant, and
seeing it once is enough to know that something is wrong).

> + goto out;
> +
>   /*
>* interrupt register has interrupt status, enable and clear bits, we
>* must hold irq_lock to avoid a race with enable/disable_vblank().
> @@ -1209,8 +1215,11 @@ static irqreturn_t vop_isr(int irq, void *data)
>   spin_unlock(>irq_lock);
>  
>   /* This is expected for vop iommu irqs, since the irq is shared */
> - if (!active_irqs)
> - return IRQ_NONE;
> + if (!active_irqs) {
> + ret = IRQ_NONE;
> + vop_core_clks_disable(vop);
> + goto out;
> + }
>  
>   if (active_irqs & DSP_HOLD_VALID_INTR) {
>   complete(>dsp_hold_completion);
> @@ -1236,6 +1245,10 @@ static irqreturn_t vop_isr(int irq, void *data)
>   DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
> active_irqs);
>  
> + vop_core_clks_disable(vop);
> +
> +out:
> + pm_runtime_put(vop->dev);
>   return ret;
>  }
>  
> @@ -1614,9 +1627,6 @@ static int vop_bind(struct device *dev, struct device 
> *master, void *data)
>   if (ret)
>   goto err_disable_pm_runtime;
>  
> - /* IRQ is initially disabled; it gets enabled in power_on */
> - disable_irq(vop->irq);
> -
>   return 0;
>  
>  err_disable_pm_runtime:
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: vop: fix irq disabled after vop driver probed

[Marc Zyngier]

[Thanks Robin for pointing me to that patch.]

Hi Heiko,

On 24/05/18 23:06, Heiko Stübner wrote:
> From: Sandy Huang 
> 
> The vop irq is shared between vop and iommu and irq probing in the
> iommu driver moved to the probe function recently. This can in some
> cases lead to a stall if the irq is triggered while the vop driver
> still has it disabled.
> 
> But there is no real need to disable the irq, as the vop can simply
> also track its enabled state and ignore irqs in that case.
> 
> So remove the enable/disable handling and add appropriate condition
> to the irq handler.
> 
> Signed-off-by: Sandy Huang 
> [added an actual commit message]
> Signed-off-by: Heiko Stuebner 
> ---
> Hi Ezequiel,
> 
> this patch came from a discussion I had with Rockchip people over the
> iommu changes and resulting issues back in april, but somehow was
> forgotten and not posted to the lists. Correcting that now.
> 
> So removing the enable/disable voodoo on the shared interrupt is
> the preferred way.
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 ++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 510cdf0..61493d4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -549,8 +549,6 @@ static int vop_enable(struct drm_crtc *crtc)
>  
>   spin_unlock(>reg_lock);
>  
> - enable_irq(vop->irq);
> -
>   drm_crtc_vblank_on(crtc);
>  
>   return 0;
> @@ -596,8 +594,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>   vop_dsp_hold_valid_irq_disable(vop);
>  
> - disable_irq(vop->irq);
> -
>   vop->is_enabled = false;
>  
>   /*
> @@ -1168,6 +1164,13 @@ static irqreturn_t vop_isr(int irq, void *data)
>   int ret = IRQ_NONE;
>  
>   /*
> +  * since the irq is shared with iommu, iommu irq is enabled before vop
> +  * enable, so before vop enable we do nothing.
> +  */
> + if (!vop->is_enabled)
> + return IRQ_NONE;
> +

What guarantee do we have that the IOMMU will actually service that
interrupt? Can't we get into a situation where the interrupt gets
ignored by both drivers and subsequently disabled by the core irq code
as being spurious?

From what I understood of the way things are plugged together on the
rockchip platforms, each individual VOP is behind a single IOMMU, hence
there there is hardly any point in handling IOMMU interrupts if the VOP
is off.

But I'm missing the actual reason behind this patch. Could you enlighten me?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 05/43] drm/bridge: analogix_dp: Don't power bridge in analogix_dp_bind

[Marc Zyngier]

On 28/02/18 14:37, Heiko Stübner wrote:
> Am Dienstag, 30. Januar 2018, 21:28:35 CET schrieb Thierry Escande:
>> From: zain wang 
>>
>> The bridge does not need to be powered in analogix_dp_bind(), so
>> remove the calls to pm_runtime_get()/phy_power_on()/analogix_dp_init_dp()
>> as well as their power-off counterparts.
>>
>> Cc: Stéphane Marchesin 
>> Signed-off-by: zain wang 
>> Signed-off-by: Caesar Wang 
>> [the patch originally just removed the power_on portion, seanpaul removed
>> the power off code as well as improved the commit message]
>> Signed-off-by: Sean Paul 
>> Signed-off-by: Thierry Escande 
>> ---
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 10 --
>>  1 file changed, 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index
>> cb5e18d6ba04..1477ea9ba85d 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1382,11 +1382,6 @@ analogix_dp_bind(struct device *dev, struct
>> drm_device *drm_dev,
>>
>>  pm_runtime_enable(dev);
>>
>> -pm_runtime_get_sync(dev);
>> -phy_power_on(dp->phy);
>> -
>> -analogix_dp_init_dp(dp);
>> -
>>  ret = devm_request_threaded_irq(>dev, dp->irq,
>>  analogix_dp_hardirq,
>>  analogix_dp_irq_thread,
> 
> Not 100% sure here, as the driver has the request-irq + disable-irq hack
> here. So a pending interrupt could possibly fire between request and
> disable.
> 
> Right now the block should be on, but can it still handle such an irq
> when the power is removed?

Probably not (see below).

> So before removing the power here, we might want something
> similar to what Marc posted for the vop [0] for the analogix-dp?

You can do that trick only if the interrupt is not shared. In the VOP
case, it is shared with the IOMMU, which makes it more... interesting.

And when it comes to power and the analogix-dp driver, I've been
carrying this[1] for a while. Fully exploitable from userspace. I know
it is about to be replaced by this series, but at least 4.15 and 4.16
are affected.

M.

[1] https://www.spinics.net/lists/arm-kernel/msg623892.html
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/rockchip: Clear all interrupts before requesting the IRQ

[Marc Zyngier]

Calling request_irq() followed by disable_irq() is usually a bad idea,
specially if the interrupt can be pending, and you're not yet in a
position to handle it.

This is exactly what happens on my kevin system when rebooting in a
second kernel using kexec: Some interrupt is left pending from
the previous kernel, and we take it too early, before disable_irq()
could do anything.

Let's clear the pending interrupts as we initialize the HW, and move
the interrupt request after that point. This ensures that we're in
a sane state when the interrupt is requested.

Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ba7505292b78..7b224e08cbf1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1414,6 +1414,9 @@ static int vop_initial(struct vop *vop)
usleep_range(10, 20);
reset_control_deassert(ahb_rst);
 
+   VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1);
+   VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0);
+
memcpy(vop->regsbak, vop->regs, vop->len);
 
VOP_REG_SET(vop, misc, global_regdone_en, 1);
@@ -1569,17 +1572,9 @@ static int vop_bind(struct device *dev, struct device 
*master, void *data)
 
mutex_init(>vsync_mutex);
 
-   ret = devm_request_irq(dev, vop->irq, vop_isr,
-  IRQF_SHARED, dev_name(dev), vop);
-   if (ret)
-   return ret;
-
-   /* IRQ is initially disabled; it gets enabled in power_on */
-   disable_irq(vop->irq);
-
ret = vop_create_crtc(vop);
if (ret)
-   goto err_enable_irq;
+   return ret;
 
pm_runtime_enable(>dev);
 
@@ -1590,13 +1585,19 @@ static int vop_bind(struct device *dev, struct device 
*master, void *data)
goto err_disable_pm_runtime;
}
 
+   ret = devm_request_irq(dev, vop->irq, vop_isr,
+  IRQF_SHARED, dev_name(dev), vop);
+   if (ret)
+   goto err_disable_pm_runtime;
+
+   /* IRQ is initially disabled; it gets enabled in power_on */
+   disable_irq(vop->irq);
+
return 0;
 
 err_disable_pm_runtime:
pm_runtime_disable(>dev);
vop_destroy_crtc(vop);
-err_enable_irq:
-   enable_irq(vop->irq); /* To balance out the disable_irq above */
return ret;
 }
 
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/rockchip: Don't use spin_lock_irqsave in interrupt context

[Marc Zyngier]

The rockchip DRM driver is quite careful to disable interrupts
when taking a lock that is also taken in interrupt context,
which is a good thing.

What is a bit over the top is to use spin_lock_irqsave when
already in interrupt context, as you cannot take another
interrupt again, and disabling interrupt is just pure
overhead.

Switching to the non _irqsave version in interrupt context is
more logical, and less heavy handed.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f2bde1c76bbe..48d27f6fb16d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1160,15 +1160,14 @@ static void vop_handle_vblank(struct vop *vop)
 {
struct drm_device *drm = vop->drm_dev;
struct drm_crtc *crtc = >crtc;
-   unsigned long flags;
 
-   spin_lock_irqsave(>event_lock, flags);
+   spin_lock(>event_lock);
if (vop->event) {
drm_crtc_send_vblank_event(crtc, vop->event);
drm_crtc_vblank_put(crtc);
vop->event = NULL;
}
-   spin_unlock_irqrestore(>event_lock, flags);
+   spin_unlock(>event_lock);
 
if (test_and_clear_bit(VOP_PENDING_FB_UNREF, >pending))
drm_flip_work_commit(>fb_unref_work, system_unbound_wq);
@@ -1179,21 +1178,20 @@ static irqreturn_t vop_isr(int irq, void *data)
struct vop *vop = data;
struct drm_crtc *crtc = >crtc;
uint32_t active_irqs;
-   unsigned long flags;
int ret = IRQ_NONE;
 
/*
 * interrupt register has interrupt status, enable and clear bits, we
 * must hold irq_lock to avoid a race with enable/disable_vblank().
*/
-   spin_lock_irqsave(>irq_lock, flags);
+   spin_lock(>irq_lock);
 
active_irqs = VOP_INTR_GET_TYPE(vop, status, INTR_MASK);
/* Clear all active interrupt sources */
if (active_irqs)
VOP_INTR_SET_TYPE(vop, clear, active_irqs, 1);
 
-   spin_unlock_irqrestore(>irq_lock, flags);
+   spin_unlock(>irq_lock);
 
/* This is expected for vop iommu irqs, since the irq is shared */
if (!active_irqs)
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] drm/rockchip: VOP interrupt fixes

[Marc Zyngier]

This small series fixes a number of issues that I found while trying
to get kexec working on the Chromebook Plus (aka rk3399-gru-kevin) in
order to use it as some sort of interactive bootloader.

The main issue is that the vop driver expects the interrupts to be
cleared and disabled when booting. Nothing could be more wrong. The
device should be expected to be alive and screaming, and it is the
driver's job to put it back into a sane state.

This is what the first patch does, making sure the interrupt is
requested only when the device has been put back into a known
state. Given that this is an observable bug that has been around for a
while, I've tagged it with a Cc: stable.

The two following patches are less important: Using memcpy on MMIO
ranges is plain wrong, and using spin_lock_irqsave in irq context is
slightly pointless.

With these patches in, I'm able to get kexec to work. There is still
some funny issues at the iommu level, but that's for another day.

Patches on top of 4.16-rc2.

Marc Zyngier (3):
  drm/rockchip: Clear all interrupts before requesting the IRQ
  drm/rockchip: Do not use memcpy for MMIO addresses
  drm/rockchip: Don't use spin_lock_irqsave in interrupt context

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 44 +++--
 1 file changed, 23 insertions(+), 21 deletions(-)

-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/rockchip: Do not use memcpy for MMIO addresses

[Marc Zyngier]

memcpy is only meant to be used for memory, and only that.
MMIO accessors should be used to access MMIO regions, preferably
the ones that correspond to the size of the register accessed.

Let's convert the bulk register copy to writel/readl_relaxed,
which is the correct API.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 7b224e08cbf1..f2bde1c76bbe 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -528,7 +528,10 @@ static int vop_enable(struct drm_crtc *crtc)
goto err_disable_aclk;
}
 
-   memcpy(vop->regs, vop->regsbak, vop->len);
+   spin_lock(>reg_lock);
+   for (i = 0; i < vop->len; i += 4)
+   writel_relaxed(vop->regsbak[i / 4], vop->regs + i);
+
/*
 * We need to make sure that all windows are disabled before we
 * enable the crtc. Otherwise we might try to scan from a destroyed
@@ -538,10 +541,9 @@ static int vop_enable(struct drm_crtc *crtc)
struct vop_win *vop_win = >win[i];
const struct vop_win_data *win = vop_win->data;
 
-   spin_lock(>reg_lock);
VOP_WIN_SET(vop, win, enable, 0);
-   spin_unlock(>reg_lock);
}
+   spin_unlock(>reg_lock);
 
vop_cfg_done(vop);
 
@@ -1417,7 +1419,8 @@ static int vop_initial(struct vop *vop)
VOP_INTR_SET_TYPE(vop, clear, INTR_MASK, 1);
VOP_INTR_SET_TYPE(vop, enable, INTR_MASK, 0);
 
-   memcpy(vop->regsbak, vop->regs, vop->len);
+   for (i = 0; i < vop->len; i += sizeof(u32))
+   vop->regsbak[i / 4] = readl_relaxed(vop->regs + i);
 
VOP_REG_SET(vop, misc, global_regdone_en, 1);
VOP_REG_SET(vop, common, dsp_blank, 0);
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: Set IRQ_NOAUTOEN flag before requesting the interrupt

[Marc Zyngier]

On 10/02/18 14:20, Marc Zyngier wrote:
> Calling request_irq() followed by disable_irq() is usually a bad idea,
> specially if the interrupt can be pending, and you're not yet in a
> position to handle it.
> 
> This is exactly what happens on my kevin system when rebooting in a
> second kernel using kexec: Some interrupt is left pending from
> the previous kernel, and we take it too early, before disable_irq()
> could do anything.
> 
> A better way of ensuring safety is to set the IRQ_NOAUTOEN flag
> on the irq before requesting it.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>

For the record, I've posted a (much) improved version of this as part of
a series here[1].

Thanks,

M.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-February/560703.html
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: Don't use spin_lock_irqsave in interrupt context

[Marc Zyngier]

Hi Heiko,

On 10/02/18 16:23, Heiko Stuebner wrote:
> Hi Marc,
> 
> Am Samstag, 10. Februar 2018, 15:35:01 CET schrieb Marc Zyngier:
>> The rockchip DRM driver is quite careful to disable interrupts
>> when taking a lock that is also taken in interrupt context,
>> which is a good thing.
>>
>> What is a bit over the top is to use spin_lock_irqsave when
>> already in interrupt context, as you cannot take another
>> interrupt again, and disabling interrupt is just pure
>> overhead.
>>
>> Switching to the non _irqsave version in interrupt context is
>> more logical, and less heavy handed.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
> 
> please note, that we had a maintainer swap for the Rockchip drm-component,
> where Sandy replaced Mark [0] ... with me acting as sort-of (and not yet
> up to speed) backup.
> 
> So I guess Rockchip drm patches should also include
>   Sandy Huang <h...@rock-chips.com>

I guess that update didn't make it into 4.15, which is why I didn't spot
it. I'll repost the patches shortly including Sandy on Cc.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: Don't use spin_lock_irqsave in interrupt context

[Marc Zyngier]

The rockchip DRM driver is quite careful to disable interrupts
when taking a lock that is also taken in interrupt context,
which is a good thing.

What is a bit over the top is to use spin_lock_irqsave when
already in interrupt context, as you cannot take another
interrupt again, and disabling interrupt is just pure
overhead.

Switching to the non _irqsave version in interrupt context is
more logical, and less heavy handed.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 72554f404b7e..e23877e72c43 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1159,15 +1159,14 @@ static void vop_handle_vblank(struct vop *vop)
 {
struct drm_device *drm = vop->drm_dev;
struct drm_crtc *crtc = >crtc;
-   unsigned long flags;
 
-   spin_lock_irqsave(>event_lock, flags);
+   spin_lock(>event_lock);
if (vop->event) {
drm_crtc_send_vblank_event(crtc, vop->event);
drm_crtc_vblank_put(crtc);
vop->event = NULL;
}
-   spin_unlock_irqrestore(>event_lock, flags);
+   spin_unlock(>event_lock);
 
if (test_and_clear_bit(VOP_PENDING_FB_UNREF, >pending))
drm_flip_work_commit(>fb_unref_work, system_unbound_wq);
@@ -1178,21 +1177,20 @@ static irqreturn_t vop_isr(int irq, void *data)
struct vop *vop = data;
struct drm_crtc *crtc = >crtc;
uint32_t active_irqs;
-   unsigned long flags;
int ret = IRQ_NONE;
 
/*
 * interrupt register has interrupt status, enable and clear bits, we
 * must hold irq_lock to avoid a race with enable/disable_vblank().
*/
-   spin_lock_irqsave(>irq_lock, flags);
+   spin_lock(>irq_lock);
 
active_irqs = VOP_INTR_GET_TYPE(vop, status, INTR_MASK);
/* Clear all active interrupt sources */
if (active_irqs)
VOP_INTR_SET_TYPE(vop, clear, active_irqs, 1);
 
-   spin_unlock_irqrestore(>irq_lock, flags);
+   spin_unlock(>irq_lock);
 
/* This is expected for vop iommu irqs, since the irq is shared */
if (!active_irqs)
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: Set IRQ_NOAUTOEN flag before requesting the interrupt

[Marc Zyngier]

Calling request_irq() followed by disable_irq() is usually a bad idea,
specially if the interrupt can be pending, and you're not yet in a
position to handle it.

This is exactly what happens on my kevin system when rebooting in a
second kernel using kexec: Some interrupt is left pending from
the previous kernel, and we take it too early, before disable_irq()
could do anything.

A better way of ensuring safety is to set the IRQ_NOAUTOEN flag
on the irq before requesting it.

Cc: sta...@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 19128b4dea54..72554f404b7e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1569,14 +1570,12 @@ static int vop_bind(struct device *dev, struct device 
*master, void *data)
 
mutex_init(>vsync_mutex);
 
+   irq_set_status_flags(vop->irq, IRQ_NOAUTOEN);
ret = devm_request_irq(dev, vop->irq, vop_isr,
   IRQF_SHARED, dev_name(dev), vop);
if (ret)
return ret;
 
-   /* IRQ is initially disabled; it gets enabled in power_on */
-   disable_irq(vop->irq);
-
ret = vop_create_crtc(vop);
if (ret)
goto err_enable_irq;
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: analogix_dp: Ensure that the bridge is powered before poking it

[Marc Zyngier]

On 19/12/17 07:55, Andrzej Hajda wrote:
> On 18.12.2017 12:28, Marc Zyngier wrote:
>> Stopping the X display manager on a kevin platform results in the
>> following crash:
>>
>> [  674.833536] Synchronous External Abort: synchronous external abort 
>> (0x9610) at 0x0c970640
>> [  674.843886] Internal error: : 9610 [#1] PREEMPT SMP
>> [  674.849744] Modules linked in:
>> [  674.849755] CPU: 1 PID: 86 Comm: kworker/1:1 Not tainted 
>> 4.15.0-rc3-00057-gff24f8cf492d-dirty #3
>> [  674.849760] detected fb_set_par error, error code: -16
>> [  674.849761] Hardware name: Google Kevin (DT)
>> [  674.849773] Workqueue: events analogix_dp_psr_work
>> [  674.849778] pstate: 6005 (nZCv daif -PAN -UAO)
>> [  674.849784] pc : analogix_dp_send_psr_spd+0x8/0x168
>> [  674.849788] lr : analogix_dp_enable_psr+0x54/0x60
>> [  674.849789] sp : 09b2bd60
>> [  674.849790] x29: 09b2bd60 x28: 
>> [  674.849794] x27: 09913d20 x26: 0900fbf0
>> [  674.849797] x25: 8000f1b3 x24: 8000f0c21d98
>> [  674.849800] x23:  x22: 8000f7d3aa00
>> [  674.849803] x21: 8000f7d36980 x20: 8000f0c21c18
>> [  674.849806] x19: 8000f0c21db8 x18: 0001
>> [  674.849809] x17: 89f2ed58 x16: 08222908
>> [  674.849812] x15:  x14: 0400
>> [  674.849815] x13: 0400 x12: 
>> [  674.849817] x11: 1414 x10: 0a00
>> [  674.849820] x9 : 09b2bbb0 x8 : 8000f1b30a60
>> [  674.849823] x7 : 0008 x6 : 0001
>> [  674.849826] x5 : 0010 x4 : 0007
>> [  674.849829] x3 : 0002 x2 : 0c970640
>> [  674.849832] x1 : 09b2bd78 x0 : 8000f1624018
>> [  674.849836] Process kworker/1:1 (pid: 86, stack limit = 
>> 0x83e5f7c3)
>> [  674.849838] Call trace:
>> [  674.849842]  analogix_dp_send_psr_spd+0x8/0x168
>> [  674.849844]  analogix_dp_psr_work+0x9c/0xa0
>> [  674.849849]  process_one_work+0x1cc/0x328
>> [  674.849852]  worker_thread+0x50/0x450
>> [  674.849856]  kthread+0xf8/0x128
>> [  674.849860]  ret_from_fork+0x10/0x18
>> [  674.849864] Code: b901 d65f03c0 f9445802 91190042 (b9400042)
>>
>> Further investigation show that this happens because the the workqueue
>> races with the analogix_dp_bridge_disable() call from the core DRM code,
>> and end up trying to write to the DP bridge that has already been powered
>> down. This result is a very black screen, and a hard reset.
>>
>> Instead of counting on luck to keep the bridge alive, let's use the
>> pm_runtime framework and take a reference on the device when we're about
>> to poke it. That is a fairly big hammer, but one that allows the system
>> to stay alive across dozens of X start/stop sequences.
> 
> Wouldn't be better to cancel the work in analogix_dp_bridge_disable, it
> looks safer.

Not sure. That would only cancel a single work that would be in flight
right when we hit disable, but won't prevent the work from being queued
right after the cancel.

In summary, I think you're trading a race between pm_runtime_put_sync
and analogix_dp_send_psr_spd for another between cancel_work_sync and
analogix_dp_send_psr_spd. Also, I seem to remember that the disable can
occur in its own work queue:

commit_tail -> drm_atomic_helper_commit_modeset_disables ->
disable_outputs -> drm_bridge_disable -> analogix_dp_bridge_disable

making it racy by nature. But I'm no DRM expert (as you can probably tell).

My approach is to guarantee that analogix_dp_send_psr_spd cannot fault
due to the IP being powered off, which feels a bit more bullet proof.

Please shoot me down if I got it wrong!

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: analogix_dp: Use relaxed MMIO accessors

[Marc Zyngier]

On 19/12/17 08:53, Andrzej Hajda wrote:
> On 18.12.2017 12:39, Marc Zyngier wrote:
>> The analogix DP bridge is entierely driven via MMIO accesses, and
>> does not do any DMA that requires coherency with the CPU. Yet, the
>> driver uses the non-relaxed accessors, forcing strong barriers to
>> be emitted on architectures with a relaxed memory ordering.
>>
>> This is of course completely unnecessary, and only serves as a way
>> to pointlessly reduce the performance of unsuspecting platforms.
>>
>> Switch the driver to the _relaxed accessors, making my kevin platform
>> a slightly better machine.
> 
> Do you have any stats to justify this change?

I do (sort of): "hackbench 100 process 1000" is a almost a second faster
with that change (that's about 2% on something that is essentially a
scheduler benchmark) on my rk3399 system. Yes, this is a silly
benchmark, but that's one you can easily run on about anything.

> The common practice is/was to use writel/readl accessors to access MMIO,
> even if it is suboptimal in many cases. Has something changed in these
> practices?

Having it as a default is acceptable, unless you actually understand the
implications of the relaxed accessors and find that they have an impact
on the system.

To give you an idea of the magnitude of the impact on an arm64 system,
each barrier does:
- force the completion of any memory access in the whole system
- force the completion of any in-flight TLB invalidation and cache
maintenance in the whole system

When I say "in the whole system", it means CPUs, GPU, and any other
piece of HW connected on the system. This is effectively a "wait until
everything that was in-flight is done".

This is a widespread issue that takes time to address, unfortunately.

Now, I've definitely applied brute force to this driver, not really
knowing all the intricacies of the device. I haven't seen anything
bizarre there (it is mostly a bunch of RMWs), but I'd appreciate someone
who knows this IP to review it.

> To be clear, I am not against this change. I am just curious.

There is a really good article that explains all (and quite a bit more)
a kernel developer needs to know about MMIO and relaxed accessors here:

https://lwn.net/Articles/698014/

Hope this helps,

M.
-- 
Jazz is not dead. It just smells funny...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge: analogix_dp: Use relaxed MMIO accessors

[Marc Zyngier]

The analogix DP bridge is entierely driven via MMIO accesses, and
does not do any DMA that requires coherency with the CPU. Yet, the
driver uses the non-relaxed accessors, forcing strong barriers to
be emitted on architectures with a relaxed memory ordering.

This is of course completely unnecessary, and only serves as a way
to pointlessly reduce the performance of unsuspecting platforms.

Switch the driver to the _relaxed accessors, making my kevin platform
a slightly better machine.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 488 +++---
 1 file changed, 244 insertions(+), 244 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 303083ad28e3..248d6ba913dc 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -31,13 +31,13 @@ void analogix_dp_enable_video_mute(struct 
analogix_dp_device *dp, bool enable)
u32 reg;
 
if (enable) {
-   reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   reg = readl_relaxed(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
reg |= HDCP_VIDEO_MUTE;
-   writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
} else {
-   reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   reg = readl_relaxed(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
reg &= ~HDCP_VIDEO_MUTE;
-   writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
}
 }
 
@@ -45,9 +45,9 @@ void analogix_dp_stop_video(struct analogix_dp_device *dp)
 {
u32 reg;
 
-   reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   reg = readl_relaxed(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
reg &= ~VIDEO_EN;
-   writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
 }
 
 void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
@@ -61,7 +61,7 @@ void analogix_dp_lane_swap(struct analogix_dp_device *dp, 
bool enable)
reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
  LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
 
-   writel(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_LANE_MAP);
 }
 
 void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
@@ -69,53 +69,53 @@ void analogix_dp_init_analog_param(struct 
analogix_dp_device *dp)
u32 reg;
 
reg = TX_TERMINAL_CTRL_50_OHM;
-   writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_1);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_1);
 
reg = SEL_24M | TX_DVDD_BIT_1_0625V;
-   writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
 
if (dp->plat_data && is_rockchip(dp->plat_data->dev_type)) {
reg = REF_CLK_24M;
if (dp->plat_data->dev_type == RK3288_DP)
reg ^= REF_CLK_MASK;
 
-   writel(reg, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
-   writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
-   writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
-   writel(0x58, dp->reg_base + ANALOGIX_DP_PLL_REG_4);
-   writel(0x22, dp->reg_base + ANALOGIX_DP_PLL_REG_5);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
+   writel_relaxed(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
+   writel_relaxed(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
+   writel_relaxed(0x58, dp->reg_base + ANALOGIX_DP_PLL_REG_4);
+   writel_relaxed(0x22, dp->reg_base + ANALOGIX_DP_PLL_REG_5);
}
 
reg = DRIVE_DVDD_BIT_1_0625V | VCO_BIT_600_MICRO;
-   writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_3);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_3);
 
reg = PD_RING_OSC | AUX_TERMINAL_CTRL_50_OHM |
TX_CUR1_2X | TX_CUR_16_MA;
-   writel(reg, dp->reg_base + ANALOGIX_DP_PLL_FILTER_CTL_1);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_PLL_FILTER_CTL_1);
 
reg = CH3_AMP_400_MV | CH2_AMP_400_MV |
CH1_AMP_400_MV | CH0_AMP_400_MV;
-   writel(reg, dp->reg_base + ANALOGIX_DP_TX_AMP_TUNING_CTL);
+   writel_relaxed(reg, dp->reg_base + ANALOGIX_DP_TX_AMP_TUNING_CTL);
 }
 
 void analogix_dp_init_interrupt(struct analogix_dp_device *dp)
 {
/* Set interrupt pin assertion 

[PATCH] drm/rockchip: analogix_dp: Ensure that the bridge is powered before poking it

[Marc Zyngier]

Stopping the X display manager on a kevin platform results in the
following crash:

[  674.833536] Synchronous External Abort: synchronous external abort 
(0x9610) at 0x0c970640
[  674.843886] Internal error: : 9610 [#1] PREEMPT SMP
[  674.849744] Modules linked in:
[  674.849755] CPU: 1 PID: 86 Comm: kworker/1:1 Not tainted 
4.15.0-rc3-00057-gff24f8cf492d-dirty #3
[  674.849760] detected fb_set_par error, error code: -16
[  674.849761] Hardware name: Google Kevin (DT)
[  674.849773] Workqueue: events analogix_dp_psr_work
[  674.849778] pstate: 6005 (nZCv daif -PAN -UAO)
[  674.849784] pc : analogix_dp_send_psr_spd+0x8/0x168
[  674.849788] lr : analogix_dp_enable_psr+0x54/0x60
[  674.849789] sp : 09b2bd60
[  674.849790] x29: 09b2bd60 x28: 
[  674.849794] x27: 09913d20 x26: 0900fbf0
[  674.849797] x25: 8000f1b3 x24: 8000f0c21d98
[  674.849800] x23:  x22: 8000f7d3aa00
[  674.849803] x21: 8000f7d36980 x20: 8000f0c21c18
[  674.849806] x19: 8000f0c21db8 x18: 0001
[  674.849809] x17: 89f2ed58 x16: 08222908
[  674.849812] x15:  x14: 0400
[  674.849815] x13: 0400 x12: 
[  674.849817] x11: 1414 x10: 0a00
[  674.849820] x9 : 09b2bbb0 x8 : 8000f1b30a60
[  674.849823] x7 : 0008 x6 : 0001
[  674.849826] x5 : 0010 x4 : 0007
[  674.849829] x3 : 0002 x2 : 0c970640
[  674.849832] x1 : 09b2bd78 x0 : 8000f1624018
[  674.849836] Process kworker/1:1 (pid: 86, stack limit = 0x83e5f7c3)
[  674.849838] Call trace:
[  674.849842]  analogix_dp_send_psr_spd+0x8/0x168
[  674.849844]  analogix_dp_psr_work+0x9c/0xa0
[  674.849849]  process_one_work+0x1cc/0x328
[  674.849852]  worker_thread+0x50/0x450
[  674.849856]  kthread+0xf8/0x128
[  674.849860]  ret_from_fork+0x10/0x18
[  674.849864] Code: b901 d65f03c0 f9445802 91190042 (b9400042)

Further investigation show that this happens because the the workqueue
races with the analogix_dp_bridge_disable() call from the core DRM code,
and end up trying to write to the DP bridge that has already been powered
down. This result is a very black screen, and a hard reset.

Instead of counting on luck to keep the bridge alive, let's use the
pm_runtime framework and take a reference on the device when we're about
to poke it. That is a fairly big hammer, but one that allows the system
to stay alive across dozens of X start/stop sequences.

Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c 
b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 08743ad96cb9..7f2c190f75e7 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -113,10 +114,12 @@ static void analogix_dp_psr_work(struct work_struct *work)
}
 
mutex_lock(>psr_lock);
+   pm_runtime_get_sync(dp->dev);
if (dp->psr_state == EDP_VSC_PSR_STATE_ACTIVE)
analogix_dp_enable_psr(dp->dev);
else
analogix_dp_disable_psr(dp->dev);
+   pm_runtime_put_sync(dp->dev);
mutex_unlock(>psr_lock);
 }
 
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm: vc4: WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8

[Marc Zyngier]

On Sat, 09 Dec 2017 14:17:58 +,
Stefan Schake wrote:
> 
> On Sat, Dec 9, 2017 at 1:34 PM, Marc Zyngier <marc.zyng...@arm.com> wrote:
> > On Fri, 08 Dec 2017 22:44:27 +,
> > Stefan Wahren wrote:
> >>
> >> Hi,
> >>
> >> the commit 253696ccd613 ("drm/vc4: Account for interrupts in
> >> flight") triggers a warning during boot of Raspberry Pi 2 with
> >> multi_v7_defconfig:
> >>
> >> [7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi 
> >> mapping ok
> >> [7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> >> [7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops 
> >> [vc4])
> >> [7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> >> [7.973788] vc4-drm soc:gpu: bound 3f40.hvs (ops vc4_hvs_ops [vc4])
> >> [7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops 
> >> vc4_crtc_ops [vc4])
> >> [7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops 
> >> vc4_crtc_ops [vc4])
> >> [7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops 
> >> vc4_crtc_ops [vc4])
> >> [8.018820] [ cut here ]
> >> [8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 
> >> __irq_startup+0x9c/0xa8
> >> [8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus 
> >> snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec
> >> [8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted 
> >> 4.15.0-rc1-00291-g75f64f6 #10
> >> [8.018914] Hardware name: BCM2835
> >> [8.018950] [] (unwind_backtrace) from [] 
> >> (show_stack+0x10/0x14)
> >> [8.018970] [] (show_stack) from [] 
> >> (dump_stack+0x88/0xa4)
> >> [8.018993] [] (dump_stack) from [] 
> >> (__warn+0xe4/0x110)
> >> [8.019012] [] (__warn) from [] 
> >> (warn_slowpath_null+0x3c/0x48)
> >> [8.019029] [] (warn_slowpath_null) from [] 
> >> (__irq_startup+0x9c/0xa8)
> >> [8.019045] [] (__irq_startup) from [] 
> >> (irq_startup+0x44/0x134)
> >> [8.019061] [] (irq_startup) from [] 
> >> (enable_irq+0x34/0x6c)
> >> [8.019210] [] (enable_irq) from [] 
> >> (vc4_irq_postinstall+0x14/0x34 [vc4])
> >> [8.019338] [] (vc4_irq_postinstall [vc4]) from [] 
> >> (drm_irq_install+0xc0/0x134)
> >> [8.019456] [] (drm_irq_install) from [] 
> >> (vc4_v3d_bind+0x12c/0x238 [vc4])
> >
> > The sequence "request_irq/enable" feels pretty odd, given that the
> > interrupt wasn't requested with NOAUTOEN. What are the expected
> > semantics of that patch?
> >
> > Thanks,
> >
> > M.
> >
> > --
> > Jazz is not dead, it just smell funny.
> 
> There is a second path to vc4_irq_postinstall when we return from
> power management disable. Maybe the enable would be better situated there?

I've no idea. But enabling an already enabled interrupts feels a bit
wrong, to say the least

> That said, some more study of the irq code is giving me more questions than
> answers. If disable/enable are depth-counted, why isn't it also warning
> about that (there is a check)? Depth must be 1 after request_irq.
> Similarly, the irq is activated in request_irq, but the warning reported
> here is complaining that it is not.

I'm afraid you'll have to trace why the irq is not flagged as
activated. From what I can see, the irqchip driver of this HW doesn't
even implement an activate callback, so the interrupt should be
trivially flagged as activated. If it is not, it means something
rather bad has happened. Do you see this splat on each enable_irq()?

M.

-- 
Jazz is not dead, it just smell funny.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm: vc4: WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8

[Marc Zyngier]

On Sun, 10 Dec 2017 03:05:07 +,
Stefan Schake wrote:
> 
> On Fri, Dec 8, 2017 at 11:44 PM, Stefan Wahren  wrote:
> > Hi,
> >
> > the commit 253696ccd613 ("drm/vc4: Account for interrupts in flight") 
> > triggers a warning during boot of Raspberry Pi 2 with multi_v7_defconfig:
> >
> > [7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi 
> > mapping ok
> > [7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> > [7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> > [7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> > [7.973788] vc4-drm soc:gpu: bound 3f40.hvs (ops vc4_hvs_ops [vc4])
> > [7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops 
> > [vc4])
> > [7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops 
> > [vc4])
> > [7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops 
> > [vc4])
> > [8.018820] [ cut here ]
> > [8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 
> > __irq_startup+0x9c/0xa8
> > [8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus 
> > snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec
> > [8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted 
> > 4.15.0-rc1-00291-g75f64f6 #10
> > [8.018914] Hardware name: BCM2835
> > [8.018950] [] (unwind_backtrace) from [] 
> > (show_stack+0x10/0x14)
> > [8.018970] [] (show_stack) from [] 
> > (dump_stack+0x88/0xa4)
> > [8.018993] [] (dump_stack) from [] 
> > (__warn+0xe4/0x110)
> > [8.019012] [] (__warn) from [] 
> > (warn_slowpath_null+0x3c/0x48)
> > [8.019029] [] (warn_slowpath_null) from [] 
> > (__irq_startup+0x9c/0xa8)
> > [8.019045] [] (__irq_startup) from [] 
> > (irq_startup+0x44/0x134)
> > [8.019061] [] (irq_startup) from [] 
> > (enable_irq+0x34/0x6c)
> > [8.019210] [] (enable_irq) from [] 
> > (vc4_irq_postinstall+0x14/0x34 [vc4])
> > [8.019338] [] (vc4_irq_postinstall [vc4]) from [] 
> > (drm_irq_install+0xc0/0x134)
> > [8.019456] [] (drm_irq_install) from [] 
> > (vc4_v3d_bind+0x12c/0x238 [vc4])
> > [8.019550] [] (vc4_v3d_bind [vc4]) from [] 
> > (component_bind_all+0xe8/0x21c)
> > [8.019635] [] (component_bind_all) from [] 
> > (vc4_drm_bind+0x78/0x130 [vc4])
> > [8.019721] [] (vc4_drm_bind [vc4]) from [] 
> > (try_to_bring_up_master+0x13c/0x184)
> > [8.019739] [] (try_to_bring_up_master) from [] 
> > (component_master_add_with_match+0x80/0xb8)
> > [8.019822] [] (component_master_add_with_match) from 
> > [] (vc4_platform_drm_probe+0x90/0xa8 [vc4])
> > [8.019905] [] (vc4_platform_drm_probe [vc4]) from 
> > [] (platform_drv_probe+0x4c/0xac)
> > [8.019924] [] (platform_drv_probe) from [] 
> > (driver_probe_device+0x234/0x338)
> > [8.019939] [] (driver_probe_device) from [] 
> > (__driver_attach+0xac/0xb0)
> > [8.019953] [] (__driver_attach) from [] 
> > (bus_for_each_dev+0x64/0x94)
> > [8.019967] [] (bus_for_each_dev) from [] 
> > (bus_add_driver+0x184/0x20c)
> > [8.019981] [] (bus_add_driver) from [] 
> > (driver_register+0x78/0xf8)
> > [8.019997] [] (driver_register) from [] 
> > (do_one_initcall+0x3c/0x16c)
> > [8.020018] [] (do_one_initcall) from [] 
> > (do_init_module+0x5c/0x1f0)
> > [8.020037] [] (do_init_module) from [] 
> > (load_module+0x1ba4/0x2248)
> > [8.020058] [] (load_module) from [] 
> > (SyS_finit_module+0x8c/0x9c)
> > [8.020076] [] (SyS_finit_module) from [] 
> > (__sys_trace_return+0x0/0x10)
> > [8.020085] ---[ end trace d5c350f831cb07d2 ]---
> > [8.020201] vc4-drm soc:gpu: bound 3fc0.v3d (ops vc4_v3d_ops [vc4])
> 
> So I've done some testing on an RPi 3b that is also affected.
> It turns out that dev->irq is only initialized *after* postinstall by
> drm_irq_install, so we were calling irq_enable on IRQ 0 and that just happens
> to be not activated, hence the warning.

And IRQ 0 is definitely not a valid IRQ (I'd expect the DRM subsystem
to should in that case...).

> Having learned some more about the IRQ subsystem, my plan would be first to
> simply replace the disable/enable dance with one synchronize_irq. We do
> reenable the interrupt in the HW registers at the end of the binner work
> callback so this may not suffice. In that case, we could simply move the
> enable_irq to the power management handler that is calling postinstall.

I don't know anything about VC4 (or DRM), so I can't be of any advise
here. But if you intend to perform some enable/disable on the IRQ, I
wonder if you're actually ready to handle stuff by the time you
perform the request_irq. You may want to consider controlling the
interrupt at the device level instead.

Thanks,

M.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v14.1 01/17] drm: bridge: analogix/dp: split exynos dp driver to bridge directory

[Marc Zyngier]

On Wed, 8 Jun 2016 09:28:32 +0800
Yakir Yang  wrote:

> Hi Javier,
> 
> On 06/08/2016 01:06 AM, Javier Martinez Canillas wrote:
> > Hello Yakir,
> >
> > On 03/17/2016 05:47 PM, Heiko Stübner wrote:  
> >> Split the dp core driver from exynos directory to bridge directory,
> >> and rename the core driver to analogix_dp_*, rename the platform
> >> code to exynos_dp.
> >>
> >> Beside the new analogix_dp driver would export six hooks.
> >> "analogix_dp_bind()" and "analogix_dp_unbind()"
> >> "analogix_dp_suspned()" and "analogix_dp_resume()"
> >> "analogix_dp_detect()" and "analogix_dp_get_modes()"
> >>
> >> The bind/unbind symbols is used for analogix platform driver to connect
> >> with analogix_dp core driver. And the detect/get_modes is used for analogix
> >> platform driver to init the connector.
> >>
> >> They reason why connector need register in helper driver is rockchip drm
> >> haven't implement the atomic API, but Exynos drm have implement it, so
> >> there would need two different connector helper functions, that's why we
> >> leave the connector register in helper driver.
> >>
> >> Signed-off-by: Yakir Yang 
> >> ---  
> > Marc reported that his Exynos5250 Snow Chromebook fails to boot with 
> > v4.7-rc.
> >
> > I've done a git bisect and tracked down to this commit. The problem is a 
> > NULL
> > pointer dereference to connector->dev in drm_mode_create(connector->dev) 
> > when
> > called from exynos_dp_get_modes(). The error log is at [1].
> >
> > I'm trying to figure out the issue but wanted to mention in case you have 
> > any
> > hints about what could be the cause. AFAICT the problem is related to the 
> > fact
> > that drm_connector_init() is called in analogix_dp_bridge_attach() and the
> > connector passed as argument is the one in struct analogix_dp_device *dp, 
> > but
> > later exynos_dp_get_modes() calls drm_mode_create() passing the connector in
> > struct exynos_dp_device *dp, which has not been previously initialized.  
> 
> Agree, this should be the problem, exynos_dp->connector haven't been
> initialized, driver should make exynos_dp->dp to a connector point, and
> record the passing connector in exynos_dp_bridge_attach(), that should
> fix this problem.
> 
> 
> Thanks,
> - Yakir
> 
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c 
> b/drivers/gpu/drm/exynos/exynos_dp.c
> index 468498e..4c1fb3f 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -34,7 +34,7 @@
> 
>   struct exynos_dp_device {
>  struct drm_encoder encoder;
> -   struct drm_connector   connector;
> +   struct drm_connector   *connector;
>  struct drm_bridge  *ptn_bridge;
>  struct drm_device  *drm_dev;
>  struct device  *dev;
> @@ -70,7 +70,7 @@ static int exynos_dp_poweroff(struct 
> analogix_dp_plat_data *plat_data)
>   static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
>   {
>  struct exynos_dp_device *dp = to_dp(plat_data);
> -   struct drm_connector *connector = >connector;
> +   struct drm_connector *connector = dp->connector;
>  struct drm_display_mode *mode;
>  int num_modes = 0;
> 
> @@ -103,6 +103,7 @@ static int exynos_dp_bridge_attach(struct 
> analogix_dp_plat_data *plat_data,
>  int ret;
> 
>  drm_connector_register(connector);
> +   dp->connector = connector;
> 
>  /* Pre-empt DP connector creation if there's a bridge */
>  if (dp->ptn_bridge) {
> 

I've just tested this change, and in combination with Javier's DT patch,
my Snow is back to its useful state (I'm writing this email from that
very Chromebook).

Once you make this a proper patch, please add my:

Tested-by: Marc Zyngier 

to it.

Thanks a lot to you and Javier for tracking this down!

M.
-- 
Jazz is not dead. It just smells funny.

[PATCH] drm/exynos: fimd: fix trigger mode change regression

[Marc Zyngier]

On 2016-06-01 06:53, Inki Dae wrote:
> This patch fixes a regression that Display panel doesn't work
> since HW trigger mode was supported.
>
> The trigger mode should be changed on PSR(Panel Self Refresh)
> mode of Panel device according to HW guy's saying. However,
> with previous HW trigger support, trigger mode could been changed
> in normal mode of Panel device.
>
> So this patch makes sure to change the trigger mode after power off
> and on again. Later we need to add PSR relevant codes instead.

I'm afraid this patch doesn't fix the regression I'm seeing on my 
Exynos5250 Chromebook.

I found out that using the 4.6 device tree allows the display to 
function correctly (at least for a while), just enough to see it dying 
before reaching userspace (and it scrolls up way to quickly for me to 
read it). Adding printk_delay results in a hanging system.

Can someone with a servo-board investigate this? I'll try to bisect it 
further, but an actual backtrace would help.

Thanks,

 M.
-- 
Fast, cheap, reliable. Pick two.

[PATCH 2/3] drm/exynos: fimd: add HW trigger support

[Marc Zyngier]

On 2016-05-30 23:58, Javier Martinez Canillas wrote:
> Hello Inki,
>
> On 04/05/2016 04:27 AM, Inki Dae wrote:
>> This patch adds HW trigger support on i80 mode.
>>
>> Until now, Exynos DRM only supported SW trigger which was set
>> SWTRGCMD bit of TRIGCON register by CPU to transfer scanout
>> buffer to Display bus device or panel.
>>
>> With this patch, the transmission to Display bus device or
>> panel will be initiated by FIMD controller.
>>
>> Signed-off-by: Inki Dae 
>> ---
>
> There is a regression for the Exynos5800 Peach Pi Chromebook display 
> due
> this patch. The display is blank and I noticed that it only happens 
> when
> HW start trigger is enabled, but works with SW trigger (as it was 
> before).
>
> So for example with the following diff on top of v4.7-rc1, display 
> works
> again. Do you have any hints about what could be the issue?
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 0444d7fc400d..8c62830e9514 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -171,7 +171,7 @@ static struct fimd_driver_data
> exynos5420_fimd_driver_data = {
>   .lcdblk_vt_shift = 24,
>   .lcdblk_bypass_shift = 15,
>   .lcdblk_mic_bypass_shift = 11,
> - .trg_type = I80_HW_TRG,
>   .has_shadowcon = 1,
>   .has_vidoutcon = 1,
>   .has_vtsel = 1,
>
> Best regards,

On a related note, my Exynos5250 Chromebook (snow) has stopped working 
as well since -rc1 has landed. I haven't had time to bisect it yet, but 
the symptoms are vaguely similar (bright white screen).

I'm happy to test patches.

Thanks,

 M.
-- 
Who you jivin' with that Cosmik Debris?