Re: [PATCH 4/6] drm/i915: alloc intel_fb in the intel_fbdev struct

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 07, 2014 at 12:10:38PM -0800, Jesse Barnes wrote:
> Allocate this struct instead, so we can re-use another allocated
> elsewhere if needed.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c   |   27 +++++++++++++++++++--------
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 450bb40..112da42 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7975,11 +7975,11 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  	if (dev_priv->fbdev == NULL)
>  		return NULL;
>  
> -	obj = dev_priv->fbdev->ifb.obj;
> +	obj = dev_priv->fbdev->fb->obj;
>  	if (obj == NULL)
>  		return NULL;
>  
> -	fb = &dev_priv->fbdev->ifb.base;
> +	fb = &dev_priv->fbdev->fb->base;
>  	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
>  							       fb->bits_per_pixel))
>  		return NULL;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1e5a0a6..8f5e798 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -110,7 +110,7 @@ struct intel_framebuffer {
>  
>  struct intel_fbdev {
>  	struct drm_fb_helper helper;
> -	struct intel_framebuffer ifb;
> +	struct intel_framebuffer *fb;
>  	struct list_head fbdev_list;
>  	struct drm_display_mode *our_mode;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index d6a8a71..fb07ba6 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -62,11 +62,20 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> +	struct intel_framebuffer *fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
>  	struct drm_i915_gem_object *obj;
>  	int size, ret;
>  
> +	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> +	if (!fb) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ifbdev->fb = fb;
> +
>  	/* we don't do packed 24bpp */
>  	if (sizes->surface_bpp == 24)
>  		sizes->surface_bpp = 32;
> @@ -97,7 +106,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  		goto out_unref;
>  	}
>  
> -	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
> +	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
>  	if (ret)
>  		goto out_unpin;
>  
> @@ -116,7 +125,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> -	struct intel_framebuffer *intel_fb = &ifbdev->ifb;
> +	struct intel_framebuffer *intel_fb = ifbdev->fb;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct fb_info *info;
> @@ -126,11 +135,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	if (!intel_fb->obj) {
> +	if (!intel_fb || !intel_fb->obj) {

The ->obj check is redundant since an fb without backing storage is a bug.
I've wrapped it up into a WARN. Note that the original ->obj check was
just a proxy for checking whether we have an fb, which isn't possible any
other way.
-Daniel

>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
>  		if (ret)
>  			goto out_unlock;
> +		intel_fb = ifbdev->fb;
>  	} else {
>  		DRM_DEBUG_KMS("re-using BIOS fb\n");
>  		sizes->fb_width = intel_fb->base.width;
> @@ -148,7 +158,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	info->par = helper;
>  
> -	fb = &ifbdev->ifb.base;
> +	fb = &ifbdev->fb->base;
>  
>  	ifbdev->helper.fb = fb;
>  	ifbdev->helper.fbdev = info;
> @@ -194,7 +204,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->ifb.obj->stolen)
> +	if (ifbdev->fb->obj->stolen)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> @@ -258,8 +268,9 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  
>  	drm_fb_helper_fini(&ifbdev->helper);
>  
> -	drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> -	intel_framebuffer_fini(&ifbdev->ifb);
> +	drm_framebuffer_unregister_private(&ifbdev->fb->base);
> +	intel_framebuffer_fini(ifbdev->fb);
> +	kfree(ifbdev->fb);
>  }
>  
>  int intel_fbdev_init(struct drm_device *dev)
> @@ -322,7 +333,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
>  	 * been restored from swap. If the object is stolen however, it will be
>  	 * full of whatever garbage was left in there.
>  	 */
> -	if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> +	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	fb_set_suspend(info, state);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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