From: Akhil P Oommen <akhilpo@codeaurora.org>
To: Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
Jordan Crouse <jordan@cosmicpenguin.net>,
Rob Clark <robdclark@chromium.org>, Sean Paul <sean@poorly.run>,
David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
Iskren Chernev <iskren.chernev@gmail.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org>,
Konrad Dybcio <konrad.dybcio@somainline.org>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Marijn Suijten <marijn.suijten@somainline.org>,
Jonathan Marek <jonathan@marek.ca>,
Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
Sharat Masetty <smasetty@codeaurora.org>,
Douglas Anderson <dianders@chromium.org>,
Yangtao Li <tiny.windzz@gmail.com>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/5] drm/msm: Drop priv->lastctx
Date: Thu, 11 Nov 2021 22:14:31 +0530 [thread overview]
Message-ID: <1aa5b508-d225-1dbc-63ab-0958ac94c18a@codeaurora.org> (raw)
In-Reply-To: <20211109181117.591148-3-robdclark@gmail.com>
On 11/9/2021 11:41 PM, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> cur_ctx_seqno already does the same thing, but handles the edge cases
> where a refcnt'd context can live after lastclose. So let's not have
> two ways to do the same thing.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 3 +--
> drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 3 +--
> drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 3 +--
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 8 +++-----
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 +++------
> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 10 ----------
> drivers/gpu/drm/msm/msm_drv.c | 6 ------
> drivers/gpu/drm/msm/msm_drv.h | 2 +-
> drivers/gpu/drm/msm/msm_gpu.c | 2 +-
> drivers/gpu/drm/msm/msm_gpu.h | 11 +++++++++++
> 10 files changed, 22 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index bdc989183c64..22e8295a5e2b 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -12,7 +12,6 @@ static bool a2xx_idle(struct msm_gpu *gpu);
>
> static void a2xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct msm_ringbuffer *ring = submit->ring;
> unsigned int i;
>
> @@ -23,7 +22,7 @@ static void a2xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> /* ignore if there has not been a ctx switch: */
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index 8fb847c174ff..2e481e2692ba 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -30,7 +30,6 @@ static bool a3xx_idle(struct msm_gpu *gpu);
>
> static void a3xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct msm_ringbuffer *ring = submit->ring;
> unsigned int i;
>
> @@ -41,7 +40,7 @@ static void a3xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> /* ignore if there has not been a ctx switch: */
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index a96ee79cc5e0..c5524d6e8705 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -24,7 +24,6 @@ static bool a4xx_idle(struct msm_gpu *gpu);
>
> static void a4xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct msm_ringbuffer *ring = submit->ring;
> unsigned int i;
>
> @@ -35,7 +34,7 @@ static void a4xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> /* ignore if there has not been a ctx switch: */
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 5e2750eb3810..6163990a4d09 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -65,7 +65,6 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>
> static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct msm_ringbuffer *ring = submit->ring;
> struct msm_gem_object *obj;
> uint32_t *ptr, dwords;
> @@ -76,7 +75,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct msm_gem_submit *submit
> case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> @@ -126,12 +125,11 @@ static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu);
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct msm_ringbuffer *ring = submit->ring;
> unsigned int i, ibs = 0;
>
> if (IS_ENABLED(CONFIG_DRM_MSM_GPU_SUDO) && submit->in_rb) {
> - priv->lastctx = NULL;
> + gpu->cur_ctx_seqno = 0;
> a5xx_submit_in_rb(gpu, submit);
> return;
> }
> @@ -166,7 +164,7 @@ static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 33da25b81615..3d2da81cb2c9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -106,7 +106,7 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
> u32 asid;
> u64 memptr = rbmemptr(ring, ttbr0);
>
> - if (ctx->seqno == a6xx_gpu->cur_ctx_seqno)
> + if (ctx->seqno == a6xx_gpu->base.base.cur_ctx_seqno)
> return;
>
> if (msm_iommu_pagetable_params(ctx->aspace->mmu, &ttbr, &asid))
> @@ -138,14 +138,11 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
>
> OUT_PKT7(ring, CP_EVENT_WRITE, 1);
> OUT_RING(ring, 0x31);
> -
> - a6xx_gpu->cur_ctx_seqno = ctx->seqno;
> }
>
> static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> {
> unsigned int index = submit->seqno % MSM_GPU_SUBMIT_STATS_COUNT;
> - struct msm_drm_private *priv = gpu->dev->dev_private;
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> struct msm_ringbuffer *ring = submit->ring;
> @@ -177,7 +174,7 @@ static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> break;
> case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> - if (priv->lastctx == submit->queue->ctx)
> + if (gpu->cur_ctx_seqno == submit->queue->ctx->seqno)
> break;
> fallthrough;
> case MSM_SUBMIT_CMD_BUF:
> @@ -1081,7 +1078,7 @@ static int hw_init(struct msm_gpu *gpu)
> /* Always come up on rb 0 */
> a6xx_gpu->cur_ring = gpu->rb[0];
>
> - a6xx_gpu->cur_ctx_seqno = 0;
> + gpu->cur_ctx_seqno = 0;
>
> /* Enable the SQE_to start the CP engine */
> gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 1);
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index 8e5527c881b1..86e0a7c3fe6d 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -20,16 +20,6 @@ struct a6xx_gpu {
>
> struct msm_ringbuffer *cur_ring;
>
> - /**
> - * cur_ctx_seqno:
> - *
> - * The ctx->seqno value of the context with current pgtables
> - * installed. Tracked by seqno rather than pointer value to
> - * avoid dangling pointers, and cases where a ctx can be freed
> - * and a new one created with the same address.
> - */
> - int cur_ctx_seqno;
> -
> struct a6xx_gmu gmu;
>
> struct drm_gem_object *shadow_bo;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 7936e8d498dd..73e827641024 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -752,14 +752,8 @@ static void context_close(struct msm_file_private *ctx)
>
> static void msm_postclose(struct drm_device *dev, struct drm_file *file)
> {
> - struct msm_drm_private *priv = dev->dev_private;
> struct msm_file_private *ctx = file->driver_priv;
>
> - mutex_lock(&dev->struct_mutex);
> - if (ctx == priv->lastctx)
> - priv->lastctx = NULL;
> - mutex_unlock(&dev->struct_mutex);
> -
> context_close(ctx);
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 69952b239384..2943c21d9aac 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -164,7 +164,7 @@ struct msm_drm_private {
>
> /* when we have more than one 'msm_gpu' these need to be an array: */
> struct msm_gpu *gpu;
> - struct msm_file_private *lastctx;
> +
> /* gpu is only set on open(), but we need this info earlier */
> bool is_a2xx;
> bool has_cached_coherent;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 2c46cd968ac4..3dfc58e6498f 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -763,7 +763,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> mutex_unlock(&gpu->active_lock);
>
> gpu->funcs->submit(gpu, submit);
> - priv->lastctx = submit->queue->ctx;
> + gpu->cur_ctx_seqno = submit->queue->ctx->seqno;
>
> hangcheck_timer_reset(gpu);
> }
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 59870095ea41..623ee416c568 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -144,6 +144,17 @@ struct msm_gpu {
> struct msm_ringbuffer *rb[MSM_GPU_MAX_RINGS];
> int nr_rings;
>
> + /**
> + * cur_ctx_seqno:
> + *
> + * The ctx->seqno value of the last context to submit rendering,
> + * and the one with current pgtables installed (for generations
> + * that support per-context pgtables). Tracked by seqno rather
> + * than pointer value to avoid dangling pointers, and cases where
> + * a ctx can be freed and a new one created with the same address.
> + */
> + int cur_ctx_seqno;
> +
> /*
> * List of GEM active objects on this gpu. Protected by
> * msm_drm_private::mm_lock
>
Reviewed-by: Akhil P Oommen <akhilpo@codeaurora.org>
-Akhil.
next prev parent reply other threads:[~2021-11-11 16:45 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-09 18:11 [PATCH 0/5] drm/msm: Cleanup and drm/sched tdr prep Rob Clark
2021-11-09 18:11 ` [PATCH 1/5] drm/msm: Remove unnecessary struct_mutex Rob Clark
2021-11-09 18:11 ` [PATCH 2/5] drm/msm: Drop priv->lastctx Rob Clark
2021-11-11 16:44 ` Akhil P Oommen [this message]
2021-11-09 18:11 ` [PATCH 3/5] drm/msm: Remove struct_mutex usage Rob Clark
2021-11-09 18:11 ` [PATCH 4/5] drm/msm: Handle fence rollover Rob Clark
2021-11-11 16:58 ` Akhil P Oommen
2021-11-09 18:11 ` [PATCH 5/5] drm/msm: Add debugfs to disable hw err handling Rob Clark
2021-11-11 17:14 ` Akhil P Oommen
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=1aa5b508-d225-1dbc-63ab-0958ac94c18a@codeaurora.org \
--to=akhilpo@codeaurora.org \
--cc=airlied@linux.ie \
--cc=angelogioacchino.delregno@somainline.org \
--cc=dan.carpenter@oracle.com \
--cc=daniel@ffwll.ch \
--cc=dianders@chromium.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=iskren.chernev@gmail.com \
--cc=jonathan@marek.ca \
--cc=jordan@cosmicpenguin.net \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=robdclark@chromium.org \
--cc=robdclark@gmail.com \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=sean@poorly.run \
--cc=smasetty@codeaurora.org \
--cc=tiny.windzz@gmail.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox