On Wed, Feb 05, 2014 at 05:30:48PM +0000, Jesse Barnes wrote:
> +static bool intel_fbdev_init_bios(struct drm_device *dev,
> + 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, max_size = 0, tmp;
> +
> + /* 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) {
> + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> + pipe_name(intel_crtc->pipe));
> + continue;
> + }
> +
> + tmp = intel_crtc->config.adjusted_mode.crtc_hdisplay *
> + intel_crtc->config.adjusted_mode.crtc_vdisplay *
> + intel_crtc->plane_config.fb->base.pitches[0];
This reads as width * height * stride. I presume you meant bpp/8 here,
but since we keep the fb and its pitch intact, you need height * stride.
Actually, it preserves a completely different fb, so this estimate of
size is incomplete.
> + max_size = max(max_size, tmp);
> +
> + /*
> + * Make sure the fb will fit the biggest active pipe. If
> + * not, clear out any previous usage.
> + */
> + if (intel_crtc->plane_config.size > last_size &&
> + intel_crtc->plane_config.size >= max_size) {
> + plane_config = &intel_crtc->plane_config;
> + last_size = plane_config->size;
> + fb = plane_config->fb;
> + } else {
> + plane_config = NULL;
> + fb = NULL;
> + }
Two CRTCs sharing a plane_config will now end up with plane_config/fb =
NULL, and so bail out. This is confusing me. If we here just found the
largest fb and then did a second pass confirming that all CRTC and planes
fitted into it, I would find it easier to understand.
> + }
> +
> + /* 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;
> + }
> +
> + 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);
> + }
> +
> + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> + return true;
Somewhere around here we must disable all CRTCs that are active and have
no fb.
> +
> +out_unref_obj:
> + intel_framebuffer_fini(fb);
> +out:
> +
> + return false;
> +}
> +
> int intel_fbdev_init(struct drm_device *dev)
> {
> struct intel_fbdev *ifbdev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
A useful assertion here would be:
if (WARN_ON(INTEL_INFO(dev)->num_pipes == 0)) return -ENODEV;
> - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> - if (!ifbdev)
> + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> + if (ifbdev == NULL)
> return -ENOMEM;
>
> - dev_priv->fbdev = ifbdev;
> ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + dev_priv->fbdev = ifbdev;
> +
> + if (!i915.fastboot || !intel_fbdev_init_bios(dev, ifbdev))
> + ifbdev->preferred_bpp = 32;
Bikeshed: move i915.fastboot to intel_fbdev_init_bios and rearrange like
if (!intel_fbdev_init(intel_fbdev_init_bios(dev, ifbdev)) {
ifbdev->helper.funcs = &intel_fb_helper_funcs;
ifbdev->preferred_bpp = 32;
}
(then dev_priv->fbdev = ifbdev can be done last on success as before)
>
> ret = drm_fb_helper_init(dev, &ifbdev->helper,
> - INTEL_INFO(dev)->num_pipes,
> - 4);
> + INTEL_INFO(dev)->num_pipes, 4);
> if (ret) {
> + dev_priv->fbdev = NULL;
> kfree(ifbdev);
> return ret;
> }
> void intel_fbdev_restore_mode(struct drm_device *dev)
> @@ -441,7 +546,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
> int ret;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (INTEL_INFO(dev)->num_pipes == 0)
> + if (INTEL_INFO(dev)->num_pipes == 0 || !dev_priv->fbdev)
num_pipes == 0 => dev_priv->fbdev == NULL
ergo this can be reduced to just
if (dev_priv->fbdev == NULL)
> return;
>
> drm_modeset_lock_all(dev);
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
