A couple of GTT page allocation patches

A couple of GTT page allocation patches

[Chris Wilson]

Just a couple of patches from watching talos constantly swap in/out.
Doing shrink_all after a shrink in this situation was just scanning the
same list as the shrink to no avail, so useless without mixing in the
full slab shrinker. And since there is no way to actually disable
SWIOTLB (weird kconfig), we should do our best in its presence.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail

[Chris Wilson]

When we notice the system under memory pressure, we try to evict some
driver pages before asking the VM to shrink all caches. As a final step
in that process, we tried to evict everything, including active buffers.
This is harming ourselves, and we can mix shrinking all caches as well
as our residual buffers (after the first pass of trying to shrink just
our own buffers).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38a183faf9a7..ca1a5a5c6f19 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2246,7 +2246,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			 * our own buffer, now let the real VM do its job and
 			 * go down in flames if truly OOM.
 			 */
-			i915_gem_shrink_all(dev_priv);
 			page = shmem_read_mapping_page(mapping, i);
 			if (IS_ERR(page)) {
 				ret = PTR_ERR(page);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Chris Wilson]

commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
work with SWIOTLB backend") took a heavy handed approach to undo the
scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
whereby we tried to pass a segment larger than SWIOTLB could handle.) We
can be a little more intelligent and try compacting the scatterlist up
to the maximum SWIOTLB segment size (when using SWIOTLB).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
CC: Imre Deak <imre.deak@intel.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca1a5a5c6f19..8b3474d215a5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	struct sgt_iter sgt_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
+	unsigned long max_segment;
 	int ret;
 	gfp_t gfp;
 
@@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
 	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
 
+	max_segment = obj->base.size;
+#ifdef CONFIG_SWIOTLB
+	if (swiotlb_nr_tbl())
+		max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
+#endif
+
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
 		return ERR_PTR(-ENOMEM);
@@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 				goto err_pages;
 			}
 		}
-#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->length >= max_segment ||
+		    page_to_pfn(page) != last_pfn + 1) {
 			if (i)
 				sg = sg_next(sg);
 			st->nents++;
@@ -2273,9 +2274,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
 	}
-#ifdef CONFIG_SWIOTLB
-	if (!swiotlb_nr_tbl())
-#endif
+	if (st->nents < st->orig_nents)
 		sg_mark_end(sg);
 
 	ret = i915_gem_gtt_prepare_pages(obj, st);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail

[Michał Winiarski]

On Mon, Oct 10, 2016 at 12:49:58PM +0100, Chris Wilson wrote:
> When we notice the system under memory pressure, we try to evict some
> driver pages before asking the VM to shrink all caches. As a final step
> in that process, we tried to evict everything, including active buffers.
> This is harming ourselves, and we can mix shrinking all caches as well
> as our residual buffers (after the first pass of trying to shrink just
> our own buffers).
 
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 1 -
>  1 file changed, 1 deletion(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Tvrtko Ursulin]

On 10/10/2016 12:49, Chris Wilson wrote:
> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> work with SWIOTLB backend") took a heavy handed approach to undo the
> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> can be a little more intelligent and try compacting the scatterlist up
> to the maximum SWIOTLB segment size (when using SWIOTLB).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Imre Deak <imre.deak@intel.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ca1a5a5c6f19..8b3474d215a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   	struct sgt_iter sgt_iter;
>   	struct page *page;
>   	unsigned long last_pfn = 0;	/* suppress gcc warning */
> +	unsigned long max_segment;

unsigned int would be enough.

>   	int ret;
>   	gfp_t gfp;
>   
> @@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>   	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>   
> +	max_segment = obj->base.size;
> +#ifdef CONFIG_SWIOTLB
> +	if (swiotlb_nr_tbl())
> +		max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
> +#endif
> +

Do you want to use IS_ENABLED here?

>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (st == NULL)
>   		return ERR_PTR(-ENOMEM);
> @@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   				goto err_pages;
>   			}
>   		}
> -#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->length >= max_segment ||

I think this can overflow by a page, should be "sg->length >= 
(max_segment - PAGE_SIZE)", or alternatively substract one page at the 
max_segment assignment.

