diff mbox

[2/2] drm/exynos: reorder framebuffer init sequence

Message ID 1355396719-25286-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Dec. 13, 2012, 11:05 a.m. UTC
For user framebuffers it's easier to just inline the
exynos_drm_framebuffer_init helper instead of trying to adjust it -
most of the things that helper sets up need to be overwritten anyway
again due to the multiple backing storage objects support exynos has,
but does not use for the fbdev.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Inki Dae Dec. 13, 2012, 12:26 p.m. UTC | #1
> -----Original Message-----
> From: dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org
> [mailto:dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org] On
> Behalf Of Daniel Vetter
> Sent: Thursday, December 13, 2012 8:05 PM
> To: DRI Development
> Cc: Nouveau Dev; Intel Graphics Development; Daniel Vetter
> Subject: [PATCH 2/2] drm/exynos: reorder framebuffer init sequence
> 
> For user framebuffers it's easier to just inline the
> exynos_drm_framebuffer_init helper instead of trying to adjust it -
> most of the things that helper sets up need to be overwritten anyway
> again due to the multiple backing storage objects support exynos has,
> but does not use for the fbdev.
> 

Hi Daniel,

I'd rebase this patch to -next. This patch is conflicted with -next.
And if there is no any problem after test, will apply it.

Thanks,
Inki Dae

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |   32
++++++++++++++++++++--------
> ----
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 4ef4cd3..aea6500 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -136,15 +136,15 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
>  		return ERR_PTR(-ENOMEM);
>  	}
> 
> +	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
> +	exynos_fb->exynos_gem_obj[0] = to_exynos_gem_obj(obj);
> +
>  	ret = drm_framebuffer_init(dev, &exynos_fb->fb,
> &exynos_drm_fb_funcs);
>  	if (ret) {
>  		DRM_ERROR("failed to initialize framebuffer\n");
>  		return ERR_PTR(ret);
>  	}
> 
> -	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
> -	exynos_fb->exynos_gem_obj[0] = to_exynos_gem_obj(obj);
> -
>  	return &exynos_fb->fb;
>  }
> 
> @@ -190,9 +190,8 @@ exynos_user_fb_create(struct drm_device *dev, struct
> drm_file *file_priv,
>  		      struct drm_mode_fb_cmd2 *mode_cmd)
>  {
>  	struct drm_gem_object *obj;
> -	struct drm_framebuffer *fb;
>  	struct exynos_drm_fb *exynos_fb;
> -	int i;
> +	int i, ret;
> 
>  	DRM_DEBUG_KMS("%s\n", __FILE__);
> 
> @@ -202,13 +201,13 @@ exynos_user_fb_create(struct drm_device *dev, struct
> drm_file *file_priv,
>  		return ERR_PTR(-ENOENT);
>  	}
> 
> -	fb = exynos_drm_framebuffer_init(dev, mode_cmd, obj);
> -	if (IS_ERR(fb)) {
> -		drm_gem_object_unreference_unlocked(obj);
> -		return fb;
> +	exynos_fb = kzalloc(sizeof(*exynos_fb), GFP_KERNEL);
> +	if (!exynos_fb) {
> +		DRM_ERROR("failed to allocate exynos drm framebuffer\n");
> +		return ERR_PTR(-ENOMEM);
>  	}
> 
> -	exynos_fb = to_exynos_fb(fb);
> +	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
>  	exynos_fb->buf_cnt = exynos_drm_format_num_buffers(mode_cmd);
> 
>  	DRM_DEBUG_KMS("buf_cnt = %d\n", exynos_fb->buf_cnt);
> @@ -218,14 +217,23 @@ exynos_user_fb_create(struct drm_device *dev, struct
> drm_file *file_priv,
>  				mode_cmd->handles[i]);
>  		if (!obj) {
>  			DRM_ERROR("failed to lookup gem object\n");
> -			exynos_drm_fb_destroy(fb);
> +			kfree(exynos_fb);
>  			return ERR_PTR(-ENOENT);
>  		}
> 
>  		exynos_fb->exynos_gem_obj[i] = to_exynos_gem_obj(obj);
>  	}
> 
> -	return fb;
> +	ret = drm_framebuffer_init(dev, &exynos_fb->fb,
> &exynos_drm_fb_funcs);
> +	if (ret) {
> +		for (i = 0; i < exynos_fb->buf_cnt; i++)
> +			drm_gem_object_unreference_unlocked(&exynos_fb-
> >exynos_gem_obj[i]->base);
> +
> +		kfree(exynos_fb);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return &exynos_fb->fb;
>  }
> 
>  struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer
> *fb,
> --
> 1.7.10.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Dec. 13, 2012, 12:43 p.m. UTC | #2
Hi Inki,

I've pushed out the latest bits to
http://cgit.freedesktop.org/~danvet/drm/log/?h=drm-kms-locking with
some hacks on top to be able to compile all the arm drivers. Testing
feedback of the entire pile would be awesome, especially since you've
had some issues with framebuffer lifecycle and those should now be
correctly fixable with the proper refcounting. If you have too many
conflicts pls yell so that I can include your base into mine and
rebase the entire series.

Thanks, Daniel

On Thu, Dec 13, 2012 at 1:26 PM, Inki Dae <inki.dae@samsung.com> wrote:
>> -----Original Message-----
>> From: dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org
>> [mailto:dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org] On
>> Behalf Of Daniel Vetter
>> Sent: Thursday, December 13, 2012 8:05 PM
>> To: DRI Development
>> Cc: Nouveau Dev; Intel Graphics Development; Daniel Vetter
>> Subject: [PATCH 2/2] drm/exynos: reorder framebuffer init sequence
>>
>> For user framebuffers it's easier to just inline the
>> exynos_drm_framebuffer_init helper instead of trying to adjust it -
>> most of the things that helper sets up need to be overwritten anyway
>> again due to the multiple backing storage objects support exynos has,
>> but does not use for the fbdev.
>>
>
> Hi Daniel,
>
> I'd rebase this patch to -next. This patch is conflicted with -next.
> And if there is no any problem after test, will apply it.
Inki Dae Dec. 13, 2012, 3:16 p.m. UTC | #3
2012/12/13 Daniel Vetter <daniel.vetter@ffwll.ch>

> Hi Inki,
>
> I've pushed out the latest bits to
> http://cgit.freedesktop.org/~danvet/drm/log/?h=drm-kms-locking with
> some hacks on top to be able to compile all the arm drivers. Testing
> feedback of the entire pile would be awesome, especially since you've
> had some issues with framebuffer lifecycle and those should now be
> correctly fixable with the proper refcounting. If you have too many
> conflicts pls yell so that I can include your base into mine and
> rebase the entire series.
>
>
Hi Daniel,

How about rebasing this patch to top of git://
people.freedesktop.org/~airlied/linux.git drm-next?
Exynos's many patches have already been merged to drm-next. Or if you are
ok, I'd like to rebase your patch to -next and test it. I don't care either
way. :)

