[PATCH] drm/i915/gem: Calculate object page offset for partial memory mapping

[Andi Shyti]

Hi Nirmoy,

On Tue, Mar 26, 2024 at 01:05:37PM +0100, Nirmoy Das wrote:
> On 3/26/2024 12:12 PM, Andi Shyti wrote:

> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > index a2195e28b625..57a2dda2c3cc 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > @@ -276,7 +276,7 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf)
> > > >    	/* PTEs are revoked in obj->ops->put_pages() */
> > > >    	err = remap_io_sg(area,
> > > >    			  area->vm_start, area->vm_end - area->vm_start,
> > > > -			  obj->mm.pages->sgl, iomap);
> > > > +			  obj->mm.pages->sgl, 0, iomap);
> > > Why don't we need partial mmap for CPU but only for GTT ?
> > As far as I understood we don't. I have a version with the CPU
> > offset as well in trybot[*]
> > 
> > But without support for segmented buffer objects, I don't know
> > how much this has any effect.
> 
> You confused me more :) Why segmented buffer object is needed for partial
> CPU mmap but not for GTT  ?

atually segmented bo's were introduced to support single dma
buffers instead of fragmented buffers. But this goes beyond the
scope of this patch.

> From high level,  GTT and CPU both should support partial mmap unless I
> missing something here.

But yes, we could take the patch I linked which adds some offset
to the cpu memory. I will add it in V2.

> > 
> > > Sounds like this also need to be cover by a IGT tests.
> > Yes, I it does need some igt work, working on it.
> > 
> > > Don't we need "Fixes" tag for this?
> > Why should we? I'm not fixing anything here,
> 
> If userspace  expects partial mmap to work then this is a bug/gap in i915 so
> we need to
> 
> backport this as far as possible. Need some information about the
> requirement about  why we need this patch suddenly?

But a gap is not a bug. Theoretically we are adding a feature.

On the other hand it would be a bug if the API promises to add
the offset but in reality it doesn't. I will check if this is the
case and it needs to be well described in the commit message.

Thanks,
Andi