> +		    page_to_pfn(page) != last_pfn + 1) {
>   			if (i)
>   				sg = sg_next(sg);
>   			st->nents++;
> @@ -2273,9 +2274,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   		/* Check that the i965g/gm workaround works. */
>   		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>   	}
> -#ifdef CONFIG_SWIOTLB
> -	if (!swiotlb_nr_tbl())
> -#endif
> +	if (st->nents < st->orig_nents)
>   		sg_mark_end(sg);

I wondered a few times that we could just terminate the table 
unconditionally.

>   
>   	ret = i915_gem_gtt_prepare_pages(obj, st);

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Tvrtko Ursulin]

On 10/10/2016 14:30, Tvrtko Ursulin wrote:
>
> On 10/10/2016 12:49, Chris Wilson wrote:
>> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
>> work with SWIOTLB backend") took a heavy handed approach to undo the
>> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
>> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
>> can be a little more intelligent and try compacting the scatterlist up
>> to the maximum SWIOTLB segment size (when using SWIOTLB).
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> CC: Imre Deak <imre.deak@intel.com>
>> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index ca1a5a5c6f19..8b3474d215a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct 
>> drm_i915_gem_object *obj)
>>       struct sgt_iter sgt_iter;
>>       struct page *page;
>>       unsigned long last_pfn = 0;    /* suppress gcc warning */
>> +    unsigned long max_segment;
>
> unsigned int would be enough.
>
>>       int ret;
>>       gfp_t gfp;
>>   @@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct 
>> drm_i915_gem_object *obj)
>>       GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>>       GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>>   +    max_segment = obj->base.size;
>> +#ifdef CONFIG_SWIOTLB
>> +    if (swiotlb_nr_tbl())
>> +        max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
>> +#endif
>> +
>
> Do you want to use IS_ENABLED here?
>
>>       st = kmalloc(sizeof(*st), GFP_KERNEL);
>>       if (st == NULL)
>>           return ERR_PTR(-ENOMEM);
>> @@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct 
>> drm_i915_gem_object *obj)
>>                   goto err_pages;
>>               }
>>           }
>> -#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->length >= max_segment ||
>
> I think this can overflow by a page, should be "sg->length >= 
> (max_segment - PAGE_SIZE)", or alternatively substract one page at the 
> max_segment assignment.
>

Or not. :)

Regards.

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Chris Wilson]

On Mon, Oct 10, 2016 at 02:30:40PM +0100, Tvrtko Ursulin wrote:
> 
> On 10/10/2016 12:49, Chris Wilson wrote:
> >commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> >work with SWIOTLB backend") took a heavy handed approach to undo the
> >scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> >whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> >can be a little more intelligent and try compacting the scatterlist up
> >to the maximum SWIOTLB segment size (when using SWIOTLB).
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >CC: Imre Deak <imre.deak@intel.com>
> >CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index ca1a5a5c6f19..8b3474d215a5 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	struct sgt_iter sgt_iter;
> >  	struct page *page;
> >  	unsigned long last_pfn = 0;	/* suppress gcc warning */
> >+	unsigned long max_segment;
> 
> unsigned int would be enough.
> 

Current maximum object size >> PAGE_SHIFT is 36 bits. We don't impose
any other restriction that would limit a sg chunk.

> >  	int ret;
> >  	gfp_t gfp;
> >@@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> >  	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
> >+	max_segment = obj->base.size;
> >+#ifdef CONFIG_SWIOTLB
> >+	if (swiotlb_nr_tbl())
> >+		max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
> >+#endif
> >+
> 
> Do you want to use IS_ENABLED here?

The symbol swiotlb_nr_tbl() is absent unless SWIOTLB is enabled at compile
time.  So we need the cpp guard, or do you mean switch to
	#if IS_ENABLED(CONFIG_SWIOTLB)
which we probably should indeed.
 
> >  	st = kmalloc(sizeof(*st), GFP_KERNEL);
> >  	if (st == NULL)
> >  		return ERR_PTR(-ENOMEM);
> >@@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  				goto err_pages;
> >  			}
> >  		}
> >-#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->length >= max_segment ||
> 
> I think this can overflow by a page, should be "sg->length >=
> (max_segment - PAGE_SIZE)", or alternatively substract one page at
> the max_segment assignment.

