On Tue, Apr 26, 2016 at 1:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

Except for the ->lasclose driver callback evrything in drm_lastclose() is all legacy cruft and can be hidden. Which means another dev->struct_mutex site disappears entirely for modern drivers!

Also while at it change the return value of drm_lastclose to void since it will always succeed. No one checks the return value of close() anyway, ever.

v2: Move misplaced hunk, spotted by 0day.

Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_fops.c | 42 +++++++++++++++++++----------------------- drivers/gpu/drm/drm_internal.h | 2 +- 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7b5a13cda7a6..c3d0aaac0669 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -381,14 +381,26 @@ static void drm_events_release(struct drm_file *file_priv) */ static void drm_legacy_dev_reinit(struct drm_device *dev) {

•   if (drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return;
  

•   if (dev->irq_enabled)
  

•           drm_irq_uninstall(dev);
  

•   mutex_lock(&dev->struct_mutex);
  

•   drm_legacy_agp_clear(dev);
  

•   drm_legacy_sg_cleanup(dev);
  

•   drm_legacy_vma_flush(dev);
  

•   drm_legacy_dma_takedown(dev);
  

•   mutex_unlock(&dev->struct_mutex);
  
    dev->sigdata.lock = NULL;
  
    dev->context_flag = 0;
    dev->last_context = 0;
    dev->if_version = 0;
  

•   DRM_DEBUG("lastclose completed\n");
  

}

/* @@ -400,7 +412,7 @@ static void drm_legacy_dev_reinit(struct drm_device *dev)

• \sa drm_device

*/ -int drm_lastclose(struct drm_device * dev) +void drm_lastclose(struct drm_device * dev) { DRM_DEBUG("\n");

@@ -408,23 +420,8 @@ int drm_lastclose(struct drm_device * dev) dev->driver->lastclose(dev); DRM_DEBUG("driver lastclose completed\n");

•   if (dev->irq_enabled && !drm_core_check_feature(dev, DRIVER_MODESET))
  

•           drm_irq_uninstall(dev);
  

•   mutex_lock(&dev->struct_mutex);
  

•   drm_legacy_agp_clear(dev);
  

•   drm_legacy_sg_cleanup(dev);
  

•   drm_legacy_vma_flush(dev);
  

•   drm_legacy_dma_takedown(dev);
  

•   mutex_unlock(&dev->struct_mutex);
  

•   drm_legacy_dev_reinit(dev);
  

•   DRM_DEBUG("lastclose completed\n");
  

•   return 0;
  

•   if (!drm_core_check_feature(dev, DRIVER_MODESET))
  

•           drm_legacy_dev_reinit(dev);
  

}

/** @@ -445,7 +442,6 @@ int drm_release(struct inode *inode, struct file *filp) struct drm_file *file_priv = filp->private_data; struct drm_minor *minor = file_priv->minor; struct drm_device *dev = minor->dev;

•   int retcode = 0;
  
    mutex_lock(&drm_global_mutex);
  
  

@@ -538,7 +534,7 @@ int drm_release(struct inode *inode, struct file *filp) */

    if (!--dev->open_count) {

•           retcode = drm_lastclose(dev);
  

•           drm_lastclose(dev);
            if (drm_device_is_unplugged(dev))
                    drm_put_dev(dev);
    }
  

@@ -546,7 +542,7 @@ int drm_release(struct inode *inode, struct file *filp)

    drm_minor_release(minor);

•   return retcode;
  

•   return 0;
  

} EXPORT_SYMBOL(drm_release);

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 43cbda3306ac..c81ff4769e7b 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -26,7 +26,7 @@ extern unsigned int drm_timestamp_monotonic;

/* drm_fops.c */ extern struct mutex drm_global_mutex; -int drm_lastclose(struct drm_device *dev); +void drm_lastclose(struct drm_device *dev);

