Re: [PATCH v3 05/17] drm/imagination: Get GPU resources

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maxime,

On Fri, 2023-07-07 at 14:47 +0200, Maxime Ripard wrote:
> On Tue, Jun 13, 2023 at 03:47:48PM +0100, Sarah Walker wrote:
> > Acquire clock, regulator and register resources, and enable/map as
> > appropriate.
> > 
> > Signed-off-by: Sarah Walker <sarah.walker@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/imagination/Makefile     |   1 +
> >  drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
> >  drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
> >  drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
> >  4 files changed, 496 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
> > index 62ccf0ccbd51..186f920d615b 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -4,6 +4,7 @@
> >  subdir-ccflags-y := -I$(srctree)/$(src)
> > 
> >  powervr-y := \
> > +     pvr_device.o \
> >       pvr_drv.o \
> > 
> >  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> > new file mode 100644
> > index 000000000000..790c36cebec1
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#include "pvr_device.h"
> > +
> > +#include <drm/drm_print.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/compiler_attributes.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/stddef.h>
> > +#include <linux/types.h>
> > +
> > +/**
> > + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->regs.
> > + *
> > + * This method of mapping the device control registers into memory ensures that
> > + * they are unmapped when the driver is detached (i.e. no explicit cleanup is
> > + * required).
> > + *
> > + * Return:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_platform_ioremap_resource().
> > + */
> > +static int
> > +pvr_device_reg_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> > +     struct resource *regs_resource;
> > +     void __iomem *regs;
> > +     int err;
> > +
> > +     pvr_dev->regs_resource = NULL;
> > +     pvr_dev->regs = NULL;
> > +
> > +     regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
> > +     if (IS_ERR(regs)) {
> > +             err = PTR_ERR(regs);
> > +             drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> > +                     err);
> > +             return err;
> > +     }
> > +
> > +     pvr_dev->regs = regs;
> > +     pvr_dev->regs_resource = regs_resource;
> 
> Do you actually need the resources somewhere?

This is needed when setting up the boot data for the MIPS firmware processor in
patch 11 (drm/imagination: Implement MIPS firmware processor and MMU support).
We'll move it to that patch instead.

> 
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * This is essentially a no-op, since pvr_device_reg_init() already ensures that
> > + * struct pvr_device->regs is unmapped when the device is detached. This
> > + * function just sets struct pvr_device->regs to %NULL.
> > + */
> > +static __always_inline void
> > +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> > +{
> > +     pvr_dev->regs = NULL;
> 
> But if you do, I guess clearing the regs_resource pointer would be nice too.

pvr_device_reg_fini() will be gone in the next version.

> 
> > +}
> > +
> > +/**
> > + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> > + * struct pvr_device->mem_clk.
> > + *
> > + * Three clocks are required by the PowerVR device: core, sys and mem. On
> > + * return, this function guarantees that the clocks are in one of the following
> > + * states:
> > + *
> > + *  * All successfully initialized,
> > + *  * Core errored, sys and mem uninitialized,
> > + *  * Core deinitialized, sys errored, mem uninitialized, or
> > + *  * Core and sys deinitialized, mem errored.
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by devm_clk_get(), or
> > + *  * Any error returned by clk_prepare_enable().
> > + */
> > +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct clk *core_clk;
> > +     struct clk *sys_clk;
> > +     struct clk *mem_clk;
> > +     int err;
> > +
> > +     pvr_dev->core_clk = NULL;
> > +     pvr_dev->sys_clk = NULL;
> > +     pvr_dev->mem_clk = NULL;
> > +
> > +     core_clk = devm_clk_get(drm_dev->dev, "core");
> > +     if (IS_ERR(core_clk)) {
> > +             err = PTR_ERR(core_clk);
> > +             drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
> > +             goto err_out;
> > +     }
> > +
> > +     sys_clk = devm_clk_get(drm_dev->dev, "sys");
> > +     if (IS_ERR(sys_clk))
> > +             sys_clk = NULL;
> > +
> > +     mem_clk = devm_clk_get(drm_dev->dev, "mem");
> > +     if (IS_ERR(mem_clk))
> > +             mem_clk = NULL;
> 
> If those two are optionals, you can use devm_clk_get_optional. This has
> the advantage of only ignoring the case where the clock isn't there, but
> not the other error conditions.

