Re: [PATCH 13/64] drm/vc4: hvs: Protect device resources after removal

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

 



Hi Maxime

On Fri, 10 Jun 2022 at 10:30, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Whenever the device and driver are unbound, the main device and all the
> subdevices will be removed by calling their unbind() method.
>
> However, the DRM device itself will only be freed when the last user will
> have closed it.
>
> It means that there is a time window where the device and its resources
> aren't there anymore, but the userspace can still call into our driver.
>
> Fortunately, the DRM framework provides the drm_dev_enter() and
> drm_dev_exit() functions to make sure our underlying device is still there
> for the section protected by those calls. Let's add them to the HVS driver.

The framework appears to rely on the remove function calling
drm_dev_unplug instead of drm_dev_unregister, but I haven't seen a
patch that makes that change in the vc4 driver.
Have I missed it, or is there some other route to set the unplugged
flag that drm_dev_enter/exit are relying on?

  Dave

> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h |   1 +
>  drivers/gpu/drm/vc4/vc4_hvs.c | 106 +++++++++++++++++++++++++++++++---
>  2 files changed, 99 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index aa4c5910ea05..080deae55f64 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -317,6 +317,7 @@ struct vc4_v3d {
>  };
>
>  struct vc4_hvs {
> +       struct drm_device *dev;
>         struct platform_device *pdev;
>         void __iomem *regs;
>         u32 __iomem *dlist;
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 2a58fc421cf6..483053e7b14f 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -25,6 +25,7 @@
>  #include <linux/platform_device.h>
>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_vblank.h>
>
>  #include "vc4_drv.h"
> @@ -66,11 +67,15 @@ static const struct debugfs_reg32 hvs_regs[] = {
>
>  void vc4_hvs_dump_state(struct vc4_hvs *hvs)
>  {
> +       struct drm_device *drm = hvs->dev;
>         struct drm_printer p = drm_info_printer(&hvs->pdev->dev);
> -       int i;
> +       int idx, i;
>
>         drm_print_regset32(&p, &hvs->regset);
>
> +       if (!drm_dev_enter(drm, &idx))
> +               return;
> +
>         DRM_INFO("HVS ctx:\n");
>         for (i = 0; i < 64; i += 4) {
>                 DRM_INFO("0x%08x (%s): 0x%08x 0x%08x 0x%08x 0x%08x\n",
> @@ -80,6 +85,8 @@ void vc4_hvs_dump_state(struct vc4_hvs *hvs)
>                          readl((u32 __iomem *)hvs->dlist + i + 2),
>                          readl((u32 __iomem *)hvs->dlist + i + 3));
>         }
> +
> +       drm_dev_exit(idx);
>  }
>
>  static int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data)
> @@ -132,14 +139,18 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
>                                         struct drm_mm_node *space,
>                                         const u32 *kernel)
>  {
> -       int ret, i;
> +       struct drm_device *drm = hvs->dev;
> +       int idx, ret, i;
>         u32 __iomem *dst_kernel;
>
> +       if (!drm_dev_enter(drm, &idx))
> +               return -ENODEV;
> +
>         ret = drm_mm_insert_node(&hvs->dlist_mm, space, VC4_KERNEL_DWORDS);
>         if (ret) {
>                 DRM_ERROR("Failed to allocate space for filter kernel: %d\n",
>                           ret);
> -               return ret;
> +               goto err_dev_exit;
>         }
>
>         dst_kernel = hvs->dlist + space->start;
> @@ -153,16 +164,26 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
>                 }
>         }
>
> +       drm_dev_exit(idx);
>         return 0;
> +
> +err_dev_exit:
> +       drm_dev_exit(idx);
> +       return ret;
>  }
>
>  static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>                              struct vc4_crtc *vc4_crtc)
>  {
> +       struct drm_device *drm = hvs->dev;
>         struct drm_crtc *crtc = &vc4_crtc->base;
>         struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> +       int idx;
>         u32 i;
>
> +       if (!drm_dev_enter(drm, &idx))
> +               return;
> +
>         /* The LUT memory is laid out with each HVS channel in order,
>          * each of which takes 256 writes for R, 256 for G, then 256
>          * for B.
> @@ -177,6 +198,8 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>                 HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_g[i]);
>         for (i = 0; i < crtc->gamma_size; i++)
>                 HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
> +
> +       drm_dev_exit(idx);
>  }
>
>  static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs,
> @@ -198,7 +221,12 @@ static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs,
>
>  u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs, unsigned int fifo)
>  {
> +       struct drm_device *drm = hvs->dev;
>         u8 field = 0;
> +       int idx;
> +
> +       if (!drm_dev_enter(drm, &idx))
> +               return 0;
>
>         switch (fifo) {
>         case 0:
> @@ -215,6 +243,7 @@ u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs, unsigned int fifo)
>                 break;
>         }
>
> +       drm_dev_exit(idx);
>         return field;
>  }
>
> @@ -226,6 +255,12 @@ int vc4_hvs_get_fifo_from_output(struct vc4_hvs *hvs, unsigned int output)
>         if (!hvs->hvs5)
>                 return output;
>
> +       /*
> +        * NOTE: We should probably use drm_dev_enter()/drm_dev_exit()
> +        * here, but this function is only used during the DRM device
> +        * initialization, so we should be fine.
> +        */
> +
>         switch (output) {
>         case 0:
>                 return 0;
> @@ -273,12 +308,17 @@ int vc4_hvs_get_fifo_from_output(struct vc4_hvs *hvs, unsigned int output)
>  static int vc4_hvs_init_channel(struct vc4_hvs *hvs, struct drm_crtc *crtc,
>                                 struct drm_display_mode *mode, bool oneshot)
>  {
> +       struct drm_device *drm = hvs->dev;
>         struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
>         struct vc4_crtc_state *vc4_crtc_state = to_vc4_crtc_state(crtc->state);
>         unsigned int chan = vc4_crtc_state->assigned_channel;
>         bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
>         u32 dispbkgndx;
>         u32 dispctrl;
> +       int idx;
> +
> +       if (!drm_dev_enter(drm, &idx))
> +               return -ENODEV;
>
>         HVS_WRITE(SCALER_DISPCTRLX(chan), 0);
>         HVS_WRITE(SCALER_DISPCTRLX(chan), SCALER_DISPCTRLX_RESET);
> @@ -320,13 +360,21 @@ static int vc4_hvs_init_channel(struct vc4_hvs *hvs, struct drm_crtc *crtc,
>          */
>         vc4_hvs_lut_load(hvs, vc4_crtc);
>
> +       drm_dev_exit(idx);
> +
>         return 0;
>  }
>
>  void vc4_hvs_stop_channel(struct vc4_hvs *hvs, unsigned int chan)
>  {
> +       struct drm_device *drm = hvs->dev;
> +       int idx;
> +
> +       if (!drm_dev_enter(drm, &idx))
> +               return;
> +
>         if (HVS_READ(SCALER_DISPCTRLX(chan)) & SCALER_DISPCTRLX_ENABLE)
> -               return;
> +               goto out;
>
>         HVS_WRITE(SCALER_DISPCTRLX(chan),
>                   HVS_READ(SCALER_DISPCTRLX(chan)) | SCALER_DISPCTRLX_RESET);
> @@ -343,6 +391,9 @@ void vc4_hvs_stop_channel(struct vc4_hvs *hvs, unsigned int chan)
>         WARN_ON_ONCE((HVS_READ(SCALER_DISPSTATX(chan)) &
>                       (SCALER_DISPSTATX_FULL | SCALER_DISPSTATX_EMPTY)) !=
>                      SCALER_DISPSTATX_EMPTY);
> +
> +out:
> +       drm_dev_exit(idx);
>  }
>
>  int vc4_hvs_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state)
> @@ -384,9 +435,15 @@ static void vc4_hvs_install_dlist(struct drm_crtc *crtc)
>         struct vc4_dev *vc4 = to_vc4_dev(dev);
>         struct vc4_hvs *hvs = vc4->hvs;
>         struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> +       int idx;
> +
> +       if (!drm_dev_enter(dev, &idx))
> +               return;
>
>         HVS_WRITE(SCALER_DISPLISTX(vc4_state->assigned_channel),
>                   vc4_state->mm.start);
> +
> +       drm_dev_exit(idx);
>  }
>
>  static void vc4_hvs_update_dlist(struct drm_crtc *crtc)
> @@ -471,6 +528,10 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>         bool enable_bg_fill = false;
>         u32 __iomem *dlist_start = vc4->hvs->dlist + vc4_state->mm.start;
>         u32 __iomem *dlist_next = dlist_start;
> +       int idx;
> +
> +       if (!drm_dev_enter(dev, &idx))
> +               return;
>
>         if (debug_dump_regs) {
>                 DRM_INFO("CRTC %d HVS before:\n", drm_crtc_index(crtc));
> @@ -541,26 +602,44 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>                 DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc));
>                 vc4_hvs_dump_state(hvs);
>         }
> +
> +       drm_dev_exit(idx);
>  }
>
>  void vc4_hvs_mask_underrun(struct vc4_hvs *hvs, int channel)
>  {
> -       u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +       struct drm_device *drm = hvs->dev;
> +       u32 dispctrl;
> +       int idx;
>
> +       if (!drm_dev_enter(drm, &idx))
> +               return;
> +
> +       dispctrl = HVS_READ(SCALER_DISPCTRL);
>         dispctrl &= ~SCALER_DISPCTRL_DSPEISLUR(channel);
>
>         HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +
> +       drm_dev_exit(idx);
>  }
>
>  void vc4_hvs_unmask_underrun(struct vc4_hvs *hvs, int channel)
>  {
> -       u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +       struct drm_device *drm = hvs->dev;
> +       u32 dispctrl;
> +       int idx;
>
> +       if (!drm_dev_enter(drm, &idx))
> +               return;
> +
> +       dispctrl = HVS_READ(SCALER_DISPCTRL);
>         dispctrl |= SCALER_DISPCTRL_DSPEISLUR(channel);
>
>         HVS_WRITE(SCALER_DISPSTAT,
>                   SCALER_DISPSTAT_EUFLOW(channel));
>         HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +
> +       drm_dev_exit(idx);
>  }
>
>  static void vc4_hvs_report_underrun(struct drm_device *dev)
> @@ -581,6 +660,17 @@ static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
>         u32 control;
>         u32 status;
>
> +       /*
> +        * NOTE: We don't need to protect the register access using
> +        * drm_dev_enter() there because the interrupt handler lifetime
> +        * is tied to the device itself, and not to the DRM device.
> +        *
> +        * So when the device will be gone, one of the first thing we
> +        * will be doing will be to unregister the interrupt handler,
> +        * and then unregister the DRM device. drm_dev_enter() would
> +        * thus always succeed if we are here.
> +        */
> +
>         status = HVS_READ(SCALER_DISPSTAT);
>         control = HVS_READ(SCALER_DISPCTRL);
>
> @@ -613,10 +703,10 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>         u32 dispctrl;
>         u32 reg;
>
> -       hvs = devm_kzalloc(&pdev->dev, sizeof(*hvs), GFP_KERNEL);
> +       hvs = drmm_kzalloc(drm, sizeof(*hvs), GFP_KERNEL);
>         if (!hvs)
>                 return -ENOMEM;
> -
> +       hvs->dev = drm;
>         hvs->pdev = pdev;
>
>         if (of_device_is_compatible(pdev->dev.of_node, "brcm,bcm2711-hvs"))
> --
> 2.36.1
>



[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