All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ira Weiny <ira.weiny@intel.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Patrik Jakobsson <patrik.r.jakobsson@gmail.com>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error
Date: Mon, 13 Dec 2021 19:37:25 -0800	[thread overview]
Message-ID: <20211214033725.GR3538886@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <5bbd3c48-1388-9469-8b6f-deed64406d7d@amd.com>

On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The default case leaves the buffer object mapped in error.
> > 
> > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> 
> Mhm, good catch. But why do you want to do this in the first place?

I'm not sure I understand the question.

Any mapping of memory should be paired with an unmapping when no longer needed.
And this is supported by the call to amdgpu_bo_kunmap() in the other
non-default cases.

Do you believe the mapping is not needed?

Ira

> 
> Christian.
> 
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > NOTE: It seems like this function could use a fair bit of refactoring
> > but this is the easiest way to fix the actual bug.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> >   1 file changed, 1 insertion(+)
> > nice
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> >   		return 0;
> >   	default:
> > +		amdgpu_bo_kunmap(bo);
> >   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> >   	}
> 

WARNING: multiple messages have this Message-ID (diff)
From: Ira Weiny <ira.weiny@intel.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error
Date: Mon, 13 Dec 2021 19:37:25 -0800	[thread overview]
Message-ID: <20211214033725.GR3538886@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <5bbd3c48-1388-9469-8b6f-deed64406d7d@amd.com>

On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The default case leaves the buffer object mapped in error.
> > 
> > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> 
> Mhm, good catch. But why do you want to do this in the first place?

I'm not sure I understand the question.

Any mapping of memory should be paired with an unmapping when no longer needed.
And this is supported by the call to amdgpu_bo_kunmap() in the other
non-default cases.

Do you believe the mapping is not needed?

Ira

> 
> Christian.
> 
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > NOTE: It seems like this function could use a fair bit of refactoring
> > but this is the easiest way to fix the actual bug.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> >   1 file changed, 1 insertion(+)
> > nice
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> >   		return 0;
> >   	default:
> > +		amdgpu_bo_kunmap(bo);
> >   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> >   	}
> 

WARNING: multiple messages have this Message-ID (diff)
From: Ira Weiny <ira.weiny@intel.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error
Date: Mon, 13 Dec 2021 19:37:25 -0800	[thread overview]
Message-ID: <20211214033725.GR3538886@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <5bbd3c48-1388-9469-8b6f-deed64406d7d@amd.com>

On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The default case leaves the buffer object mapped in error.
> > 
> > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> 
> Mhm, good catch. But why do you want to do this in the first place?

I'm not sure I understand the question.

Any mapping of memory should be paired with an unmapping when no longer needed.
And this is supported by the call to amdgpu_bo_kunmap() in the other
non-default cases.

Do you believe the mapping is not needed?

Ira

> 
> Christian.
> 
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > NOTE: It seems like this function could use a fair bit of refactoring
> > but this is the easiest way to fix the actual bug.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> >   1 file changed, 1 insertion(+)
> > nice
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> >   		return 0;
> >   	default:
> > +		amdgpu_bo_kunmap(bo);
> >   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> >   	}
> 

WARNING: multiple messages have this Message-ID (diff)
From: Ira Weiny <ira.weiny@intel.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org,
	Patrik Jakobsson <patrik.r.jakobsson@gmail.com>,
	Rob Clark <robdclark@gmail.com>,
	dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error
Date: Mon, 13 Dec 2021 19:37:25 -0800	[thread overview]
Message-ID: <20211214033725.GR3538886@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <5bbd3c48-1388-9469-8b6f-deed64406d7d@amd.com>

On Mon, Dec 13, 2021 at 09:37:32PM +0100, Christian König wrote:
> Am 11.12.21 um 00:24 schrieb ira.weiny@intel.com:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The default case leaves the buffer object mapped in error.
> > 
> > Add amdgpu_bo_kunmap() to that case to ensure the mapping is cleaned up.
> 
> Mhm, good catch. But why do you want to do this in the first place?

I'm not sure I understand the question.

Any mapping of memory should be paired with an unmapping when no longer needed.
And this is supported by the call to amdgpu_bo_kunmap() in the other
non-default cases.

Do you believe the mapping is not needed?

Ira

