On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
>
> 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 at 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 f20555e..dc58b01 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6422,13 +6422,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 005a91f..f93653d 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 1c510da..5afc31b 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)
kfree(info->apertures) is missing. The same goes for
intel_fbdev_destroy().
> + goto err_cmap;
> +
> + /* 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);
Should be fine to call w/o checking cmap.len.
> +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 the way pitches[0] is calculated for surface_bpp % 8 != 0,
but there's no mention of it in the commit message.
> + 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)