Ack

> 
> > +
> > +     err = clk_prepare(core_clk);
> > +     if (err)
> > +             goto err_out;
> > +
> > +     if (sys_clk) {
> > +             err = clk_prepare(sys_clk);
> 
> It's valid to call clk_prepare(NULL), which will be a nop. I think you
> can remove the check on the null pointer.

Ack

> 
> Also do you need to split prepare and enable? The usual reason to do so
> is that prepare can sleep and thus can't be called in an atomic context,
> but enable can. I can't think of a case where that would happen for a
> GPU though, so you should probably do both at once?

No, we don't need the split, so we'll fix this.

> 
> You should also consider using the devm variants there like
> devm_clk_get_optional_prepared and similar, depending on how you address
> the comments above. This will greatly simplify your exit / cleanup path.

Ack

> 
> > +             if (err)
> > +                     goto err_deinit_core_clk;
> > +     }
> > +
> > +     if (mem_clk) {
> > +             err = clk_prepare(mem_clk);
> > +             if (err)
> > +                     goto err_deinit_sys_clk;
> > +     }
> > +
> > +     pvr_dev->core_clk = core_clk;
> > +     pvr_dev->sys_clk = sys_clk;
> > +     pvr_dev->mem_clk = mem_clk;
> > +
> > +     return 0;
> > +
> > +err_deinit_sys_clk:
> > +     if (sys_clk)
> > +             clk_disable_unprepare(sys_clk);
> > +err_deinit_core_clk:
> > +     clk_disable_unprepare(core_clk);
> 
> Since the clocks haven't been enabled yet, this will create an enable
> count imbalance.

Ack

> 
> > +/**
> > + * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * The regulator is not a required devicetree property. If it is not present then this function will
> > + * succeed, but &pvr_device->regulator will be %NULL.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_regulator_get().
> > + */
> > +static int
> > +pvr_device_regulator_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct regulator *regulator;
> > +     int err;
> > +
> > +     regulator = devm_regulator_get(drm_dev->dev, "power");
> > +     if (IS_ERR(regulator)) {
> > +             err = PTR_ERR(regulator);
> > +             /* Regulator is not required, so ENODEV is allowed here. */
> > +             if (err != -ENODEV)
> > +                     goto err_out;
> 
> This is what regulator_get_optional does :)

Thanks for point it out :)

