Re: [PATCH 05/10] drm/i915: make dsm struct resource centric

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 06, 2017 at 06:30:33PM +0000, Chris Wilson wrote:
> Quoting Matthew Auld (2017-12-06 18:17:25)
> > Now that we are using struct resource to track the stolen region, it is
> > more convenient if we track dsm in a resource as well.
> > 
> > v2: check range_overflow when writing to 32b registers (Chris)
> >     pepper in some comments (Chris)
> > v3: refit i915_stolen_to_dma()
> > v4: kill ggtt->stolen_size
> > 
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > -static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
> > +static int i915_adjust_stolen(struct drm_i915_private *dev_priv,
> > +                             struct resource *dsm)
> >  {
> >         struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > -       dma_addr_t base = intel_graphics_stolen_res.start;
> >         struct resource *r;
> >  
> > -       GEM_BUG_ON(overflows_type(intel_graphics_stolen_res.start, base));
> > +       if (dsm->start == 0 || add_overflows(dsm->start, resource_size(dsm)))
> 
> Now s/add_overflows/dsm->end <= dsm->start/
> 
> > +               return -EINVAL;
> >  
> > -       if (base == 0 || add_overflows(base, ggtt->stolen_size))
> > -               return 0;
> > -
> > -       /* make sure we don't clobber the GTT if it's within stolen memory */
> > +       /* Make sure we don't clobber the GTT if it's within stolen memory */
> >         if (INTEL_GEN(dev_priv) <= 4 &&
> >             !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) && !IS_G4X(dev_priv)) {
> > -               struct {
> > -                       dma_addr_t start, end;
> > -               } stolen[2] = {
> > -                       { .start = base, .end = base + ggtt->stolen_size, },
> > -                       { .start = base, .end = base + ggtt->stolen_size, },
> > +               struct resource stolen[2] = {
> > +                       DEFINE_RES_MEM(dsm->start, resource_size(dsm)),
> > +                       DEFINE_RES_MEM(dsm->start, resource_size(dsm)),
> 
> struct resource stolen[2] = { *dsm, *dsm } ?

BTW I don't think I've ever seen a case where the GTT wasn't at the end
of stolen. So we could simplify this code by making that assumption.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx