[PATCH v3 09/17] drm/imagination: Implement power management

[Frank Binns]

Hi Maxime,

On Fri, 2023-07-07 at 14:48 +0200, Maxime Ripard wrote:
> On Tue, Jun 13, 2023 at 03:47:52PM +0100, Sarah Walker wrote:
> > @@ -503,21 +506,31 @@ pvr_device_init(struct pvr_device *pvr_dev)
> >       if (err)
> >               goto err_device_clk_fini;
> > 
> > +     /* Explicitly power the GPU so we can access control registers before the FW is booted. */
> > +     err = pm_runtime_resume_and_get(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_clk_fini;
> > +             goto err_pm_runtime_put;
> > 
> >       /* Perform GPU-specific initialization steps. */
> >       err = pvr_device_gpu_init(pvr_dev);
> >       if (err)
> >               goto err_device_reg_fini;
> > 
> > +     pm_runtime_put_autosuspend(dev);
> > +
> 
> You probably can use pm_runtime_put here

Ack

> 
> > @@ -532,12 +545,17 @@ pvr_device_init(struct pvr_device *pvr_dev)
> >  void
> >  pvr_device_fini(struct pvr_device *pvr_dev)
> >  {
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct device *dev = drm_dev->dev;
> > +
> >       /*
> >        * Deinitialization stages are performed in reverse order compared to
> >        * the initialization stages in pvr_device_init().
> >        */
> > +     pm_runtime_get_sync(dev);
> >       pvr_device_gpu_fini(pvr_dev);
> >       pvr_device_reg_fini(pvr_dev);
> 
> AFAIK gpu_fini releases the firmware and reg_fini drops the register
> mapping address, I don't think you need the device powered up for that.

Very true, we'll fix that :)

> 
> > @@ -130,6 +133,20 @@ struct pvr_device {
> > 
> >       /** @fw_dev: Firmware related data. */
> >       struct pvr_fw_device fw_dev;
> > +
> > +     struct {
> > +             /** @work: Work item for watchdog callback. */
> > +             struct delayed_work work;
> > +
> > +             /** @old_kccb_cmds_executed: KCCB command execution count at last watchdog poll. */
> > +             u32 old_kccb_cmds_executed;
> > +
> > +             /** @kccb_stall_count: Number of watchdog polls KCCB has been stalled for. */
> > +             u32 kccb_stall_count;
> > +     } watchdog;
> > +
> > +     /** @lost: %true if the device has been lost. */
> > +     bool lost;
> 
> The device being "lost" isn't clear to me. Does it mean it's
> unresponsive or stuck somehow?

The former. For example, this will be set when the firmware has become
unresponsive and hard resetting the GPU doesn't resolve this. We'll update the
docs to clarify.

> 
> > @@ -1285,9 +1303,15 @@ pvr_probe(struct platform_device *plat_dev)
> > 
> >       platform_set_drvdata(plat_dev, drm_dev);
> > 
> > +     devm_pm_runtime_enable(&plat_dev->dev);
> > +
> > +     pm_runtime_set_autosuspend_delay(&plat_dev->dev, 50);
> > +     pm_runtime_use_autosuspend(&plat_dev->dev);
> > +     pvr_power_init(pvr_dev);
> 
> The name threw me off a bit. It doesn't look like it's power related but
> you init the watchdog timer?

Yup, we'll update the name to reflect its real purpose.

> 
> I can't really tell from that patch, but if it's not done in a later
> patch you'll probably need a call to sprinkle your driver with a few
> _mark_last_busy calls (at least in the job submission path?)

We weren't doing this before so we'll add some calls in.

Thanks
Frank

> 
> Maxime