[PATCH] drm/msm: Do not run snapshot on non-DPU devices

[PATCH] drm/msm: Do not run snapshot on non-DPU devices

[Fabio Estevam]

Since commit 98659487b845 ("drm/msm: add support to take dpu snapshot")
the following NULL pointer dereference is seen on i.MX53:

[ 3.275493] msm msm: bound 30000000.gpu (ops a3xx_ops)
[ 3.287174] [drm] Initialized msm 1.8.0 20130625 for msm on minor 0
[ 3.293915] 8<--- cut here ---
[ 3.297012] Unable to handle kernel NULL pointer dereference at virtual address 00000028
[ 3.305244] pgd = (ptrval)
[ 3.307989] [00000028] *pgd=00000000
[ 3.311624] Internal error: Oops: 805 [#1] SMP ARM
[ 3.316430] Modules linked in:
[ 3.319503] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0+g682d702b426b #1
[ 3.326652] Hardware name: Freescale i.MX53 (Device Tree Support)
[ 3.332754] PC is at __mutex_init+0x14/0x54
[ 3.336969] LR is at msm_disp_snapshot_init+0x24/0xa0

i.MX53 does not use the DPU controller.

Fix the problem by only calling msm_disp_snapshot_init() on platforms that
use the DPU controller.

Cc: stable@vger.kernel.org
Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 2e6fc185e54d..2aa2266454b7 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -630,10 +630,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	if (ret)
 		goto err_msm_uninit;
 
-	ret = msm_disp_snapshot_init(ddev);
-	if (ret)
-		DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
-
+	if (kms) {
+		ret = msm_disp_snapshot_init(ddev);
+		if (ret)
+			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
+	}
 	drm_mode_config_reset(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-- 
2.25.1

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[abhinavk]

Hi Fabio

On 2021-09-14 10:48, Fabio Estevam wrote:
> Since commit 98659487b845 ("drm/msm: add support to take dpu snapshot")
> the following NULL pointer dereference is seen on i.MX53:
> 
> [ 3.275493] msm msm: bound 30000000.gpu (ops a3xx_ops)
> [ 3.287174] [drm] Initialized msm 1.8.0 20130625 for msm on minor 0
> [ 3.293915] 8<--- cut here ---
> [ 3.297012] Unable to handle kernel NULL pointer dereference at
> virtual address 00000028
> [ 3.305244] pgd = (ptrval)
> [ 3.307989] [00000028] *pgd=00000000
> [ 3.311624] Internal error: Oops: 805 [#1] SMP ARM
> [ 3.316430] Modules linked in:
> [ 3.319503] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
> 5.14.0+g682d702b426b #1
> [ 3.326652] Hardware name: Freescale i.MX53 (Device Tree Support)
> [ 3.332754] PC is at __mutex_init+0x14/0x54
> [ 3.336969] LR is at msm_disp_snapshot_init+0x24/0xa0
> 
> i.MX53 does not use the DPU controller.
> 
> Fix the problem by only calling msm_disp_snapshot_init() on platforms 
> that
> use the DPU controller.
> 
> Cc: stable@vger.kernel.org
> Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
> b/drivers/gpu/drm/msm/msm_drv.c
> index 2e6fc185e54d..2aa2266454b7 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -630,10 +630,11 @@ static int msm_drm_init(struct device *dev,
> const struct drm_driver *drv)
>  	if (ret)
>  		goto err_msm_uninit;
> 
> -	ret = msm_disp_snapshot_init(ddev);
> -	if (ret)
> -		DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
> -
> +	if (kms) {
> +		ret = msm_disp_snapshot_init(ddev);
> +		if (ret)
> +			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", 
> ret);
> +	}
Are you not using DPU or are you not using mdp4/mdp5 as well? Even if 
you are using any of mdps, kms should
not be NULL. Hence wanted to check the test case.

>  	drm_mode_config_reset(ddev);
> 
>  #ifdef CONFIG_DRM_FBDEV_EMULATION

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[Fabio Estevam]

Hi Abhinav,

On Wed, Sep 15, 2021 at 11:22 PM <abhinavk@codeaurora.org> wrote:

> Are you not using DPU or are you not using mdp4/mdp5 as well? Even if
> you are using any of mdps, kms should
> not be NULL. Hence wanted to check the test case.

I am running i.MX53, which is an NXP SoC, not Qualcomm's.

It does not use DPU, nor MDP4/5 and kms is NULL in this case.

Some debug prints to confirm:

--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -557,18 +557,22 @@ static int msm_drm_init(struct device *dev,
const struct drm_driver *drv)
        case KMS_MDP4:
                kms = mdp4_kms_init(ddev);
                priv->kms = kms;
+               pr_err("******** KMS_MDP4\n");
                break;
        case KMS_MDP5:
                kms = mdp5_kms_init(ddev);
+               pr_err("******** KMS_MDP5\n");
                break;
        case KMS_DPU:
                kms = dpu_kms_init(ddev);
+               pr_err("******** KMS_DPU\n");
                priv->kms = kms;
                break;
        default:
                /* valid only for the dummy headless case, where of_node=NULL */
                WARN_ON(dev->of_node);
                kms = NULL;
+               pr_err("******** KMS is NULL\n");
                break;
        }

# dmesg | grep KMS
[    3.153215] ******** KMS is NULL

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[Fabio Estevam]

Hi Abhinav,

On Wed, Sep 15, 2021 at 11:22 PM <abhinavk@codeaurora.org> wrote:

> Are you not using DPU or are you not using mdp4/mdp5 as well? Even if
> you are using any of mdps, kms should
> not be NULL. Hence wanted to check the test case.

I am running i.MX53, which is an NXP SoC, not Qualcomm's.

It does not use DPU, nor MDP4/5 and kms is NULL in this case.

Some debug prints to confirm:

--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -557,18 +557,22 @@ static int msm_drm_init(struct device *dev,
const struct drm_driver *drv)
        case KMS_MDP4:
                kms = mdp4_kms_init(ddev);
                priv->kms = kms;
+               pr_err("******** KMS_MDP4\n");
                break;
        case KMS_MDP5:
                kms = mdp5_kms_init(ddev);
+               pr_err("******** KMS_MDP5\n");
                break;
        case KMS_DPU:
                kms = dpu_kms_init(ddev);
+               pr_err("******** KMS_DPU\n");
                priv->kms = kms;
                break;
        default:
                /* valid only for the dummy headless case, where of_node=NULL */
                WARN_ON(dev->of_node);
                kms = NULL;
+               pr_err("******** KMS is NULL\n");
                break;
        }

