[PATCH v4] drm/i915: ensure segment offset never exceeds allowed max

[Andi Shyti]

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 at intel.com>
> ---
>  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