Re: [Xen-devel] [konrad.wilk@xxxxxxxxxx: [PATCH] drm/i915: make compact dma scatter lists creation work with SWIOTLB backend.]

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.13 at 16:13, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> On Tue, Jun 25, 2013 at 08:29:02AM +0100, Jan Beulich wrote:
>> >>> On 24.06.13 at 17:53, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
>> >>> wrote:
>> > --- a/drivers/gpu/drm/i915/i915_gem.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem.c
>> > @@ -1801,7 +1801,14 @@ i915_gem_object_get_pages_gtt(struct 
> drm_i915_gem_object *obj)
>> >                    gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>> >                    gfp &= ~(__GFP_IO | __GFP_WAIT);
>> >            }
>> > -
>> > +#ifdef CONFIG_SWIOTLB
>> > +          if (swiotlb_nr_tbl()) {
>> > +                  st->nents++;
>> > +                  sg_set_page(sg, page, PAGE_SIZE, 0);
>> > +                  sg = sg_next(sg);
>> > +                  continue;
>> > +          }
>> > +#endif
>> >            if (!i || page_to_pfn(page) != last_pfn + 1) {
>> >                    if (i)
>> >                            sg = sg_next(sg);
>> > @@ -1812,8 +1819,10 @@ i915_gem_object_get_pages_gtt(struct 
> drm_i915_gem_object *obj)
>> >            }
>> >            last_pfn = page_to_pfn(page);
>> >    }
>> > -
>> > -  sg_mark_end(sg);
>> > +#ifdef CONFIG_SWIOTLB
>> > +  if (!swiotlb_nr_tbl())
>> > +#endif
>> > +          sg_mark_end(sg);
>> >    obj->pages = st;
>> >  
>> >    if (i915_gem_object_needs_bit17_swizzle(obj))
>> 
>> Out of curiosity - while I can see the point of the first hunk, why do
>> you need to also suppress the setting of the list terminator?
> 
> It was crashing for me as sg was NULL for one-item sg structures. I didn't
> dig deep in it, but the combination of 'sg = sg_nex(sg)' and then
> sg_mark_end(sg) ended up with sg being NULL.
> 
> I could have made the 'sg = sg_next(sg)' be wrapped with a 'if (i < 
> page_count)'
> but figured it would be easier to reproduce the original code as faithfully
> as possible.

So that's because the first hunk is sort of backwards then:

#ifdef CONFIG_SWIOTLB
                if (swiotlb_nr_tbl()) {
                        if (i)
                                sg = sg_next(sg);
                        st->nents++;
                        sg_set_page(sg, page, PAGE_SIZE, 0);
                        continue;
                }
#endif

i.e. you want to match how the code immediately following this
addition works.

Once you do that, it also becomes obvious that you really
would just need to extend that following if() condition, i.e. by
latching the result of swiotlb_nr_tbl() into a local variable
(storing zero if !SWIOTLB), and then simply doing

                if (!i || page_to_pfn(page) != last_pfn + 1 || using_swiotlb) {

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel