| Message ID | 1386880917-2951-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > Retrieve current framebuffer config info from the regs and create an fb > object for the buffer the BIOS or boot loader left us. This should > allow for smooth transitions to userspace apps once we finish the > initial configuration construction. > > v2: check for non-native modes and adjust (Jesse) > fixup aperture and cmap frees (Imre) > use unlocked unref if init_bios fails (Jesse) > fix curly brace around DSPADDR check (Imre) > comment failure path for pin_and_fence (Imre) > v3: fixup fixup of aperture frees (Chris) > v4: update to current bits (locking & pin_and_fence hack) (Jesse) > v5: move fb config fetch to display code (Jesse) > re-order hw state readout on initial load to suit fb inherit (Jesse) > re-add pin_and_fence in fbdev code to make sure we refcount properly (Je > v6: rename to plane_config (Daniel) > check for valid object when initializing BIOS fb (Jesse) > split from plane_config readout and other display changes (Jesse) > drop use_bios_fb option (Chris) > update comments (Jesse) > rework fbdev_init_bios for clarity (Jesse) > drop fb obj ref under lock (Chris) > v7: use fb object from plane_config instead (Ville) > take ref on fb object (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> [snip] > @@ -258,8 +357,102 @@ 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); No need to go the private fb route here anymore since now the fb is free-standing. Normal refcounting should work. But a separate prep/cleanup patch (prep since switching ifbdev->fb from struct to point would look neat as a separate patch). > +} > + > +/* > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible. > + * The core display code will have read out the current plane configuration, > + * so we use that to figure out if there's an object for us to use as the > + * fb, and if so, we re-use it for the fbdev configuration. > + * > + * Note we only support a single fb shared across pipes for boot (mostly for > + * fbcon), so we just find the biggest and use that. > + */ > +void intel_fbdev_init_bios(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_fbdev *ifbdev; > + struct intel_framebuffer *fb = NULL; > + struct drm_crtc *crtc; > + struct intel_crtc *intel_crtc; > + struct intel_plane_config *plane_config = NULL; > + unsigned int last_size = 0; > + > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > + if (ifbdev == NULL) { > + DRM_DEBUG_KMS("failed to alloc intel fbdev\n"); > + return; > + } > + > + /* Find the largest framebuffer to use, then free the others */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc = to_intel_crtc(crtc); > + > + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) { > + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n", > + pipe_name(intel_crtc->pipe)); > + continue; > + } > + > + if (intel_crtc->plane_config.size > last_size) { > + plane_config = &intel_crtc->plane_config; > + last_size = plane_config->size; > + fb = plane_config->fb; > + } > + } > + > + /* Free unused fbs */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + struct intel_framebuffer *cur_fb; > + > + intel_crtc = to_intel_crtc(crtc); > + cur_fb = intel_crtc->plane_config.fb; > + > + if (cur_fb && cur_fb != fb) > + intel_framebuffer_fini(cur_fb); > + } > + > + if (!fb) { > + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n"); > + goto out_free; > + } > + > + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel; > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; > + ifbdev->fb = fb; > + > + /* Assuming a single fb across all pipes here */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc = to_intel_crtc(crtc); > + > + if (!intel_crtc->active) > + continue; > + > + /* > + * This should only fail on the first one so we don't need > + * to cleanup any secondary crtc->fbs > + */ > + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) > + goto out_unref_obj; > + > + crtc->fb = &fb->base; > + drm_gem_object_reference(&fb->obj->base); > + drm_framebuffer_reference(&fb->base); > + } > + > + dev_priv->fbdev = ifbdev; > + > + DRM_DEBUG_KMS("using BIOS fb for initial console\n"); > + return; > + > +out_unref_obj: > + intel_framebuffer_fini(fb); > +out_free: > + kfree(ifbdev); > } > > int intel_fbdev_init(struct drm_device *dev) > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL); > - if (!ifbdev) > - return -ENOMEM; > + /* This may fail, if so, dev_priv->fbdev won't be set below */ If you need a comment to explain your control flow, it's probably too clever ;-) > + intel_fbdev_init_bios(dev); > > - dev_priv->fbdev = ifbdev; > - ifbdev->helper.funcs = &intel_fb_helper_funcs; > + if ((ifbdev = dev_priv->fbdev) == NULL) { > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > + if (ifbdev == NULL) > + return -ENOMEM; > + > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > + ifbdev->preferred_bpp = 32; > + > + dev_priv->fbdev = ifbdev; > + } > > ret = drm_fb_helper_init(dev, &ifbdev->helper, > INTEL_INFO(dev)->num_pipes, > 4); > if (ret) { > + dev_priv->fbdev = NULL; > kfree(ifbdev); > return ret; > } > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev) > void intel_fbdev_initial_config(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_fbdev *ifbdev = dev_priv->fbdev; > > /* Due to peculiar init order wrt to hpd handling this is separate. */ > - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32); > + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); > } > > void intel_fbdev_fini(struct drm_device *dev) > @@ -322,7 +524,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); > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights"); > void intel_fbdev_output_poll_changed(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > + if (dev_priv->fbdev) > + drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); Ok, here's the real reason I've actually replied to this patch. This looks like a separate bugfix. And there's not mention of it in the commit message. Please split it out and give it the nice commit message explanation it deserves (whatever it's doing here). Cheers, Daniel > } > > void intel_fbdev_restore_mode(struct drm_device *dev) > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 12 Dec 2013 23:54:37 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > > Retrieve current framebuffer config info from the regs and create an fb > > object for the buffer the BIOS or boot loader left us. This should > > allow for smooth transitions to userspace apps once we finish the > > initial configuration construction. > > > > v2: check for non-native modes and adjust (Jesse) > > fixup aperture and cmap frees (Imre) > > use unlocked unref if init_bios fails (Jesse) > > fix curly brace around DSPADDR check (Imre) > > comment failure path for pin_and_fence (Imre) > > v3: fixup fixup of aperture frees (Chris) > > v4: update to current bits (locking & pin_and_fence hack) (Jesse) > > v5: move fb config fetch to display code (Jesse) > > re-order hw state readout on initial load to suit fb inherit (Jesse) > > re-add pin_and_fence in fbdev code to make sure we refcount properly (Je > > v6: rename to plane_config (Daniel) > > check for valid object when initializing BIOS fb (Jesse) > > split from plane_config readout and other display changes (Jesse) > > drop use_bios_fb option (Chris) > > update comments (Jesse) > > rework fbdev_init_bios for clarity (Jesse) > > drop fb obj ref under lock (Chris) > > v7: use fb object from plane_config instead (Ville) > > take ref on fb object (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > [snip] > > > @@ -258,8 +357,102 @@ 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); > > No need to go the private fb route here anymore since now the fb is > free-standing. Normal refcounting should work. But a separate prep/cleanup > patch (prep since switching ifbdev->fb from struct to point would look > neat as a separate patch). > > > +} > > + > > +/* > > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible. > > + * The core display code will have read out the current plane configuration, > > + * so we use that to figure out if there's an object for us to use as the > > + * fb, and if so, we re-use it for the fbdev configuration. > > + * > > + * Note we only support a single fb shared across pipes for boot (mostly for > > + * fbcon), so we just find the biggest and use that. > > + */ > > +void intel_fbdev_init_bios(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_fbdev *ifbdev; > > + struct intel_framebuffer *fb = NULL; > > + struct drm_crtc *crtc; > > + struct intel_crtc *intel_crtc; > > + struct intel_plane_config *plane_config = NULL; > > + unsigned int last_size = 0; > > + > > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > > + if (ifbdev == NULL) { > > + DRM_DEBUG_KMS("failed to alloc intel fbdev\n"); > > + return; > > + } > > + > > + /* Find the largest framebuffer to use, then free the others */ > > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > + intel_crtc = to_intel_crtc(crtc); > > + > > + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) { > > + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n", > > + pipe_name(intel_crtc->pipe)); > > + continue; > > + } > > + > > + if (intel_crtc->plane_config.size > last_size) { > > + plane_config = &intel_crtc->plane_config; > > + last_size = plane_config->size; > > + fb = plane_config->fb; > > + } > > + } > > + > > + /* Free unused fbs */ > > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > + struct intel_framebuffer *cur_fb; > > + > > + intel_crtc = to_intel_crtc(crtc); > > + cur_fb = intel_crtc->plane_config.fb; > > + > > + if (cur_fb && cur_fb != fb) > > + intel_framebuffer_fini(cur_fb); > > + } > > + > > + if (!fb) { > > + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n"); > > + goto out_free; > > + } > > + > > + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel; > > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; > > + ifbdev->fb = fb; > > + > > + /* Assuming a single fb across all pipes here */ > > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > + intel_crtc = to_intel_crtc(crtc); > > + > > + if (!intel_crtc->active) > > + continue; > > + > > + /* > > + * This should only fail on the first one so we don't need > > + * to cleanup any secondary crtc->fbs > > + */ > > + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) > > + goto out_unref_obj; > > + > > + crtc->fb = &fb->base; > > + drm_gem_object_reference(&fb->obj->base); > > + drm_framebuffer_reference(&fb->base); > > + } > > + > > + dev_priv->fbdev = ifbdev; > > + > > + DRM_DEBUG_KMS("using BIOS fb for initial console\n"); > > + return; > > + > > +out_unref_obj: > > + intel_framebuffer_fini(fb); > > +out_free: > > + kfree(ifbdev); > > } > > > > int intel_fbdev_init(struct drm_device *dev) > > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ret; > > > > - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL); > > - if (!ifbdev) > > - return -ENOMEM; > > + /* This may fail, if so, dev_priv->fbdev won't be set below */ > > If you need a comment to explain your control flow, it's probably too > clever ;-) I could add a return value I suppose, but I didn't think it was too clever... > > > + intel_fbdev_init_bios(dev); > > > > - dev_priv->fbdev = ifbdev; > > - ifbdev->helper.funcs = &intel_fb_helper_funcs; > > + if ((ifbdev = dev_priv->fbdev) == NULL) { > > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > > + if (ifbdev == NULL) > > + return -ENOMEM; > > + > > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > > + ifbdev->preferred_bpp = 32; > > + > > + dev_priv->fbdev = ifbdev; > > + } > > > > ret = drm_fb_helper_init(dev, &ifbdev->helper, > > INTEL_INFO(dev)->num_pipes, > > 4); > > if (ret) { > > + dev_priv->fbdev = NULL; > > kfree(ifbdev); > > return ret; > > } > > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev) > > void intel_fbdev_initial_config(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_fbdev *ifbdev = dev_priv->fbdev; > > > > /* Due to peculiar init order wrt to hpd handling this is separate. */ > > - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32); > > + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); > > } > > > > void intel_fbdev_fini(struct drm_device *dev) > > @@ -322,7 +524,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); > > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights"); > > void intel_fbdev_output_poll_changed(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > > + if (dev_priv->fbdev) > > + drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > > Ok, here's the real reason I've actually replied to this patch. This looks > like a separate bugfix. And there's not mention of it in the commit > message. Please split it out and give it the nice commit message > explanation it deserves (whatever it's doing here). Oh this hunk may be spurious... I think it hit this case when we were failing to init dev_priv->fbdev and got an early hotplug event. But that should no longer be the case.
On Thu, 12 Dec 2013 23:54:37 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > > @@ -258,8 +357,102 @@ 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); > > No need to go the private fb route here anymore since now the fb is > free-standing. Normal refcounting should work. But a separate prep/cleanup > patch (prep since switching ifbdev->fb from struct to point would look > neat as a separate patch). Oh and can you explain this? I wouldn't be surprised if I got the refcounting wrong, but given how tricky it can be, can you explain where we'll take the ref here, and show that the right thing will happen if/when we mode set away from this buffer? I haven't actually seen a bug here with or without this patch (no crashes or warns), but I thought I needed this to make sure the obj didn't get a negative count after a mode set...
On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Thu, 12 Dec 2013 23:54:37 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> > @@ -258,8 +357,102 @@ 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); >> >> No need to go the private fb route here anymore since now the fb is >> free-standing. Normal refcounting should work. But a separate prep/cleanup >> patch (prep since switching ifbdev->fb from struct to point would look >> neat as a separate patch). > > Oh and can you explain this? I wouldn't be surprised if I got the > refcounting wrong, but given how tricky it can be, can you explain > where we'll take the ref here, and show that the right thing will > happen if/when we mode set away from this buffer? > > I haven't actually seen a bug here with or without this patch (no > crashes or warns), but I thought I needed this to make sure the obj > didn't get a negative count after a mode set... There's no bug here, and if you underrun the the refcount the current logic here won't help. It's just that the explicit call to _fini was an artifact of the old logic with embedding the framebuffer into the fbdev structure. But now that the fbdev framebuffer is freestanding there's no need for that - you exactly duplicate intel_user_framebuffer_destroy now. So a simple drm_framebuffer_unreference will do the trick and makes it clearer that this is now just another fb object with normal lifetime rules. I guess I score points for unclear review here ;-) -Daniel
On Fri, 13 Dec 2013 21:47:45 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Thu, 12 Dec 2013 23:54:37 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> > @@ -258,8 +357,102 @@ 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); > >> > >> No need to go the private fb route here anymore since now the fb is > >> free-standing. Normal refcounting should work. But a separate prep/cleanup > >> patch (prep since switching ifbdev->fb from struct to point would look > >> neat as a separate patch). > > > > Oh and can you explain this? I wouldn't be surprised if I got the > > refcounting wrong, but given how tricky it can be, can you explain > > where we'll take the ref here, and show that the right thing will > > happen if/when we mode set away from this buffer? > > > > I haven't actually seen a bug here with or without this patch (no > > crashes or warns), but I thought I needed this to make sure the obj > > didn't get a negative count after a mode set... > > There's no bug here, and if you underrun the the refcount the current > logic here won't help. It's just that the explicit call to _fini was > an artifact of the old logic with embedding the framebuffer into the > fbdev structure. But now that the fbdev framebuffer is freestanding > there's no need for that - you exactly duplicate > intel_user_framebuffer_destroy now. > > So a simple drm_framebuffer_unreference will do the trick and makes it > clearer that this is now just another fb object with normal lifetime > rules. > > I guess I score points for unclear review here ;-) Oh! I had this mixed up with the refs I take in the init_bios code... I thought you were saying those weren't necessary and I was getting really confused. This is just fixing up existing code to use the new field name, so no functional change. I see what you mean about splitting out the field change, but now that would be a pain :/ Do you want the above removed as a separate patch regardless of where the rest of the patches go? Thanks,
On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote: > On Fri, 13 Dec 2013 21:47:45 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Thu, 12 Dec 2013 23:54:37 +0100 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > >> > @@ -258,8 +357,102 @@ 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); > > >> > > >> No need to go the private fb route here anymore since now the fb is > > >> free-standing. Normal refcounting should work. But a separate prep/cleanup > > >> patch (prep since switching ifbdev->fb from struct to point would look > > >> neat as a separate patch). > > > > > > Oh and can you explain this? I wouldn't be surprised if I got the > > > refcounting wrong, but given how tricky it can be, can you explain > > > where we'll take the ref here, and show that the right thing will > > > happen if/when we mode set away from this buffer? > > > > > > I haven't actually seen a bug here with or without this patch (no > > > crashes or warns), but I thought I needed this to make sure the obj > > > didn't get a negative count after a mode set... > > > > There's no bug here, and if you underrun the the refcount the current > > logic here won't help. It's just that the explicit call to _fini was > > an artifact of the old logic with embedding the framebuffer into the > > fbdev structure. But now that the fbdev framebuffer is freestanding > > there's no need for that - you exactly duplicate > > intel_user_framebuffer_destroy now. > > > > So a simple drm_framebuffer_unreference will do the trick and makes it > > clearer that this is now just another fb object with normal lifetime > > rules. > > > > I guess I score points for unclear review here ;-) > > Oh! I had this mixed up with the refs I take in the init_bios code... > I thought you were saying those weren't necessary and I was getting > really confused. > > This is just fixing up existing code to use the new field name, so no > functional change. I see what you mean about splitting out the field > change, but now that would be a pain :/ Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a separate patch, but also real pain to split out now. > Do you want the above removed as a separate patch regardless of where > the rest of the patches go? Imo it should go with the switch to pointers, so this patch here is fine. Maybe a quick mention in the commit message about it. Cheers, Daniel
On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > Retrieve current framebuffer config info from the regs and create an fb > object for the buffer the BIOS or boot loader left us. This should > allow for smooth transitions to userspace apps once we finish the > initial configuration construction. > > v2: check for non-native modes and adjust (Jesse) > fixup aperture and cmap frees (Imre) > use unlocked unref if init_bios fails (Jesse) > fix curly brace around DSPADDR check (Imre) > comment failure path for pin_and_fence (Imre) > v3: fixup fixup of aperture frees (Chris) > v4: update to current bits (locking & pin_and_fence hack) (Jesse) > v5: move fb config fetch to display code (Jesse) > re-order hw state readout on initial load to suit fb inherit (Jesse) > re-add pin_and_fence in fbdev code to make sure we refcount properly (Je > v6: rename to plane_config (Daniel) > check for valid object when initializing BIOS fb (Jesse) > split from plane_config readout and other display changes (Jesse) > drop use_bios_fb option (Chris) > update comments (Jesse) > rework fbdev_init_bios for clarity (Jesse) > drop fb obj ref under lock (Chris) > v7: use fb object from plane_config instead (Ville) > take ref on fb object (Jesse) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Ok, I've thought through the lifetime rules for the fbcon takeover and I think we need some more polish for that. I also think we could simplify the inital_config logic quite a bit. More comments below. Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 4 +- > drivers/gpu/drm/i915/intel_drv.h | 4 +- > drivers/gpu/drm/i915/intel_fbdev.c | 235 ++++++++++++++++++++++++++++++++--- > 3 files changed, 224 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 94183af..b868331 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7817,11 +7817,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 4787773..5f9182e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -110,9 +110,10 @@ 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; > + int preferred_bpp; > }; > > struct intel_encoder { > @@ -671,6 +672,7 @@ int intel_framebuffer_init(struct drm_device *dev, > struct intel_framebuffer *ifb, > struct drm_mode_fb_cmd2 *mode_cmd, > struct drm_i915_gem_object *obj); > +void intel_fbdev_init_bios(struct drm_device *dev); > void intel_framebuffer_fini(struct intel_framebuffer *fb); > void intel_prepare_page_flip(struct drm_device *dev, int plane); > void intel_finish_page_flip(struct drm_device *dev, int pipe); > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 284c3eb..db75f22 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; > @@ -148,7 +157,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,14 +203,14 @@ 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) */ > > - DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n", > + DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n", > fb->width, fb->height, > - i915_gem_obj_ggtt_offset(obj), obj); > + info->screen_base, obj, info->screen_size); > > mutex_unlock(&dev->struct_mutex); > vga_switcheroo_client_fb_set(dev->pdev, info); > @@ -236,6 +245,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green, > *blue = intel_crtc->lut_b[regno] << 8; > } > > +static struct drm_fb_helper_crtc * > +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc) > +{ > + int i; > + > + for (i = 0; i < fb_helper->crtc_count; i++) > + if (fb_helper->crtc_info[i].mode_set.crtc == crtc) > + return &fb_helper->crtc_info[i]; > + > + return NULL; > +} > + > +/* > + * Try to read the BIOS display configuration and use it for the initial > + * fb configuration. > + * > + * The BIOS or boot loader will generally create an initial display > + * configuration for us that includes some set of active pipes and displays. > + * This routine tries to figure out which pipes and connectors are active > + * and stuffs them into the crtcs and modes array given to us by the > + * drm_fb_helper code. > + * > + * The overall sequence is: > + * intel_fbdev_init - from driver load > + * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data > + * drm_fb_helper_init - build fb helper structs > + * drm_fb_helper_single_add_all_connectors - more fb helper structs > + * intel_fbdev_initial_config - apply the config > + * drm_fb_helper_initial_config - call ->probe then register_framebuffer() > + * drm_setup_crtcs - build crtc config for fbdev > + * intel_fb_initial_config - find active connectors etc > + * drm_fb_helper_single_fb_probe - set up fbdev > + * intelfb_create - re-use or alloc fb, build out fbdev structs > + * > + * If the BIOS or boot loader leaves the display in VGA mode, there's not > + * much we can do; switching out of that mode involves allocating a new, > + * high res buffer, and also recalculating bandwidth requirements for the > + * new bpp configuration. > + */ > +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, > + struct drm_fb_helper_crtc **crtcs, > + struct drm_display_mode **modes, > + bool *enabled, int width, int height) > +{ > + int i; > + > + for (i = 0; i < fb_helper->connector_count; i++) { > + struct drm_connector *connector; > + struct drm_encoder *encoder; > + > + connector = fb_helper->connector_info[i]->connector; > + if (!enabled[i]) { > + DRM_DEBUG_KMS("connector %d not enabled, skipping\n", > + connector->base.id); > + continue; > + } > + > + encoder = connector->encoder; > + if (!encoder || !encoder->crtc) { > + DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n", > + connector->base.id); > + continue; > + } > + > + if (WARN_ON(!encoder->crtc->enabled)) { > + DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n", > + drm_get_connector_name(connector), > + encoder->crtc->base.id); > + return false; > + } > + > + if (!to_intel_crtc(encoder->crtc)->active) { > + DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n", > + drm_get_connector_name(connector), > + encoder->crtc->base.id); > + return false; > + } > + > + modes[i] = &encoder->crtc->mode; > + crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc); > + > + DRM_DEBUG_KMS("connector %s on crtc %d: %s\n", > + drm_get_connector_name(connector), > + encoder->crtc->base.id, > + modes[i]->name); > + } > + > + return true; > +} > + > static struct drm_fb_helper_funcs intel_fb_helper_funcs = { > .gamma_set = intel_crtc_fb_gamma_set, > .gamma_get = intel_crtc_fb_gamma_get, > @@ -258,8 +357,102 @@ 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); > +} > + > +/* > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible. > + * The core display code will have read out the current plane configuration, > + * so we use that to figure out if there's an object for us to use as the > + * fb, and if so, we re-use it for the fbdev configuration. > + * > + * Note we only support a single fb shared across pipes for boot (mostly for > + * fbcon), so we just find the biggest and use that. > + */ > +void intel_fbdev_init_bios(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_fbdev *ifbdev; > + struct intel_framebuffer *fb = NULL; > + struct drm_crtc *crtc; > + struct intel_crtc *intel_crtc; > + struct intel_plane_config *plane_config = NULL; > + unsigned int last_size = 0; > + > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > + if (ifbdev == NULL) { > + DRM_DEBUG_KMS("failed to alloc intel fbdev\n"); > + return; > + } > + > + /* Find the largest framebuffer to use, then free the others */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc = to_intel_crtc(crtc); > + > + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) { > + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n", > + pipe_name(intel_crtc->pipe)); > + continue; > + } > + > + if (intel_crtc->plane_config.size > last_size) { > + plane_config = &intel_crtc->plane_config; > + last_size = plane_config->size; > + fb = plane_config->fb; > + } > + } > + > + /* Free unused fbs */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + struct intel_framebuffer *cur_fb; > + > + intel_crtc = to_intel_crtc(crtc); > + cur_fb = intel_crtc->plane_config.fb; > + > + if (cur_fb && cur_fb != fb) > + intel_framebuffer_fini(cur_fb); > + } > + > + if (!fb) { > + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n"); > + goto out_free; > + } > + > + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel; > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; This here is a bit surprising - my model of operation here presumed that if we correctly assign the crtc->fb and the ifbdev->fb pointers we could fully rely on the fastboot setcrtc logic to eschew the modeset. Being the ever-vary of special-purpose logic I'd much prefer this implicit approach - otherwise we have one more special case to care about in the fastboot=y/n and CONFIG_FB=y/n matrix. So have you tried to ditch this special initial_config functions (obviously only looks good with fastboot=1) or what precise corner-case does this fix? > + ifbdev->fb = fb; > + > + /* Assuming a single fb across all pipes here */ > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > + intel_crtc = to_intel_crtc(crtc); > + > + if (!intel_crtc->active) > + continue; > + > + /* > + * This should only fail on the first one so we don't need > + * to cleanup any secondary crtc->fbs > + */ > + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) > + goto out_unref_obj; > + > + crtc->fb = &fb->base; > + drm_gem_object_reference(&fb->obj->base); > + drm_framebuffer_reference(&fb->base); As mentioned I think this part here needs to be moved to the earlier plane_config reconstruction loop. But checking the fb refcounting now it looks like we leak the initial references that the plane_config struct holds (or I didn't spot it). And we miss the reference for ifbdev->fb when stealing the bios framebuffer. With the current code of using intel_framebuffer_fini(ifbdev->fb); kfree(ifbdev->fb); You get away with this since the kfree will ignore the surplus references from the plane_config. But if you replace this with the drm_framebuffer_unreference call like I've suggested we'll have a real leak. So I think this needs to be fixed. > + } > + > + dev_priv->fbdev = ifbdev; > + > + DRM_DEBUG_KMS("using BIOS fb for initial console\n"); > + return; > + > +out_unref_obj: > + intel_framebuffer_fini(fb); > +out_free: > + kfree(ifbdev); > } > > int intel_fbdev_init(struct drm_device *dev) > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL); > - if (!ifbdev) > - return -ENOMEM; > + /* This may fail, if so, dev_priv->fbdev won't be set below */ > + intel_fbdev_init_bios(dev); > > - dev_priv->fbdev = ifbdev; > - ifbdev->helper.funcs = &intel_fb_helper_funcs; > + if ((ifbdev = dev_priv->fbdev) == NULL) { > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); > + if (ifbdev == NULL) > + return -ENOMEM; > + > + ifbdev->helper.funcs = &intel_fb_helper_funcs; > + ifbdev->preferred_bpp = 32; > + > + dev_priv->fbdev = ifbdev; > + } > > ret = drm_fb_helper_init(dev, &ifbdev->helper, > INTEL_INFO(dev)->num_pipes, > 4); > if (ret) { > + dev_priv->fbdev = NULL; > kfree(ifbdev); > return ret; > } > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev) > void intel_fbdev_initial_config(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_fbdev *ifbdev = dev_priv->fbdev; > > /* Due to peculiar init order wrt to hpd handling this is separate. */ > - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32); > + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); > } > > void intel_fbdev_fini(struct drm_device *dev) > @@ -322,7 +524,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); > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights"); > void intel_fbdev_output_poll_changed(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > + if (dev_priv->fbdev) > + drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > } > > void intel_fbdev_restore_mode(struct drm_device *dev) > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Dec 14, 2013 at 11:44:06AM +0100, Daniel Vetter wrote: > On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote: > > This is just fixing up existing code to use the new field name, so no > > functional change. I see what you mean about splitting out the field > > change, but now that would be a pain :/ > > Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a > separate patch, but also real pain to split out now. I'm inclined to say that we actually need the split-out conversion to a pointer. I might have lost my traces somewhere in this maze, but I think with the current patches we can have a NULL deref on ifbdev->fb in intelfb_create. Not sure, but a split-out patch would definitely help ;-) -Daniel
On Sat, Dec 14, 2013 at 12:13:45PM +0100, Daniel Vetter wrote: > On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; > > This here is a bit surprising - my model of operation here presumed that > if we correctly assign the crtc->fb and the ifbdev->fb pointers we could > fully rely on the fastboot setcrtc logic to eschew the modeset. > > Being the ever-vary of special-purpose logic I'd much prefer this implicit > approach - otherwise we have one more special case to care about in the > fastboot=y/n and CONFIG_FB=y/n matrix. > > So have you tried to ditch this special initial_config functions > (obviously only looks good with fastboot=1) or what precise corner-case > does this fix? Ok, I've dug out your old patch from almost a year ago which added the ->initial_config hook. I see the point now of copying exactly the bios config in the hope that we end up with something that has a higher chance of working. But imo this is an issue separate from the "take over bios fb" feature here, so this should be - split into a separate patch - used even when we fail to take over the bios fb The later point will require some mode-from-pipe_config reconstruction to work outside of the fastboot=1 hack mode. I really like the idea though. -Daniel
On Sat, 14 Dec 2013 12:36:30 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Sat, Dec 14, 2013 at 12:13:45PM +0100, Daniel Vetter wrote: > > On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > > > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; > > > > This here is a bit surprising - my model of operation here presumed that > > if we correctly assign the crtc->fb and the ifbdev->fb pointers we could > > fully rely on the fastboot setcrtc logic to eschew the modeset. > > > > Being the ever-vary of special-purpose logic I'd much prefer this implicit > > approach - otherwise we have one more special case to care about in the > > fastboot=y/n and CONFIG_FB=y/n matrix. > > > > So have you tried to ditch this special initial_config functions > > (obviously only looks good with fastboot=1) or what precise corner-case > > does this fix? > > Ok, I've dug out your old patch from almost a year ago which added the > ->initial_config hook. I see the point now of copying exactly the bios > config in the hope that we end up with something that has a higher chance > of working. > > But imo this is an issue separate from the "take over bios fb" feature > here, so this should be > - split into a separate patch > - used even when we fail to take over the bios fb > The later point will require some mode-from-pipe_config reconstruction to > work outside of the fastboot=1 hack mode. > > I really like the idea though. Ok I split this up, made fb a ptr, fixed up the CONFIG_FB bits, and I think we figured out the crtc timing stuff. I think that's all the feedback from the last round, so I'll re-post for some (hopefully) final review. There are some additional improvements that would be nice: - compute_mode_changes needs to get smarter in general and look at pfit state. Eventually we'll probably need a platform specific callback for this that tells us whether a pipe shutdown is needed for a given global configuration change. - pfit disable should be split out into a separate callback from our mode_set function (which also needs to get smarter after the compute_mode_changes improvements) - need to detect audio and infoframe configs and cross-check, though I don't think these affect fastboot at all since we ought to be able to leave them alone All of the above aren't strictly necessary though, they're just improvements on current code, some of which will overlap with the atomic mode set work. So we may be able to flip the i915.fastboot=1 default switch once these bits land after some additional HSW and BDW testing... Thanks, Jesse
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 94183af..b868331 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7817,11 +7817,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 4787773..5f9182e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -110,9 +110,10 @@ 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; + int preferred_bpp; }; struct intel_encoder { @@ -671,6 +672,7 @@ int intel_framebuffer_init(struct drm_device *dev, struct intel_framebuffer *ifb, struct drm_mode_fb_cmd2 *mode_cmd, struct drm_i915_gem_object *obj); +void intel_fbdev_init_bios(struct drm_device *dev); void intel_framebuffer_fini(struct intel_framebuffer *fb); void intel_prepare_page_flip(struct drm_device *dev, int plane); void intel_finish_page_flip(struct drm_device *dev, int pipe); diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 284c3eb..db75f22 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; @@ -148,7 +157,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,14 +203,14 @@ 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) */ - DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n", + DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n", fb->width, fb->height, - i915_gem_obj_ggtt_offset(obj), obj); + info->screen_base, obj, info->screen_size); mutex_unlock(&dev->struct_mutex); vga_switcheroo_client_fb_set(dev->pdev, info); @@ -236,6 +245,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green, *blue = intel_crtc->lut_b[regno] << 8; } +static struct drm_fb_helper_crtc * +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc) +{ + int i; + + for (i = 0; i < fb_helper->crtc_count; i++) + if (fb_helper->crtc_info[i].mode_set.crtc == crtc) + return &fb_helper->crtc_info[i]; + + return NULL; +} + +/* + * Try to read the BIOS display configuration and use it for the initial + * fb configuration. + * + * The BIOS or boot loader will generally create an initial display + * configuration for us that includes some set of active pipes and displays. + * This routine tries to figure out which pipes and connectors are active + * and stuffs them into the crtcs and modes array given to us by the + * drm_fb_helper code. + * + * The overall sequence is: + * intel_fbdev_init - from driver load + * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data + * drm_fb_helper_init - build fb helper structs + * drm_fb_helper_single_add_all_connectors - more fb helper structs + * intel_fbdev_initial_config - apply the config + * drm_fb_helper_initial_config - call ->probe then register_framebuffer() + * drm_setup_crtcs - build crtc config for fbdev + * intel_fb_initial_config - find active connectors etc + * drm_fb_helper_single_fb_probe - set up fbdev + * intelfb_create - re-use or alloc fb, build out fbdev structs + * + * If the BIOS or boot loader leaves the display in VGA mode, there's not + * much we can do; switching out of that mode involves allocating a new, + * high res buffer, and also recalculating bandwidth requirements for the + * new bpp configuration. + */ +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, + struct drm_fb_helper_crtc **crtcs, + struct drm_display_mode **modes, + bool *enabled, int width, int height) +{ + int i; + + for (i = 0; i < fb_helper->connector_count; i++) { + struct drm_connector *connector; + struct drm_encoder *encoder; + + connector = fb_helper->connector_info[i]->connector; + if (!enabled[i]) { + DRM_DEBUG_KMS("connector %d not enabled, skipping\n", + connector->base.id); + continue; + } + + encoder = connector->encoder; + if (!encoder || !encoder->crtc) { + DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n", + connector->base.id); + continue; + } + + if (WARN_ON(!encoder->crtc->enabled)) { + DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n", + drm_get_connector_name(connector), + encoder->crtc->base.id); + return false; + } + + if (!to_intel_crtc(encoder->crtc)->active) { + DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n", + drm_get_connector_name(connector), + encoder->crtc->base.id); + return false; + } + + modes[i] = &encoder->crtc->mode; + crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc); + + DRM_DEBUG_KMS("connector %s on crtc %d: %s\n", + drm_get_connector_name(connector), + encoder->crtc->base.id, + modes[i]->name); + } + + return true; +} + static struct drm_fb_helper_funcs intel_fb_helper_funcs = { .gamma_set = intel_crtc_fb_gamma_set, .gamma_get = intel_crtc_fb_gamma_get, @@ -258,8 +357,102 @@ 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); +} + +/* + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible. + * The core display code will have read out the current plane configuration, + * so we use that to figure out if there's an object for us to use as the + * fb, and if so, we re-use it for the fbdev configuration. + * + * Note we only support a single fb shared across pipes for boot (mostly for + * fbcon), so we just find the biggest and use that. + */ +void intel_fbdev_init_bios(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_fbdev *ifbdev; + struct intel_framebuffer *fb = NULL; + struct drm_crtc *crtc; + struct intel_crtc *intel_crtc; + struct intel_plane_config *plane_config = NULL; + unsigned int last_size = 0; + + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); + if (ifbdev == NULL) { + DRM_DEBUG_KMS("failed to alloc intel fbdev\n"); + return; + } + + /* Find the largest framebuffer to use, then free the others */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + intel_crtc = to_intel_crtc(crtc); + + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) { + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n", + pipe_name(intel_crtc->pipe)); + continue; + } + + if (intel_crtc->plane_config.size > last_size) { + plane_config = &intel_crtc->plane_config; + last_size = plane_config->size; + fb = plane_config->fb; + } + } + + /* Free unused fbs */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + struct intel_framebuffer *cur_fb; + + intel_crtc = to_intel_crtc(crtc); + cur_fb = intel_crtc->plane_config.fb; + + if (cur_fb && cur_fb != fb) + intel_framebuffer_fini(cur_fb); + } + + if (!fb) { + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n"); + goto out_free; + } + + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel; + ifbdev->helper.funcs = &intel_fb_helper_funcs; + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; + ifbdev->fb = fb; + + /* Assuming a single fb across all pipes here */ + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + intel_crtc = to_intel_crtc(crtc); + + if (!intel_crtc->active) + continue; + + /* + * This should only fail on the first one so we don't need + * to cleanup any secondary crtc->fbs + */ + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) + goto out_unref_obj; + + crtc->fb = &fb->base; + drm_gem_object_reference(&fb->obj->base); + drm_framebuffer_reference(&fb->base); + } + + dev_priv->fbdev = ifbdev; + + DRM_DEBUG_KMS("using BIOS fb for initial console\n"); + return; + +out_unref_obj: + intel_framebuffer_fini(fb); +out_free: + kfree(ifbdev); } int intel_fbdev_init(struct drm_device *dev) @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL); - if (!ifbdev) - return -ENOMEM; + /* This may fail, if so, dev_priv->fbdev won't be set below */ + intel_fbdev_init_bios(dev); - dev_priv->fbdev = ifbdev; - ifbdev->helper.funcs = &intel_fb_helper_funcs; + if ((ifbdev = dev_priv->fbdev) == NULL) { + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL); + if (ifbdev == NULL) + return -ENOMEM; + + ifbdev->helper.funcs = &intel_fb_helper_funcs; + ifbdev->preferred_bpp = 32; + + dev_priv->fbdev = ifbdev; + } ret = drm_fb_helper_init(dev, &ifbdev->helper, INTEL_INFO(dev)->num_pipes, 4); if (ret) { + dev_priv->fbdev = NULL; kfree(ifbdev); return ret; } @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev) void intel_fbdev_initial_config(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_fbdev *ifbdev = dev_priv->fbdev; /* Due to peculiar init order wrt to hpd handling this is separate. */ - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32); + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); } void intel_fbdev_fini(struct drm_device *dev) @@ -322,7 +524,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); @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights"); void intel_fbdev_output_poll_changed(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); + if (dev_priv->fbdev) + drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); } void intel_fbdev_restore_mode(struct drm_device *dev)
Retrieve current framebuffer config info from the regs and create an fb object for the buffer the BIOS or boot loader left us. This should allow for smooth transitions to userspace apps once we finish the initial configuration construction. v2: check for non-native modes and adjust (Jesse) fixup aperture and cmap frees (Imre) use unlocked unref if init_bios fails (Jesse) fix curly brace around DSPADDR check (Imre) comment failure path for pin_and_fence (Imre) v3: fixup fixup of aperture frees (Chris) v4: update to current bits (locking & pin_and_fence hack) (Jesse) v5: move fb config fetch to display code (Jesse) re-order hw state readout on initial load to suit fb inherit (Jesse) re-add pin_and_fence in fbdev code to make sure we refcount properly (Je v6: rename to plane_config (Daniel) check for valid object when initializing BIOS fb (Jesse) split from plane_config readout and other display changes (Jesse) drop use_bios_fb option (Chris) update comments (Jesse) rework fbdev_init_bios for clarity (Jesse) drop fb obj ref under lock (Chris) v7: use fb object from plane_config instead (Ville) take ref on fb object (Jesse) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_display.c | 4 +- drivers/gpu/drm/i915/intel_drv.h | 4 +- drivers/gpu/drm/i915/intel_fbdev.c | 235 ++++++++++++++++++++++++++++++++--- 3 files changed, 224 insertions(+), 19 deletions(-)