> 
> Christian.
> 
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > NOTE: It seems like this function could use a fair bit of refactoring
> > but this is the easiest way to fix the actual bug.
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 1 +
> >   1 file changed, 1 insertion(+)
> > nice
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > index 6f8de11a17f1..b3ffd0f6b35f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> > @@ -889,6 +889,7 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
> >   		return 0;
> >   	default:
> > +		amdgpu_bo_kunmap(bo);
> >   		DRM_ERROR("Illegal UVD message type (%d)!\n", msg_type);
> >   	}
> 

  reply	other threads:[~2021-12-14  3:37 UTC|newest]

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-10 23:23 [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions ira.weiny
2021-12-10 23:23 ` [Intel-gfx] " ira.weiny
2021-12-10 23:23 ` ira.weiny
2021-12-10 23:23 ` [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page() ira.weiny
2021-12-10 23:23   ` [Intel-gfx] " ira.weiny
2021-12-10 23:23   ` ira.weiny
2021-12-13  9:04   ` Christoph Hellwig
2021-12-13  9:04     ` Christoph Hellwig
2021-12-13  9:04     ` [Intel-gfx] " Christoph Hellwig
2021-12-13 16:02     ` Ira Weiny
2021-12-13 16:02       ` Ira Weiny
2021-12-13 16:02       ` [Intel-gfx] " Ira Weiny
2021-12-13 16:02       ` Ira Weiny
2021-12-13 12:39   ` Ville Syrjälä
2021-12-13 12:39     ` Ville Syrjälä
2021-12-13 12:39     ` [Intel-gfx] " Ville Syrjälä
2021-12-13 12:39     ` Ville Syrjälä
2021-12-13 15:45     ` Ira Weiny
2021-12-13 15:45       ` Ira Weiny
2021-12-13 15:45       ` [Intel-gfx] " Ira Weiny
2021-12-13 15:45       ` Ira Weiny
2021-12-22  6:08   ` [PATCH V2] " ira.weiny
2021-12-22  6:08     ` [Intel-gfx] " ira.weiny
2021-12-22  6:08     ` ira.weiny
2021-12-10 23:23 ` [PATCH 2/7] drm/amd: " ira.weiny
2021-12-10 23:23   ` [Intel-gfx] " ira.weiny
2021-12-10 23:23   ` ira.weiny
2021-12-10 23:24 ` [PATCH 3/7] drm/gma: Remove calls to kmap() ira.weiny
2021-12-10 23:24   ` [Intel-gfx] " ira.weiny
2021-12-10 23:24   ` ira.weiny
2021-12-10 23:24 ` [PATCH 4/7] drm/radeon: Replace kmap() with kmap_local_page() ira.weiny
2021-12-10 23:24   ` [Intel-gfx] " ira.weiny
2021-12-10 23:24   ` ira.weiny
2021-12-10 23:24 ` [PATCH 5/7] drm/msm: Alter comment to use kmap_local_page() ira.weiny
2021-12-10 23:24   ` [Intel-gfx] " ira.weiny
2021-12-10 23:24   ` ira.weiny
2021-12-10 23:24 ` [PATCH 6/7] drm/amdgpu: Ensure kunmap is called on error ira.weiny
2021-12-10 23:24   ` [Intel-gfx] " ira.weiny
2021-12-10 23:24   ` ira.weiny
2021-12-13 20:37   ` Christian König
2021-12-13 20:37     ` [Intel-gfx] " Christian König
2021-12-13 20:37     ` Christian König
2021-12-14  3:37     ` Ira Weiny [this message]
2021-12-14  3:37       ` Ira Weiny
2021-12-14  3:37       ` [Intel-gfx] " Ira Weiny
2021-12-14  3:37       ` Ira Weiny
2021-12-14  7:09       ` Christian König
2021-12-14  7:09         ` Christian König
2021-12-14  7:09         ` [Intel-gfx] " Christian König
2021-12-14  7:09         ` Christian König
2021-12-15 21:09         ` Ira Weiny
2021-12-15 21:09           ` Ira Weiny
2021-12-15 21:09           ` [Intel-gfx] " Ira Weiny
2021-12-15 21:09           ` Ira Weiny
2021-12-16  8:32           ` Christian König
2021-12-16  8:32             ` Christian König
2021-12-16  8:32             ` [Intel-gfx] " Christian König
2021-12-16  8:32             ` Christian König
2021-12-10 23:24 ` [PATCH 7/7] drm/radeon: " ira.weiny
2021-12-10 23:24   ` [Intel-gfx] " ira.weiny
2021-12-10 23:24   ` ira.weiny
2021-12-10 23:36 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions Patchwork
2021-12-13 19:13 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions (rev2) Patchwork
2021-12-22  6:12 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DRM kmap() fixes and kmap_local_page() conversions (rev3) Patchwork
2022-01-19 16:53 ` [PATCH 0/7] DRM kmap() fixes and kmap_local_page() conversions Ira Weiny
2022-01-19 16:53   ` [Intel-gfx] " Ira Weiny
2022-01-19 16:53   ` Ira Weiny
2022-01-19 17:24   ` Daniel Vetter
2022-01-19 17:24     ` Daniel Vetter
2022-01-19 17:24     ` [Intel-gfx] " Daniel Vetter
2022-01-19 17:24     ` Daniel Vetter
2022-01-19 23:55     ` Ira Weiny
2022-01-19 23:55       ` [Intel-gfx] " Ira Weiny
2022-01-20  8:16       ` Christian König
2022-01-20  8:16         ` [Intel-gfx] " Christian König
2022-01-20 15:48         ` Daniel Vetter
2022-01-20 15:48           ` Daniel Vetter
2022-01-20 15:48           ` Daniel Vetter
2022-01-20 15:48           ` Daniel Vetter
2022-01-20 16:57           ` Ira Weiny

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=20211214033725.GR3538886@iweiny-DESK2.sc.intel.com \
    --to=ira.weiny@intel.com \
    --cc=airlied@linux.ie \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=christian.koenig@amd.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=patrik.r.jakobsson@gmail.com \
    --cc=robdclark@gmail.com \
    --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: link
Be 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.