/* drm_pci.c */ int drm_pci_set_unique(struct drm_device *dev, -- 2.8.1

---

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

On Tue, Apr 26, 2016 at 1:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

Like in

commit 0e975980d435d58df2d430d688b8c18778b42218 Author: Peter Antoine peter.antoine@intel.com Date: Tue Jun 23 08:18:49 2015 +0100

drm: Turn off Legacy Context Functions

we need to again make an exception for nouveau, but everyone else really doesn't need this.

Cc: Peter Antoine peter.antoine@intel.com Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Not really familiar with why nouveau needs this, but the logic seems correct. Acked-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_bufs.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index d92db7007f62..e8a12a4fd400 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -396,6 +396,10 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void *data, if (!(capable(CAP_SYS_ADMIN) || map->type == _DRM_AGP || map->type == _DRM_SHM)) return -EPERM;

•   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
  

•       drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return -EINVAL;
  

•   err = drm_addmap_core(dev, map->offset, map->size, map->type,
                          map->flags, &maplist);
  
  

@@ -438,6 +442,10 @@ int drm_legacy_getmap_ioctl(struct drm_device *dev, void *data, int idx; int i;

•   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
  

•       drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return -EINVAL;
  

•   idx = map->offset;
    if (idx < 0)
            return -EINVAL;
  

@@ -569,6 +577,10 @@ int drm_legacy_rmmap_ioctl(struct drm_device *dev, void *data, struct drm_map_list *r_list; int ret;

•   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
  

•       drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return -EINVAL;
  

•   mutex_lock(&dev->struct_mutex);
    list_for_each_entry(r_list, &dev->maplist, head) {
            if (r_list->map &&
  

-- 2.8.1

---

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

On Wed, Apr 27, 2016 at 09:21:29AM +0200, Daniel Vetter wrote:

From: Benjamin Gaignard benjamin.gaignard@linaro.org

No need to protect debugfs functions with dev->struct_mutex

Signed-off-by: Benjamin Gaignard benjamin.gaignard@linaro.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

Oops, resent wrong patch, but the iteration that dropped the nouveau special case was busted anyway. -Daniel

---

drivers/gpu/drm/sti/sti_cursor.c | 7 ------- drivers/gpu/drm/sti/sti_drv.c | 6 ------ drivers/gpu/drm/sti/sti_dvo.c | 7 ------- drivers/gpu/drm/sti/sti_gdp.c | 14 -------------- drivers/gpu/drm/sti/sti_hda.c | 7 ------- drivers/gpu/drm/sti/sti_hdmi.c | 7 ------- drivers/gpu/drm/sti/sti_hqvdp.c | 7 ------- drivers/gpu/drm/sti/sti_mixer.c | 7 ------- drivers/gpu/drm/sti/sti_tvout.c | 7 ------- drivers/gpu/drm/sti/sti_vid.c | 7 ------- 10 files changed, 76 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c index 3abb400151ac..e6ee4c526665 100644 --- a/drivers/gpu/drm/sti/sti_cursor.c +++ b/drivers/gpu/drm/sti/sti_cursor.c @@ -103,12 +103,6 @@ static int cursor_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_cursor *cursor = (struct sti_cursor *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "%s: (vaddr = 0x%p)", sti_plane_to_str(&cursor->plane), cursor->regs);

@@ -127,7 +121,6 @@ static int cursor_dbg_show(struct seq_file *s, void *data) DBGFS_DUMP(CUR_AWE); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c index 6bd6abaa5a70..a8eece20b6b4 100644 --- a/drivers/gpu/drm/sti/sti_drv.c +++ b/drivers/gpu/drm/sti/sti_drv.c @@ -72,11 +72,6 @@ static int sti_drm_fps_dbg_show(struct seq_file *s, void *data) struct drm_info_node *node = s->private; struct drm_device *dev = node->minor->dev; struct drm_plane *p;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  list_for_each_entry(p, &dev->mode_config.plane_list, head) { struct sti_plane *plane = to_sti_plane(p);

@@ -86,7 +81,6 @@ static int sti_drm_fps_dbg_show(struct seq_file *s, void *data) plane->fps_info.fips_str); }

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c index 25f76632002c..d439128e6309 100644 --- a/drivers/gpu/drm/sti/sti_dvo.c +++ b/drivers/gpu/drm/sti/sti_dvo.c @@ -177,12 +177,6 @@ static int dvo_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_dvo *dvo = (struct sti_dvo *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "DVO: (vaddr = 0x%p)", dvo->regs); DBGFS_DUMP(DVO_AWG_DIGSYNC_CTRL);

@@ -193,7 +187,6 @@ static int dvo_dbg_show(struct seq_file *s, void *data) dvo_dbg_awg_microcode(s, dvo->regs + DVO_DIGSYNC_INSTR_I); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c index ff3d3e7e7704..3d098f1ab919 100644 --- a/drivers/gpu/drm/sti/sti_gdp.c +++ b/drivers/gpu/drm/sti/sti_gdp.c @@ -207,14 +207,8 @@ static int gdp_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_gdp *gdp = (struct sti_gdp *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev; struct drm_plane *drm_plane = &gdp->plane.drm_plane; struct drm_crtc *crtc = drm_plane->crtc;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "%s: (vaddr = 0x%p)", sti_plane_to_str(&gdp->plane), gdp->regs);

@@ -247,7 +241,6 @@ static int gdp_dbg_show(struct seq_file *s, void *data) seq_printf(s, " Connected to DRM CRTC #%d (%s)\n", crtc->base.id, sti_mixer_to_str(to_sti_mixer(crtc)));

• mutex_unlock(&dev->struct_mutex); return 0;

}

@@ -278,13 +271,7 @@ static int gdp_node_dbg_show(struct seq_file *s, void *arg) { struct drm_info_node *node = s->private; struct sti_gdp *gdp = (struct sti_gdp *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev; unsigned int b;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  for (b = 0; b < GDP_NODE_NB_BANK; b++) { seq_printf(s, "\n%s[%d].top", sti_plane_to_str(&gdp->plane), b);

@@ -293,7 +280,6 @@ static int gdp_node_dbg_show(struct seq_file *s, void *arg) gdp_node_dump_node(s, gdp->node_list[b].btm_field); }

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c index ec0d017eaf1a..d0e1647ce41a 100644 --- a/drivers/gpu/drm/sti/sti_hda.c +++ b/drivers/gpu/drm/sti/sti_hda.c @@ -375,12 +375,6 @@ static int hda_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_hda *hda = (struct sti_hda *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "HD Analog: (vaddr = 0x%p)", hda->regs); DBGFS_DUMP(HDA_ANA_CFG);

@@ -396,7 +390,6 @@ static int hda_dbg_show(struct seq_file *s, void *data) hda_dbg_video_dacs_ctrl(s, hda->video_dacs_ctrl); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index 6ef0715bd5b9..85545ebf88d3 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -628,12 +628,6 @@ static int hdmi_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_hdmi *hdmi = (struct sti_hdmi *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "HDMI: (vaddr = 0x%p)", hdmi->regs); DBGFS_DUMP("\n", HDMI_CFG);

@@ -690,7 +684,6 @@ static int hdmi_dbg_show(struct seq_file *s, void *data) DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_VENDOR); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c index e05b0dc523ff..6230565048c3 100644 --- a/drivers/gpu/drm/sti/sti_hqvdp.c +++ b/drivers/gpu/drm/sti/sti_hqvdp.c @@ -554,14 +554,8 @@ static int hqvdp_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_hqvdp *hqvdp = (struct sti_hqvdp *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev; int cmd, cmd_offset, infoxp70; void *virt;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "%s: (vaddr = 0x%p)", sti_plane_to_str(&hqvdp->plane), hqvdp->regs);

@@ -629,7 +623,6 @@ static int hqvdp_dbg_show(struct seq_file *s, void *data)

seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_mixer.c b/drivers/gpu/drm/sti/sti_mixer.c index e7425c38fc93..cc89d75273df 100644 --- a/drivers/gpu/drm/sti/sti_mixer.c +++ b/drivers/gpu/drm/sti/sti_mixer.c @@ -150,12 +150,6 @@ static int mixer_dbg_show(struct seq_file *s, void *arg) { struct drm_info_node *node = s->private; struct sti_mixer *mixer = (struct sti_mixer *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "%s: (vaddr = 0x%p)", sti_mixer_to_str(mixer), mixer->regs);

@@ -175,7 +169,6 @@ static int mixer_dbg_show(struct seq_file *s, void *arg) mixer_dbg_mxn(s, mixer->regs + GAM_MIXER_MX0); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c index 2c99016443e5..d641a5fe512b 100644 --- a/drivers/gpu/drm/sti/sti_tvout.c +++ b/drivers/gpu/drm/sti/sti_tvout.c @@ -514,13 +514,7 @@ static int tvout_dbg_show(struct seq_file *s, void *data) { struct drm_info_node *node = s->private; struct sti_tvout *tvout = (struct sti_tvout *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev; struct drm_crtc *crtc;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "TVOUT: (vaddr = 0x%p)", tvout->regs);

@@ -586,7 +580,6 @@ static int tvout_dbg_show(struct seq_file *s, void *data) DBGFS_DUMP(TVO_AUX_IN_VID_FORMAT); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

diff --git a/drivers/gpu/drm/sti/sti_vid.c b/drivers/gpu/drm/sti/sti_vid.c index 5a2c5dc3687b..cba32140a763 100644 --- a/drivers/gpu/drm/sti/sti_vid.c +++ b/drivers/gpu/drm/sti/sti_vid.c @@ -91,12 +91,6 @@ static int vid_dbg_show(struct seq_file *s, void *arg) { struct drm_info_node *node = s->private; struct sti_vid *vid = (struct sti_vid *)node->info_ent->data;

• struct drm_device *dev = node->minor->dev;

• int ret;

• ret = mutex_lock_interruptible(&dev->struct_mutex);

• if (ret)

• return ret;
  
  

  seq_printf(s, "VID: (vaddr= 0x%p)", vid->regs);

@@ -121,7 +115,6 @@ static int vid_dbg_show(struct seq_file *s, void *arg) DBGFS_DUMP(VID_CSAT); seq_puts(s, "\n");

• mutex_unlock(&dev->struct_mutex); return 0;

}

-- 2.8.1

On Tue, Apr 26, 2016 at 1:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

And again make sure it's a no-op for modern drivers, again with the exception of nouveau. Another case of dev->struct_mutex gone for modern drivers!

v2: Also add a DRIVER_* check like for all other maps functions to really short-circuit the code. And give drm_legacy_rmmap used by the dev unregister code the same treatment.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Just one comment below.

Acked-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_bufs.c | 28 ++++++++++++++++++++++++---- drivers/gpu/drm/drm_drv.c | 10 +--------- include/drm/drm_legacy.h | 4 +++- 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index e8a12a4fd400..5a51633da033 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -542,18 +542,38 @@ int drm_legacy_rmmap_locked(struct drm_device *dev, struct drm_local_map *map) } EXPORT_SYMBOL(drm_legacy_rmmap_locked);

-int drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map) +void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map) {

•   int ret;
  

•   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
  

•       drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return;
  
    mutex_lock(&dev->struct_mutex);
  

•   ret = drm_legacy_rmmap_locked(dev, map);
  

•   drm_legacy_rmmap_locked(dev, map);
    mutex_unlock(&dev->struct_mutex);
  
  

•   return ret;
  

•   return;
  

Can drop this.

} EXPORT_SYMBOL(drm_legacy_rmmap);

+void drm_legacy_master_rmmaps(struct drm_device *dev, struct drm_master *master) +{

•   struct drm_map_list *r_list, *list_temp;
  

•   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
  

•       drm_core_check_feature(dev, DRIVER_MODESET))
  

•           return;
  

•   mutex_lock(&dev->struct_mutex);
  

•   list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) {
  

•           if (r_list->master == master) {
  

•                   drm_legacy_rmmap_locked(dev, r_list->map);
  

•                   r_list = NULL;
  

•           }
  

•   }
  

•   mutex_unlock(&dev->struct_mutex);
  

+}

/* The rmmap ioctl appears to be unnecessary. All mappings are torn down on

• the last close of the device, and this is necessary for cleanup when things
• exit uncleanly. Therefore, having userland manually remove mappings seems

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 55273f8f3acb..e1fb52d4f72c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -121,19 +121,11 @@ static void drm_master_destroy(struct kref *kref) { struct drm_master *master = container_of(kref, struct drm_master, refcount); struct drm_device *dev = master->minor->dev;

•   struct drm_map_list *r_list, *list_temp;
  
    if (dev->driver->master_destroy)
            dev->driver->master_destroy(dev, master);
  
  

•   mutex_lock(&dev->struct_mutex);
  

•   list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) {
  

•           if (r_list->master == master) {
  

•                   drm_legacy_rmmap_locked(dev, r_list->map);
  

•                   r_list = NULL;
  

•           }
  

•   }
  

•   mutex_unlock(&dev->struct_mutex);
  

•   drm_legacy_master_rmmaps(dev, master);
  
    idr_destroy(&master->magic_map);
    kfree(master->unique);
  

diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index 3e698038dc7b..a5ef2c7e40f8 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -154,8 +154,10 @@ struct drm_map_list { int drm_legacy_addmap(struct drm_device *d, resource_size_t offset, unsigned int size, enum drm_map_type type, enum drm_map_flags flags, struct drm_local_map **map_p); -int drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map); +void drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map); int drm_legacy_rmmap_locked(struct drm_device *d, struct drm_local_map *map); +void drm_legacy_master_rmmaps(struct drm_device *dev,

•                         struct drm_master *master);
  

struct drm_local_map *drm_legacy_getsarea(struct drm_device *dev); int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma);

-- 2.8.1

---

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

And again make sure it's a no-op for modern drivers. Another case of dev->struct_mutex gone for modern drivers!

Note that the entirety of the legacy addmap interface is now protected by DRIVER_MODESET. Note that just auditing kernel code is not enough, since userspace loves to set up legacy maps on it's own for various things - with ums userspace and kernel space share control over resources.

v2: Also add a DRIVER_* check like for all other maps functions to really short-circuit the code. And give drm_legacy_rmmap used by the dev unregister code the same treatment.

v3: - remove redundant return; (Alex, Chris) - don't special case nouveau with DRIVER_KMS_LEGACY_CONTEXT.

v4: Again special case nouveau. The problem is not directly in the ddx, but that it calls dri1 functions from the X server. And those do call drmAddMap. Fixed only in

commit b1a630b48210d6a3c44994fce1b73273000ace5c Author: Dave Airlie airlied@redhat.com Date: Wed Nov 7 14:45:14 2012 +1000

nouveau: drop DRI1 device open interface.

Acked-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk Cc: Alex Deucher alexander.deucher@amd.com Cc: Chris Wilson chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1461741618-12679-1-git-send-ema... --- drivers/gpu/drm/drm_bufs.c | 27 ++++++++++++++++++++++----- drivers/gpu/drm/drm_drv.c | 10 +--------- include/drm/drm_legacy.h | 4 +++- 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index e8a12a4fd400..9b34158c0f77 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -542,18 +542,35 @@ int drm_legacy_rmmap_locked(struct drm_device *dev, struct drm_local_map *map) } EXPORT_SYMBOL(drm_legacy_rmmap_locked);

-int drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map) +void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map) { - int ret; + if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) && + drm_core_check_feature(dev, DRIVER_MODESET)) + return;

mutex_lock(&dev->struct_mutex); - ret = drm_legacy_rmmap_locked(dev, map); + drm_legacy_rmmap_locked(dev, map); mutex_unlock(&dev->struct_mutex); - - return ret; } EXPORT_SYMBOL(drm_legacy_rmmap);

+void drm_legacy_master_rmmaps(struct drm_device *dev, struct drm_master *master) +{ + struct drm_map_list *r_list, *list_temp; + + if (drm_core_check_feature(dev, DRIVER_MODESET)) + return; + + mutex_lock(&dev->struct_mutex); + list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) { + if (r_list->master == master) { + drm_legacy_rmmap_locked(dev, r_list->map); + r_list = NULL; + } + } + mutex_unlock(&dev->struct_mutex); +} + /* The rmmap ioctl appears to be unnecessary. All mappings are torn down on * the last close of the device, and this is necessary for cleanup when things * exit uncleanly. Therefore, having userland manually remove mappings seems diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 55273f8f3acb..e1fb52d4f72c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -121,19 +121,11 @@ static void drm_master_destroy(struct kref *kref) { struct drm_master *master = container_of(kref, struct drm_master, refcount); struct drm_device *dev = master->minor->dev; - struct drm_map_list *r_list, *list_temp;

if (dev->driver->master_destroy) dev->driver->master_destroy(dev, master);

- mutex_lock(&dev->struct_mutex); - list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) { - if (r_list->master == master) { - drm_legacy_rmmap_locked(dev, r_list->map); - r_list = NULL; - } - } - mutex_unlock(&dev->struct_mutex); + drm_legacy_master_rmmaps(dev, master);

idr_destroy(&master->magic_map); kfree(master->unique); diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h index 3e698038dc7b..a5ef2c7e40f8 100644 --- a/include/drm/drm_legacy.h +++ b/include/drm/drm_legacy.h @@ -154,8 +154,10 @@ struct drm_map_list { int drm_legacy_addmap(struct drm_device *d, resource_size_t offset, unsigned int size, enum drm_map_type type, enum drm_map_flags flags, struct drm_local_map **map_p); -int drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map); +void drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map); int drm_legacy_rmmap_locked(struct drm_device *d, struct drm_local_map *map); +void drm_legacy_master_rmmaps(struct drm_device *dev, + struct drm_master *master); struct drm_local_map *drm_legacy_getsarea(struct drm_device *dev); int drm_legacy_mmap(struct file *filp, struct vm_area_struct *vma);

On Tue, Apr 26, 2016 at 1:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

It's only used for legacy mmaping support now.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Alex Deucher alexander.deucher@amd.com

---

drivers/gpu/drm/drm_internal.h | 2 -- drivers/gpu/drm/drm_vm.c | 16 ++++------------ 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index c81ff4769e7b..902cf6a15212 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -37,8 +37,6 @@ int drm_irq_by_busid(struct drm_device *dev, void *data,

/* drm_vm.c */ int drm_vma_info(struct seq_file *m, void *data); -void drm_vm_open_locked(struct drm_device *dev, struct vm_area_struct *vma); -void drm_vm_close_locked(struct drm_device *dev, struct vm_area_struct *vma);

/* drm_prime.c */ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c index f90bd5fe35ba..ac9f4b3ec615 100644 --- a/drivers/gpu/drm/drm_vm.c +++ b/drivers/gpu/drm/drm_vm.c @@ -395,16 +395,8 @@ static const struct vm_operations_struct drm_vm_sg_ops = { .close = drm_vm_close, };

-/**

• • \c open method for shared virtual memory.
• • \param vma virtual memory area.
• • Create a new drm_vma_entry structure as the \p vma private data entry and
• • add it to drm_device::vmalist.
• */

-void drm_vm_open_locked(struct drm_device *dev,

•           struct vm_area_struct *vma)
  

+static void drm_vm_open_locked(struct drm_device *dev,

•                          struct vm_area_struct *vma)
  

{ struct drm_vma_entry *vma_entry;

@@ -429,8 +421,8 @@ static void drm_vm_open(struct vm_area_struct *vma) mutex_unlock(&dev->struct_mutex); }

-void drm_vm_close_locked(struct drm_device *dev,

•           struct vm_area_struct *vma)
  

+static void drm_vm_close_locked(struct drm_device *dev,

•                           struct vm_area_struct *vma)
  

{ struct drm_vma_entry *pt, *temp;

-- 2.8.1

---

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

On Tue, Apr 26, 2016 at 07:29:41PM +0200, Daniel Vetter wrote:

amdgpu gained dev->struct_mutex usage, and that's because it's walking the dev->filelist list. Protect that list with it's own lock to take one more step towards getting rid of struct_mutex usage in drivers once and for all.

While doing the conversion I noticed that 2 debugfs files in i915 completely lacked appropriate locking. Fix that up too.

v2: don't forget to switch to drm_gem_object_unreference_unlocked.

Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Ok, merged up to this patch to drm-misc, thanks a lot for feedback and review. -Daniel

---

drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 +++++----- drivers/gpu/drm/drm_drv.c | 1 + drivers/gpu/drm/drm_fops.c | 9 ++++++--- drivers/gpu/drm/drm_info.c | 4 ++-- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++-- include/drm/drmP.h | 1 + 6 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index fa6a27bff298..a087b9638cde 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -93,7 +93,7 @@ void amdgpu_gem_force_release(struct amdgpu_device *adev) struct drm_device *ddev = adev->ddev; struct drm_file *file;

• mutex_lock(&ddev->struct_mutex);

• mutex_lock(&ddev->filelist_mutex);

  list_for_each_entry(file, &ddev->filelist, lhead) { struct drm_gem_object *gobj;

@@ -103,13 +103,13 @@ void amdgpu_gem_force_release(struct amdgpu_device *adev) spin_lock(&file->table_lock); idr_for_each_entry(&file->object_idr, gobj, handle) { WARN_ONCE(1, "And also active allocations!\n");

• 	drm_gem_object_unreference(gobj);
  

• 	drm_gem_object_unreference_unlocked(gobj);
  

  } idr_destroy(&file->object_idr); spin_unlock(&file->table_lock); }

• mutex_unlock(&ddev->struct_mutex);

• mutex_unlock(&ddev->filelist_mutex);

}

/* @@ -769,7 +769,7 @@ static int amdgpu_debugfs_gem_info(struct seq_file *m, void *data) struct drm_file *file; int r;

• r = mutex_lock_interruptible(&dev->struct_mutex);

• r = mutex_lock_interruptible(&dev->filelist_mutex); if (r) return r;

@@ -793,7 +793,7 @@ static int amdgpu_debugfs_gem_info(struct seq_file *m, void *data) spin_unlock(&file->table_lock); }

• mutex_unlock(&dev->struct_mutex);

• mutex_unlock(&dev->filelist_mutex); return 0;

}

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index e1fb52d4f72c..bff89226a344 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -590,6 +590,7 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, spin_lock_init(&dev->buf_lock); spin_lock_init(&dev->event_lock); mutex_init(&dev->struct_mutex);

• mutex_init(&dev->filelist_mutex); mutex_init(&dev->ctxlist_mutex); mutex_init(&dev->master_mutex);

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index c3d0aaac0669..7af7f8bcb355 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -297,9 +297,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) } mutex_unlock(&dev->master_mutex);

• mutex_lock(&dev->struct_mutex);

• mutex_lock(&dev->filelist_mutex); list_add(&priv->lhead, &dev->filelist);

• mutex_unlock(&dev->struct_mutex);

• mutex_unlock(&dev->filelist_mutex);

#ifdef __alpha__ /* @@ -447,8 +447,11 @@ int drm_release(struct inode *inode, struct file *filp)

DRM_DEBUG("open_count = %d\n", dev->open_count);

• mutex_lock(&dev->struct_mutex);

• mutex_lock(&dev->filelist_mutex); list_del(&file_priv->lhead);
• mutex_unlock(&dev->filelist_mutex);
• mutex_lock(&dev->struct_mutex); if (file_priv->magic) idr_remove(&file_priv->master->magic_map, file_priv->magic); mutex_unlock(&dev->struct_mutex);

diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index cbb4fc0fc969..5d469b2f26f4 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -174,7 +174,7 @@ int drm_clients_info(struct seq_file *m, void *data) /* dev->filelist is sorted youngest first, but we want to present * oldest first (i.e. kernel, servers, clients), so walk backwardss. */

• mutex_lock(&dev->struct_mutex);

• mutex_lock(&dev->filelist_mutex); list_for_each_entry_reverse(priv, &dev->filelist, lhead) { struct task_struct *task;

@@ -190,7 +190,7 @@ int drm_clients_info(struct seq_file *m, void *data) priv->magic); rcu_read_unlock(); }

