Re: [PATCH 03/35] drm/<drivers>: reorder framebuffer init sequence

[Rob Clark <robdclark@gmail.com>]

On Thu, Jan 10, 2013 at 2:47 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> With more fine-grained locking we can no longer rely on the big
> mode_config lock to prevent concurrent access to mode resources
> like framebuffers. Instead a framebuffer becomes accessible to
> other threads as soon as it is added to the relevant lookup
> structures. Hence it needs to be fully set up by the time drivers
> call drm_framebuffer_init.
>
> This patch here is the drivers part of that reorg. Nothing really fancy
> going on safe for three special cases.
>
> - exynos needs to be careful to properly unref all handles.
> - nouveau gets a resource leak fixed for free: one of the error
>   cases didn't cleanup the framebuffer, which is now moot since
>   the framebuffer is only registered once it is fully set up.
> - vmwgfx requires a slight reordering of operations, I'm hoping I didn't
>   break anything (but it's refcount management only, so should be safe).
>
> v2: Split out exynos, since it's a bit more hairy than expected.
>
> v3: Drop bogus cirrus hunk noticed by Richard Wilbur.
>
> v4: Split out vmwgfx since there's a small change in return values.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

for the omapdrm part:

Reviewed-by: Rob Clark <rob@ti.com>

> ---
>  drivers/gpu/drm/ast/ast_main.c            |    4 ++--
>  drivers/gpu/drm/cirrus/cirrus_main.c      |    4 ++--
>  drivers/gpu/drm/drm_fb_cma_helper.c       |   10 +++++-----
>  drivers/gpu/drm/gma500/framebuffer.c      |    4 ++--
>  drivers/gpu/drm/i915/intel_display.c      |    5 +++--
>  drivers/gpu/drm/mgag200/mgag200_main.c    |    8 +++++---
>  drivers/gpu/drm/nouveau/nouveau_display.c |   10 +++++-----
>  drivers/gpu/drm/radeon/radeon_display.c   |    2 +-
>  drivers/gpu/drm/udl/udl_fb.c              |    2 +-
>  drivers/staging/omapdrm/omap_fb.c         |   16 ++++++++--------
>  10 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index f668e6c..d5ba709 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -266,13 +266,13 @@ int ast_framebuffer_init(struct drm_device *dev,
>  {
>         int ret;
>
> +       drm_helper_mode_fill_fb_struct(&ast_fb->base, mode_cmd);
> +       ast_fb->obj = obj;
>         ret = drm_framebuffer_init(dev, &ast_fb->base, &ast_fb_funcs);
>         if (ret) {
>                 DRM_ERROR("framebuffer init failed %d\n", ret);
>                 return ret;
>         }
> -       drm_helper_mode_fill_fb_struct(&ast_fb->base, mode_cmd);
> -       ast_fb->obj = obj;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
> index 6a9b12e..364474c 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_main.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_main.c
> @@ -42,13 +42,13 @@ int cirrus_framebuffer_init(struct drm_device *dev,
>  {
>         int ret;
>
> +       drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd);
> +       gfb->obj = obj;
>         ret = drm_framebuffer_init(dev, &gfb->base, &cirrus_fb_funcs);
>         if (ret) {
>                 DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
>                 return ret;
>         }
> -       drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd);
> -       gfb->obj = obj;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index fd9d0af..e1e0cb0 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -85,6 +85,11 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>         if (!fb_cma)
>                 return ERR_PTR(-ENOMEM);
>
> +       drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
> +
> +       for (i = 0; i < num_planes; i++)
> +               fb_cma->obj[i] = obj[i];
> +
>         ret = drm_framebuffer_init(dev, &fb_cma->fb, &drm_fb_cma_funcs);
>         if (ret) {
>                 dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);
> @@ -92,11 +97,6 @@ static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>                 return ERR_PTR(ret);
>         }
>
> -       drm_helper_mode_fill_fb_struct(&fb_cma->fb, mode_cmd);
> -
> -       for (i = 0; i < num_planes; i++)
> -               fb_cma->obj[i] = obj[i];
> -
>         return fb_cma;
>  }
>
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index afded54..38e7e75 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -260,13 +260,13 @@ static int psb_framebuffer_init(struct drm_device *dev,
>         default:
>                 return -EINVAL;
>         }
> +       drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd);
> +       fb->gtt = gt;
>         ret = drm_framebuffer_init(dev, &fb->base, &psb_fb_funcs);
>         if (ret) {
>                 dev_err(dev->dev, "framebuffer init failed: %d\n", ret);
>                 return ret;
>         }
> -       drm_helper_mode_fill_fb_struct(&fb->base, mode_cmd);
> -       fb->gtt = gt;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cf0c713..b2af790 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8387,14 +8387,15 @@ int intel_framebuffer_init(struct drm_device *dev,
>         if (mode_cmd->offsets[0] != 0)
>                 return -EINVAL;
>
> +       drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
> +       intel_fb->obj = obj;
> +
>         ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
>         if (ret) {
>                 DRM_ERROR("framebuffer init failed %d\n", ret);
>                 return ret;
>         }
>
> -       drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
> -       intel_fb->obj = obj;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 70dd3c5..266438a 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -40,13 +40,15 @@ int mgag200_framebuffer_init(struct drm_device *dev,
>                              struct drm_mode_fb_cmd2 *mode_cmd,
>                              struct drm_gem_object *obj)
>  {
> -       int ret = drm_framebuffer_init(dev, &gfb->base, &mga_fb_funcs);
> +       int ret;
> +
> +       drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd);
> +       gfb->obj = obj;
> +       ret = drm_framebuffer_init(dev, &gfb->base, &mga_fb_funcs);
>         if (ret) {
>                 DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
>                 return ret;
>         }
> -       drm_helper_mode_fill_fb_struct(&gfb->base, mode_cmd);
> -       gfb->obj = obj;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index e4188f2..2591ff2 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -78,11 +78,6 @@ nouveau_framebuffer_init(struct drm_device *dev,
>         struct drm_framebuffer *fb = &nv_fb->base;
>         int ret;
>
> -       ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs);
> -       if (ret) {
> -               return ret;
> -       }
> -
>         drm_helper_mode_fill_fb_struct(fb, mode_cmd);
>         nv_fb->nvbo = nvbo;
>
> @@ -125,6 +120,11 @@ nouveau_framebuffer_init(struct drm_device *dev,
>                 }
>         }
>
> +       ret = drm_framebuffer_init(dev, fb, &nouveau_framebuffer_funcs);
> +       if (ret) {
> +               return ret;
> +       }
> +
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 1da2386..12ec3ef 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -1089,12 +1089,12 @@ radeon_framebuffer_init(struct drm_device *dev,
>  {
>         int ret;
>         rfb->obj = obj;
> +       drm_helper_mode_fill_fb_struct(&rfb->base, mode_cmd);
>         ret = drm_framebuffer_init(dev, &rfb->base, &radeon_fb_funcs);
>         if (ret) {
>                 rfb->obj = NULL;
>                 return ret;
>         }
> -       drm_helper_mode_fill_fb_struct(&rfb->base, mode_cmd);
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index d4ab3be..829c4a7 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -435,8 +435,8 @@ udl_framebuffer_init(struct drm_device *dev,
>         int ret;
>
>         ufb->obj = obj;
> -       ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs);
>         drm_helper_mode_fill_fb_struct(&ufb->base, mode_cmd);
> +       ret = drm_framebuffer_init(dev, &ufb->base, &udlfb_funcs);
>         return ret;
>  }
>
> diff --git a/drivers/staging/omapdrm/omap_fb.c b/drivers/staging/omapdrm/omap_fb.c
> index 09028e9..bf6421f 100644
> --- a/drivers/staging/omapdrm/omap_fb.c
> +++ b/drivers/staging/omapdrm/omap_fb.c
> @@ -424,14 +424,6 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>         }
>
>         fb = &omap_fb->base;
> -       ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
> -       if (ret) {
> -               dev_err(dev->dev, "framebuffer init failed: %d\n", ret);
> -               goto fail;
> -       }
> -
> -       DBG("create: FB ID: %d (%p)", fb->base.id, fb);
> -
>         omap_fb->format = format;
>
>         for (i = 0; i < n; i++) {
> @@ -462,6 +454,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>
>         drm_helper_mode_fill_fb_struct(fb, mode_cmd);
>
> +       ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
> +       if (ret) {
> +               dev_err(dev->dev, "framebuffer init failed: %d\n", ret);
> +               goto fail;
> +       }
> +
> +       DBG("create: FB ID: %d (%p)", fb->base.id, fb);
> +
>         return fb;
>
>  fail:
> --
> 1.7.10.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel