diff mbox

[4/5] drm/i915: Make scatter-gather helper available to the driver

Message ID 1414687178-18562-5-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Oct. 30, 2014, 4:39 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It will be used by other call sites shortly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         | 38 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_userptr.c | 43 ++-------------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  4 +++
 3 files changed, 44 insertions(+), 41 deletions(-)

Comments

Daniel Vetter Nov. 3, 2014, 4:02 p.m. UTC | #1 
On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:

Just a sideline comment here for now ;-)

> +/* intel_dma.c */
> +int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages);

Comments all over should tell you that i915_dma.c is super legacy
territory and really you should never touch it. I think i915_gem.c is a
better place.
-Daniel
Daniel Vetter Nov. 3, 2014, 4:05 p.m. UTC | #2 
On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It will be used by other call sites shortly.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         | 38 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 43 ++-------------------------------
>  drivers/gpu/drm/i915/intel_drv.h        |  4 +++
>  3 files changed, 44 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5b157bb..0b34571 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2070,3 +2070,41 @@ int i915_driver_device_is_agp(struct drm_device *dev)
>  {
>  	return 1;
>  }
> +
> +#if IS_ENABLED(CONFIG_SWIOTLB)
> +#define swiotlb_active() swiotlb_nr_tbl()
> +#else
> +#define swiotlb_active() 0
> +#endif
> +
> +int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
> +{
> +	struct scatterlist *sg;
> +	int ret, n;
> +
> +	*st = kmalloc(sizeof(**st), GFP_KERNEL);
> +	if (*st == NULL)
> +		return -ENOMEM;
> +
> +	if (swiotlb_active()) {
> +		ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
> +		if (ret)
> +			goto err;
> +
> +		for_each_sg((*st)->sgl, sg, num_pages, n)
> +			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
> +	} else {
> +		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
> +						0, num_pages << PAGE_SHIFT,
> +						GFP_KERNEL);
> +		if (ret)
> +			goto err;
> +	}

Ok, I don't really understand why we don't just always use
sg_alloc_table_from_pages undconditionally - if swiotlb _ever_ gets in
between us and the hw, everything will pretty much fall apart.

git blame doesn't shed light on this particular issue. Chris?
Tvrtko Ursulin Nov. 3, 2014, 4:18 p.m. UTC | #3 
On 11/03/2014 04:02 PM, Daniel Vetter wrote:
> On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:
>
> Just a sideline comment here for now ;-)
>
>> +/* intel_dma.c */
>> +int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages);
>
> Comments all over should tell you that i915_dma.c is super legacy
> territory and really you should never touch it. I think i915_gem.c is a
> better place.

Didn't spot any comments to this effect, in fact it looks like a 
mish-mash of stuff which is exactly why it looked appropriate for this. 
:) That said I don't mind moving it.

Regards,

Tvrtko
Daniel Vetter Nov. 3, 2014, 4:53 p.m. UTC | #4 
On Mon, Nov 03, 2014 at 04:18:30PM +0000, Tvrtko Ursulin wrote:
> 
> On 11/03/2014 04:02 PM, Daniel Vetter wrote:
> >On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:
> >
> >Just a sideline comment here for now ;-)
> >
> >>+/* intel_dma.c */
> >>+int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages);
> >
> >Comments all over should tell you that i915_dma.c is super legacy
> >territory and really you should never touch it. I think i915_gem.c is a
> >better place.
> 
> Didn't spot any comments to this effect, in fact it looks like a mish-mash
> of stuff which is exactly why it looked appropriate for this. :) That said I
> don't mind moving it.