• mutex_unlock(&dev->struct_mutex);

• mutex_unlock(&dev->filelist_mutex); return 0;

}

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 4950d05d2948..8b8d6f07d7aa 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -528,6 +528,10 @@ static int i915_gem_object_info(struct seq_file *m, void* data)

seq_putc(m, '\n'); print_batch_pool_stats(m, dev_priv);

• mutex_unlock(&dev->struct_mutex);
• mutex_lock(&dev->filelist_mutex); list_for_each_entry_reverse(file, &dev->filelist, lhead) { struct file_stats stats; struct task_struct *task;

@@ -548,8 +552,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data) print_file_stats(m, task ? task->comm : "<unknown>", stats); rcu_read_unlock(); }

• mutex_unlock(&dev->struct_mutex);

• mutex_unlock(&dev->filelist_mutex);

  return 0;

} @@ -2354,6 +2357,7 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) else if (INTEL_INFO(dev)->gen >= 6) gen6_ppgtt_info(m, dev);

• mutex_lock(&dev->filelist_mutex); list_for_each_entry_reverse(file, &dev->filelist, lhead) { struct drm_i915_file_private *file_priv = file->driver_priv; struct task_struct *task;

@@ -2368,6 +2372,7 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) idr_for_each(&file_priv->context_idr, per_file_ctx, (void *)(unsigned long)m); }

