diff mbox series

[6/7] drm/amdgpu: Ensure kunmap is called on error

Message ID 20211210232404.4098157-7-ira.weiny@intel.com (mailing list archive)
State New, archived
Headers show
Series DRM kmap() fixes and kmap_local_page() conversions | expand

Commit Message

Ira Weiny Dec. 10, 2021, 11:24 p.m. UTC
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.

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(+)

Comments

Christian König Dec. 13, 2021, 8:37 p.m. UTC | #1
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?

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);
>   	}
>
Ira Weiny Dec. 14, 2021, 3:37 a.m. UTC | #2
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);
> >   	}
>
Christian König Dec. 14, 2021, 7:09 a.m. UTC | #3
Am 14.12.21 um 04:37 schrieb Ira Weiny:
> 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?

No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), 
it either creates the mapping or return the cached pointer.

A call to amdgpu_bo_kunmap() is only done in a few places where we know 
that the created mapping most likely won't be needed any more. If that's 
not done the mapping is automatically destroyed when the BO is moved or 
freed up.

I mean good bug fix, but you seem to see this as some kind of 
prerequisite to some follow up work converting TTM to use kmap_local() 
which most likely won't work in the first place.

Regards,
Christian.

>
> 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);
>>>    	}
Ira Weiny Dec. 15, 2021, 9:09 p.m. UTC | #4
On Tue, Dec 14, 2021 at 08:09:29AM +0100, Christian König wrote:
> Am 14.12.21 um 04:37 schrieb Ira Weiny:
> > 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?
> 
> No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), it
> either creates the mapping or return the cached pointer.

Ah I missed that.  Thanks.

> 
> A call to amdgpu_bo_kunmap() is only done in a few places where we know that
> the created mapping most likely won't be needed any more. If that's not done
> the mapping is automatically destroyed when the BO is moved or freed up.
> 
> I mean good bug fix, but you seem to see this as some kind of prerequisite
> to some follow up work converting TTM to use kmap_local() which most likely
> won't work in the first place.

Sure.  I see now that it is more complicated than I thought but I never thought
of this as a strict prerequisite.  Just something I found while trying to
figure out how this works.

How much of a speed up is it to maintain the ttm_bo_map_kmap map type?  Could
this all be done with vmap and just remove the kmap stuff?

Ira

> 
> Regards,
> Christian.
> 
> > 
> > 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);
> > > >    	}
>
Christian König Dec. 16, 2021, 8:32 a.m. UTC | #5
Am 15.12.21 um 22:09 schrieb Ira Weiny:
> On Tue, Dec 14, 2021 at 08:09:29AM +0100, Christian König wrote:
>> Am 14.12.21 um 04:37 schrieb Ira Weiny:
>>> 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?
>> No, the unmapping is not needed here. See the function amdgpu_bo_kmap(), it
>> either creates the mapping or return the cached pointer.
> Ah I missed that.  Thanks.
>
>> A call to amdgpu_bo_kunmap() is only done in a few places where we know that
>> the created mapping most likely won't be needed any more. If that's not done
>> the mapping is automatically destroyed when the BO is moved or freed up.
>>
>> I mean good bug fix, but you seem to see this as some kind of prerequisite
>> to some follow up work converting TTM to use kmap_local() which most likely
>> won't work in the first place.
> Sure.  I see now that it is more complicated than I thought but I never thought
> of this as a strict prerequisite.  Just something I found while trying to
> figure out how this works.
>
> How much of a speed up is it to maintain the ttm_bo_map_kmap map type?

Good question. I don't really know.

This used to be pretty important for older drivers since there the 
kernel needs to kmap individual pages and patch them up before sending 
the command stream to the hardware.

It most likely doesn't matter for modern hardware.

> Could this all be done with vmap and just remove the kmap stuff?

Maybe, but I wouldn't bet on it and I don't really want to touch any of 
the old drivers to figure that out.

Christian.

>
> Ira
>
>> Regards,
>> Christian.
>>
>>> 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);
>>>>>     	}
diff mbox series

Patch

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);
 	}