There are 1-2 things in there still which needed saving, but overall this
file is doomed ;-)
-Daniel
Chris Wilson Nov. 3, 2014, 8:30 p.m. UTC | #5 
On Mon, Nov 03, 2014 at 05:05:55PM +0100, Daniel Vetter wrote:
> On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > It will be used by other call sites shortly.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c         | 38 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 43 ++-------------------------------
> >  drivers/gpu/drm/i915/intel_drv.h        |  4 +++
> >  3 files changed, 44 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 5b157bb..0b34571 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -2070,3 +2070,41 @@ int i915_driver_device_is_agp(struct drm_device *dev)
> >  {
> >  	return 1;
> >  }
> > +
> > +#if IS_ENABLED(CONFIG_SWIOTLB)
> > +#define swiotlb_active() swiotlb_nr_tbl()
> > +#else
> > +#define swiotlb_active() 0
> > +#endif
> > +
> > +int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
> > +{
> > +	struct scatterlist *sg;
> > +	int ret, n;
> > +
> > +	*st = kmalloc(sizeof(**st), GFP_KERNEL);
> > +	if (*st == NULL)
> > +		return -ENOMEM;
> > +
> > +	if (swiotlb_active()) {
> > +		ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
> > +		if (ret)
> > +			goto err;
> > +
> > +		for_each_sg((*st)->sgl, sg, num_pages, n)
> > +			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
> > +	} else {
> > +		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
> > +						0, num_pages << PAGE_SHIFT,
> > +						GFP_KERNEL);
> > +		if (ret)
> > +			goto err;
> > +	}
> 
> Ok, I don't really understand why we don't just always use
> sg_alloc_table_from_pages undconditionally - if swiotlb _ever_ gets in
> between us and the hw, everything will pretty much fall apart.
> 
> git blame doesn't shed light on this particular issue. Chris?

