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

[Imre Deak]

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 at intel.com>
> > > 
> > > It will be used by other call sites shortly.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at 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 at 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.