• mutex_unlock(&dev->filelist_mutex);

out_put: intel_runtime_pm_put(dev_priv); @@ -2403,6 +2408,8 @@ static int i915_rps_boost_info(struct seq_file *m, void *data) intel_gpu_freq(dev_priv, dev_priv->rps.min_freq_softlimit), intel_gpu_freq(dev_priv, dev_priv->rps.max_freq_softlimit), intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));

• mutex_lock(&dev->filelist_mutex); spin_lock(&dev_priv->rps.client_lock); list_for_each_entry_reverse(file, &dev->filelist, lhead) { struct drm_i915_file_private *file_priv = file->driver_priv;

@@ -2425,6 +2432,7 @@ static int i915_rps_boost_info(struct seq_file *m, void *data) list_empty(&dev_priv->rps.mmioflips.link) ? "" : ", active"); seq_printf(m, "Kernel boosts: %d\n", dev_priv->rps.boosts); spin_unlock(&dev_priv->rps.client_lock);

• mutex_unlock(&dev->filelist_mutex);

  return 0;

} diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f82979dee647..c81dd2250fc6 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -769,6 +769,7 @@ struct drm_device { atomic_t buf_alloc; /**< Buffer allocation in progress */ /*@} */

• struct mutex filelist_mutex; struct list_head filelist;

  /** \name Memory management */

-- 2.8.1

On Tue, Apr 26, 2016 at 07:29:42PM +0200, Daniel Vetter wrote:

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 12 ++++++++- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..8f2eff448bb5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• if (dev->driver->gem_free_object != NULL)

• if (dev->driver->gem_free_object_unlocked != NULL)

• dev->driver->gem_free_object_unlocked(obj);
  

• else if (dev->driver->gem_free_object != NULL) dev->driver->gem_free_object(obj);

} -EXPORT_SYMBOL(drm_gem_object_free);