# dmesg | grep KMS
[    3.153215] ******** KMS is NULL

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[abhinavk]

Hi Fabio

Thanks for confirming.

Although I have no issues with your change, I am curious why even msm is 
probing and/or binding.
Your device tree should not be having any mdp/dpu nodes then.

Thanks

Abhinav
On 2021-09-16 04:42, Fabio Estevam wrote:
> Hi Abhinav,
> 
> On Wed, Sep 15, 2021 at 11:22 PM <abhinavk@codeaurora.org> wrote:
> 
>> Are you not using DPU or are you not using mdp4/mdp5 as well? Even if
>> you are using any of mdps, kms should
>> not be NULL. Hence wanted to check the test case.
> 
> I am running i.MX53, which is an NXP SoC, not Qualcomm's.
> 
> It does not use DPU, nor MDP4/5 and kms is NULL in this case.
> 
> Some debug prints to confirm:
> 
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -557,18 +557,22 @@ static int msm_drm_init(struct device *dev,
> const struct drm_driver *drv)
>         case KMS_MDP4:
>                 kms = mdp4_kms_init(ddev);
>                 priv->kms = kms;
> +               pr_err("******** KMS_MDP4\n");
>                 break;
>         case KMS_MDP5:
>                 kms = mdp5_kms_init(ddev);
> +               pr_err("******** KMS_MDP5\n");
>                 break;
>         case KMS_DPU:
>                 kms = dpu_kms_init(ddev);
> +               pr_err("******** KMS_DPU\n");
>                 priv->kms = kms;
>                 break;
>         default:
>                 /* valid only for the dummy headless case, where 
> of_node=NULL */
>                 WARN_ON(dev->of_node);
>                 kms = NULL;
> +               pr_err("******** KMS is NULL\n");
>                 break;
>         }
> 
> # dmesg | grep KMS
> [    3.153215] ******** KMS is NULL

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[abhinavk]

Hi Fabio

Thanks for confirming.

Although I have no issues with your change, I am curious why even msm is 
probing and/or binding.
Your device tree should not be having any mdp/dpu nodes then.

Thanks

Abhinav
On 2021-09-16 04:42, Fabio Estevam wrote:
> Hi Abhinav,
> 
> On Wed, Sep 15, 2021 at 11:22 PM <abhinavk@codeaurora.org> wrote:
> 
>> Are you not using DPU or are you not using mdp4/mdp5 as well? Even if
>> you are using any of mdps, kms should
>> not be NULL. Hence wanted to check the test case.
> 
> I am running i.MX53, which is an NXP SoC, not Qualcomm's.
> 
> It does not use DPU, nor MDP4/5 and kms is NULL in this case.
> 
> Some debug prints to confirm:
> 
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -557,18 +557,22 @@ static int msm_drm_init(struct device *dev,
> const struct drm_driver *drv)
>         case KMS_MDP4:
>                 kms = mdp4_kms_init(ddev);
>                 priv->kms = kms;
> +               pr_err("******** KMS_MDP4\n");
>                 break;
>         case KMS_MDP5:
>                 kms = mdp5_kms_init(ddev);
> +               pr_err("******** KMS_MDP5\n");
>                 break;
>         case KMS_DPU:
>                 kms = dpu_kms_init(ddev);
> +               pr_err("******** KMS_DPU\n");
>                 priv->kms = kms;
>                 break;
>         default:
>                 /* valid only for the dummy headless case, where 
> of_node=NULL */
>                 WARN_ON(dev->of_node);
>                 kms = NULL;
> +               pr_err("******** KMS is NULL\n");
>                 break;
>         }
> 
> # dmesg | grep KMS
> [    3.153215] ******** KMS is NULL

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[Fabio Estevam]

