Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu: > The conversion of stolen to use phys_addr_t (from essentially u32) > sparked an interesting discussion. We treat stolen memory as only > accessible from the GPU (the DMA device) - an attempt to use it from > the > CPU will generate a MCE on gen6 onwards, although it is in theory a > physical address that can be dereferenced from the CPU as > demonstrated > by earlier generations. As such, using phys_addr_t has the wrong > connotations and as we pass the address into the DMA device via > dma_addr_t (through the scatterlists used to program the GTT > entries), > we should treat it as dma_addr_t throughout. I'm not a specialist here, but from what I could learn/understand, this seems good. The patch seems to do what it says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> But now that I looked some more at the code, I started to think about the following: shouldn't we convert our "stolen offset" variables from u32 to dma_addr_t or some other appropriate type? I mean, if we ever get 64 bit stolen pointers we may also get 64 bit stolen offsets... The i915_ggtt struct contains 4 of such u32 pointers. For example, i915_ggtt->stolen_reserved_base is another pointer to stolen memory, it's generated directly from one of our drm_addr_t variables. Of course, this could be a separate patch. > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/i915_gem_stolen.c | 36 +++++++++++++++++------- > ---------- > 2 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index d0a48cceb15b..a43ba7872d7c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1467,7 +1467,7 @@ struct i915_gem_mm { > struct work_struct free_work; > > /** Usable portion of the GTT for GEM */ > - phys_addr_t stolen_base; /* limited to low memory (32-bit) > */ > + dma_addr_t stolen_base; /* limited to low memory (32-bit) */ > > /** PPGTT used for aliasing the PPGTT with the GTT */ > struct i915_hw_ppgtt *aliasing_ppgtt; > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 7226e4a21097..42bbc4b04fd6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -79,12 +79,12 @@ void i915_gem_stolen_remove_node(struct > drm_i915_private *dev_priv, > mutex_unlock(&dev_priv->mm.stolen_lock); > } > > -static phys_addr_t i915_stolen_to_physical(struct drm_i915_private > *dev_priv) > +static dma_addr_t i915_stolen_to_dma(struct drm_i915_private > *dev_priv) > { > struct pci_dev *pdev = dev_priv->drm.pdev; > struct i915_ggtt *ggtt = &dev_priv->ggtt; > struct resource *r; > - phys_addr_t base; > + dma_addr_t base; > > /* Almost universally we can find the Graphics Base of > Stolen Memory > * at register BSM (0x5c) in the igfx configuration space. > On a few > @@ -196,7 +196,7 @@ static phys_addr_t i915_stolen_to_physical(struct > drm_i915_private *dev_priv) > if (INTEL_GEN(dev_priv) <= 4 && > !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) && > !IS_G4X(dev_priv)) { > struct { > - phys_addr_t start, end; > + dma_addr_t start, end; > } stolen[2] = { > { .start = base, .end = base + ggtt- > >stolen_size, }, > { .start = base, .end = base + ggtt- > >stolen_size, }, > @@ -228,12 +228,12 @@ static phys_addr_t > i915_stolen_to_physical(struct drm_i915_private *dev_priv) > > if (stolen[0].start != stolen[1].start || > stolen[0].end != stolen[1].end) { > - phys_addr_t end = base + ggtt->stolen_size - > 1; > + dma_addr_t end = base + ggtt->stolen_size - > 1; > > DRM_DEBUG_KMS("GTT within stolen memory at > 0x%llx-0x%llx\n", > (unsigned long > long)ggtt_start, > (unsigned long long)ggtt_end - > 1); > - DRM_DEBUG_KMS("Stolen memory adjusted to %pa > - %pa\n", > + DRM_DEBUG_KMS("Stolen memory adjusted to > %pad - %pad\n", > &base, &end); > } > } > @@ -263,9 +263,9 @@ static phys_addr_t i915_stolen_to_physical(struct > drm_i915_private *dev_priv) > * range. Apparently this works. > */ > if (r == NULL && !IS_GEN3(dev_priv)) { > - phys_addr_t end = base + ggtt->stolen_size; > + dma_addr_t end = base + ggtt->stolen_size; > > - DRM_ERROR("conflict detected with stolen > region: [%pa - %pa]\n", > + DRM_ERROR("conflict detected with stolen > region: [%pad - %pad]\n", > &base, &end); > base = 0; > } > @@ -285,13 +285,13 @@ void i915_gem_cleanup_stolen(struct drm_device > *dev) > } > > static void g4x_get_stolen_reserved(struct drm_i915_private > *dev_priv, > - phys_addr_t *base, u32 *size) > + dma_addr_t *base, u32 *size) > { > struct i915_ggtt *ggtt = &dev_priv->ggtt; > uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ? > CTG_STOLEN_RESERVED : > ELK_STOLEN_RESERVED); > - phys_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt- > >stolen_size; > + dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt- > >stolen_size; > > *base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16; > > @@ -308,7 +308,7 @@ static void g4x_get_stolen_reserved(struct > drm_i915_private *dev_priv, > } > > static void gen6_get_stolen_reserved(struct drm_i915_private > *dev_priv, > - phys_addr_t *base, u32 *size) > + dma_addr_t *base, u32 *size) > { > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); > > @@ -334,7 +334,7 @@ static void gen6_get_stolen_reserved(struct > drm_i915_private *dev_priv, > } > > static void gen7_get_stolen_reserved(struct drm_i915_private > *dev_priv, > - phys_addr_t *base, u32 *size) > + dma_addr_t *base, u32 *size) > { > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); > > @@ -354,7 +354,7 @@ static void gen7_get_stolen_reserved(struct > drm_i915_private *dev_priv, > } > > static void chv_get_stolen_reserved(struct drm_i915_private > *dev_priv, > - phys_addr_t *base, u32 *size) > + dma_addr_t *base, u32 *size) > { > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); > > @@ -380,11 +380,11 @@ static void chv_get_stolen_reserved(struct > drm_i915_private *dev_priv, > } > > static void bdw_get_stolen_reserved(struct drm_i915_private > *dev_priv, > - phys_addr_t *base, u32 *size) > + dma_addr_t *base, u32 *size) > { > struct i915_ggtt *ggtt = &dev_priv->ggtt; > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); > - phys_addr_t stolen_top; > + dma_addr_t stolen_top; > > stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size; > > @@ -403,7 +403,7 @@ static void bdw_get_stolen_reserved(struct > drm_i915_private *dev_priv, > int i915_gem_init_stolen(struct drm_i915_private *dev_priv) > { > struct i915_ggtt *ggtt = &dev_priv->ggtt; > - phys_addr_t reserved_base, stolen_top; > + dma_addr_t reserved_base, stolen_top; > u32 reserved_total, reserved_size; > u32 stolen_usable_start; > > @@ -424,7 +424,7 @@ int i915_gem_init_stolen(struct drm_i915_private > *dev_priv) > if (ggtt->stolen_size == 0) > return 0; > > - dev_priv->mm.stolen_base = > i915_stolen_to_physical(dev_priv); > + dev_priv->mm.stolen_base = i915_stolen_to_dma(dev_priv); > if (dev_priv->mm.stolen_base == 0) > return 0; > > @@ -473,8 +473,8 @@ int i915_gem_init_stolen(struct drm_i915_private > *dev_priv) > > if (reserved_base < dev_priv->mm.stolen_base || > reserved_base + reserved_size > stolen_top) { > - phys_addr_t reserved_top = reserved_base + > reserved_size; > - DRM_DEBUG_KMS("Stolen reserved area [%pa - %pa] > outside stolen memory [%pa - %pa]\n", > + dma_addr_t reserved_top = reserved_base + > reserved_size; > + DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad] > outside stolen memory [%pad - %pad]\n", > &reserved_base, &reserved_top, > &dev_priv->mm.stolen_base, > &stolen_top); > return 0; _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx