[PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[Daniel Vetter]

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);
-- 
1.8.5.2

Re: [PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[Jesse Barnes]

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...

Re: [PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[Ben Widawsky]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/mm: Don't WARN if drm_mm_reserve_node

[Ben Widawsky]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center