Re: [PATCH] Change the meaning of the fields in the ttm_place structure from pfn to bytes

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Fri, Mar 03, 2023 at 03:55:56PM +0100, Michel Dänzer wrote:
> On 3/3/23 08:16, Somalapuram Amaranath wrote:
> > Change the ttm_place structure member fpfn, lpfn, mem_type to
> > res_start, res_end, res_type.
> > Change the unsigned to u64.
> > Fix the dependence in all the DRM drivers and
> > clean up PAGE_SHIFT operation.
> > 
> > Signed-off-by: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
> > 
> > [...]
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> > index 44367f03316f..5b5104e724e3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
> > @@ -131,11 +131,12 @@ static int amdgpu_gtt_mgr_new(struct ttm_resource_manager *man,
> >  		goto err_free;
> >  	}
> >  
> > -	if (place->lpfn) {
> > +	if (place->res_end) {
> >  		spin_lock(&mgr->lock);
> >  		r = drm_mm_insert_node_in_range(&mgr->mm, &node->mm_nodes[0],
> > -						num_pages, tbo->page_alignment,
> > -						0, place->fpfn, place->lpfn,
> > +						num_pages, tbo->page_alignment,	0,
> > +						place->res_start << PAGE_SHIFT,
> > +						place->res_end << PAGE_SHIFT,
> >  						DRM_MM_INSERT_BEST);
> 
> This should be >> or no shift instead of <<, shouldn't it? Multiplying a value in bytes by the page size doesn't make sense.
> 
> 
> I didn't check the rest of the patch in detail, but it's easy introduce subtle regressions with this kind of change. It'll require a lot of review & testing scrutiny.

Also good justification. The changelog says only what is done, nothing about why the change is needed.

Regards
Stanislaw