We are looking at the previous sg, right? (and we only ever increment by
PAGE_SIZE).

So: when the previous sg reaches the maximum length, start a new sg
element. Otherwise we extend the previous sg element by a PAGE, so on
the else branch the maximum of sg->length after the increment is
max_segment.

> >+		    page_to_pfn(page) != last_pfn + 1) {
> >  			if (i)
> >  				sg = sg_next(sg);
> >  			st->nents++;
> >@@ -2273,9 +2274,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  		/* Check that the i965g/gm workaround works. */
> >  		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
> >  	}
> >-#ifdef CONFIG_SWIOTLB
> >-	if (!swiotlb_nr_tbl())
> >-#endif
> >+	if (st->nents < st->orig_nents)
> >  		sg_mark_end(sg);
> 
> I wondered a few times that we could just terminate the table
> unconditionally.

The caveat being that if we do insert orig_nents, then sg at this point
is NULL. Which is clearer:

	if (st->nents < st->orig_nents) sg_mark_end(sg);

or

	if (sg) sg_mark_end(sg); /* coalesced sg table */

?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Tvrtko Ursulin]

On 10/10/2016 14:43, Chris Wilson wrote:
> On Mon, Oct 10, 2016 at 02:30:40PM +0100, Tvrtko Ursulin wrote:
>> On 10/10/2016 12:49, Chris Wilson wrote:
>>> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
>>> work with SWIOTLB backend") took a heavy handed approach to undo the
>>> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
>>> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
>>> can be a little more intelligent and try compacting the scatterlist up
>>> to the maximum SWIOTLB segment size (when using SWIOTLB).
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> CC: Imre Deak <imre.deak@intel.com>
>>> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++------------
>>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index ca1a5a5c6f19..8b3474d215a5 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2201,6 +2201,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>>>   	struct sgt_iter sgt_iter;
>>>   	struct page *page;
>>>   	unsigned long last_pfn = 0;	/* suppress gcc warning */
>>> +	unsigned long max_segment;
>> unsigned int would be enough.
>>
> Current maximum object size >> PAGE_SHIFT is 36 bits. We don't impose
> any other restriction that would limit a sg chunk.

My bad, for some reason I thought it is used only under the 
CONFIG_SWIOTLB guard.

>>>   	int ret;
>>>   	gfp_t gfp;
>>> @@ -2211,6 +2212,12 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>>>   	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>>>   	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>>> +	max_segment = obj->base.size;
>>> +#ifdef CONFIG_SWIOTLB
>>> +	if (swiotlb_nr_tbl())
>>> +		max_segment = IO_TLB_SEGSIZE << PAGE_SHIFT;
>>> +#endif
>>> +
>> Do you want to use IS_ENABLED here?
> The symbol swiotlb_nr_tbl() is absent unless SWIOTLB is enabled at compile
> time.  So we need the cpp guard, or do you mean switch to
> 	#if IS_ENABLED(CONFIG_SWIOTLB)
> which we probably should indeed.

Ok, doesn't matter then.

>   
>>>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>>>   	if (st == NULL)
>>>   		return ERR_PTR(-ENOMEM);
>>> @@ -2252,15 +2259,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>>>   				goto err_pages;
>>>   			}
>>>   		}
>>> -#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->length >= max_segment ||
>> I think this can overflow by a page, should be "sg->length >=
>> (max_segment - PAGE_SIZE)", or alternatively substract one page at
>> the max_segment assignment.
> We are looking at the previous sg, right? (and we only ever increment by
> PAGE_SIZE).
>
> So: when the previous sg reaches the maximum length, start a new sg
> element. Otherwise we extend the previous sg element by a PAGE, so on
> the else branch the maximum of sg->length after the increment is
> max_segment.

Yes I've corrected myself shortly after posting.