This is Imre's work...
-Chris
Imre Deak Nov. 4, 2014, 11:56 a.m. UTC | #6 
On Mon, 2014-11-03 at 20:30 +0000, Chris Wilson wrote:
> On Mon, Nov 03, 2014 at 05:05:55PM +0100, Daniel Vetter wrote:
> > On Thu, Oct 30, 2014 at 04:39:37PM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > It will be used by other call sites shortly.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c         | 38 +++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_gem_userptr.c | 43 ++-------------------------------
> > >  drivers/gpu/drm/i915/intel_drv.h        |  4 +++
> > >  3 files changed, 44 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 5b157bb..0b34571 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -2070,3 +2070,41 @@ int i915_driver_device_is_agp(struct drm_device *dev)
> > >  {
> > >  	return 1;
> > >  }
> > > +
> > > +#if IS_ENABLED(CONFIG_SWIOTLB)
> > > +#define swiotlb_active() swiotlb_nr_tbl()
> > > +#else
> > > +#define swiotlb_active() 0
> > > +#endif
> > > +
> > > +int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
> > > +{
> > > +	struct scatterlist *sg;
> > > +	int ret, n;
> > > +
> > > +	*st = kmalloc(sizeof(**st), GFP_KERNEL);
> > > +	if (*st == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	if (swiotlb_active()) {
> > > +		ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
> > > +		if (ret)
> > > +			goto err;
> > > +
> > > +		for_each_sg((*st)->sgl, sg, num_pages, n)
> > > +			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
> > > +	} else {
> > > +		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
> > > +						0, num_pages << PAGE_SHIFT,
> > > +						GFP_KERNEL);
> > > +		if (ret)
> > > +			goto err;
> > > +	}
> > 
> > Ok, I don't really understand why we don't just always use
> > sg_alloc_table_from_pages undconditionally - if swiotlb _ever_ gets in
> > between us and the hw, everything will pretty much fall apart.
> > 
> > git blame doesn't shed light on this particular issue. Chris?
> 
> This is Imre's work...

Afaict, the distinction was made because of swiotlb's limitation, which
prevented us from using compact sg tables produced by
sg_alloc_table_from_pages(). The commit below explains it more:

commit 426729dcc713b3d1ae802e314030e5556a62da53
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Mon Jun 24 11:47:48 2013 -0400

    drm/i915: make compact dma scatter lists creation work with SWIOTLB
backend.
    
    Git commit 90797e6d1ec0dfde6ba62a48b9ee3803887d6ed4
    ("drm/i915: create compact dma scatter lists for gem objects") makes
    certain assumptions about the under laying DMA API that are not
always
    correct.
    
    On a ThinkPad X230 with an Intel HD 4000 with Xen during the bootup
    I see:
    
    [drm:intel_pipe_set_base] *ERROR* pin & fence failed
    [drm:intel_crtc_set_config] *ERROR* failed to set mode on [CRTC:3],
err = -28
    
    Bit of debugging traced it down to dma_map_sg failing (in
    i915_gem_gtt_prepare_object) as some of the SG entries were huge
(3MB).
    
    That unfortunately are sizes that the SWIOTLB is incapable of
handling -
    the maximum it can handle is a an entry of 512KB of virtual
contiguous
    memory for its bounce buffer. (See IO_TLB_SEGSIZE).
    
    Previous to the above mention git commit the SG entries were of 4KB,
and
    the code introduced by above git commit squashed the CPU contiguous
PFNs
    in one big virtual address provided to DMA API.
    
    This patch is a simple semi-revert - were we emulate the old
behavior
    if we detect that SWIOTLB is online. If it is not online then we
continue
    on with the new compact scatter gather mechanism.
    
    An alternative solution would be for the the '.get_pages' and the
    i915_gem_gtt_prepare_object to retry with smaller max gap of the
    amount of PFNs that can be combined together - but with this issue
    discovered during rc7 that might be too risky.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5b157bb..0b34571 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2070,3 +2070,41 @@  int i915_driver_device_is_agp(struct drm_device *dev)
 {
 	return 1;
 }
+
+#if IS_ENABLED(CONFIG_SWIOTLB)
+#define swiotlb_active() swiotlb_nr_tbl()
+#else
+#define swiotlb_active() 0
+#endif
+
+int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
+{
+	struct scatterlist *sg;
+	int ret, n;
+
+	*st = kmalloc(sizeof(**st), GFP_KERNEL);
+	if (*st == NULL)
+		return -ENOMEM;
+
+	if (swiotlb_active()) {
+		ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
+		if (ret)
+			goto err;
+
+		for_each_sg((*st)->sgl, sg, num_pages, n)
+			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
+	} else {
+		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
+						0, num_pages << PAGE_SHIFT,
+						GFP_KERNEL);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	kfree(*st);
+	*st = NULL;
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index d384139..d3d55b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -479,45 +479,6 @@  struct get_pages_work {
 	struct task_struct *task;
 };
 
-#if IS_ENABLED(CONFIG_SWIOTLB)
-#define swiotlb_active() swiotlb_nr_tbl()
-#else
-#define swiotlb_active() 0
-#endif
-
-static int
-st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
-{
-	struct scatterlist *sg;
-	int ret, n;
-
-	*st = kmalloc(sizeof(**st), GFP_KERNEL);
-	if (*st == NULL)
-		return -ENOMEM;
-
-	if (swiotlb_active()) {
-		ret = sg_alloc_table(*st, num_pages, GFP_KERNEL);
-		if (ret)
-			goto err;
-
-		for_each_sg((*st)->sgl, sg, num_pages, n)
-			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
-	} else {
-		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
-						0, num_pages << PAGE_SHIFT,
-						GFP_KERNEL);
-		if (ret)
-			goto err;
-	}
-
-	return 0;
-
-err:
-	kfree(*st);
-	*st = NULL;
-	return ret;
-}
-
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
@@ -557,7 +518,7 @@  __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (obj->userptr.work != &work->work) {
 		ret = 0;
 	} else if (pinned == num_pages) {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = i915_st_set_pages(&obj->pages, pvec, num_pages);
 		if (ret == 0) {
 			list_add_tail(&obj->global_list, &to_i915(dev)->mm.unbound_list);
 			pinned = 0;
@@ -666,7 +627,7 @@  i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 			}
 		}
 	} else {
-		ret = st_set_pages(&obj->pages, pvec, num_pages);
+		ret = i915_st_set_pages(&obj->pages, pvec, num_pages);
 		if (ret == 0) {
 			obj->userptr.work = NULL;
 			pinned = 0;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 787d5af..7e208d6d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -998,6 +998,10 @@  void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
+
+/* intel_dma.c */
+int i915_st_set_pages(struct sg_table **st, struct page **pvec, int num_pages);
+
 /* intel_dsi.c */
 void intel_dsi_init(struct drm_device *dev);