If there is any problem, please let me know.

Thanks,
Inki Dae

Thanks, Daniel
>
> On Thu, Dec 13, 2012 at 1:26 PM, Inki Dae <inki.dae@samsung.com> wrote:
> >> -----Original Message-----
> >> From: dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org
> >> [mailto:dri-devel-bounces+inki.dae=samsung.com@lists.freedesktop.org]
> On
> >> Behalf Of Daniel Vetter
> >> Sent: Thursday, December 13, 2012 8:05 PM
> >> To: DRI Development
> >> Cc: Nouveau Dev; Intel Graphics Development; Daniel Vetter
> >> Subject: [PATCH 2/2] drm/exynos: reorder framebuffer init sequence
> >>
> >> For user framebuffers it's easier to just inline the
> >> exynos_drm_framebuffer_init helper instead of trying to adjust it -
> >> most of the things that helper sets up need to be overwritten anyway
> >> again due to the multiple backing storage objects support exynos has,
> >> but does not use for the fbdev.
> >>
> >
> > Hi Daniel,
> >
> > I'd rebase this patch to -next. This patch is conflicted with -next.
> > And if there is no any problem after test, will apply it.
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter Dec. 13, 2012, 4:38 p.m. UTC | #4
On Thu, Dec 13, 2012 at 4:16 PM, Inki Dae <inki.dae@samsung.com> wrote:
> How about rebasing this patch to top of
> git://people.freedesktop.org/~airlied/linux.git drm-next?
> Exynos's many patches have already been merged to drm-next. Or if you are
> ok, I'd like to rebase your patch to -next and test it. I don't care either
> way. :)

I've pushed out a rebased version, but somehow I can't compile-test
exynos any more :( Probably missing some other stuff from linux-next.
Anyway, I don't mind which patch you pick or whether you just roll
your own, the important thing is that drm_framebuffer_init is called
_after_ everything is set up and initialized. Everything else doesn't
really matter.
-Daniel
Inki Dae Dec. 14, 2012, 4:57 a.m. UTC | #5
2012/12/14 Daniel Vetter <daniel.vetter@ffwll.ch>

> On Thu, Dec 13, 2012 at 4:16 PM, Inki Dae <inki.dae@samsung.com> wrote:
> > How about rebasing this patch to top of
> > git://people.freedesktop.org/~airlied/linux.git drm-next?
> > Exynos's many patches have already been merged to drm-next. Or if you are
> > ok, I'd like to rebase your patch to -next and test it. I don't care
> either
> > way. :)
>
> I've pushed out a rebased version, but somehow I can't compile-test
> exynos any more :( Probably missing some other stuff from linux-next.
> Anyway, I don't mind which patch you pick or whether you just roll
> your own, the important thing is that drm_framebuffer_init is called
> _after_ everything is set up and initialized. Everything else doesn't
> really matter.
>

I've rebase your patch to -next and tested it. But your patch had null
pointer issue to gem object so I fixed it.
For this, you can refer to the below link,

http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git;a=shortlog;h=refs/heads/exynos-drm-next

And I gonna request git pull to -next including that patch today. If there
is any problem, please let me know.

Thanks,
Inki Dae.



> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter Dec. 14, 2012, 8:40 a.m. UTC | #6
On Fri, Dec 14, 2012 at 5:57 AM, Inki Dae <inki.dae@samsung.com> wrote:
> 2012/12/14 Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> On Thu, Dec 13, 2012 at 4:16 PM, Inki Dae <inki.dae@samsung.com> wrote:
>> > How about rebasing this patch to top of
>> > git://people.freedesktop.org/~airlied/linux.git drm-next?
>> > Exynos's many patches have already been merged to drm-next. Or if you
>> > are
>> > ok, I'd like to rebase your patch to -next and test it. I don't care
>> > either
>> > way. :)
>>
>> I've pushed out a rebased version, but somehow I can't compile-test
>> exynos any more :( Probably missing some other stuff from linux-next.
>> Anyway, I don't mind which patch you pick or whether you just roll
>> your own, the important thing is that drm_framebuffer_init is called
>> _after_ everything is set up and initialized. Everything else doesn't
>> really matter.
>
>
> I've rebase your patch to -next and tested it. But your patch had null
> pointer issue to gem object so I fixed it.
> For this, you can refer to the below link,
>
> http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git;a=shortlog;h=refs/heads/exynos-drm-next
>
> And I gonna request git pull to -next including that patch today. If there
> is any problem, please let me know.

Oops, sorry for missing the gem obj assignment, your updated version
looks good. Thanks for fixing it up.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 4ef4cd3..aea6500 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -136,15 +136,15 @@  exynos_drm_framebuffer_init(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
+	exynos_fb->exynos_gem_obj[0] = to_exynos_gem_obj(obj);
+
 	ret = drm_framebuffer_init(dev, &exynos_fb->fb, &exynos_drm_fb_funcs);
 	if (ret) {
 		DRM_ERROR("failed to initialize framebuffer\n");
 		return ERR_PTR(ret);
 	}
 
-	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
-	exynos_fb->exynos_gem_obj[0] = to_exynos_gem_obj(obj);
-
 	return &exynos_fb->fb;
 }
 
@@ -190,9 +190,8 @@  exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 		      struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_gem_object *obj;
-	struct drm_framebuffer *fb;
 	struct exynos_drm_fb *exynos_fb;
-	int i;
+	int i, ret;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
@@ -202,13 +201,13 @@  exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 		return ERR_PTR(-ENOENT);
 	}
 
-	fb = exynos_drm_framebuffer_init(dev, mode_cmd, obj);
-	if (IS_ERR(fb)) {
-		drm_gem_object_unreference_unlocked(obj);
-		return fb;
+	exynos_fb = kzalloc(sizeof(*exynos_fb), GFP_KERNEL);
+	if (!exynos_fb) {
+		DRM_ERROR("failed to allocate exynos drm framebuffer\n");
+		return ERR_PTR(-ENOMEM);
 	}
 
-	exynos_fb = to_exynos_fb(fb);
+	drm_helper_mode_fill_fb_struct(&exynos_fb->fb, mode_cmd);
 	exynos_fb->buf_cnt = exynos_drm_format_num_buffers(mode_cmd);
 
 	DRM_DEBUG_KMS("buf_cnt = %d\n", exynos_fb->buf_cnt);
@@ -218,14 +217,23 @@  exynos_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 				mode_cmd->handles[i]);
 		if (!obj) {
 			DRM_ERROR("failed to lookup gem object\n");
-			exynos_drm_fb_destroy(fb);
+			kfree(exynos_fb);
 			return ERR_PTR(-ENOENT);
 		}
 
 		exynos_fb->exynos_gem_obj[i] = to_exynos_gem_obj(obj);
 	}
 
-	return fb;
+	ret = drm_framebuffer_init(dev, &exynos_fb->fb, &exynos_drm_fb_funcs);
+	if (ret) {
+		for (i = 0; i < exynos_fb->buf_cnt; i++)
+			drm_gem_object_unreference_unlocked(&exynos_fb->exynos_gem_obj[i]->base);
+
+		kfree(exynos_fb);
+		return ERR_PTR(ret);
+	}
+
+	return &exynos_fb->fb;
 }
 
 struct exynos_drm_gem_buf *exynos_drm_fb_buffer(struct drm_framebuffer *fb,