diff mbox series

[06/11] drm/radeon: stop using TTM_MEMTYPE_FLAG_MAPPABLE

Message ID 20200721073245.2484-6-christian.koenig@amd.com (mailing list archive)
State New, archived
Headers show
Series [01/11] drm: remove optional dummy function from drivers using TTM | expand

Commit Message

Christian König July 21, 2020, 7:32 a.m. UTC
The driver doesn't expose any not-mapable memory resources.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Daniel Vetter July 21, 2020, 9:24 a.m. UTC | #1
On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
> The driver doesn't expose any not-mapable memory resources.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 54af06df865b..b474781a0920 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  	switch (type) {
>  	case TTM_PL_SYSTEM:
>  		/* System memory */
> -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> +		man->flags = 0;
>  		man->available_caching = TTM_PL_MASK_CACHING;
>  		man->default_caching = TTM_PL_FLAG_CACHED;
>  		break;
> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  		man->func = &ttm_bo_manager_func;
>  		man->available_caching = TTM_PL_MASK_CACHING;
>  		man->default_caching = TTM_PL_FLAG_CACHED;
> -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> +		man->flags = 0;
>  #if IS_ENABLED(CONFIG_AGP)
>  		if (rdev->flags & RADEON_IS_AGP) {
>  			if (!rdev->ddev->agp) {
> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  					  (unsigned)type);
>  				return -EINVAL;
>  			}
> -			if (!rdev->ddev->agp->cant_use_aperture)
> -				man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;

There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
flag set. And radeon.ko did at least once work on these. And your patch to
disable agp only changes the default, it doesn't rip out the code.

So not sure your assumption here is correct.
-Daniel

