| Message ID | 1396905920-30049-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 7 Apr 2014 23:25:20 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Jesse's BIOS fb reconstruction code actually relies on the -ENOSPC > return value to detect overlapping framebuffers (which the bios uses > always when lighting up more than one screen). All this fanciness > happens in intel_alloc_plane_obj in intel_display.c. > > Since no one else uses this we can savely remove the WARN without > repercursions. > > Reported-by: Ben Widawsky <benjamin.widawsky@intel.com> > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Dave Airlie <airlied@redhat.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_mm.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > index a2d45b748f86..e4dfd5c3b15e 100644 > --- a/drivers/gpu/drm/drm_mm.c > +++ b/drivers/gpu/drm/drm_mm.c > @@ -192,8 +192,6 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node) > return 0; > } > > - WARN(1, "no hole found for node 0x%lx + 0x%lx\n", > - node->start, node->size); > return -ENOSPC; > } > EXPORT_SYMBOL(drm_mm_reserve_node); Yeah thanks, pushing this has been on my list for weeks now...
On Mon, Apr 07, 2014 at 10:13:13PM -0700, Jesse Barnes wrote: > On Mon, 7 Apr 2014 23:25:20 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > Jesse's BIOS fb reconstruction code actually relies on the -ENOSPC > > return value to detect overlapping framebuffers (which the bios uses > > always when lighting up more than one screen). All this fanciness > > happens in intel_alloc_plane_obj in intel_display.c. > > > > Since no one else uses this we can savely remove the WARN without > > repercursions. > > > > Reported-by: Ben Widawsky <benjamin.widawsky@intel.com> > > Cc: Ben Widawsky <benjamin.widawsky@intel.com> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > Cc: Dave Airlie <airlied@redhat.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/drm_mm.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > > index a2d45b748f86..e4dfd5c3b15e 100644 > > --- a/drivers/gpu/drm/drm_mm.c > > +++ b/drivers/gpu/drm/drm_mm.c > > @@ -192,8 +192,6 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node) > > return 0; > > } > > > > - WARN(1, "no hole found for node 0x%lx + 0x%lx\n", > > - node->start, node->size); > > return -ENOSPC; > > } > > EXPORT_SYMBOL(drm_mm_reserve_node); > > Yeah thanks, pushing this has been on my list for weeks now... I am not convinced this is the correct solution. At least the way we used this interface, it isn't meant to ever fail. I also didn't look into exactly why we depend an ENOSPC return. That sounds fragile to me, especially for a public interface. Obviously it makes the WARN go away, and we have only one other user of the interface, so it's correct. So if both of you are happy, I won't stand in the way.
On Tue, Apr 08, 2014 at 10:21:44AM -0700, Ben Widawsky wrote: > I am not convinced this is the correct solution. At least the way we > used this interface, it isn't meant to ever fail. I also didn't look > into exactly why we depend an ENOSPC return. That sounds fragile to me, > especially for a public interface. Eh? This interface is explicitly used to check that the requested range is available. -Chris
On Wed, Apr 09, 2014 at 07:25:37AM +0100, Chris Wilson wrote: > On Tue, Apr 08, 2014 at 10:21:44AM -0700, Ben Widawsky wrote: > > I am not convinced this is the correct solution. At least the way we > > used this interface, it isn't meant to ever fail. I also didn't look > > into exactly why we depend an ENOSPC return. That sounds fragile to me, > > especially for a public interface. > > Eh? This interface is explicitly used to check that the requested range > is available. > -Chris > What I mean is, the node is already initialized, and we always expect it to be available - at least with all the callers prior to the fastboot. I didn't look very closely at how we get the fb objects from the existing stolen memory, but my drive-by review would suggest it's much better to deal with the redundancy at that level (or make this an i915 private function). Removing the WARN is fine with me though, it's: Tested-by: Ben Widawsky <ben@bwidawsk.net> My complaint was more with how we solved the problem initially, and not with this patch itself.
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index a2d45b748f86..e4dfd5c3b15e 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -192,8 +192,6 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node) return 0; } - WARN(1, "no hole found for node 0x%lx + 0x%lx\n", - node->start, node->size); return -ENOSPC; } EXPORT_SYMBOL(drm_mm_reserve_node);
Jesse's BIOS fb reconstruction code actually relies on the -ENOSPC return value to detect overlapping framebuffers (which the bios uses always when lighting up more than one screen). All this fanciness happens in intel_alloc_plane_obj in intel_display.c. Since no one else uses this we can savely remove the WARN without repercursions. Reported-by: Ben Widawsky <benjamin.widawsky@intel.com> Cc: Ben Widawsky <benjamin.widawsky@intel.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Dave Airlie <airlied@redhat.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_mm.c | 2 -- 1 file changed, 2 deletions(-)