On Wed, Mar 27, 2013 at 01:49:06PM +0200, Imre Deak wrote:
> On Tue, 2013-03-26 at 16:07 -0700, Jesse Barnes wrote:
> > On Wed, 20 Mar 2013 14:23:48 +0200
> > Imre Deak <imre.d...@intel.com> 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.

Not a bug in Jesse's patch though, but an earlier one of mine trying to
use the kernel macros and failing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to