+/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

+void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{

I have i915.ko in a state where we could use this as well. I would like to have our inlines back though. I would add

static inline void i915_gem_object_put(struct drm_i915_gem_object *obj) { kref_put(&obj->base.refcount, drm_gem_object_free); }

Hmm, considering how simple that is, maybe I won't ask for

static inline void __drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) { BUG_ON(obj->dev->driver->gem_free_object == NULL); kref_put(&obj->refcount, drm_gem_object_free); }

after all! -Chris

On Tue, Apr 26, 2016 at 10:10:14PM +0200, Daniel Vetter wrote:

On Tue, Apr 26, 2016 at 08:47:32PM +0100, Chris Wilson wrote:

...

On Tue, Apr 26, 2016 at 07:29:42PM +0200, Daniel Vetter wrote:

...

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 12 ++++++++- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..8f2eff448bb5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• if (dev->driver->gem_free_object != NULL)

• if (dev->driver->gem_free_object_unlocked != NULL)

• dev->driver->gem_free_object_unlocked(obj);
  

• else if (dev->driver->gem_free_object != NULL) dev->driver->gem_free_object(obj);

} -EXPORT_SYMBOL(drm_gem_object_free);

+/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

+void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{

I have i915.ko in a state where we could use this as well. I would like to have our inlines back though. I would add

static inline void i915_gem_object_put(struct drm_i915_gem_object *obj) { kref_put(&obj->base.refcount, drm_gem_object_free); }

Hmm, considering how simple that is, maybe I won't ask for

static inline void __drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) { BUG_ON(obj->dev->driver->gem_free_object == NULL); kref_put(&obj->refcount, drm_gem_object_free); }

Yeah, something like that makes sense. Imo the core versions really need all the silly locking checks and all that to make sure no one abuses them by accident, or breaks a driver still using struct_mutex. Adding a fastpath for drivers using gem_free_object_unlocked just wrapping the kref_put certainly looks like a good idea.

And if we document the exact use case I think we could even nuke the BUG_ON too. This can be audited with a simple

$ git grep __drm_gem_object_unreference_unlocked -- drivers/gpu/drm/$driver

so imo nothing too dangerous.

Ack/r-b on the patch itself?

Only an ack, I'm afraid. I haven't considered the ramifictions to know if the intermediate steps are going to be painful (in some cases we'll have an all a new hot function in our profiles, but overall there'll be little change I hope). The end goal is definitely an improvement.

I've talked myself into having a better read of the core patches... -Chris

On Tue, Apr 26, 2016 at 05:52:31PM -0400, Alex Deucher wrote:

On Tue, Apr 26, 2016 at 1:29 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

This and the rest of the series are: Acked-by: Alex Deucher alexander.deucher@amd.com

Before I pull this in, I'd like at least an r-b on this and one of the driver patches. t-b even better. -Daniel

...

---

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 12 ++++++++- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..8f2eff448bb5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

    WARN_ON(!mutex_is_locked(&dev->struct_mutex));

•   if (dev->driver->gem_free_object != NULL)
  

•   if (dev->driver->gem_free_object_unlocked != NULL)
  

•           dev->driver->gem_free_object_unlocked(obj);
  

•   else if (dev->driver->gem_free_object != NULL)
            dev->driver->gem_free_object(obj);
  

} -EXPORT_SYMBOL(drm_gem_object_free);

+/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

+void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{

•   struct drm_device *dev;
  

•   if (!obj)
  

•           return;
  

•   dev = obj->dev;
  

•   might_lock(&dev->struct_mutex);
  

•   if (dev->driver->gem_free_object != NULL)
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
  

•                           &dev->struct_mutex))
  

•           mutex_unlock(&dev->struct_mutex);
  

+} +EXPORT_SYMBOL(drm_gem_object_unreference_unlocked);

+/**

• • drm_gem_object_unreference - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.
• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.
• */

+void +drm_gem_object_unreference(struct drm_gem_object *obj) +{

•   if (obj != NULL) {
  

•           WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   }
  

+} +EXPORT_SYMBOL(drm_gem_object_unreference);

/**

• drm_gem_vm_open - vma->ops->open implementation for GEM

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c81dd2250fc6..7e30b3d2b25c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -583,9 +583,19 @@ struct drm_driver { * Driver-specific constructor for drm_gem_objects, to set up * obj->driver_private. *

•    * Returns 0 on success.
  

•    * This is deprecated and should not be used by new drivers. Use
  

•    * @gem_free_object_unlocked instead.
     */
    void (*gem_free_object) (struct drm_gem_object *obj);
  

•   /**
  

•    * Driver-specific constructor for drm_gem_objects, to set up
  

•    * obj->driver_private. This is for drivers which are not encumbered
  

•    * with dev->struct_mutex legacy locking schemes. Use this hook instead
  

•    * of @gem_free_object.
  

•    */
  

•   void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
  

•   int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
    void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
  
  

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 0b3e11ab8757..ae1c7f18eec0 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -175,7 +175,6 @@ struct drm_gem_object { };

void drm_gem_object_release(struct drm_gem_object *obj); -void drm_gem_object_free(struct kref *kref); int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); void drm_gem_private_object_init(struct drm_device *dev, @@ -199,48 +198,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) kref_get(&obj->refcount); }

-/**

• • drm_gem_object_unreference - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.
• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.
• */

-static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) -{

•   if (obj != NULL) {
  

•           WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   }
  

-}

-/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

-static inline void -drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) -{

•   struct drm_device *dev;
  

•   if (!obj)
  

•           return;
  

•   dev = obj->dev;
  

•   if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex))
  

•           mutex_unlock(&dev->struct_mutex);
  

•   else
  

•           might_lock(&dev->struct_mutex);
  

-} +void drm_gem_object_unreference_unlocked(struct drm_gem_object *obj); +void drm_gem_object_unreference(struct drm_gem_object *obj);

int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj, -- 2.8.1

---

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

On Wed, Apr 27, 2016 at 11:31:00AM +0200, Lucas Stach wrote:

Am Dienstag, den 26.04.2016, 19:29 +0200 schrieb Daniel Vetter:

...

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Two mostly cosmetic comments below, otherwise looks good:

Reviewed-by: Lucas Stach l.stach@pengutronix.de

...

---

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 12 ++++++++- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 65 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..8f2eff448bb5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• if (dev->driver->gem_free_object != NULL)

• if (dev->driver->gem_free_object_unlocked != NULL)

Why those explicit != NULL checks? The style mostly used in this file is to omit those, like "if (dev->driver->gem_free_object_unlocked)". Same comment applies to several hunks of this patch.