> 
> > +/**
> > + * pvr_device_init() - Initialize a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * If this function returns successfully, the device will have been fully
> > + * initialized. Otherwise, any parts of the device initialized before an error
> > + * occurs will be de-initialized before returning.
> > + *
> > + * NOTE: The initialization steps currently taken are the bare minimum required
> > + *       to read from the control registers. The device is unlikely to function
> > + *       until further initialization steps are added. [This note should be
> > + *       removed when that happens.]
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by pvr_device_reg_init(),
> > + *  * Any error returned by pvr_device_clk_init(), or
> > + *  * Any error returned by pvr_device_gpu_init().
> > + */
> > +int
> > +pvr_device_init(struct pvr_device *pvr_dev)
> > +{
> > +     int err;
> > +
> > +     /* Enable and initialize clocks required for the device to operate. */
> > +     err = pvr_device_clk_init(pvr_dev);
> > +     if (err)
> > +             goto err_out;
> > +
> > +     err = pvr_device_regulator_init(pvr_dev);
> > +     if (err)
> > +             goto err_device_clk_fini;
> > +
> > +     /* Map the control registers into memory. */
> > +     err = pvr_device_reg_init(pvr_dev);
> > +     if (err)
> > +             goto err_device_reg_fini;
> > +
> > +     return 0;
> > +
> > +err_device_reg_fini:
> > +     pvr_device_reg_fini(pvr_dev);
> 
> I think you got that one wrong, I don't think you should call
> pvr_device_reg_fini if pvr_device_reg_init failed?
> 
> But switching to devm will solve this too.

Ack

> 
> >  /**
> >   * struct pvr_device - powervr-specific wrapper for &struct drm_device
> > @@ -26,6 +42,29 @@ struct pvr_device {
> >        * from_pvr_device().
> >        */
> >       struct drm_device base;
> > +
> > +     /** @regs_resource: Resource representing device control registers. */
> > +     struct resource *regs_resource;
> > +
> > +     /**
> > +      * @regs: Device control registers.
> > +      *
> > +      * These are mapped into memory when the device is initialized; that
> > +      * location is where this pointer points.
> > +      */
> > +     void __iomem *regs;
> > +
> > +     /** @core_clk: General core clock. */
> > +     struct clk *core_clk;
> > +
> > +     /** @sys_clk: System bus clock. */
> > +     struct clk *sys_clk;
> > +
> > +     /** @mem_clk: Memory clock. */
> > +     struct clk *mem_clk;
> 
> It's not really a review but more of a suggestion: the semantics around
> the clocks vary from one vendor to another, so having a bit more context
> here in what those clocks are used for by the hardware and how we should
> use them in the driver would be great.

We'll flesh this out a bit more in the next version.

> 
> > +     /** @regulator: Power regulator. */
> > +     struct regulator *regulator;
> >  };
> > 
> >  /**
> > @@ -72,6 +111,181 @@ to_pvr_file(struct drm_file *file)
> >       return file->driver_priv;
> >  }
> > 
> > +int pvr_device_init(struct pvr_device *pvr_dev);
> > +void pvr_device_fini(struct pvr_device *pvr_dev);
> > +
> > +int
> > +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);
> 
> Do we really need a wrapper around clk_get_rate(pvr_dev->core_clk) ?

No, we'll drop this.

> 
> Also, this function is defined in patch 7.
> 
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> > index e203a2370f55..48a870715426 100644
> > --- a/drivers/gpu/drm/imagination/pvr_drv.c
> > +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> > @@ -485,12 +485,19 @@ pvr_probe(struct platform_device *plat_dev)
> > 
> >       platform_set_drvdata(plat_dev, drm_dev);
> > 
> > -     err = drm_dev_register(drm_dev, 0);
> > +     err = pvr_device_init(pvr_dev);
> >       if (err)
> >               goto err_out;
> > 
> > +     err = drm_dev_register(drm_dev, 0);
> > +     if (err)
> > +             goto err_device_fini;
> > +
> >       return 0;
> > 
> > +err_device_fini:
> > +     pvr_device_fini(pvr_dev);
> > +
> >  err_out:
> >       return err;
> >  }
> > @@ -499,8 +506,10 @@ static int
> >  pvr_remove(struct platform_device *plat_dev)
> >  {
> >       struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> > +     struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
> > 
> >       drm_dev_unregister(drm_dev);
> > +     pvr_device_fini(pvr_dev);
> > 
> >       return 0;
> >  }
> 
> There's one gotcha you'll need to consider: DRM devices are unregistered
> and freed not when the device goes away but when the last (userspace)
> application closed its file descriptor.
> 
> This means that you have an undefined window during which devm will have
> run (and thus cleaned up the resources) but the driver is still live and
> can still get called.
> 
> So you need to protect all device resources access (registers, clocks
> and regulator in your case I guess?) by calls to
> drm_dev_enter/drm_dev_exit and use drm_dev_unplug instead of
> drm_dev_unregister.

Thanks for highlighting this. We'll make sure we use these functions in the next
version.

Thanks
Frank

> 
> Maxime




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux