public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
From: mark.yao@rock-chips.com (Mark yao)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] drm/rockchip: fix fbdev crash when not use DRM_FBDEV_EMULATION
Date: Wed, 3 Aug 2016 16:56:20 +0800	[thread overview]
Message-ID: <57A1B1B4.7040501@rock-chips.com> (raw)
In-Reply-To: <20160803084627.GK6232@phenom.ffwll.local>

On 2016?08?03? 16:46, Daniel Vetter wrote:
> On Wed, Aug 03, 2016 at 10:43:21AM +0200, Daniel Vetter wrote:
>> On Wed, Aug 03, 2016 at 04:13:45PM +0800, Mark Yao wrote:
>>> [    1.162571] Unable to handle kernel NULL pointer dereference at virtual address 00000200
>>> [    1.165656] Modules linked in:
>>> [    1.165941] CPU: 5 PID: 143 Comm: kworker/5:2 Not tainted 4.4.15 #237
>>> [    1.166506] Hardware name: Rockchip RK3399 Evaluation Board v1 (Android) (DT)
>>> [    1.167153] Workqueue: events output_poll_execute
>>> [    1.168231] PC is at mutex_lock+0x14/0x44
>>> [    1.168586] LR is at drm_fb_helper_hotplug_event+0x28/0xcc
>>> [    1.172192] [<ffffff8008982110>] mutex_lock+0x14/0x44
>>> [    1.172196] [<ffffff80084025a4>] drm_fb_helper_hotplug_event+0x28/0xcc
>>> [    1.172201] [<ffffff8008427ae4>] rockchip_drm_output_poll_changed+0x14/0x1c
>>> [    1.172204] [<ffffff80083f7c4c>] drm_kms_helper_hotplug_event+0x28/0x34
>>> [    1.172207] [<ffffff80083f7ddc>] output_poll_execute+0x150/0x198
>>> [    1.172212] [<ffffff80080b0ea8>] process_one_work+0x218/0x3dc
>>> [    1.172215] [<ffffff80080b1578>] worker_thread+0x24c/0x374
>>> [    1.172217] [<ffffff80080b5bcc>] kthread+0xdc/0xe4
>>> [    1.172222] [<ffffff8008084cd0>] ret_from_fork+0x10/0x40
>>>
>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> Erhm, how exactly did you manage to blow up in there? Without fbdev
>> support enable drm_fb_helper_hotplug_event() does nothing at all.
>>
>> The fbdev helper is designed such that you _don't_ have to check for NULL
>> everywhere in the driver, that would be pretty bad code.
> And indeed this issue seems preexisting, and was already attempt to fix in
>
> commit 765c35bbd267e93eabe15a94534688ddaa0b9dc7
> Author: Heiko St?bner <heiko@sntech.de>
> Date:   Tue Jun 2 16:41:45 2015 +0200
>
>      drm/rockchip: only call drm_fb_helper_hotplug_event if fb_helper present
>
> except that patch is complete nonsense - the added check is always true.
> Oh and it's missing your s-o-b, which is not good at all.
>
> The proper fix is to make delayed fbdev loading work correctly, Thierry
> has patches for that on the mailing list. Not add even more hacks like the
> above (and then slap a misleading subject onto your patch).
> -Daniel

Hmmm, there is a mistake on Heiko's patch:

         struct drm_fb_helper *fb_helper = &private->fbdev_helper;

        -       drm_fb_helper_hotplug_event(fb_helper);
       +       if (fb_helper)
       +               drm_fb_helper_hotplug_event(fb_helper);

But the fb_helper would never be NULL, because the private->fbdev_helper 
is not a pointer.

So the first step is making private->fbdev_helper to a pointer, that's 
what I do on this patch.