I move some of these functions around, the != NULL checks where there already. I can easily change all the ones I touch.

...

• dev->driver->gem_free_object_unlocked(obj);
  

• else if (dev->driver->gem_free_object != NULL) dev->driver->gem_free_object(obj);

} -EXPORT_SYMBOL(drm_gem_object_free);

+/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

+void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{

• struct drm_device *dev;
• if (!obj)

• return;
  

• dev = obj->dev;
• might_lock(&dev->struct_mutex);
• if (dev->driver->gem_free_object != NULL)

• kref_put(&obj->refcount, drm_gem_object_free);
  

• else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,

• 		&dev->struct_mutex))
  

• mutex_unlock(&dev->struct_mutex);
  

+} +EXPORT_SYMBOL(drm_gem_object_unreference_unlocked);

+/**

• • drm_gem_object_unreference - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.
• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.
• */

+void +drm_gem_object_unreference(struct drm_gem_object *obj) +{

• if (obj != NULL) {

• WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

• kref_put(&obj->refcount, drm_gem_object_free);
  

• }

+} +EXPORT_SYMBOL(drm_gem_object_unreference);

/**

• drm_gem_vm_open - vma->ops->open implementation for GEM

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c81dd2250fc6..7e30b3d2b25c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -583,9 +583,19 @@ struct drm_driver { * Driver-specific constructor for drm_gem_objects, to set up * obj->driver_private. *

• * Returns 0 on success.
  

• * This is deprecated and should not be used by new drivers. Use
  

• * @gem_free_object_unlocked instead.
  

  */ void (*gem_free_object) (struct drm_gem_object *obj);
• /**

• * Driver-specific constructor for drm_gem_objects, to set up
  

• * obj->driver_private.
  

This part of the comment is really off. I see it's just copy&paste from the comment above gem_free_object, but I think it should have been placed above gem_open_object and has nothing to do with the free* callbacks.

Oops, wanted to add proper kerneldoc for this new hook and the old one to explain the differences, and totally failed at that. Will fix up. -Daniel

...

This is for drivers which are not encumbered

• * with dev->struct_mutex legacy locking schemes. Use this hook instead
  

• * of @gem_free_object.
  

• */
  

• void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
• int (*gem_open_object) (struct drm_gem_object *, struct drm_file *); void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 0b3e11ab8757..ae1c7f18eec0 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -175,7 +175,6 @@ struct drm_gem_object { };

void drm_gem_object_release(struct drm_gem_object *obj); -void drm_gem_object_free(struct kref *kref); int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); void drm_gem_private_object_init(struct drm_device *dev, @@ -199,48 +198,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) kref_get(&obj->refcount); }

-/**

• • drm_gem_object_unreference - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.
• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.
• */

-static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) -{

• if (obj != NULL) {

• WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

• kref_put(&obj->refcount, drm_gem_object_free);
  

• }

-}

-/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

-static inline void -drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) -{

• struct drm_device *dev;
• if (!obj)

• return;
  

• dev = obj->dev;
• if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex))

• mutex_unlock(&dev->struct_mutex);
  

• else

• might_lock(&dev->struct_mutex);
  

-} +void drm_gem_object_unreference_unlocked(struct drm_gem_object *obj); +void drm_gem_object_unreference(struct drm_gem_object *obj);

int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj,

On Wed, Apr 27, 2016 at 01:50:00PM +0200, Daniel Vetter wrote:

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

v3: Review from Lucas:

• drop != NULL in pointer checks.
• fixup copypasted kerneldoc to actually match the functions.

Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Alex Deucher alexdeucher@gmail.com Cc: Lucas Stach l.stach@pengutronix.de Reviewed-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 15 ++++++++--- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 66 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..a4684a306c48 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• if (dev->driver->gem_free_object != NULL)

• if (dev->driver->gem_free_object_unlocked)

• dev->driver->gem_free_object_unlocked(obj);
  

• else if (dev->driver->gem_free_object) dev->driver->gem_free_object(obj);

} -EXPORT_SYMBOL(drm_gem_object_free);

With my plan, I want to keep drm_gem_object_free exported. The plan being a __drm_gem_object_free_lockless fastpath. -Chris

On Wed, Apr 27, 2016 at 02:12:46PM +0200, Daniel Vetter wrote:

On Wed, Apr 27, 2016 at 12:58:51PM +0100, Chris Wilson wrote:

...

On Wed, Apr 27, 2016 at 01:50:00PM +0200, Daniel Vetter wrote:

...

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

v3: Review from Lucas:

• drop != NULL in pointer checks.
• fixup copypasted kerneldoc to actually match the functions.

Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Alex Deucher alexdeucher@gmail.com Cc: Lucas Stach l.stach@pengutronix.de Reviewed-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 15 ++++++++--- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 66 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..a4684a306c48 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release);

-/**

• • drm_gem_object_free - free a GEM object
• • @kref: kref of the object to free
• • Called after the last reference to the object has been lost.
• • Must be called holding struct_ mutex
• • Frees the object
• */

-void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

• if (dev->driver->gem_free_object != NULL)

• if (dev->driver->gem_free_object_unlocked)

• dev->driver->gem_free_object_unlocked(obj);
  

• else if (dev->driver->gem_free_object) dev->driver->gem_free_object(obj);

} -EXPORT_SYMBOL(drm_gem_object_free);

With my plan, I want to keep drm_gem_object_free exported. The plan being a __drm_gem_object_free_lockless fastpath.

Yeah, but I think we can reexport again in the patch that adds __drm_gem_object_free_lockless. Just not much personally a friend of sprawling EXPORT_SYMBOL when not needed. But I can drop that part too if you feel strongly.

The alternative is to add the function now and then I can just drop the support into my tree and start testing :) -Chris

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

v3: Review from Lucas: - drop != NULL in pointer checks. - fixup copypasted kerneldoc to actually match the functions.

v4: Add __drm_gem_object_unreference as a fastpath helper for drivers who abolished dev->struct_mutex, requested by Chris.

v5: Fix silly mistake in drm_gem_object_unreference_unlocked caught by intel-gfx CI - I checked for gem_free_object instead of gem_free_object_unlocked ...

Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Alex Deucher alexdeucher@gmail.com Cc: Lucas Stach l.stach@pengutronix.de Reviewed-by: Lucas Stach l.stach@pengutronix.de (v3) Reviewed-by: Chris Wilson chris@chris-wilson.co.uk (v4) Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_gem.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 15 ++++++++++--- include/drm/drm_gem.h | 48 +++++++++++++---------------------------- 3 files changed, 80 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..973eb8805ce0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -806,12 +806,64 @@ drm_gem_object_free(struct kref *kref)

WARN_ON(!mutex_is_locked(&dev->struct_mutex));

- if (dev->driver->gem_free_object != NULL) + if (dev->driver->gem_free_object_unlocked) + dev->driver->gem_free_object_unlocked(obj); + else if (dev->driver->gem_free_object) dev->driver->gem_free_object(obj); } EXPORT_SYMBOL(drm_gem_object_free);

