Re: [PATCH 4/6] drm/i915: don't memset the fb buffer if preallocated

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 16, 2013 at 04:17:07PM -0800, Jesse Barnes wrote:
> On Sat, 14 Dec 2013 12:28:49 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Dec 12, 2013 at 12:41:55PM -0800, Jesse Barnes wrote:
> > > We want to preserve the BIOS/bootloader contents for later.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index db75f22..53675d2 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	struct drm_framebuffer *fb;
> > >  	struct drm_i915_gem_object *obj;
> > >  	int size, ret;
> > > +	bool prealloc = false;
> > >  
> > >  	mutex_lock(&dev->struct_mutex);
> > >  
> > > @@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  			goto out_unlock;
> > >  	} else {
> > >  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> > > +		prealloc = true;
> > >  		sizes->fb_width = intel_fb->base.width;
> > >  		sizes->fb_height = intel_fb->base.height;
> > >  	}
> > > @@ -203,7 +205,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	 * If the object is stolen however, it will be full of whatever
> > >  	 * garbage was left in there.
> > >  	 */
> > > -	if (ifbdev->fb->obj->stolen)
> > > +	if (ifbdev->fb->obj->stolen && !prealloc)
> > >  		memset_io(info->screen_base, 0, info->screen_size);
> > 
> > Shouldn't we just move this into intelfb_alloc? Now that allocating our
> > own fb isn't the only way any more to get at one this code + comment above
> > looks a bit misplaced.
> 
> We can't since we haven't ioremapped the buffer and such until after
> the alloc or reuse.  It's a little awkward, I agree, but we need more
> refactoring on the fbdev code to make it look nice.

Argh, I've missed that. My idea was to get it more nearby the actual
stolen allocation so that we could move it into the stolen gem bo
allocation itself once we allow userspace such objects. Since then we
definitely need a more generic stolen mem allocator where we can specify
clearing with a flag or so. I just wanted my cake and eat it, too ;-)

So current code looks fine indeed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch