Hi Krzysztof,
First of all I need to apologize for not being responsive on your
patches. We had offline reviews and some discussions though, but
they were not reported in the v1-v3 reviews. Next time, the
reviews need to have more visibility for the community.
On Mon, Oct 28, 2024 at 04:00:48PM +0000, Karas, Krzysztof wrote:
> Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for
> partial memory mapping") introduced a new offset that is compared to
> sg_dma_len(r.sgt.sgp) in remap_io_sg() function. However, later in
> remap_sg() the offset (which at that point resides in r->sgt.curr)
> is compared to r->sgt.max. Scatter-gather list's max relies on one
what we compare to max is not the offset, but the current sg
item, right? Or did I miss something?
> of two values (see i915_scatterlist.h):
> a) sg_dma_len(s.sgp) when `dma` is true,
> b) s.sgp->length otherwise.
> This suggests that in cases where `dma` is false, we should use
> s.sgp->length to determine the max value instead of sg_dma_len(),
> which is used regardless in remap_io_sg() (use_dma(iobase) might return
> false there).
>
> This patch uses r.sgt.max to check if offset is within allowed bounds,
> because that max value is already set according to the `dma` value.
are you trying to fix any issues here? If so, which one?
> v3:
> - instead of checking if r.sgt.curr would exceed allowed max, changed
> the value in the while loop to be aligned with `dma` value.
>
> v4:
> - remove unnecessary parent relation
>
> Signed-off-by: Krzysztof Karas <krzysztof.karas@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_mm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c
> index f5c97a620962..76e2801619f0 100644
> --- a/drivers/gpu/drm/i915/i915_mm.c
> +++ b/drivers/gpu/drm/i915/i915_mm.c
> @@ -143,8 +143,8 @@ int remap_io_sg(struct vm_area_struct *vma,
> /* We rely on prevalidation of the io-mapping to skip track_pfn(). */
> GEM_BUG_ON((vma->vm_flags & EXPECTED_FLAGS) != EXPECTED_FLAGS);
>
> - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) {
> - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT;
> + while (offset >= r.sgt.max >> PAGE_SHIFT) {
> + offset -= r.sgt.max >> PAGE_SHIFT;
To me looks right sg_dma_len(), why max?
Thanks a lot for your patch,
Andi
> r.sgt = __sgt_iter(__sg_next(r.sgt.sgp), use_dma(iobase));
> if (!r.sgt.sgp)
> return -EINVAL;
> --
> 2.34.1
