| Message ID | 20240807104553.481763-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Improve pfn calculation readability in vm_fault_gtt() | expand |
On 2024-08-07 at 11:45:53 GMT, Andi Shyti wrote: > By moving the pfn calculation to the set_address_limits() > function we improve code readability. This way, > set_address_limits() is responsible for calculating all memory > mapping paramenters: "start", "end" and "pfn". > > This suggestion from Jonathan was made during the review of > commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix Virtual Memory mapping > boundaries calculation"), which I liked, but it got lost on the > way. > > Suggested-by: Jonathan Cavitt <Jonathan.cavitt@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> Reviewed-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com> Thanks Krzysztof
-----Original Message----- From: Andi Shyti <andi.shyti@linux.intel.com> Sent: Wednesday, August 7, 2024 3:46 AM To: intel-gfx <intel-gfx@lists.freedesktop.org>; dri-devel <dri-devel@lists.freedesktop.org> Cc: Niemiec, Krzysztof <krzysztof.niemiec@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com> Subject: [PATCH] drm/i915/gem: Improve pfn calculation readability in vm_fault_gtt() > > By moving the pfn calculation to the set_address_limits() > function we improve code readability. This way, > set_address_limits() is responsible for calculating all memory > mapping paramenters: "start", "end" and "pfn". > > This suggestion from Jonathan was made during the review of > commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix Virtual Memory mapping > boundaries calculation"), which I liked, but it got lost on the > way. > > Suggested-by: Jonathan Cavitt <Jonathan.cavitt@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index cac6d4184506..e9b2424156f0 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -293,8 +293,10 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf) > static void set_address_limits(struct vm_area_struct *area, > struct i915_vma *vma, > unsigned long obj_offset, > + resource_size_t gmadr_start, > unsigned long *start_vaddr, > - unsigned long *end_vaddr) > + unsigned long *end_vaddr, > + unsigned long *pfn) > { > unsigned long vm_start, vm_end, vma_size; /* user's memory parameters */ > long start, end; /* memory boundaries */ > @@ -323,6 +325,10 @@ static void set_address_limits(struct vm_area_struct *area, > /* Let's move back into the "<< PAGE_SHIFT" domain */ > *start_vaddr = (unsigned long)start << PAGE_SHIFT; > *end_vaddr = (unsigned long)end << PAGE_SHIFT; > + > + *pfn = (gmadr_start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; > + *pfn += (*start_vaddr - area->vm_start) >> PAGE_SHIFT; > + *pfn += obj_offset - vma->gtt_view.partial.offset; > } > > static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > @@ -441,11 +447,13 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > if (ret) > goto err_unpin; > > - set_address_limits(area, vma, obj_offset, &start, &end); > - > - pfn = (ggtt->gmadr.start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; > - pfn += (start - area->vm_start) >> PAGE_SHIFT; > - pfn += obj_offset - vma->gtt_view.partial.offset; > + /* > + * Dump all the necessary parameters in this function to perform the > + * arithmetic calculation for the virtual address start and end and > + * the PFN (Page Frame Number). > + */ > + set_address_limits(area, vma, obj_offset, ggtt->gmadr.start, > + &start, &end, &pfn); > > /* Finally, remap it using the new GTT offset */ > ret = remap_io_mapping(area, start, pfn, end - start, &ggtt->iomap); > -- > 2.45.2 > >
Hi, On Wed, Aug 07, 2024 at 11:45:53AM +0100, Andi Shyti wrote: > By moving the pfn calculation to the set_address_limits() > function we improve code readability. This way, > set_address_limits() is responsible for calculating all memory > mapping paramenters: "start", "end" and "pfn". > > This suggestion from Jonathan was made during the review of > commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix Virtual Memory mapping > boundaries calculation"), which I liked, but it got lost on the > way. > > Suggested-by: Jonathan Cavitt <Jonathan.cavitt@intel.com> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> merged to drm-intel-gt-next. Andi
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index cac6d4184506..e9b2424156f0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -293,8 +293,10 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf) static void set_address_limits(struct vm_area_struct *area, struct i915_vma *vma, unsigned long obj_offset, + resource_size_t gmadr_start, unsigned long *start_vaddr, - unsigned long *end_vaddr) + unsigned long *end_vaddr, + unsigned long *pfn) { unsigned long vm_start, vm_end, vma_size; /* user's memory parameters */ long start, end; /* memory boundaries */ @@ -323,6 +325,10 @@ static void set_address_limits(struct vm_area_struct *area, /* Let's move back into the "<< PAGE_SHIFT" domain */ *start_vaddr = (unsigned long)start << PAGE_SHIFT; *end_vaddr = (unsigned long)end << PAGE_SHIFT; + + *pfn = (gmadr_start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; + *pfn += (*start_vaddr - area->vm_start) >> PAGE_SHIFT; + *pfn += obj_offset - vma->gtt_view.partial.offset; } static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) @@ -441,11 +447,13 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) if (ret) goto err_unpin; - set_address_limits(area, vma, obj_offset, &start, &end); - - pfn = (ggtt->gmadr.start + i915_ggtt_offset(vma)) >> PAGE_SHIFT; - pfn += (start - area->vm_start) >> PAGE_SHIFT; - pfn += obj_offset - vma->gtt_view.partial.offset; + /* + * Dump all the necessary parameters in this function to perform the + * arithmetic calculation for the virtual address start and end and + * the PFN (Page Frame Number). + */ + set_address_limits(area, vma, obj_offset, ggtt->gmadr.start, + &start, &end, &pfn); /* Finally, remap it using the new GTT offset */ ret = remap_io_mapping(area, start, pfn, end - start, &ggtt->iomap);
By moving the pfn calculation to the set_address_limits() function we improve code readability. This way, set_address_limits() is responsible for calculating all memory mapping paramenters: "start", "end" and "pfn". This suggestion from Jonathan was made during the review of commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix Virtual Memory mapping boundaries calculation"), which I liked, but it got lost on the way. Suggested-by: Jonathan Cavitt <Jonathan.cavitt@intel.com> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)