[PATCH v2] drm/etnaviv: add optional reset support
LECOINTRE Philippe
philippe.lecointre at thalesgroup.com
Thu Dec 5 15:06:18 UTC 2024
Hi Lucas,
I am grateful to you for your answer as this is my first attempt to contribute to the kernel.
> -----Message d'origine-----
> De : Lucas Stach <l.stach at pengutronix.de>
> Envoyé : mardi 3 décembre 2024 18:58
> À : LECOINTRE Philippe <philippe.lecointre at thalesgroup.com>; Russell King
> <linux+etnaviv at armlinux.org.uk>; Christian Gmeiner
> <christian.gmeiner at gmail.com>
> Cc : David Airlie <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>;
> etnaviv at lists.freedesktop.org; dri-devel at lists.freedesktop.org; linux-
> kernel at vger.kernel.org; LENAIN Simon <simon.lenain at thalesgroup.com>;
> BARBEAU Etienne <etienne.barbeau at thalesgroup.com>; LEJEUNE Sebastien
> <sebastien.lejeune at thalesgroup.com>
> Objet : Re: [PATCH v2] drm/etnaviv: add optional reset support
>
> Hi Philippe,
>
> Am Freitag, dem 08.11.2024 um 14:00 +0000 schrieb LECOINTRE Philippe:
> > Add optional reset support which is mentioned in vivante,gc.yaml to
> > allow the driver to work on SoCs whose reset signal is asserted by default
> > Avoid enabling the interrupt until everything is ready
> >
> > Signed-off-by: Philippe Lecointre <philippe.lecointre at thalesgroup.com>
> > Reviewed-by: Simon Lenain <simon.lenain at thalesgroup.com>
> > ---
> > v2:
> > - Add missing include of irq.h
> > ---
> > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 30
> +++++++++++++++++++++++++++
> > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 2 ++
> > 2 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > index 7c7f97793ddd..3e0c5dd9f74b 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > * Copyright (C) 2015-2018 Etnaviv Project
> > + * Copyright (C) 2024 Thales
> > */
> >
> > #include <linux/clk.h>
> > @@ -8,11 +9,13 @@
> > #include <linux/delay.h>
> > #include <linux/dma-fence.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/irq.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/module.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/reset.h>
> > #include <linux/thermal.h>
> >
> > #include "etnaviv_cmdbuf.h"
> > @@ -1629,8 +1632,24 @@ static int etnaviv_gpu_clk_enable(struct
> etnaviv_gpu *gpu)
> > if (ret)
> > goto disable_clk_core;
> >
> > + /* 32 core clock cycles (slowest clock) required before deassertion. */
> > + /* 1 microsecond might match all implementations */
> > + usleep_range(1, 2);
> > +
> > + ret = reset_control_deassert(gpu->rst);
> > + if (ret)
> > + goto disable_clk_shader;
> > +
> > + /* 128 core clock cycles (slowest clock) required before any activity on
> AHB. */
> > + /* 1 microsecond might match all implementations */
> > + usleep_range(1, 2);
>
> Mashing the reset handling into the clock handling is a bad idea. The
> clocks are en-/disabled during runtime PM handling. The etnaviv driver
> is written in a way that the GPU does not necessarily need to be reset
> during a runtime PM transition, which allow for faster startup times.
>
> The reset handling should really be its own separate function and would
> logically go into etnaviv_gpu_init() between the pm_runtime_get_sync()
> and etnaviv_hw_identify().
>
I will rework this part to match your feedback.
> > +
> > + enable_irq(gpu->irq);
>
> Do you see any issues with the IRQ being enabled earlier? A GPU being
> held in reset should not be able to trigger a IRQ.
>
I intend to avoid situations where IRQ are triggered for some reason and the GPU is not fully operational.
Such issues might appear considering the state of reset and clocks from internal boot to kernel.
I assume this situation never appeared with SoCs that are using this driver at the moment.
But it appear on a SoC developed by an industrial partner, this lead to IRQ spamming with AXI bus error in the console.
> > +
> > return 0;
> >
> > +disable_clk_shader:
> > + clk_disable_unprepare(gpu->clk_shader);
> > disable_clk_core:
> > clk_disable_unprepare(gpu->clk_core);
> > disable_clk_bus:
> > @@ -1643,6 +1662,8 @@ static int etnaviv_gpu_clk_enable(struct
> etnaviv_gpu *gpu)
> >
> > static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
> > {
> > + disable_irq(gpu->irq);
> > + reset_control_assert(gpu->rst);
> > clk_disable_unprepare(gpu->clk_shader);
> > clk_disable_unprepare(gpu->clk_core);
> > clk_disable_unprepare(gpu->clk_bus);
> > @@ -1876,6 +1897,9 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> > if (gpu->irq < 0)
> > return gpu->irq;
> >
> > + /* Avoid enabling the interrupt until everything is ready */
> > + irq_set_status_flags(gpu->irq, IRQ_NOAUTOEN);
> > +
> > err = devm_request_irq(&pdev->dev, gpu->irq, irq_handler, 0,
> > dev_name(gpu->dev), gpu);
> > if (err) {
> > @@ -1883,6 +1907,12 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> > return err;
> > }
> >
> > + /* Get Reset: */
> > + gpu->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> > + if (IS_ERR(gpu->rst))
> > + return dev_err_probe(dev, PTR_ERR(gpu->rst),
> > + "failed to get reset\n");
> > +
> > /* Get Clocks: */
> > gpu->clk_reg = devm_clk_get_optional(&pdev->dev, "reg");
> > DBG("clk_reg: %p", gpu->clk_reg);
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > index 31322195b9e4..8c181191755e 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> > @@ -1,6 +1,7 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > /*
> > * Copyright (C) 2015-2018 Etnaviv Project
> > + * Copyright (C) 2024 Thales
>
> I don't think adding a single member here does warrant a Copyright
> statement, in fact I would prefer them to not be touched at all.
> Authorship of individual changes to the driver a clearly attributable
> via the git history.
>
I add these Copyright while following the rules to contribute to an OSS project from my company.
After clarifications with the legal department, I will remove these changes in my next revision.
Regards,
Philippe
> Regards,
> Lucas
> > */
> >
> > #ifndef __ETNAVIV_GPU_H__
> > @@ -157,6 +158,7 @@ struct etnaviv_gpu {
> > struct clk *clk_reg;
> > struct clk *clk_core;
> > struct clk *clk_shader;
> > + struct reset_control *rst;
> >
> > unsigned int freq_scale;
> > unsigned int fe_waitcycles;
More information about the etnaviv
mailing list