Re: [PATCH 3/8] drm/i915: Split the framebuffer_info creation into a separate routine

[Imre Deak <imre.deak@intel.com>]

On Wed, 2013-02-06 at 11:10 +0000, Chris Wilson wrote:
> This will be shared with wrapping the BIOS framebuffer into the fbdev
> later. In the meantime, we can tidy the code slightly and improve the
> error path handling.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 --
>  drivers/gpu/drm/i915/intel_drv.h     |    7 ++
>  drivers/gpu/drm/i915/intel_fb.c      |  154 ++++++++++++++++++----------------
>  3 files changed, 91 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f1dbd01..8f9cdd7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6413,13 +6413,6 @@ intel_framebuffer_create(struct drm_device *dev,
>  }
>  
>  static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
>  intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
>  {
>  	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 13afb37..07d95a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -134,6 +134,13 @@ struct intel_framebuffer {
>  	struct drm_i915_gem_object *obj;
>  };
>  
> +inline static u32
> +intel_framebuffer_pitch_for_width(int width, int bpp)
> +{
> +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> +	return ALIGN(pitch, 64);
> +}
> +
>  struct intel_fbdev {
>  	struct drm_fb_helper helper;
>  	struct intel_framebuffer ifb;
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 6591029..de0ac4c 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -57,29 +57,96 @@ static struct fb_ops intelfb_ops = {
>  	.fb_debug_leave = drm_fb_helper_debug_leave,
>  };
>  
> +static struct fb_info *intelfb_create_info(struct intel_fbdev *ifbdev)
> +{
> +	struct drm_framebuffer *fb = &ifbdev->ifb.base;
> +	struct drm_device *dev = fb->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct fb_info *info;
> +	u32 gtt_offset, size;
> +	int ret;
> +
> +	info = framebuffer_alloc(0, &dev->pdev->dev);
> +	if (!info)
> +		return NULL;
> +
> +	info->par = ifbdev;
> +	ifbdev->helper.fb = fb;
> +	ifbdev->helper.fbdev = info;
> +
> +	strcpy(info->fix.id, "inteldrmfb");
> +
> +	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> +	info->fbops = &intelfb_ops;
> +
> +	ret = fb_alloc_cmap(&info->cmap, 256, 0);
> +	if (ret)
> +		goto err_info;
> +
> +	/* setup aperture base/size for vesafb takeover */
> +	info->apertures = alloc_apertures(1);
> +	if (!info->apertures)
> +		goto err_cmap;
> +
> +	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> +	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
> +
> +	gtt_offset = ifbdev->ifb.obj->gtt_offset;
> +	size = ifbdev->ifb.obj->base.size;
> +
> +	info->fix.smem_start = dev->mode_config.fb_base + gtt_offset;
> +	info->fix.smem_len = size;
> +
> +	info->screen_size = size;
> +	info->screen_base = ioremap_wc(dev_priv->gtt.mappable_base + gtt_offset,
> +				       size);
> +	if (!info->screen_base)
> +		goto err_cmap;

kfree(info->apertures) is missing. Same in intel_fbdev_destroy().

> +
> +	/* If the object is shmemfs backed, it will have given us zeroed pages.
> +	 * If the object is stolen however, it will be full of whatever
> +	 * garbage was left in there.
> +	 */
> +	if (ifbdev->ifb.obj->stolen)
> +		memset_io(info->screen_base, 0, info->screen_size);
> +
> +	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> +
> +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> +	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
> +
> +	return info;
> +
> +err_cmap:
> +	if (info->cmap.len)
> +		fb_dealloc_cmap(&info->cmap);

fb_dealloc_cmap() could be called unconditionally.

> +err_info:
> +	framebuffer_release(info);
> +	return NULL;
> +}
> +
>  static int intelfb_create(struct intel_fbdev *ifbdev,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
>  	struct drm_device *dev = ifbdev->helper.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct fb_info *info;
> -	struct drm_framebuffer *fb;
> -	struct drm_mode_fb_cmd2 mode_cmd = {};
> +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  	struct drm_i915_gem_object *obj;
> -	struct device *device = &dev->pdev->dev;
> +	struct fb_info *info;
>  	int size, ret;
>  
>  	/* we don't do packed 24bpp */
>  	if (sizes->surface_bpp == 24)
>  		sizes->surface_bpp = 32;
>  
> -	mode_cmd.width = sizes->surface_width;
> +	mode_cmd.width  = sizes->surface_width;
>  	mode_cmd.height = sizes->surface_height;
>  
> -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
> -						      8), 64);
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> +	mode_cmd.pitches[0] =
> +		intel_framebuffer_pitch_for_width(mode_cmd.width,
> +						  sizes->surface_bpp);

This changes pitches[0] for non 8-bit aligned bpps. If it's a fix a
comment about it in the commit message would be nice.

--Imre

> +	mode_cmd.pixel_format =
> +		drm_mode_legacy_fb_format(sizes->surface_bpp,
> +					  sizes->surface_depth);
>  
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = ALIGN(size, PAGE_SIZE);
> @@ -101,72 +168,19 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>  		goto out_unref;
>  	}
>  
> -	info = framebuffer_alloc(0, device);
> -	if (!info) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -
> -	info->par = ifbdev;
> -
>  	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
>  	if (ret)
>  		goto out_unpin;
>  
> -	fb = &ifbdev->ifb.base;
> -
> -	ifbdev->helper.fb = fb;
> -	ifbdev->helper.fbdev = info;
> -
> -	strcpy(info->fix.id, "inteldrmfb");
> -
> -	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> -	info->fbops = &intelfb_ops;
> +	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
> +		      mode_cmd.width, mode_cmd.height,
> +		      obj->gtt_offset, obj);
>  
> -	ret = fb_alloc_cmap(&info->cmap, 256, 0);
> -	if (ret) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -	/* setup aperture base/size for vesafb takeover */
> -	info->apertures = alloc_apertures(1);
> -	if (!info->apertures) {
> +	info = intelfb_create_info(ifbdev);
> +	if (info == NULL) {
>  		ret = -ENOMEM;
>  		goto out_unpin;
>  	}
> -	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> -	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
> -
> -	info->fix.smem_start = dev->mode_config.fb_base + obj->gtt_offset;
> -	info->fix.smem_len = size;
> -
> -	info->screen_base =
> -		ioremap_wc(dev_priv->gtt.mappable_base + obj->gtt_offset,
> -			   size);
> -	if (!info->screen_base) {
> -		ret = -ENOSPC;
> -		goto out_unpin;
> -	}
> -	info->screen_size = size;
> -
> -//	memset(info->screen_base, 0, size);
> -
> -	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> -	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> -
> -	/* If the object is shmemfs backed, it will have given us zeroed pages.
> -	 * If the object is stolen however, it will be full of whatever
> -	 * garbage was left in there.
> -	 */
> -	if (ifbdev->ifb.obj->stolen)
> -		memset_io(info->screen_base, 0, info->screen_size);
> -
> -	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> -
> -	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
> -		      fb->width, fb->height,
> -		      obj->gtt_offset, obj);
> -
>  
>  	mutex_unlock(&dev->struct_mutex);
>  	vga_switcheroo_client_fb_set(dev->pdev, info);
> @@ -206,11 +220,11 @@ static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> -	struct fb_info *info;
>  	struct intel_framebuffer *ifb = &ifbdev->ifb;
>  
>  	if (ifbdev->helper.fbdev) {
> -		info = ifbdev->helper.fbdev;
> +		struct fb_info *info = ifbdev->helper.fbdev;
> +
>  		unregister_framebuffer(info);
>  		iounmap(info->screen_base);
>  		if (info->cmap.len)