>
>>> +		    page_to_pfn(page) != last_pfn + 1) {
>>>   			if (i)
>>>   				sg = sg_next(sg);
>>>   			st->nents++;
>>> @@ -2273,9 +2274,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>>>   		/* Check that the i965g/gm workaround works. */
>>>   		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>>>   	}
>>> -#ifdef CONFIG_SWIOTLB
>>> -	if (!swiotlb_nr_tbl())
>>> -#endif
>>> +	if (st->nents < st->orig_nents)
>>>   		sg_mark_end(sg);
>> I wondered a few times that we could just terminate the table
>> unconditionally.
> The caveat being that if we do insert orig_nents, then sg at this point
> is NULL. Which is clearer:
>
> 	if (st->nents < st->orig_nents) sg_mark_end(sg);
>
> or
>
> 	if (sg) sg_mark_end(sg); /* coalesced sg table */
>
> ?

I missed that as well (that it can be NULL here). Sorry for the noise 
then. "if (sg)" looks somewhat better to me FWIW.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail
URL   : https://patchwork.freedesktop.org/series/13527/
State : warning

== Summary ==

Series 13527v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13527/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-byt-n2820)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq)
Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-hsw-4770)
                pass       -> SKIP       (fi-ilk-650)
                skip       -> PASS       (fi-bdw-5557u)
                skip       -> PASS       (fi-snb-2600)

fi-bdw-5557u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:215  dwarn:0   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:210  dwarn:1   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:184  dwarn:0   dfail:0   fail:2   skip:62 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:207  dwarn:0   dfail:0   fail:0   skip:41 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:223  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:221  dwarn:1   dfail:0   fail:0   skip:26 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:210  dwarn:0   dfail:0   fail:0   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_2661/

e37a15c8d775e79dddc8345a0f6afdcfe1f607d9 drm-intel-nightly: 2016y-10m-10d-14h-33m-29s UTC integration manifest
aa705d3 drm/i915: Remove self-harming shrink_all on get_pages_gtt fail

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Chris Wilson]

commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
work with SWIOTLB backend") took a heavy handed approach to undo the
scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
whereby we tried to pass a segment larger than SWIOTLB could handle.) We
can be a little more intelligent and try compacting the scatterlist up
to the maximum SWIOTLB segment size (when using SWIOTLB).

v2: Tidy sg_mark_end() and cpp

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
CC: Imre Deak <imre.deak@intel.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dff8d05d80ee..50fd611926cb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2201,6 +2201,15 @@ unlock:
 	mutex_unlock(&obj->mm.lock);
 }
 
+static unsigned long swiotlb_max_size(void)
+{
+#if IS_ENABLED(CONFIG_SWIOTLB)
+	return swiotlb_nr_tbl() << IO_TLB_SHIFT;
+#else
+	return 0;
+#endif
+}
+
 static struct sg_table *
 i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 {
@@ -2212,6 +2221,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	struct sgt_iter sgt_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
+	unsigned long max_segment;
 	int ret;
 	gfp_t gfp;
 
@@ -2222,6 +2232,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
 	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
 
+	max_segment = swiotlb_max_size();
+	if (!max_segment)
+		max_segment = obj->base.size;
+
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
 		return ERR_PTR(-ENOMEM);
@@ -2263,15 +2277,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 				goto err_pages;
 			}
 		}
-#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->length >= max_segment ||
+		    page_to_pfn(page) != last_pfn + 1) {
 			if (i)
 				sg = sg_next(sg);
 			st->nents++;
@@ -2284,9 +2292,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
 	}
-#ifdef CONFIG_SWIOTLB
-	if (!swiotlb_nr_tbl())
-#endif
+	if (sg) /* loop terminated early; short sg table */
 		sg_mark_end(sg);
 
 	ret = i915_gem_gtt_prepare_pages(obj, st);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail (rev2)
URL   : https://patchwork.freedesktop.org/series/13527/
State : warning

== Summary ==

Series 13527v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13527/revisions/2/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-byt-n2820)
Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-bdw-5557u)

fi-bdw-5557u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:213  dwarn:2   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:210  dwarn:1   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:185  dwarn:0   dfail:0   fail:2   skip:61 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:207  dwarn:0   dfail:0   fail:0   skip:41 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:221  dwarn:1   dfail:0   fail:0   skip:26 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2667/

e37a15c8d775e79dddc8345a0f6afdcfe1f607d9 drm-intel-nightly: 2016y-10m-10d-14h-33m-29s UTC integration manifest
575430f drm/i915: Remove self-harming shrink_all on get_pages_gtt fail

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Remove self-harming shrink_all on get_pages_gtt fail (rev2)