Hi Abhinav,

On Thu, Sep 16, 2021 at 1:15 PM <abhinavk@codeaurora.org> wrote:
>
> Hi Fabio
>
> Thanks for confirming.
>
> Although I have no issues with your change, I am curious why even msm is
> probing and/or binding.
> Your device tree should not be having any mdp/dpu nodes then.

The i.MX53 does have the following GPU node:

compatible = "amd,imageon-200.0", "amd,imageon";

That's why it probes the msm driver.

However, i.MX53 does not have any of the Qualcomm display controllers.

It uses the i.MX IPU display controller instead.

Hope that clarifies.

Please reply with a Reviewed-by if you are happy with my fix.

Thanks

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[Fabio Estevam]

Hi Abhinav,

On Thu, Sep 16, 2021 at 1:15 PM <abhinavk@codeaurora.org> wrote:
>
> Hi Fabio
>
> Thanks for confirming.
>
> Although I have no issues with your change, I am curious why even msm is
> probing and/or binding.
> Your device tree should not be having any mdp/dpu nodes then.

The i.MX53 does have the following GPU node:

compatible = "amd,imageon-200.0", "amd,imageon";

That's why it probes the msm driver.

However, i.MX53 does not have any of the Qualcomm display controllers.

It uses the i.MX IPU display controller instead.

Hope that clarifies.

Please reply with a Reviewed-by if you are happy with my fix.

Thanks

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[abhinavk]

Hi Fabio

Ah, I did not realize that amd compatible is present in the list and its 
quite a surprise.

/*
  * We don't know what's the best binding to link the gpu with the drm 
device.
  * Fow now, we just hunt for all the possible gpus that we support, and 
add them
  * as components.
  */
static const struct of_device_id msm_gpu_match[] = {
	{ .compatible = "qcom,adreno" },
	{ .compatible = "qcom,adreno-3xx" },
	{ .compatible = "amd,imageon" },
	{ .compatible = "qcom,kgsl-3d0" },
	{ },
};

https://github.com/torvalds/linux/commit/e6f6d63ed14c20528aa6df05a8f0707c183c6ba3

For this change itself,
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

On 2021-09-16 09:24, Fabio Estevam wrote:
> Hi Abhinav,
> 
> On Thu, Sep 16, 2021 at 1:15 PM <abhinavk@codeaurora.org> wrote:
>> 
>> Hi Fabio
>> 
>> Thanks for confirming.
>> 
>> Although I have no issues with your change, I am curious why even msm 
>> is
>> probing and/or binding.
>> Your device tree should not be having any mdp/dpu nodes then.
> 
> The i.MX53 does have the following GPU node:
> 
> compatible = "amd,imageon-200.0", "amd,imageon";
> 
> That's why it probes the msm driver.
> 
> However, i.MX53 does not have any of the Qualcomm display controllers.
> 
> It uses the i.MX IPU display controller instead.
> 
> Hope that clarifies.
> 
> Please reply with a Reviewed-by if you are happy with my fix.
> 
> Thanks

Re: [Freedreno] [PATCH] drm/msm: Do not run snapshot on non-DPU devices

[abhinavk]

Hi Fabio

Ah, I did not realize that amd compatible is present in the list and its 
quite a surprise.

/*
  * We don't know what's the best binding to link the gpu with the drm 
device.
  * Fow now, we just hunt for all the possible gpus that we support, and 
add them
  * as components.
  */
static const struct of_device_id msm_gpu_match[] = {
	{ .compatible = "qcom,adreno" },
	{ .compatible = "qcom,adreno-3xx" },
	{ .compatible = "amd,imageon" },
	{ .compatible = "qcom,kgsl-3d0" },
	{ },
};

https://github.com/torvalds/linux/commit/e6f6d63ed14c20528aa6df05a8f0707c183c6ba3

For this change itself,
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

On 2021-09-16 09:24, Fabio Estevam wrote:
> Hi Abhinav,
> 
> On Thu, Sep 16, 2021 at 1:15 PM <abhinavk@codeaurora.org> wrote:
>> 
>> Hi Fabio
>> 
>> Thanks for confirming.
>> 
>> Although I have no issues with your change, I am curious why even msm 
>> is
>> probing and/or binding.
>> Your device tree should not be having any mdp/dpu nodes then.
> 
> The i.MX53 does have the following GPU node:
> 
> compatible = "amd,imageon-200.0", "amd,imageon";
> 
> That's why it probes the msm driver.
> 
> However, i.MX53 does not have any of the Qualcomm display controllers.
> 
> It uses the i.MX IPU display controller instead.
> 
> Hope that clarifies.
> 
> Please reply with a Reviewed-by if you are happy with my fix.
> 
> Thanks