>> -Daniel
>>
>>> ---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_drv.c   | 13 ++++++++++---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |  8 ++++++--
>>>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  6 +++---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 26 +++++++++++++++++---------
>>>   4 files changed, 36 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> index a822d49..1a4dad6 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
>>> @@ -261,7 +261,10 @@ static void rockchip_drm_lastclose(struct drm_device *dev)
>>>   {
>>>   	struct rockchip_drm_private *priv = dev->dev_private;
>>>   
>>> -	drm_fb_helper_restore_fbdev_mode_unlocked(&priv->fbdev_helper);
>>> +	if (!priv->fbdev)
>>> +		return;
>>> +
>>> +	drm_fb_helper_restore_fbdev_mode_unlocked(&priv->fbdev->fbdev_helper);
>>>   }
>>>   
>>>   static const struct file_operations rockchip_drm_driver_fops = {
>>> @@ -310,8 +313,10 @@ void rockchip_drm_fb_suspend(struct drm_device *drm)
>>>   {
>>>   	struct rockchip_drm_private *priv = drm->dev_private;
>>>   
>>> +	if (!priv->fbdev)
>>> +		return;
>>>   	console_lock();
>>> -	drm_fb_helper_set_suspend(&priv->fbdev_helper, 1);
>>> +	drm_fb_helper_set_suspend(&priv->fbdev->fbdev_helper, 1);
>>>   	console_unlock();
>>>   }
>>>   
>>> @@ -319,8 +324,10 @@ void rockchip_drm_fb_resume(struct drm_device *drm)
>>>   {
>>>   	struct rockchip_drm_private *priv = drm->dev_private;
>>>   
>>> +	if (!priv->fbdev)
>>> +		return;
>>>   	console_lock();
>>> -	drm_fb_helper_set_suspend(&priv->fbdev_helper, 0);
>>> +	drm_fb_helper_set_suspend(&priv->fbdev->fbdev_helper, 0);
>>>   	console_unlock();
>>>   }
>>>   
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>>> index ea39329..c054fc2 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>>> @@ -50,6 +50,11 @@ struct rockchip_crtc_state {
>>>   #define to_rockchip_crtc_state(s) \
>>>   		container_of(s, struct rockchip_crtc_state, base)
>>>   
>>> +struct rockchip_drm_fbdev {
>>> +	struct drm_fb_helper fbdev_helper;
>>> +	struct drm_gem_object *fbdev_bo;
>>> +};
>>> +
>>>   /*
>>>    * Rockchip drm private structure.
>>>    *
>>> @@ -57,8 +62,7 @@ struct rockchip_crtc_state {
>>>    * @num_pipe: number of pipes for this device.
>>>    */
>>>   struct rockchip_drm_private {
>>> -	struct drm_fb_helper fbdev_helper;
>>> -	struct drm_gem_object *fbdev_bo;
>>> +	struct rockchip_drm_fbdev *fbdev;
>>>   	const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
>>>   	struct drm_atomic_state *state;
>>>   };
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> index 55c5273..fef6f8d 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> @@ -156,10 +156,10 @@ err_gem_object_unreference:
>>>   static void rockchip_drm_output_poll_changed(struct drm_device *dev)
>>>   {
>>>   	struct rockchip_drm_private *private = dev->dev_private;
>>> -	struct drm_fb_helper *fb_helper = &private->fbdev_helper;
>>> +	struct rockchip_drm_fbdev *fbdev = private->fbdev;
>>>   
>>> -	if (fb_helper)
>>> -		drm_fb_helper_hotplug_event(fb_helper);
>>> +	if (fbdev)
>>> +		drm_fb_helper_hotplug_event(&fbdev->fbdev_helper);
>>>   }
>>>   
>>>   static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>>> index 207e01d..cc5781a 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c
>>> @@ -22,16 +22,16 @@
>>>   #include "rockchip_drm_fb.h"
>>>   
>>>   #define PREFERRED_BPP		32
>>> -#define to_drm_private(x) \
>>> -		container_of(x, struct rockchip_drm_private, fbdev_helper)
>>> +#define to_rockchip_fbdev(x) \
>>> +		container_of(x, struct rockchip_drm_fbdev, fbdev_helper)
>>>   
>>>   static int rockchip_fbdev_mmap(struct fb_info *info,
>>>   			       struct vm_area_struct *vma)
>>>   {
>>>   	struct drm_fb_helper *helper = info->par;
>>> -	struct rockchip_drm_private *private = to_drm_private(helper);
>>> +	struct rockchip_drm_fbdev *fbdev = to_rockchip_fbdev(helper);
>>>   
>>> -	return rockchip_gem_mmap_buf(private->fbdev_bo, vma);
>>> +	return rockchip_gem_mmap_buf(fbdev->fbdev_bo, vma);
>>>   }
>>>   
>>>   static struct fb_ops rockchip_drm_fbdev_ops = {
>>> @@ -50,7 +50,7 @@ static struct fb_ops rockchip_drm_fbdev_ops = {
>>>   static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
>>>   				     struct drm_fb_helper_surface_size *sizes)
>>>   {
>>> -	struct rockchip_drm_private *private = to_drm_private(helper);
>>> +	struct rockchip_drm_fbdev *fbdev = to_rockchip_fbdev(helper);
>>>   	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>>>   	struct drm_device *dev = helper->dev;
>>>   	struct rockchip_gem_object *rk_obj;
>>> @@ -75,7 +75,7 @@ static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
>>>   	if (IS_ERR(rk_obj))
>>>   		return -ENOMEM;
>>>   
>>> -	private->fbdev_bo = &rk_obj->base;
>>> +	fbdev->fbdev_bo = &rk_obj->base;
>>>   
>>>   	fbi = drm_fb_helper_alloc_fbi(helper);
>>>   	if (IS_ERR(fbi)) {
>>> @@ -85,7 +85,7 @@ static int rockchip_drm_fbdev_create(struct drm_fb_helper *helper,
>>>   	}
>>>   
>>>   	helper->fb = rockchip_drm_framebuffer_init(dev, &mode_cmd,
>>> -						   private->fbdev_bo);
>>> +						   fbdev->fbdev_bo);
>>>   	if (IS_ERR(helper->fb)) {
>>>   		dev_err(dev->dev, "Failed to allocate DRM framebuffer.\n");
>>>   		ret = PTR_ERR(helper->fb);
>>> @@ -130,6 +130,7 @@ static const struct drm_fb_helper_funcs rockchip_drm_fb_helper_funcs = {
>>>   int rockchip_drm_fbdev_init(struct drm_device *dev)
>>>   {
>>>   	struct rockchip_drm_private *private = dev->dev_private;
>>> +	struct rockchip_drm_fbdev *fbdev;
>>>   	struct drm_fb_helper *helper;
>>>   	unsigned int num_crtc;
>>>   	int ret;
>>> @@ -139,7 +140,12 @@ int rockchip_drm_fbdev_init(struct drm_device *dev)
>>>   
>>>   	num_crtc = dev->mode_config.num_crtc;
>>>   
>>> -	helper = &private->fbdev_helper;
>>> +	fbdev = devm_kzalloc(dev->dev, sizeof(*fbdev), GFP_KERNEL);
>>> +	if (!fbdev)
>>> +		return -ENOMEM;
>>> +
>>> +	private->fbdev = fbdev;
>>> +	helper = &fbdev->fbdev_helper;
>>>   
>>>   	drm_fb_helper_prepare(dev, helper, &rockchip_drm_fb_helper_funcs);
>>>   
>>> @@ -175,7 +181,9 @@ void rockchip_drm_fbdev_fini(struct drm_device *dev)
>>>   	struct rockchip_drm_private *private = dev->dev_private;
>>>   	struct drm_fb_helper *helper;
>>>   
>>> -	helper = &private->fbdev_helper;
>>> +	if (!private || private->fbdev)
>>> +		return;
>>> +	helper = &private->fbdev->fbdev_helper;
>>>   
>>>   	drm_fb_helper_unregister_fbi(helper);
>>>   	drm_fb_helper_release_fbi(helper);
>>> -- 
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch


-- 
?ark Yao

  reply	other threads:[~2016-08-03  8:56 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-03  8:13 [PATCH] drm/rockchip: fix fbdev crash when not use DRM_FBDEV_EMULATION Mark Yao
2016-08-03  8:43 ` Daniel Vetter
2016-08-03  8:46   ` Daniel Vetter
2016-08-03  8:56     ` Mark yao [this message]
2016-08-03 11:50       ` Daniel Vetter
2016-08-04  2:21 ` [PATCH v1] " Mark Yao
2016-08-04  8:08   ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=57A1B1B4.7040501@rock-chips.com \
    --to=mark.yao@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox