From: Rob Clark <robdclark@gmail.com> To: Doug Anderson <dianders@chromium.org> Cc: dri-devel <dri-devel@lists.freedesktop.org>, Rob Clark <robdclark@chromium.org>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Jordan Crouse <jordan@cosmicpenguin.net>, "Kristian H. Kristensen" <hoegsberg@google.com>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 3/4] drm/msm: Fix debugfs deadlock Date: Wed, 31 Mar 2021 16:31:38 -0700 [thread overview] Message-ID: <CAF6AEGvxQeTFHn_ztzP=4eTQa_B106+SZ-8NjFk2RGYgRYJgSw@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=UECYxLXQa_L572eLSVHq7pbxuA0zLvHzYHhCKy8K=9TA@mail.gmail.com> On Wed, Mar 31, 2021 at 4:13 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote: > > > > @@ -111,23 +111,15 @@ static const struct file_operations msm_gpu_fops = { > > static int msm_gem_show(struct drm_device *dev, struct seq_file *m) > > { > > struct msm_drm_private *priv = dev->dev_private; > > - struct msm_gpu *gpu = priv->gpu; > > int ret; > > > > - ret = mutex_lock_interruptible(&priv->mm_lock); > > + ret = mutex_lock_interruptible(&priv->obj_lock); > > if (ret) > > return ret; > > > > - if (gpu) { > > - seq_printf(m, "Active Objects (%s):\n", gpu->name); > > - msm_gem_describe_objects(&gpu->active_list, m); > > - } > > - > > - seq_printf(m, "Inactive Objects:\n"); > > - msm_gem_describe_objects(&priv->inactive_dontneed, m); > > - msm_gem_describe_objects(&priv->inactive_willneed, m); > > + msm_gem_describe_objects(&priv->objects, m); > > I guess we no longer sort the by Active and Inactive but that doesn't > really matter? It turned out to be less useful to sort by active/inactive, as much as just having the summary at the bottom that the last patch adds. We can already tell from the per-object entries whether it is active/purgable/purged. I did initially try to come up with an approach that let me keep this, but it would basically amount to re-writing the gem_submit path (because you cannot do any memory allocation under mm_lock) > > > @@ -174,7 +174,13 @@ struct msm_drm_private { > > struct msm_rd_state *hangrd; /* debugfs to dump hanging submits */ > > struct msm_perf_state *perf; > > > > - /* > > + /** > > + * List of all GEM objects (mainly for debugfs, protected by obj_lock > > It wouldn't hurt to talk about lock ordering here? Like: "If we need > the "obj_lock" and a "gem_lock" at the same time we always grab the > "obj_lock" first. good point > > > @@ -60,13 +60,20 @@ struct msm_gem_object { > > */ > > uint8_t vmap_count; > > > > - /* And object is either: > > - * inactive - on priv->inactive_list > > + /** > > + * Node in list of all objects (mainly for debugfs, protected by > > + * struct_mutex > > Not "struct_mutex" in comment, right? Maybe "obj_lock" I think? oh, right, forgot to fix that from an earlier iteration BR, -R
WARNING: multiple messages have this Message-ID (diff)
From: Rob Clark <robdclark@gmail.com> To: Doug Anderson <dianders@chromium.org> Cc: Rob Clark <robdclark@chromium.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, David Airlie <airlied@linux.ie>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Jordan Crouse <jordan@cosmicpenguin.net>, "Kristian H. Kristensen" <hoegsberg@google.com>, Sean Paul <sean@poorly.run> Subject: Re: [PATCH 3/4] drm/msm: Fix debugfs deadlock Date: Wed, 31 Mar 2021 16:31:38 -0700 [thread overview] Message-ID: <CAF6AEGvxQeTFHn_ztzP=4eTQa_B106+SZ-8NjFk2RGYgRYJgSw@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=UECYxLXQa_L572eLSVHq7pbxuA0zLvHzYHhCKy8K=9TA@mail.gmail.com> On Wed, Mar 31, 2021 at 4:13 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote: > > > > @@ -111,23 +111,15 @@ static const struct file_operations msm_gpu_fops = { > > static int msm_gem_show(struct drm_device *dev, struct seq_file *m) > > { > > struct msm_drm_private *priv = dev->dev_private; > > - struct msm_gpu *gpu = priv->gpu; > > int ret; > > > > - ret = mutex_lock_interruptible(&priv->mm_lock); > > + ret = mutex_lock_interruptible(&priv->obj_lock); > > if (ret) > > return ret; > > > > - if (gpu) { > > - seq_printf(m, "Active Objects (%s):\n", gpu->name); > > - msm_gem_describe_objects(&gpu->active_list, m); > > - } > > - > > - seq_printf(m, "Inactive Objects:\n"); > > - msm_gem_describe_objects(&priv->inactive_dontneed, m); > > - msm_gem_describe_objects(&priv->inactive_willneed, m); > > + msm_gem_describe_objects(&priv->objects, m); > > I guess we no longer sort the by Active and Inactive but that doesn't > really matter? It turned out to be less useful to sort by active/inactive, as much as just having the summary at the bottom that the last patch adds. We can already tell from the per-object entries whether it is active/purgable/purged. I did initially try to come up with an approach that let me keep this, but it would basically amount to re-writing the gem_submit path (because you cannot do any memory allocation under mm_lock) > > > @@ -174,7 +174,13 @@ struct msm_drm_private { > > struct msm_rd_state *hangrd; /* debugfs to dump hanging submits */ > > struct msm_perf_state *perf; > > > > - /* > > + /** > > + * List of all GEM objects (mainly for debugfs, protected by obj_lock > > It wouldn't hurt to talk about lock ordering here? Like: "If we need > the "obj_lock" and a "gem_lock" at the same time we always grab the > "obj_lock" first. good point > > > @@ -60,13 +60,20 @@ struct msm_gem_object { > > */ > > uint8_t vmap_count; > > > > - /* And object is either: > > - * inactive - on priv->inactive_list > > + /** > > + * Node in list of all objects (mainly for debugfs, protected by > > + * struct_mutex > > Not "struct_mutex" in comment, right? Maybe "obj_lock" I think? oh, right, forgot to fix that from an earlier iteration BR, -R _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-03-31 23:29 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-31 22:16 [PATCH 0/4] drm/msm: Shrinker (and related) fixes Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 22:16 ` [PATCH 1/4] drm/msm: Remove unused freed llist node Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 23:46 ` Doug Anderson 2021-03-31 23:46 ` Doug Anderson 2021-03-31 22:16 ` [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count() Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 22:44 ` Doug Anderson 2021-03-31 22:44 ` Doug Anderson 2021-03-31 23:26 ` Rob Clark 2021-03-31 23:26 ` Rob Clark 2021-03-31 23:39 ` Doug Anderson 2021-03-31 23:39 ` Doug Anderson 2021-04-01 0:17 ` Rob Clark 2021-04-01 0:17 ` Rob Clark 2021-03-31 22:16 ` [PATCH 3/4] drm/msm: Fix debugfs deadlock Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 23:13 ` Doug Anderson 2021-03-31 23:13 ` Doug Anderson 2021-03-31 23:31 ` Rob Clark [this message] 2021-03-31 23:31 ` Rob Clark 2021-03-31 22:16 ` [PATCH 4/4] drm/msm: Improved debugfs gem stats Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 23:33 ` Doug Anderson 2021-03-31 23:33 ` Doug Anderson 2021-03-31 23:46 ` [PATCH 0/4] drm/msm: Shrinker (and related) fixes Doug Anderson 2021-03-31 23:46 ` Doug Anderson 2021-04-01 1:27 ` [PATCH v2 " Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 1:27 ` [PATCH v2 1/4] drm/msm: Remove unused freed llist node Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-05-26 19:03 ` patchwork-bot+linux-arm-msm 2021-04-01 1:27 ` [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count() Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-04-01 17:37 ` Rob Clark 2021-04-01 17:37 ` Rob Clark 2021-04-01 1:27 ` [PATCH v2 3/4] drm/msm: Fix debugfs deadlock Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-04-01 1:27 ` [PATCH v2 4/4] drm/msm: Improved debugfs gem stats Rob Clark 2021-04-01 1:27 ` Rob Clark
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAF6AEGvxQeTFHn_ztzP=4eTQa_B106+SZ-8NjFk2RGYgRYJgSw@mail.gmail.com' \ --to=robdclark@gmail.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dianders@chromium.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=hoegsberg@google.com \ --cc=jordan@cosmicpenguin.net \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robdclark@chromium.org \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.