From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine Date: Wed, 27 Mar 2013 13:49:06 +0200 Message-ID: <1364384946.11117.3.camel@intelbox> References: <1361309508-4901-1-git-send-email-jbarnes@virtuousgeek.org> <1361309508-4901-4-git-send-email-jbarnes@virtuousgeek.org> <1363782228.13528.17.camel@intelbox> <20130326160727.222a31ed@jbarnes-desktop> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 3C122E5C65 for ; Wed, 27 Mar 2013 04:49:09 -0700 (PDT) In-Reply-To: <20130326160727.222a31ed@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 2013-03-26 at 16:07 -0700, Jesse Barnes wrote: > On Wed, 20 Mar 2013 14:23:48 +0200 > Imre Deak wrote: > > > > + if (!info->screen_base) > > > > kfree(info->apertures) is missing. The same goes for > > intel_fbdev_destroy(). > > Fixed in both places. > > > > > > + 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. > > Fixed in both places. > > > > > > +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. > > It just removes the open coding; we still do the rounding and alignment > to 64 bytes. Yea, but you get different results due to the different way of rounding for certain bpps. For example: sizes->surface_bpp = 4 mode_cmd.width = 1000 You get pitches[0]=1024 with the old way and 512 with the new way. --Imre