>  			man->available_caching = TTM_PL_FLAG_UNCACHED |
>  						 TTM_PL_FLAG_WC;
>  			man->default_caching = TTM_PL_FLAG_WC;
> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>  	case TTM_PL_VRAM:
>  		/* "On-card" video ram */
>  		man->func = &ttm_bo_manager_func;
> -		man->flags = TTM_MEMTYPE_FLAG_FIXED |
> -			     TTM_MEMTYPE_FLAG_MAPPABLE;
> +		man->flags = TTM_MEMTYPE_FLAG_FIXED;
>  		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
>  		man->default_caching = TTM_PL_FLAG_WC;
>  		break;
> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>  
>  static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
>  {
> -	struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>  	struct radeon_device *rdev = radeon_get_rdev(bdev);
>  
>  	mem->bus.addr = NULL;
> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
>  	mem->bus.size = mem->num_pages << PAGE_SHIFT;
>  	mem->bus.base = 0;
>  	mem->bus.is_iomem = false;
> -	if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
> -		return -EINVAL;
> +
>  	switch (mem->mem_type) {
>  	case TTM_PL_SYSTEM:
>  		/* system memory */
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König July 21, 2020, 2:46 p.m. UTC | #2
Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
> On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
>> The driver doesn't expose any not-mapable memory resources.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 54af06df865b..b474781a0920 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>   	switch (type) {
>>   	case TTM_PL_SYSTEM:
>>   		/* System memory */
>> -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>> +		man->flags = 0;
>>   		man->available_caching = TTM_PL_MASK_CACHING;
>>   		man->default_caching = TTM_PL_FLAG_CACHED;
>>   		break;
>> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>   		man->func = &ttm_bo_manager_func;
>>   		man->available_caching = TTM_PL_MASK_CACHING;
>>   		man->default_caching = TTM_PL_FLAG_CACHED;
>> -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>> +		man->flags = 0;
>>   #if IS_ENABLED(CONFIG_AGP)
>>   		if (rdev->flags & RADEON_IS_AGP) {
>>   			if (!rdev->ddev->agp) {
>> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>   					  (unsigned)type);
>>   				return -EINVAL;
>>   			}
>> -			if (!rdev->ddev->agp->cant_use_aperture)
>> -				man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
> flag set. And radeon.ko did at least once work on these. And your patch to
> disable agp only changes the default, it doesn't rip out the code.

The key pint is that the flags for AGP are the same as the one for the 
PCIe path. So no functional change at all :)

The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().

Christian.

>
> So not sure your assumption here is correct.
> -Daniel
>
>>   			man->available_caching = TTM_PL_FLAG_UNCACHED |
>>   						 TTM_PL_FLAG_WC;
>>   			man->default_caching = TTM_PL_FLAG_WC;
>> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>   	case TTM_PL_VRAM:
>>   		/* "On-card" video ram */
>>   		man->func = &ttm_bo_manager_func;
>> -		man->flags = TTM_MEMTYPE_FLAG_FIXED |
>> -			     TTM_MEMTYPE_FLAG_MAPPABLE;
>> +		man->flags = TTM_MEMTYPE_FLAG_FIXED;
>>   		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
>>   		man->default_caching = TTM_PL_FLAG_WC;
>>   		break;
>> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>   
>>   static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
>>   {
>> -	struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>>   	struct radeon_device *rdev = radeon_get_rdev(bdev);
>>   
>>   	mem->bus.addr = NULL;
>> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
>>   	mem->bus.size = mem->num_pages << PAGE_SHIFT;
>>   	mem->bus.base = 0;
>>   	mem->bus.is_iomem = false;
>> -	if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
>> -		return -EINVAL;
>> +
>>   	switch (mem->mem_type) {
>>   	case TTM_PL_SYSTEM:
>>   		/* system memory */
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter July 22, 2020, 5:34 a.m. UTC | #3
On Tue, Jul 21, 2020 at 4:46 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
> > On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
> >> The driver doesn't expose any not-mapable memory resources.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
> >>   1 file changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> >> index 54af06df865b..b474781a0920 100644
> >> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>      switch (type) {
> >>      case TTM_PL_SYSTEM:
> >>              /* System memory */
> >> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >> +            man->flags = 0;
> >>              man->available_caching = TTM_PL_MASK_CACHING;
> >>              man->default_caching = TTM_PL_FLAG_CACHED;
> >>              break;
> >> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>              man->func = &ttm_bo_manager_func;
> >>              man->available_caching = TTM_PL_MASK_CACHING;
> >>              man->default_caching = TTM_PL_FLAG_CACHED;
> >> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >> +            man->flags = 0;
> >>   #if IS_ENABLED(CONFIG_AGP)
> >>              if (rdev->flags & RADEON_IS_AGP) {
> >>                      if (!rdev->ddev->agp) {
> >> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>                                        (unsigned)type);
> >>                              return -EINVAL;
> >>                      }
> >> -                    if (!rdev->ddev->agp->cant_use_aperture)
> >> -                            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> > There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
> > flag set. And radeon.ko did at least once work on these. And your patch to
> > disable agp only changes the default, it doesn't rip out the code.
>
> The key pint is that the flags for AGP are the same as the one for the
> PCIe path. So no functional change at all :)

I misread the code somehow, I didn't spot the unconditional setting of
FLAG_MAPPABLE for all TTM_PL_TT, irrespective of agp or not, somehow
thought that's another case.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>
> The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().
>
> Christian.
>
> >
> > So not sure your assumption here is correct.
> > -Daniel
> >
> >>                      man->available_caching = TTM_PL_FLAG_UNCACHED |
> >>                                               TTM_PL_FLAG_WC;
> >>                      man->default_caching = TTM_PL_FLAG_WC;
> >> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>      case TTM_PL_VRAM:
> >>              /* "On-card" video ram */
> >>              man->func = &ttm_bo_manager_func;
> >> -            man->flags = TTM_MEMTYPE_FLAG_FIXED |
> >> -                         TTM_MEMTYPE_FLAG_MAPPABLE;
> >> +            man->flags = TTM_MEMTYPE_FLAG_FIXED;
> >>              man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
> >>              man->default_caching = TTM_PL_FLAG_WC;
> >>              break;
> >> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
> >>
> >>   static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> >>   {
> >> -    struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
> >>      struct radeon_device *rdev = radeon_get_rdev(bdev);
> >>
> >>      mem->bus.addr = NULL;
> >> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
> >>      mem->bus.size = mem->num_pages << PAGE_SHIFT;
> >>      mem->bus.base = 0;
> >>      mem->bus.is_iomem = false;
> >> -    if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
> >> -            return -EINVAL;
> >> +
> >>      switch (mem->mem_type) {
> >>      case TTM_PL_SYSTEM:
> >>              /* system memory */
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Christian König July 22, 2020, 11:13 a.m. UTC | #4
Am 22.07.20 um 07:34 schrieb Daniel Vetter:
> On Tue, Jul 21, 2020 at 4:46 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
>>> On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
>>>> The driver doesn't expose any not-mapable memory resources.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
>>>>    1 file changed, 4 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> index 54af06df865b..b474781a0920 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>       switch (type) {
>>>>       case TTM_PL_SYSTEM:
>>>>               /* System memory */
>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>>> +            man->flags = 0;
>>>>               man->available_caching = TTM_PL_MASK_CACHING;
>>>>               man->default_caching = TTM_PL_FLAG_CACHED;
>>>>               break;
>>>> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>               man->func = &ttm_bo_manager_func;
>>>>               man->available_caching = TTM_PL_MASK_CACHING;
>>>>               man->default_caching = TTM_PL_FLAG_CACHED;
>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>>> +            man->flags = 0;
>>>>    #if IS_ENABLED(CONFIG_AGP)
>>>>               if (rdev->flags & RADEON_IS_AGP) {
>>>>                       if (!rdev->ddev->agp) {
>>>> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>                                         (unsigned)type);
>>>>                               return -EINVAL;
>>>>                       }
>>>> -                    if (!rdev->ddev->agp->cant_use_aperture)
>>>> -                            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>> There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
>>> flag set. And radeon.ko did at least once work on these. And your patch to
>>> disable agp only changes the default, it doesn't rip out the code.
>> The key pint is that the flags for AGP are the same as the one for the
>> PCIe path. So no functional change at all :)
> I misread the code somehow, I didn't spot the unconditional setting of
> FLAG_MAPPABLE for all TTM_PL_TT, irrespective of agp or not, somehow
> thought that's another case.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And for the amdgpu patch? Otherwise I just ping Alex for an rb.

Thanks,
Christian.

>
>> The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().
>>
>> Christian.
>>
>>> So not sure your assumption here is correct.
>>> -Daniel
>>>
>>>>                       man->available_caching = TTM_PL_FLAG_UNCACHED |
>>>>                                                TTM_PL_FLAG_WC;
>>>>                       man->default_caching = TTM_PL_FLAG_WC;
>>>> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>       case TTM_PL_VRAM:
>>>>               /* "On-card" video ram */
>>>>               man->func = &ttm_bo_manager_func;
>>>> -            man->flags = TTM_MEMTYPE_FLAG_FIXED |
>>>> -                         TTM_MEMTYPE_FLAG_MAPPABLE;
>>>> +            man->flags = TTM_MEMTYPE_FLAG_FIXED;
>>>>               man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
>>>>               man->default_caching = TTM_PL_FLAG_WC;
>>>>               break;
>>>> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>>
>>>>    static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
>>>>    {
>>>> -    struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>>>>       struct radeon_device *rdev = radeon_get_rdev(bdev);
>>>>
>>>>       mem->bus.addr = NULL;
>>>> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
>>>>       mem->bus.size = mem->num_pages << PAGE_SHIFT;
>>>>       mem->bus.base = 0;
>>>>       mem->bus.is_iomem = false;
>>>> -    if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
>>>> -            return -EINVAL;
>>>> +
>>>>       switch (mem->mem_type) {
>>>>       case TTM_PL_SYSTEM:
>>>>               /* system memory */
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C2fb0200ef32f4afcc3e208d82e00ef7d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637309928847998899&amp;sdata=9VclRJ7e3xfohsaLsiVF6Y83c%2Bkncmbo5uqoeV6tT9M%3D&amp;reserved=0
>
Daniel Vetter July 22, 2020, 11:42 a.m. UTC | #5
On Wed, Jul 22, 2020 at 1:13 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 22.07.20 um 07:34 schrieb Daniel Vetter:
> > On Tue, Jul 21, 2020 at 4:46 PM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >> Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
> >>> On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
> >>>> The driver doesn't expose any not-mapable memory resources.
> >>>>
> >>>> Signed-off-by: Christian König <christian.koenig@amd.com>
> >>>> ---
> >>>>    drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
> >>>>    1 file changed, 4 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>> index 54af06df865b..b474781a0920 100644
> >>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>       switch (type) {
> >>>>       case TTM_PL_SYSTEM:
> >>>>               /* System memory */
> >>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>> +            man->flags = 0;
> >>>>               man->available_caching = TTM_PL_MASK_CACHING;
> >>>>               man->default_caching = TTM_PL_FLAG_CACHED;
> >>>>               break;
> >>>> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>               man->func = &ttm_bo_manager_func;
> >>>>               man->available_caching = TTM_PL_MASK_CACHING;
> >>>>               man->default_caching = TTM_PL_FLAG_CACHED;
> >>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>> +            man->flags = 0;
> >>>>    #if IS_ENABLED(CONFIG_AGP)
> >>>>               if (rdev->flags & RADEON_IS_AGP) {
> >>>>                       if (!rdev->ddev->agp) {
> >>>> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>                                         (unsigned)type);
> >>>>                               return -EINVAL;
> >>>>                       }
> >>>> -                    if (!rdev->ddev->agp->cant_use_aperture)
> >>>> -                            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>> There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
> >>> flag set. And radeon.ko did at least once work on these. And your patch to
> >>> disable agp only changes the default, it doesn't rip out the code.
> >> The key pint is that the flags for AGP are the same as the one for the
> >> PCIe path. So no functional change at all :)
> > I misread the code somehow, I didn't spot the unconditional setting of
> > FLAG_MAPPABLE for all TTM_PL_TT, irrespective of agp or not, somehow
> > thought that's another case.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> And for the amdgpu patch? Otherwise I just ping Alex for an rb.

See my question over there, I'm not seeing how the code prevents mmap
for AMDGPU_PL_* domains after your patch. Once that's cleared up happy
to r-b that one and the final one too.
-Daniel

>
> Thanks,
> Christian.
>
> >
> >> The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().
> >>
> >> Christian.
> >>
> >>> So not sure your assumption here is correct.
> >>> -Daniel
> >>>
> >>>>                       man->available_caching = TTM_PL_FLAG_UNCACHED |
> >>>>                                                TTM_PL_FLAG_WC;
> >>>>                       man->default_caching = TTM_PL_FLAG_WC;
> >>>> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>       case TTM_PL_VRAM:
> >>>>               /* "On-card" video ram */
> >>>>               man->func = &ttm_bo_manager_func;
> >>>> -            man->flags = TTM_MEMTYPE_FLAG_FIXED |
> >>>> -                         TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>> +            man->flags = TTM_MEMTYPE_FLAG_FIXED;
> >>>>               man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
> >>>>               man->default_caching = TTM_PL_FLAG_WC;
> >>>>               break;
> >>>> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
> >>>>
> >>>>    static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> >>>>    {
> >>>> -    struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
> >>>>       struct radeon_device *rdev = radeon_get_rdev(bdev);
> >>>>
> >>>>       mem->bus.addr = NULL;
> >>>> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
> >>>>       mem->bus.size = mem->num_pages << PAGE_SHIFT;
> >>>>       mem->bus.base = 0;
> >>>>       mem->bus.is_iomem = false;
> >>>> -    if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
> >>>> -            return -EINVAL;
> >>>> +
> >>>>       switch (mem->mem_type) {
> >>>>       case TTM_PL_SYSTEM:
> >>>>               /* system memory */
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C2fb0200ef32f4afcc3e208d82e00ef7d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637309928847998899&amp;sdata=9VclRJ7e3xfohsaLsiVF6Y83c%2Bkncmbo5uqoeV6tT9M%3D&amp;reserved=0
> >
>
Christian König July 22, 2020, 11:49 a.m. UTC | #6
Am 22.07.20 um 13:42 schrieb Daniel Vetter:
> On Wed, Jul 22, 2020 at 1:13 PM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 22.07.20 um 07:34 schrieb Daniel Vetter:
>>> On Tue, Jul 21, 2020 at 4:46 PM Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>> Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
>>>>> On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
>>>>>> The driver doesn't expose any not-mapable memory resources.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
>>>>>>     1 file changed, 4 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> index 54af06df865b..b474781a0920 100644
>>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>>>> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>>>        switch (type) {
>>>>>>        case TTM_PL_SYSTEM:
>>>>>>                /* System memory */
>>>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>>>>> +            man->flags = 0;
>>>>>>                man->available_caching = TTM_PL_MASK_CACHING;
>>>>>>                man->default_caching = TTM_PL_FLAG_CACHED;
>>>>>>                break;
>>>>>> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>>>                man->func = &ttm_bo_manager_func;
>>>>>>                man->available_caching = TTM_PL_MASK_CACHING;
>>>>>>                man->default_caching = TTM_PL_FLAG_CACHED;
>>>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>>>>> +            man->flags = 0;
>>>>>>     #if IS_ENABLED(CONFIG_AGP)
>>>>>>                if (rdev->flags & RADEON_IS_AGP) {
>>>>>>                        if (!rdev->ddev->agp) {
>>>>>> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>>>                                          (unsigned)type);
>>>>>>                                return -EINVAL;
>>>>>>                        }
>>>>>> -                    if (!rdev->ddev->agp->cant_use_aperture)
>>>>>> -                            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>>>>> There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
>>>>> flag set. And radeon.ko did at least once work on these. And your patch to
>>>>> disable agp only changes the default, it doesn't rip out the code.
>>>> The key pint is that the flags for AGP are the same as the one for the
>>>> PCIe path. So no functional change at all :)
>>> I misread the code somehow, I didn't spot the unconditional setting of
>>> FLAG_MAPPABLE for all TTM_PL_TT, irrespective of agp or not, somehow
>>> thought that's another case.
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> And for the amdgpu patch? Otherwise I just ping Alex for an rb.
> See my question over there, I'm not seeing how the code prevents mmap
> for AMDGPU_PL_* domains after your patch. Once that's cleared up happy
> to r-b that one and the final one too.

I already replied, sounds like you never got that.

Anyway see the switch just below the two lines I removed:
>         switch (mem->mem_type) {
>         case TTM_PL_SYSTEM:
....
>         case TTM_PL_TT:
...
>         case TTM_PL_VRAM:
...
>         default:
>                 return -EINVAL;
>         }

So again, no functional change at all.

Cheers,
Christian.

> -Daniel
>
>> Thanks,
>> Christian.
>>
>>>> The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().
>>>>
>>>> Christian.
>>>>
>>>>> So not sure your assumption here is correct.
>>>>> -Daniel
>>>>>
>>>>>>                        man->available_caching = TTM_PL_FLAG_UNCACHED |
>>>>>>                                                 TTM_PL_FLAG_WC;
>>>>>>                        man->default_caching = TTM_PL_FLAG_WC;
>>>>>> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>>>>>>        case TTM_PL_VRAM:
>>>>>>                /* "On-card" video ram */
>>>>>>                man->func = &ttm_bo_manager_func;
>>>>>> -            man->flags = TTM_MEMTYPE_FLAG_FIXED |
>>>>>> -                         TTM_MEMTYPE_FLAG_MAPPABLE;
>>>>>> +            man->flags = TTM_MEMTYPE_FLAG_FIXED;
>>>>>>                man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
>>>>>>                man->default_caching = TTM_PL_FLAG_WC;
>>>>>>                break;
>>>>>> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>>>>
>>>>>>     static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
>>>>>>     {
>>>>>> -    struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>>>>>>        struct radeon_device *rdev = radeon_get_rdev(bdev);
>>>>>>
>>>>>>        mem->bus.addr = NULL;
>>>>>> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
>>>>>>        mem->bus.size = mem->num_pages << PAGE_SHIFT;
>>>>>>        mem->bus.base = 0;
>>>>>>        mem->bus.is_iomem = false;
>>>>>> -    if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
>>>>>> -            return -EINVAL;
>>>>>> +
>>>>>>        switch (mem->mem_type) {
>>>>>>        case TTM_PL_SYSTEM:
>>>>>>                /* system memory */
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel@lists.freedesktop.org
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7Cb10b1b6716484915990d08d82e3465f4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637310149874876539&amp;sdata=Ub8%2F0d13KkJdu7YAtFWpJtoVXuT8RrTewKSuBU6QnYo%3D&amp;reserved=0
>
Daniel Vetter July 22, 2020, 11:58 a.m. UTC | #7
On Wed, Jul 22, 2020 at 1:50 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 22.07.20 um 13:42 schrieb Daniel Vetter:
> > On Wed, Jul 22, 2020 at 1:13 PM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 22.07.20 um 07:34 schrieb Daniel Vetter:
> >>> On Tue, Jul 21, 2020 at 4:46 PM Christian König
> >>> <ckoenig.leichtzumerken@gmail.com> wrote:
> >>>> Am 21.07.20 um 11:24 schrieb daniel@ffwll.ch:
> >>>>> On Tue, Jul 21, 2020 at 09:32:40AM +0200, Christian König wrote:
> >>>>>> The driver doesn't expose any not-mapable memory resources.
> >>>>>>
> >>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/radeon/radeon_ttm.c | 13 ++++---------
> >>>>>>     1 file changed, 4 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>>>> index 54af06df865b..b474781a0920 100644
> >>>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >>>>>> @@ -76,7 +76,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>>>        switch (type) {
> >>>>>>        case TTM_PL_SYSTEM:
> >>>>>>                /* System memory */
> >>>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>>>> +            man->flags = 0;
> >>>>>>                man->available_caching = TTM_PL_MASK_CACHING;
> >>>>>>                man->default_caching = TTM_PL_FLAG_CACHED;
> >>>>>>                break;
> >>>>>> @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>>>                man->func = &ttm_bo_manager_func;
> >>>>>>                man->available_caching = TTM_PL_MASK_CACHING;
> >>>>>>                man->default_caching = TTM_PL_FLAG_CACHED;
> >>>>>> -            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>>>> +            man->flags = 0;
> >>>>>>     #if IS_ENABLED(CONFIG_AGP)
> >>>>>>                if (rdev->flags & RADEON_IS_AGP) {
> >>>>>>                        if (!rdev->ddev->agp) {
> >>>>>> @@ -92,8 +92,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>>>                                          (unsigned)type);
> >>>>>>                                return -EINVAL;
> >>>>>>                        }
> >>>>>> -                    if (!rdev->ddev->agp->cant_use_aperture)
> >>>>>> -                            man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>>> There is a bunch of agp drivers (alpha, ppc, that kind of stuff) with this
> >>>>> flag set. And radeon.ko did at least once work on these. And your patch to
> >>>>> disable agp only changes the default, it doesn't rip out the code.
> >>>> The key pint is that the flags for AGP are the same as the one for the
> >>>> PCIe path. So no functional change at all :)
> >>> I misread the code somehow, I didn't spot the unconditional setting of
> >>> FLAG_MAPPABLE for all TTM_PL_TT, irrespective of agp or not, somehow
> >>> thought that's another case.
> >>>
> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> And for the amdgpu patch? Otherwise I just ping Alex for an rb.
> > See my question over there, I'm not seeing how the code prevents mmap
> > for AMDGPU_PL_* domains after your patch. Once that's cleared up happy
> > to r-b that one and the final one too.
>
> I already replied, sounds like you never got that.

I got it, but I suck at reading mailing lists.

> Anyway see the switch just below the two lines I removed:
> >         switch (mem->mem_type) {
> >         case TTM_PL_SYSTEM:
> ....
> >         case TTM_PL_TT:
> ...
> >         case TTM_PL_VRAM:
> ...
> >         default:
> >                 return -EINVAL;
> >         }
>
> So again, no functional change at all.

Indeed, I score another point for being blind. r-b: also on the amdgpu
and final cleanup patch.
-Daniel

>
> Cheers,
> Christian.
>
> > -Daniel
> >
> >> Thanks,
> >> Christian.
> >>
> >>>> The real handling of cant_use_aperture is in radeon_ttm_io_mem_reserve().
> >>>>
> >>>> Christian.
> >>>>
> >>>>> So not sure your assumption here is correct.
> >>>>> -Daniel
> >>>>>
> >>>>>>                        man->available_caching = TTM_PL_FLAG_UNCACHED |
> >>>>>>                                                 TTM_PL_FLAG_WC;
> >>>>>>                        man->default_caching = TTM_PL_FLAG_WC;
> >>>>>> @@ -103,8 +101,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >>>>>>        case TTM_PL_VRAM:
> >>>>>>                /* "On-card" video ram */
> >>>>>>                man->func = &ttm_bo_manager_func;
> >>>>>> -            man->flags = TTM_MEMTYPE_FLAG_FIXED |
> >>>>>> -                         TTM_MEMTYPE_FLAG_MAPPABLE;
> >>>>>> +            man->flags = TTM_MEMTYPE_FLAG_FIXED;
> >>>>>>                man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
> >>>>>>                man->default_caching = TTM_PL_FLAG_WC;
> >>>>>>                break;
> >>>>>> @@ -394,7 +391,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
> >>>>>>
> >>>>>>     static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> >>>>>>     {
> >>>>>> -    struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
> >>>>>>        struct radeon_device *rdev = radeon_get_rdev(bdev);
> >>>>>>
> >>>>>>        mem->bus.addr = NULL;
> >>>>>> @@ -402,8 +398,7 @@ static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
> >>>>>>        mem->bus.size = mem->num_pages << PAGE_SHIFT;
> >>>>>>        mem->bus.base = 0;
> >>>>>>        mem->bus.is_iomem = false;
> >>>>>> -    if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
> >>>>>> -            return -EINVAL;
> >>>>>> +
> >>>>>>        switch (mem->mem_type) {
> >>>>>>        case TTM_PL_SYSTEM:
> >>>>>>                /* system memory */
> >>>>>> --
> >>>>>> 2.17.1
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> dri-devel mailing list
> >>>>>> dri-devel@lists.freedesktop.org
> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7Cb10b1b6716484915990d08d82e3465f4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637310149874876539&amp;sdata=Ub8%2F0d13KkJdu7YAtFWpJtoVXuT8RrTewKSuBU6QnYo%3D&amp;reserved=0
> >
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 54af06df865b..b474781a0920 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -76,7 +76,7 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 	switch (type) {
 	case TTM_PL_SYSTEM:
 		/* System memory */
-		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
+		man->flags = 0;
 		man->available_caching = TTM_PL_MASK_CACHING;
 		man->default_caching = TTM_PL_FLAG_CACHED;
 		break;
@@ -84,7 +84,7 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 		man->func = &ttm_bo_manager_func;
 		man->available_caching = TTM_PL_MASK_CACHING;
 		man->default_caching = TTM_PL_FLAG_CACHED;
-		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
+		man->flags = 0;
 #if IS_ENABLED(CONFIG_AGP)
 		if (rdev->flags & RADEON_IS_AGP) {
 			if (!rdev->ddev->agp) {
@@ -92,8 +92,6 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 					  (unsigned)type);
 				return -EINVAL;
 			}
-			if (!rdev->ddev->agp->cant_use_aperture)
-				man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
 			man->available_caching = TTM_PL_FLAG_UNCACHED |
 						 TTM_PL_FLAG_WC;
 			man->default_caching = TTM_PL_FLAG_WC;
@@ -103,8 +101,7 @@  static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
 	case TTM_PL_VRAM:
 		/* "On-card" video ram */
 		man->func = &ttm_bo_manager_func;
-		man->flags = TTM_MEMTYPE_FLAG_FIXED |
-			     TTM_MEMTYPE_FLAG_MAPPABLE;
+		man->flags = TTM_MEMTYPE_FLAG_FIXED;
 		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
 		man->default_caching = TTM_PL_FLAG_WC;
 		break;
@@ -394,7 +391,6 @@  static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
 
 static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 {
-	struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
 	struct radeon_device *rdev = radeon_get_rdev(bdev);
 
 	mem->bus.addr = NULL;
@@ -402,8 +398,7 @@  static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_
 	mem->bus.size = mem->num_pages << PAGE_SHIFT;
 	mem->bus.base = 0;
 	mem->bus.is_iomem = false;
-	if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
-		return -EINVAL;
+
 	switch (mem->mem_type) {
 	case TTM_PL_SYSTEM:
 		/* system memory */