/** + * drm_gem_object_unreference_unlocked - release a GEM BO reference + * @obj: GEM buffer object + * + * This releases a reference to @obj. Callers must not hold the + * dev->struct_mutex lock when calling this function. + * + * See also __drm_gem_object_unreference(). + */ +void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev; + + if (!obj) + return; + + dev = obj->dev; + might_lock(&dev->struct_mutex); + + if (dev->driver->gem_free_object_unlocked) + kref_put(&obj->refcount, drm_gem_object_free); + else if (kref_put_mutex(&obj->refcount, drm_gem_object_free, + &dev->struct_mutex)) + mutex_unlock(&dev->struct_mutex); +} +EXPORT_SYMBOL(drm_gem_object_unreference_unlocked); + +/** + * drm_gem_object_unreference - release a GEM BO reference + * @obj: GEM buffer object + * + * This releases a reference to @obj. Callers must hold the dev->struct_mutex + * lock when calling this function, even when the driver doesn't use + * dev->struct_mutex for anything. + * + * For drivers not encumbered with legacy locking use + * drm_gem_object_unreference_unlocked() instead. + */ +void +drm_gem_object_unreference(struct drm_gem_object *obj) +{ + if (obj) { + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + + kref_put(&obj->refcount, drm_gem_object_free); + } +} +EXPORT_SYMBOL(drm_gem_object_unreference); + +/** * drm_gem_vm_open - vma->ops->open implementation for GEM * @vma: VM area structure * diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c81dd2250fc6..bd7b262d7af0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -580,12 +580,21 @@ struct drm_driver { void (*debugfs_cleanup)(struct drm_minor *minor);

/** - * Driver-specific constructor for drm_gem_objects, to set up - * obj->driver_private. + * @gem_free_object: deconstructor for drm_gem_objects * - * Returns 0 on success. + * This is deprecated and should not be used by new drivers. Use + * @gem_free_object_unlocked instead. */ void (*gem_free_object) (struct drm_gem_object *obj); + + /** + * @gem_free_object_unlocked: deconstructor for drm_gem_objects + * + * This is for drivers which are not encumbered with dev->struct_mutex + * legacy locking schemes. Use this hook instead of @gem_free_object. + */ + void (*gem_free_object_unlocked) (struct drm_gem_object *obj); + int (*gem_open_object) (struct drm_gem_object *, struct drm_file *); void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 0b3e11ab8757..408d6c47d98b 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -200,47 +200,29 @@ drm_gem_object_reference(struct drm_gem_object *obj) }

/** - * drm_gem_object_unreference - release a GEM BO reference + * __drm_gem_object_unreference - raw function to release a GEM BO reference * @obj: GEM buffer object * - * This releases a reference to @obj. Callers must hold the dev->struct_mutex - * lock when calling this function, even when the driver doesn't use - * dev->struct_mutex for anything. + * This function is meant to be used by drivers which are not encumbered with + * dev->struct_mutex legacy locking and which are using the + * gem_free_object_unlocked callback. It avoids all the locking checks and + * locking overhead of drm_gem_object_unreference() and + * drm_gem_object_unreference_unlocked(). * - * For drivers not encumbered with legacy locking use - * drm_gem_object_unreference_unlocked() instead. + * Drivers should never call this directly in their code. Instead they should + * wrap it up into a driver_gem_object_unreference(struct driver_gem_object + * *obj) wrapper function, and use that. Shared code should never call this, to + * avoid breaking drivers by accident which still depend upon dev->struct_mutex + * locking. */ static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) +__drm_gem_object_unreference(struct drm_gem_object *obj) { - if (obj != NULL) { - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); - - kref_put(&obj->refcount, drm_gem_object_free); - } + kref_put(&obj->refcount, drm_gem_object_free); }

-/** - * drm_gem_object_unreference_unlocked - release a GEM BO reference - * @obj: GEM buffer object - * - * This releases a reference to @obj. Callers must not hold the - * dev->struct_mutex lock when calling this function. - */ -static inline void -drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) -{ - struct drm_device *dev; - - if (!obj) - return; - - dev = obj->dev; - if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex)) - mutex_unlock(&dev->struct_mutex); - else - might_lock(&dev->struct_mutex); -} +void drm_gem_object_unreference_unlocked(struct drm_gem_object *obj); +void drm_gem_object_unreference(struct drm_gem_object *obj);

int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj,

On Tue, May 03, 2016 at 11:59:19AM -0400, Alex Deucher wrote:

On Mon, May 2, 2016 at 4:40 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Finally all the core gem and a lot of drivers are entirely free of dev->struct_mutex depencies, and we can start to have an entirely lockless unref path.

To make sure that no one who touches the core code accidentally breaks existing drivers which still require dev->struct_mutex I've made the might_lock check unconditional.

While at it de-inline the ref/unref functions, they've become a bit too big.

v2: Make it not leak like a sieve.

v3: Review from Lucas:

• drop != NULL in pointer checks.
• fixup copypasted kerneldoc to actually match the functions.

v4: Add __drm_gem_object_unreference as a fastpath helper for drivers who abolished dev->struct_mutex, requested by Chris.

v5: Fix silly mistake in drm_gem_object_unreference_unlocked caught by intel-gfx CI - I checked for gem_free_object instead of gem_free_object_unlocked ...

Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Alex Deucher alexdeucher@gmail.com Cc: Lucas Stach l.stach@pengutronix.de Reviewed-by: Lucas Stach l.stach@pengutronix.de (v3) Reviewed-by: Chris Wilson chris@chris-wilson.co.uk (v4) Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Alex Deucher alexander.deucher@amd.com

Thanks for the review. I merged this one plus the driver patches acked by maintainers to drm-misc. -Daniel

...

---

drivers/gpu/drm/drm_gem.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 15 ++++++++++--- include/drm/drm_gem.h | 48 +++++++++++++---------------------------- 3 files changed, 80 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..973eb8805ce0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -806,12 +806,64 @@ drm_gem_object_free(struct kref *kref)

    WARN_ON(!mutex_is_locked(&dev->struct_mutex));

•   if (dev->driver->gem_free_object != NULL)
  

•   if (dev->driver->gem_free_object_unlocked)
  

•           dev->driver->gem_free_object_unlocked(obj);
  

•   else if (dev->driver->gem_free_object)
            dev->driver->gem_free_object(obj);
  

} EXPORT_SYMBOL(drm_gem_object_free);

/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• • See also __drm_gem_object_unreference().
• */

+void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{

•   struct drm_device *dev;
  

•   if (!obj)
  

•           return;
  

•   dev = obj->dev;
  

•   might_lock(&dev->struct_mutex);
  

•   if (dev->driver->gem_free_object_unlocked)
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
  

•                           &dev->struct_mutex))
  

•           mutex_unlock(&dev->struct_mutex);
  

+} +EXPORT_SYMBOL(drm_gem_object_unreference_unlocked);

+/**

• • drm_gem_object_unreference - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.
• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.
• */

+void +drm_gem_object_unreference(struct drm_gem_object *obj) +{

•   if (obj) {
  

•           WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   }
  

+} +EXPORT_SYMBOL(drm_gem_object_unreference);

+/**

• drm_gem_vm_open - vma->ops->open implementation for GEM
• @vma: VM area structure

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c81dd2250fc6..bd7b262d7af0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -580,12 +580,21 @@ struct drm_driver { void (*debugfs_cleanup)(struct drm_minor *minor);

    /**

•    * Driver-specific constructor for drm_gem_objects, to set up
  

•    * obj->driver_private.
  

•    * @gem_free_object: deconstructor for drm_gem_objects
     *
  

•    * Returns 0 on success.
  

•    * This is deprecated and should not be used by new drivers. Use
  

•    * @gem_free_object_unlocked instead.
     */
    void (*gem_free_object) (struct drm_gem_object *obj);
  

•   /**
  

•    * @gem_free_object_unlocked: deconstructor for drm_gem_objects
  

•    *
  

•    * This is for drivers which are not encumbered with dev->struct_mutex
  

•    * legacy locking schemes. Use this hook instead of @gem_free_object.
  

•    */
  

