| Message ID | 1372966935-18966-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 04, 2013 at 12:42:15PM -0700, Ben Widawsky wrote: > Setting the node directly is good enough. We don't need a special value > to store the gtt_offset, and we no longer have the pointer to tuck > things away neatly. > > This is temporarily broken since: > commit 7dedae28b41000539b6c18bcf72107c97e4937e4 > Author: Ben Widawsky <ben@bwidawsk.net> > Date: Wed Jul 3 14:45:24 2013 -0700 > > drm/i915: Use gtt_space->start for stolen reservation > > However with no userspace currently, the bisection problem shouldn't be > a concern. > > CC: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - > drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++-- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ea86ad6..b9e8ba1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1201,7 +1201,6 @@ enum hdmi_force_audio { > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > }; > > -#define I915_GTT_RESERVED (ULONG_MAX) > #define I915_GTT_OFFSET_NONE ((u32)-1) > > struct drm_i915_gem_object_ops { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index f075831..c0ca103 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -632,7 +632,6 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", > i915_gem_obj_ggtt_offset(obj), obj->base.size); > > - BUG_ON(obj->gtt_space->start != I915_GTT_RESERVED); I've thought we've agreed on a WARN_ON(!drm_mm_node_allocated); here? -Daniel > obj->gtt_space = drm_mm_create_block(&dev_priv->mm.gtt_space, > i915_gem_obj_ggtt_offset(obj), > obj->base.size, > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index c6b70312..289d511d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -397,8 +397,12 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > drm_gem_object_unreference(&obj->base); > return NULL; > } > - } else > - obj->gtt_space->start = I915_GTT_RESERVED; > + } else { > + /* Generally it isn't safe to directly set attributes of the > + * drm_mm_node, however this is a special case where we'll > + * promise to handle everything properly later. */ > + obj->gtt_space->start = gtt_offset; > + } > > obj->has_global_gtt_mapping = 1; > > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 04, 2013 at 11:24:46PM +0200, Daniel Vetter wrote: > On Thu, Jul 04, 2013 at 12:42:15PM -0700, Ben Widawsky wrote: > > Setting the node directly is good enough. We don't need a special value > > to store the gtt_offset, and we no longer have the pointer to tuck > > things away neatly. > > > > This is temporarily broken since: > > commit 7dedae28b41000539b6c18bcf72107c97e4937e4 > > Author: Ben Widawsky <ben@bwidawsk.net> > > Date: Wed Jul 3 14:45:24 2013 -0700 > > > > drm/i915: Use gtt_space->start for stolen reservation > > > > However with no userspace currently, the bisection problem shouldn't be > > a concern. > > > > CC: Chris Wilson <chris@chris-wilson.co.uk> > > Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - > > drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++-- > > 3 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index ea86ad6..b9e8ba1 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1201,7 +1201,6 @@ enum hdmi_force_audio { > > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > > }; > > > > -#define I915_GTT_RESERVED (ULONG_MAX) > > #define I915_GTT_OFFSET_NONE ((u32)-1) > > > > struct drm_i915_gem_object_ops { > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index f075831..c0ca103 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -632,7 +632,6 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", > > i915_gem_obj_ggtt_offset(obj), obj->base.size); > > > > - BUG_ON(obj->gtt_space->start != I915_GTT_RESERVED); > > I've thought we've agreed on a WARN_ON(!drm_mm_node_allocated); here? WARN_ON(drm_mm_node_allocated); to be precise ;-) -Daniel
On Fri, Jul 05, 2013 at 09:12:05AM +0200, Daniel Vetter wrote: > On Thu, Jul 04, 2013 at 11:24:46PM +0200, Daniel Vetter wrote: > > On Thu, Jul 04, 2013 at 12:42:15PM -0700, Ben Widawsky wrote: > > > Setting the node directly is good enough. We don't need a special value > > > to store the gtt_offset, and we no longer have the pointer to tuck > > > things away neatly. > > > > > > This is temporarily broken since: > > > commit 7dedae28b41000539b6c18bcf72107c97e4937e4 > > > Author: Ben Widawsky <ben@bwidawsk.net> > > > Date: Wed Jul 3 14:45:24 2013 -0700 > > > > > > drm/i915: Use gtt_space->start for stolen reservation > > > > > > However with no userspace currently, the bisection problem shouldn't be > > > a concern. > > > > > > CC: Chris Wilson <chris@chris-wilson.co.uk> > > > Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - > > > drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++-- > > > 3 files changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index ea86ad6..b9e8ba1 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1201,7 +1201,6 @@ enum hdmi_force_audio { > > > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > > > }; > > > > > > -#define I915_GTT_RESERVED (ULONG_MAX) > > > #define I915_GTT_OFFSET_NONE ((u32)-1) > > > > > > struct drm_i915_gem_object_ops { > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index f075831..c0ca103 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -632,7 +632,6 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > > > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", > > > i915_gem_obj_ggtt_offset(obj), obj->base.size); > > > > > > - BUG_ON(obj->gtt_space->start != I915_GTT_RESERVED); > > > > I've thought we've agreed on a WARN_ON(!drm_mm_node_allocated); here? > > WARN_ON(drm_mm_node_allocated); to be precise ;-) > -Daniel To be honest, I forgot - but the original plan was to do it after the node embedding. I think for the same paranoia, we also want to check that obj->gtt_space != NULL. Would you mind adding it on merge?
On Thu, Jul 04, 2013 at 12:42:15PM -0700, Ben Widawsky wrote: > Setting the node directly is good enough. We don't need a special value > to store the gtt_offset, and we no longer have the pointer to tuck > things away neatly. > > This is temporarily broken since: > commit 7dedae28b41000539b6c18bcf72107c97e4937e4 > Author: Ben Widawsky <ben@bwidawsk.net> > Date: Wed Jul 3 14:45:24 2013 -0700 > > drm/i915: Use gtt_space->start for stolen reservation > > However with no userspace currently, the bisection problem shouldn't be > a concern. > > CC: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - > drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++-- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ea86ad6..b9e8ba1 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1201,7 +1201,6 @@ enum hdmi_force_audio { > HDMI_AUDIO_ON, /* force turn on HDMI audio */ > }; > > -#define I915_GTT_RESERVED (ULONG_MAX) > #define I915_GTT_OFFSET_NONE ((u32)-1) > > struct drm_i915_gem_object_ops { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index f075831..c0ca103 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -632,7 +632,6 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", > i915_gem_obj_ggtt_offset(obj), obj->base.size); > > - BUG_ON(obj->gtt_space->start != I915_GTT_RESERVED); > obj->gtt_space = drm_mm_create_block(&dev_priv->mm.gtt_space, > i915_gem_obj_ggtt_offset(obj), > obj->base.size, > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index c6b70312..289d511d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -397,8 +397,12 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, > drm_gem_object_unreference(&obj->base); > return NULL; > } > - } else > - obj->gtt_space->start = I915_GTT_RESERVED; > + } else { > + /* Generally it isn't safe to directly set attributes of the > + * drm_mm_node, however this is a special case where we'll > + * promise to handle everything properly later. */ > + obj->gtt_space->start = gtt_offset; Since obj->gtt_space isn't embedded yet this will blow up. I think we need to do the embedding first, then fix this up. -Daniel > + } > > obj->has_global_gtt_mapping = 1; > > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ea86ad6..b9e8ba1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1201,7 +1201,6 @@ enum hdmi_force_audio { HDMI_AUDIO_ON, /* force turn on HDMI audio */ }; -#define I915_GTT_RESERVED (ULONG_MAX) #define I915_GTT_OFFSET_NONE ((u32)-1) struct drm_i915_gem_object_ops { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f075831..c0ca103 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -632,7 +632,6 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", i915_gem_obj_ggtt_offset(obj), obj->base.size); - BUG_ON(obj->gtt_space->start != I915_GTT_RESERVED); obj->gtt_space = drm_mm_create_block(&dev_priv->mm.gtt_space, i915_gem_obj_ggtt_offset(obj), obj->base.size, diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index c6b70312..289d511d 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -397,8 +397,12 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, drm_gem_object_unreference(&obj->base); return NULL; } - } else - obj->gtt_space->start = I915_GTT_RESERVED; + } else { + /* Generally it isn't safe to directly set attributes of the + * drm_mm_node, however this is a special case where we'll + * promise to handle everything properly later. */ + obj->gtt_space->start = gtt_offset; + } obj->has_global_gtt_mapping = 1;
Setting the node directly is good enough. We don't need a special value to store the gtt_offset, and we no longer have the pointer to tuck things away neatly. This is temporarily broken since: commit 7dedae28b41000539b6c18bcf72107c97e4937e4 Author: Ben Widawsky <ben@bwidawsk.net> Date: Wed Jul 3 14:45:24 2013 -0700 drm/i915: Use gtt_space->start for stolen reservation However with no userspace currently, the bisection problem shouldn't be a concern. CC: Chris Wilson <chris@chris-wilson.co.uk> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_gem_gtt.c | 1 - drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++-- 3 files changed, 6 insertions(+), 4 deletions(-)