[Saarinen, Jani]

> == Summary ==
> 
> Series 13527v2 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/13527/revisions/2/mbox/
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 pass       -> DMESG-WARN (fi-byt-n2820)


Command	/opt/igt/tests/kms_flip --run-subtest basic-flip-vs-dpms

dmesg	
[  580.379142] [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 9
[  580.379149] Raw EDID:
[  580.379155]  	00 ff ff ff ff ff ff 00 04 72 5b 03 63 15 90 34
[  580.379159]  	31 17 01 03 80 35 1e 78 ee a0 a5 a6 56 52 9d 27
[  580.379162]  	0f 50 54 b3 0c 00 71 4f 81 80 81 c0 81 00 95 00
[  580.379165]  	b3 00 d1 c0 01 01 02 3a 80 18 71 38 2d 40 58 2c
[  580.379169]  	45 00 0f 28 21 00 00 1e 00 00 00 fd 00 38 4b 1f
[  580.379172]  	4b 12 00 0a 20 20 20 20 20 20 00 00 00 fc 00 47
[  580.379175]  	32 34 36 48 59 4c 0a 20 20 20 20 20 00 00 ff ff
[  580.379178]  	ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

> fi-byt-n2820     total:248  pass:210  dwarn:1   dfail:0   fail:1   skip:36
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Tvrtko Ursulin]

On 10/10/2016 23:27, Chris Wilson wrote:
> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> work with SWIOTLB backend") took a heavy handed approach to undo the
> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> can be a little more intelligent and try compacting the scatterlist up
> to the maximum SWIOTLB segment size (when using SWIOTLB).
>
> v2: Tidy sg_mark_end() and cpp
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Imre Deak <imre.deak@intel.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dff8d05d80ee..50fd611926cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2201,6 +2201,15 @@ unlock:
>   	mutex_unlock(&obj->mm.lock);
>   }
>   
> +static unsigned long swiotlb_max_size(void)
> +{
> +#if IS_ENABLED(CONFIG_SWIOTLB)
> +	return swiotlb_nr_tbl() << IO_TLB_SHIFT;
> +#else
> +	return 0;
> +#endif
> +}
> +
>   static struct sg_table *
>   i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   {
> @@ -2212,6 +2221,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   	struct sgt_iter sgt_iter;
>   	struct page *page;
>   	unsigned long last_pfn = 0;	/* suppress gcc warning */
> +	unsigned long max_segment;
>   	int ret;
>   	gfp_t gfp;
>   
> @@ -2222,6 +2232,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>   	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>   
> +	max_segment = swiotlb_max_size();
> +	if (!max_segment)
> +		max_segment = obj->base.size;
> +
>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (st == NULL)
>   		return ERR_PTR(-ENOMEM);
> @@ -2263,15 +2277,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   				goto err_pages;
>   			}
>   		}
> -#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->length >= max_segment ||
> +		    page_to_pfn(page) != last_pfn + 1) {
>   			if (i)
>   				sg = sg_next(sg);
>   			st->nents++;
> @@ -2284,9 +2292,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   		/* Check that the i965g/gm workaround works. */
>   		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>   	}
> -#ifdef CONFIG_SWIOTLB
> -	if (!swiotlb_nr_tbl())
> -#endif
> +	if (sg) /* loop terminated early; short sg table */
>   		sg_mark_end(sg);
>   
>   	ret = i915_gem_gtt_prepare_pages(obj, st);

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Konrad Rzeszutek Wilk]

On Mon, Oct 10, 2016 at 11:27:00PM +0100, Chris Wilson wrote:
> commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> work with SWIOTLB backend") took a heavy handed approach to undo the
> scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> can be a little more intelligent and try compacting the scatterlist up
> to the maximum SWIOTLB segment size (when using SWIOTLB).
> 

Won't this cause a bigger usage of the SWIOTLB bounce buffer ?