•   void (*gem_free_object_unlocked) (struct drm_gem_object *obj);
  

•   int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
    void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
  
  

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 0b3e11ab8757..408d6c47d98b 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -200,47 +200,29 @@ drm_gem_object_reference(struct drm_gem_object *obj) }

/**

• • drm_gem_object_unreference - release a GEM BO reference

• • __drm_gem_object_unreference - raw function to release a GEM BO reference
  • @obj: GEM buffer object

• • This releases a reference to @obj. Callers must hold the dev->struct_mutex
• • lock when calling this function, even when the driver doesn't use
• • dev->struct_mutex for anything.

• • This function is meant to be used by drivers which are not encumbered with
• • dev->struct_mutex legacy locking and which are using the
• • gem_free_object_unlocked callback. It avoids all the locking checks and
• • locking overhead of drm_gem_object_unreference() and
• • drm_gem_object_unreference_unlocked().

• • For drivers not encumbered with legacy locking use
• • drm_gem_object_unreference_unlocked() instead.

• • Drivers should never call this directly in their code. Instead they should
• • wrap it up into a driver_gem_object_unreference(struct driver_gem_object
• • *obj) wrapper function, and use that. Shared code should never call this, to
• • avoid breaking drivers by accident which still depend upon dev->struct_mutex
• • locking.
  */

static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) +__drm_gem_object_unreference(struct drm_gem_object *obj) {

•   if (obj != NULL) {
  

•           WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
  

•           kref_put(&obj->refcount, drm_gem_object_free);
  

•   }
  

•   kref_put(&obj->refcount, drm_gem_object_free);
  

}

-/**

• • drm_gem_object_unreference_unlocked - release a GEM BO reference
• • @obj: GEM buffer object
• • This releases a reference to @obj. Callers must not hold the
• • dev->struct_mutex lock when calling this function.
• */

-static inline void -drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) -{

•   struct drm_device *dev;
  

•   if (!obj)
  

•           return;
  

•   dev = obj->dev;
  

•   if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex))
  

•           mutex_unlock(&dev->struct_mutex);
  

•   else
  

•           might_lock(&dev->struct_mutex);
  

-} +void drm_gem_object_unreference_unlocked(struct drm_gem_object *obj); +void drm_gem_object_unreference(struct drm_gem_object *obj);

int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj, -- 2.8.1

2016년 04월 27일 02:29에 Daniel Vetter 이(가) 쓴 글:

No dev->struct_mutex anywhere to be seen.

Acked-by: Inki Dae inki.dae@samsung.com

Thanks, Inki Dae

Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 5344940c8a07..f534ed62065e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -418,7 +418,7 @@ static struct drm_driver exynos_drm_driver = { .get_vblank_counter = drm_vblank_no_hw_counter, .enable_vblank = exynos_drm_crtc_enable_vblank, .disable_vblank = exynos_drm_crtc_disable_vblank,

• .gem_free_object = exynos_drm_gem_free_object,

• .gem_free_object_unlocked = exynos_drm_gem_free_object, .gem_vm_ops = &exynos_drm_gem_vm_ops, .dumb_create = exynos_drm_gem_dumb_create, .dumb_map_offset = exynos_drm_gem_dumb_map_offset,

On Wed, Apr 27, 2016 at 12:29:34PM +0200, Philipp Zabel wrote:

Am Dienstag, den 26.04.2016, 19:29 +0200 schrieb Daniel Vetter:

...

No dev->struct_mutex anywhere to be seen.

Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index e26dcdec2aba..2453fb1c68a7 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -411,7 +411,7 @@ static struct drm_driver imx_drm_driver = { .unload = imx_drm_driver_unload, .lastclose = imx_drm_driver_lastclose, .set_busid = drm_platform_set_busid,

• .gem_free_object = drm_gem_cma_free_object,

• .gem_free_object_unlocked = drm_gem_cma_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset,

Applied to imx-drm/fixes, thank you.

And that compiles for you? Might want to drop the patch again before someone notices ;-) -Daniel

On Wed, Apr 27, 2016 at 02:01:35PM +0200, Philipp Zabel wrote:

Am Mittwoch, den 27.04.2016, 13:21 +0200 schrieb Daniel Vetter:

...

On Wed, Apr 27, 2016 at 12:29:34PM +0200, Philipp Zabel wrote:

...

Am Dienstag, den 26.04.2016, 19:29 +0200 schrieb Daniel Vetter:

...

No dev->struct_mutex anywhere to be seen.

Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/imx/imx-drm-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index e26dcdec2aba..2453fb1c68a7 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -411,7 +411,7 @@ static struct drm_driver imx_drm_driver = { .unload = imx_drm_driver_unload, .lastclose = imx_drm_driver_lastclose, .set_busid = drm_platform_set_busid,

• .gem_free_object = drm_gem_cma_free_object,

• .gem_free_object_unlocked = drm_gem_cma_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, .dumb_map_offset = drm_gem_cma_dumb_map_offset,

Applied to imx-drm/fixes, thank you.

And that compiles for you? Might want to drop the patch again before someone notices ;-)

Nope, dropped. Nobody will have to.

I presume you having tried to apply it is as good as an ack, so added to drm-misc. -Daniel

No need to reinvent this little wheel.

v2: Like, try to make it compile even.

Cc: Mark Yao mark.yao@rock-chips.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 903202224057..02fe3748bcc3 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -19,6 +19,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_gem_cma_helper.h> #include <linux/dma-mapping.h> #include <linux/pm_runtime.h> #include <linux/module.h> @@ -292,11 +293,6 @@ static const struct file_operations rockchip_drm_driver_fops = { .release = drm_release, };

-const struct vm_operations_struct rockchip_drm_vm_ops = { - .open = drm_gem_vm_open, - .close = drm_gem_vm_close, -}; - static struct drm_driver rockchip_drm_driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC, @@ -307,7 +303,7 @@ static struct drm_driver rockchip_drm_driver = { .get_vblank_counter = drm_vblank_no_hw_counter, .enable_vblank = rockchip_drm_crtc_enable_vblank, .disable_vblank = rockchip_drm_crtc_disable_vblank, - .gem_vm_ops = &rockchip_drm_vm_ops, + .gem_vm_ops = &drm_gem_cma_vm_ops, .gem_free_object_unlocked = rockchip_gem_free_object, .dumb_create = rockchip_gem_dumb_create, .dumb_map_offset = rockchip_gem_dumb_map_offset,

Hi Daniel,

2016년 04월 27일 20:38에 Daniel Vetter 이(가) 쓴 글:

It's an optional hook. Might be needed for frontbuffer rendering on manual upload displays, but a simple TODO doesn't explain at all what needs to be done or why.

We have a plan for partial update support but not now yet. Picked it up.

Thanks, Inki Dae

Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/exynos/exynos_drm_fb.c | 11 ----------- 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 81cc5537cf25..f851a40ac6cb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -97,20 +97,9 @@ static int exynos_drm_fb_create_handle(struct drm_framebuffer *fb, &exynos_fb->exynos_gem[0]->base, handle); }

-static int exynos_drm_fb_dirty(struct drm_framebuffer *fb,

• 		struct drm_file *file_priv, unsigned flags,
  

• 		unsigned color, struct drm_clip_rect *clips,
  

• 		unsigned num_clips)
  

-{

• /* TODO */
• return 0;

-}

static const struct drm_framebuffer_funcs exynos_drm_fb_funcs = { .destroy = exynos_drm_fb_destroy, .create_handle = exynos_drm_fb_create_handle,

• .dirty = exynos_drm_fb_dirty,

};

struct drm_framebuffer *