Hi Andi,
thanks for review!
> > 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?
What I meant is the 'offset' value that is used to alter r->sgt.curr.
> > 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?
That would be issue 12031 from gitlab, which describes failure in
gem_ctx_engines@invalid-engines test.
> > - 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?
I am not 100% clear on what sg_dma_len() returns. Looking at
i915_scatterlist.h, inside __sgt_iter(), I see that scatterlist's max
is determined based on 'dma' flag, which suggests to me that
sg_dma_address() and s.sgp->length may be different values. Then later,
inside remap_io_sg() we compare 'offset' to the sg_dma_len(), but then
'offset' is used to set r.sgt.curr (so it corresponds to the current sg
item, right?), which then is compared to r.sgt.max in remap_sg().
At the same time, also in remap_io_sg(), the while condition comparison
uses sg_dma_len() regardless of 'dma' value (it is always false during
the test reported in the issue), which seems wrong, if sg_dma_len() and
s.sgp->length could hold different values (so we'd use s.sgp->length
to set s.max value in i915_scatterlist.h, but then use sg_dma_len()
as 'max' in remap_io_sg()).
I also noticed that these values are different in failed test runs.
Please correct me, if I'm wrong. I want to understand this issue and
the code better.
Thanks
Krzyszotf