> v2: Tidy sg_mark_end() and cpp
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Imre Deak <imre.deak@intel.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dff8d05d80ee..50fd611926cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2201,6 +2201,15 @@ unlock:
>  	mutex_unlock(&obj->mm.lock);
>  }
>  
> +static unsigned long swiotlb_max_size(void)
> +{
> +#if IS_ENABLED(CONFIG_SWIOTLB)
> +	return swiotlb_nr_tbl() << IO_TLB_SHIFT;
> +#else
> +	return 0;
> +#endif
> +}
> +
>  static struct sg_table *
>  i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  {
> @@ -2212,6 +2221,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	struct sgt_iter sgt_iter;
>  	struct page *page;
>  	unsigned long last_pfn = 0;	/* suppress gcc warning */
> +	unsigned long max_segment;
>  	int ret;
>  	gfp_t gfp;
>  
> @@ -2222,6 +2232,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	GEM_BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
>  	GEM_BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
>  
> +	max_segment = swiotlb_max_size();
> +	if (!max_segment)
> +		max_segment = obj->base.size;
> +
>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (st == NULL)
>  		return ERR_PTR(-ENOMEM);
> @@ -2263,15 +2277,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  				goto err_pages;
>  			}
>  		}
> -#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->length >= max_segment ||
> +		    page_to_pfn(page) != last_pfn + 1) {
>  			if (i)
>  				sg = sg_next(sg);
>  			st->nents++;
> @@ -2284,9 +2292,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  		/* Check that the i965g/gm workaround works. */
>  		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
>  	}
> -#ifdef CONFIG_SWIOTLB
> -	if (!swiotlb_nr_tbl())
> -#endif
> +	if (sg) /* loop terminated early; short sg table */
>  		sg_mark_end(sg);
>  
>  	ret = i915_gem_gtt_prepare_pages(obj, st);
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Chris Wilson]

On Wed, Oct 12, 2016 at 05:19:14PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 10, 2016 at 11:27:00PM +0100, Chris Wilson wrote:
> > commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> > work with SWIOTLB backend") took a heavy handed approach to undo the
> > scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> > whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> > can be a little more intelligent and try compacting the scatterlist up
> > to the maximum SWIOTLB segment size (when using SWIOTLB).
> > 
> 
> Won't this cause a bigger usage of the SWIOTLB bounce buffer ?

It won't change the frequency of the usage of the bounce buffer, if that
is what you mean. Either you have intel-iommu and so will not go through
swiotlb, or you are forced to use swiotlb even though the hw doesn't
require it (swiotlb config is byzantium and always enabled unless you
hack it out and can rejoice at the lower cpu usage).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Allow compaction upto SWIOTLB max segment size

[Konrad Rzeszutek Wilk]

On Wed, Oct 12, 2016 at 10:51:57PM +0100, Chris Wilson wrote:
> On Wed, Oct 12, 2016 at 05:19:14PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Oct 10, 2016 at 11:27:00PM +0100, Chris Wilson wrote:
> > > commit 1625e7e549c5 ("drm/i915: make compact dma scatter lists creation
> > > work with SWIOTLB backend") took a heavy handed approach to undo the
> > > scatterlist compaction in the face of SWIOTLB. (The compaction hit a bug
> > > whereby we tried to pass a segment larger than SWIOTLB could handle.) We
> > > can be a little more intelligent and try compacting the scatterlist up
> > > to the maximum SWIOTLB segment size (when using SWIOTLB).
> > > 
> > 
> > Won't this cause a bigger usage of the SWIOTLB bounce buffer ?
> 
> It won't change the frequency of the usage of the bounce buffer, if that
> is what you mean. Either you have intel-iommu and so will not go through
> swiotlb, or you are forced to use swiotlb even though the hw doesn't
> require it (swiotlb config is byzantium and always enabled unless you
> hack it out and can rejoice at the lower cpu usage).

Hahah. Wish that was possible.

Anyhow my concern was with it under Xen, since the page_to_pfn(page) != last_pfn
check is useless there (the PFNs may be contingous but underneath
the machine frame numbers may be discontingous).

And was thinking that this check should be moved to some form of 'DMA-API'
type check, but that screams to me lots of work.

Perhaps expose another swiotlb call to query the preferred size of the
segments. So that you can get the 128 under baremetal SWIOTLB but under Xen
it may expose 1.

But that should not hold up this patch, and can be a followup patch I can
write.

Anyhow for *this* patch:

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

[as [xen,]swiotlb maintainer, in case you need this].
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx