On Fri, Mar 07, 2014 at 08:57:55AM -0800, Jesse Barnes wrote:
> By stuffing the fb allocation into the crtc, we get mode set lifetime
> refcounting for free, but have to handle the initial pin & fence
> slightly differently. It also means we can move the shared fb handling
> into the core rather than leaving it out in the fbdev code.
>
> v2: null out crtc->fb on error (Daniel)
> take fbdev fb ref and remove unused error path (Daniel)
>
> Requested-by: Daniel Vetter <daniel@xxxxxxxx>
> Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
Ok, I've merged patches 1-4 and this one here from the series. Not
terribly happy that you didn't squash in this fixup, since now people
might stumble over the strange lifetime rules of the previous patches. But
meh ;-)
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 145 ++++++++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> drivers/gpu/drm/i915/intel_fbdev.c | 38 +--------
> 3 files changed, 105 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d450ab6..718cc73 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2068,7 +2068,7 @@ int intel_format_to_fourcc(int format)
> }
> }
>
> -static void intel_alloc_plane_obj(struct intel_crtc *crtc,
> +static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
> struct intel_plane_config *plane_config)
> {
> struct drm_device *dev = crtc->base.dev;
> @@ -2076,38 +2076,76 @@ static void intel_alloc_plane_obj(struct intel_crtc *crtc,
> struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> u32 base = plane_config->base;
>
> - if (!plane_config->fb)
> - return;
> -
> obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
> plane_config->size);
> if (!obj)
> - return;
> + return false;
>
> if (plane_config->tiled) {
> obj->tiling_mode = I915_TILING_X;
> - obj->stride = plane_config->fb->base.pitches[0];
> + obj->stride = crtc->base.fb->pitches[0];
> }
>
> - mode_cmd.pixel_format = plane_config->fb->base.pixel_format;
> - mode_cmd.width = plane_config->fb->base.width;
> - mode_cmd.height = plane_config->fb->base.height;
> - mode_cmd.pitches[0] = plane_config->fb->base.pitches[0];
> + mode_cmd.pixel_format = crtc->base.fb->pixel_format;
> + mode_cmd.width = crtc->base.fb->width;
> + mode_cmd.height = crtc->base.fb->height;
> + mode_cmd.pitches[0] = crtc->base.fb->pitches[0];
>
> mutex_lock(&dev->struct_mutex);
>
> - if (intel_framebuffer_init(dev, plane_config->fb, &mode_cmd, obj)) {
> + if (intel_framebuffer_init(dev, to_intel_framebuffer(crtc->base.fb),
> + &mode_cmd, obj)) {
> DRM_DEBUG_KMS("intel fb init failed\n");
> goto out_unref_obj;
> }
>
> mutex_unlock(&dev->struct_mutex);
> - DRM_DEBUG_KMS("plane fb obj %p\n", plane_config->fb->obj);
> - return;
> +
> + DRM_DEBUG_KMS("plane fb obj %p\n", obj);
> + return true;
>
> out_unref_obj:
> drm_gem_object_unreference(&obj->base);
> mutex_unlock(&dev->struct_mutex);
> + return false;
> +}
> +
> +static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
> + struct intel_plane_config *plane_config)
> +{
> + struct drm_device *dev = intel_crtc->base.dev;
> + struct drm_crtc *c;
> + struct intel_crtc *i;
> + struct intel_framebuffer *fb;
> +
> + if (!intel_crtc->base.fb)
> + return;
> +
> + if (intel_alloc_plane_obj(intel_crtc, plane_config))
> + return;
> +
> + kfree(intel_crtc->base.fb);
> +
> + /*
> + * Failed to alloc the obj, check to see if we should share
> + * an fb with another CRTC instead
> + */
> + list_for_each_entry(c, &dev->mode_config.crtc_list, head) {
> + i = to_intel_crtc(c);
> +
> + if (c == &intel_crtc->base)
> + continue;
> +
> + if (!i->active || !c->fb)
> + continue;
> +
> + fb = to_intel_framebuffer(c->fb);
> + if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
> + drm_framebuffer_reference(c->fb);
> + intel_crtc->base.fb = c->fb;
> + break;
> + }
> + }
> }
>
> static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> @@ -5636,8 +5674,8 @@ static void i9xx_get_plane_config(struct intel_crtc *crtc,
> int fourcc, pixel_format;
> int aligned_height;
>
> - plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
> - if (!plane_config->fb) {
> + crtc->base.fb = kzalloc(sizeof(struct intel_framebuffer), GFP_KERNEL);
> + if (!crtc->base.fb) {
> DRM_DEBUG_KMS("failed to alloc fb\n");
> return;
> }
> @@ -5650,8 +5688,8 @@ static void i9xx_get_plane_config(struct intel_crtc *crtc,
>
> pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> fourcc = intel_format_to_fourcc(pixel_format);
> - plane_config->fb->base.pixel_format = fourcc;
> - plane_config->fb->base.bits_per_pixel =
> + crtc->base.fb->pixel_format = fourcc;
> + crtc->base.fb->bits_per_pixel =
> drm_format_plane_cpp(fourcc, 0) * 8;
>
> if (INTEL_INFO(dev)->gen >= 4) {
> @@ -5666,23 +5704,23 @@ static void i9xx_get_plane_config(struct intel_crtc *crtc,
> plane_config->base = base;
>
> val = I915_READ(PIPESRC(pipe));
> - plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
> - plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
> + crtc->base.fb->width = ((val >> 16) & 0xfff) + 1;
> + crtc->base.fb->height = ((val >> 0) & 0xfff) + 1;
>
> val = I915_READ(DSPSTRIDE(pipe));
> - plane_config->fb->base.pitches[0] = val & 0xffffff80;
> + crtc->base.fb->pitches[0] = val & 0xffffff80;
>
> - aligned_height = intel_align_height(dev, plane_config->fb->base.height,
> + aligned_height = intel_align_height(dev, crtc->base.fb->height,
> plane_config->tiled);
>
> - plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> + plane_config->size = ALIGN(crtc->base.fb->pitches[0] *
> aligned_height, PAGE_SIZE);
>
> DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
> - pipe, plane, plane_config->fb->base.width,
> - plane_config->fb->base.height,
> - plane_config->fb->base.bits_per_pixel, base,
> - plane_config->fb->base.pitches[0],
> + pipe, plane, crtc->base.fb->width,
> + crtc->base.fb->height,
> + crtc->base.fb->bits_per_pixel, base,
> + crtc->base.fb->pitches[0],
> plane_config->size);
>
> }
> @@ -6644,8 +6682,8 @@ static void ironlake_get_plane_config(struct intel_crtc *crtc,
> int fourcc, pixel_format;
> int aligned_height;
>
> - plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
> - if (!plane_config->fb) {
> + crtc->base.fb = kzalloc(sizeof(struct intel_framebuffer), GFP_KERNEL);
> + if (!crtc->base.fb) {
> DRM_DEBUG_KMS("failed to alloc fb\n");
> return;
> }
> @@ -6658,8 +6696,8 @@ static void ironlake_get_plane_config(struct intel_crtc *crtc,
>
> pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> fourcc = intel_format_to_fourcc(pixel_format);
> - plane_config->fb->base.pixel_format = fourcc;
> - plane_config->fb->base.bits_per_pixel =
> + crtc->base.fb->pixel_format = fourcc;
> + crtc->base.fb->bits_per_pixel =
> drm_format_plane_cpp(fourcc, 0) * 8;
>
> base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> @@ -6674,23 +6712,23 @@ static void ironlake_get_plane_config(struct intel_crtc *crtc,
> plane_config->base = base;
>
> val = I915_READ(PIPESRC(pipe));
> - plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
> - plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
> + crtc->base.fb->width = ((val >> 16) & 0xfff) + 1;
> + crtc->base.fb->height = ((val >> 0) & 0xfff) + 1;
>
> val = I915_READ(DSPSTRIDE(pipe));
> - plane_config->fb->base.pitches[0] = val & 0xffffff80;
> + crtc->base.fb->pitches[0] = val & 0xffffff80;
>
> - aligned_height = intel_align_height(dev, plane_config->fb->base.height,
> + aligned_height = intel_align_height(dev, crtc->base.fb->height,
> plane_config->tiled);
>
> - plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> + plane_config->size = ALIGN(crtc->base.fb->pitches[0] *
> aligned_height, PAGE_SIZE);
>
> DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
> - pipe, plane, plane_config->fb->base.width,
> - plane_config->fb->base.height,
> - plane_config->fb->base.bits_per_pixel, base,
> - plane_config->fb->base.pitches[0],
> + pipe, plane, crtc->base.fb->width,
> + crtc->base.fb->height,
> + crtc->base.fb->bits_per_pixel, base,
> + crtc->base.fb->pitches[0],
> plane_config->size);
> }
>
> @@ -11258,10 +11296,7 @@ void intel_modeset_init(struct drm_device *dev)
> if (!crtc->active)
> continue;
>
> -#if IS_ENABLED(CONFIG_FB)
> /*
> - * We don't have a good way of freeing the buffer w/o the FB
> - * layer owning it...
> * Note that reserving the BIOS fb up front prevents us
> * from stuffing other stolen allocations like the ring
> * on top. This prevents some ugliness at boot time, and
> @@ -11275,9 +11310,8 @@ void intel_modeset_init(struct drm_device *dev)
> * If the fb is shared between multiple heads, we'll
> * just get the first one.
> */
> - intel_alloc_plane_obj(crtc, &crtc->plane_config);
> + intel_find_plane_obj(crtc, &crtc->plane_config);
> }
> -#endif
> }
> }
>
> @@ -11648,9 +11682,32 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>
> void intel_modeset_gem_init(struct drm_device *dev)
> {
> + struct drm_crtc *c;
> + struct intel_framebuffer *fb;
> +
> intel_modeset_init_hw(dev);
>
> intel_setup_overlay(dev);
> +
> + /*
> + * Make sure any fbs we allocated at startup are properly
> + * pinned & fenced. When we do the allocation it's too early
> + * for this.
> + */
> + mutex_lock(&dev->struct_mutex);
> + list_for_each_entry(c, &dev->mode_config.crtc_list, head) {
> + if (!c->fb)
> + continue;
> +
> + fb = to_intel_framebuffer(c->fb);
> + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) {
> + DRM_ERROR("failed to pin boot fb on pipe %d\n",
> + to_intel_crtc(c)->pipe);
> + drm_framebuffer_unreference(c->fb);
> + c->fb = NULL;
> + }
> + }
> + mutex_unlock(&dev->struct_mutex);
> }
>
> void intel_connector_unregister(struct intel_connector *intel_connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3d404ab..2ed72cf 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -220,7 +220,6 @@ typedef struct dpll {
> } intel_clock_t;
>
> struct intel_plane_config {
> - struct intel_framebuffer *fb; /* ends up managed by intel_fbdev.c */
> bool tiled;
> int size;
> u32 base;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 950469c..f81e3db 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -480,7 +480,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> 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) {
> + if (!intel_crtc->active || !crtc->fb) {
> DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> pipe_name(intel_crtc->pipe));
> continue;
> @@ -490,7 +490,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> DRM_DEBUG_KMS("found possible fb from plane %c\n",
> pipe_name(intel_crtc->pipe));
> plane_config = &intel_crtc->plane_config;
> - fb = plane_config->fb;
> + fb = to_intel_framebuffer(crtc->fb);
> max_size = plane_config->size;
> }
> }
> @@ -542,43 +542,15 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> max_size, cur_size);
> }
>
> - /* 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)
> - drm_framebuffer_unreference(&cur_fb->base);
> - }
> -
> if (!fb) {
> DRM_DEBUG_KMS("BIOS fb not suitable for all pipes, not using\n");
> goto out;
> }
>
> - ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> + ifbdev->preferred_bpp = fb->base.bits_per_pixel;
> 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_framebuffer_reference(&ifbdev->fb->base);
>
> /* Final pass to check if any active pipes don't have fbs */
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> @@ -596,8 +568,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> return true;
>
> -out_unref_obj:
> - drm_framebuffer_unreference(&fb->base);
> out:
>
> return false;
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
