diff mbox series

[v3,3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

Message ID 20240106-fd-migrate-mdp5-v3-3-3d2750378063@linaro.org
State Superseded
Headers show
Series drm/msm: provide migration path from MDP5 to DPU driver | expand

Commit Message

Dmitry Baryshkov Jan. 5, 2024, 11:34 p.m. UTC 
For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
possible to support this platform via the DPU driver (e.g. to provide
support for DP, multirect, etc). Add a modparam to be able to switch
between these two drivers.

All platforms supported by both drivers are by default handled by the
MDP5 driver. To let them be handled by the DPU driver pass the
`msm.prefer_mdp5=false` kernel param.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
 drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 4 files changed, 38 insertions(+)

Comments

Carl Vanderlip Jan. 6, 2024, 12:04 a.m. UTC | #1 
On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> possible to support this platform via the DPU driver (e.g. to provide
> support for DP, multirect, etc). Add a modparam to be able to switch
> between these two drivers.
> 
> All platforms supported by both drivers are by default handled by the
> MDP5 driver. To let them be handled by the DPU driver pass the
> `msm.prefer_mdp5=false` kernel param.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
>   drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h            |  1 +
>   4 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa9e0ad33ebb..8f11a98491a1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1276,6 +1276,9 @@ static int dpu_dev_probe(struct platform_device *pdev)
>   	int irq;
>   	int ret = 0;
>   
> +	if (!msm_disp_drv_should_bind(&pdev->dev, true))
> +		return -ENODEV;
> +
>   	dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
>   	if (!dpu_kms)
>   		return -ENOMEM;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 0827634664ae..43d05851c54d 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -866,6 +866,9 @@ static int mdp5_dev_probe(struct platform_device *pdev)
>   
>   	DBG("");
>   
> +	if (!msm_disp_drv_should_bind(&pdev->dev, false))
> +		return -ENODEV;
> +
>   	mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
>   	if (!mdp5_kms)
>   		return -ENOMEM;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 50b65ffc24b1..ef57586fbeca 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>   	return 0;
>   }
>   
> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)

s/mdp5_driver/dpu_driver/

> +{
> +	/* If just a single driver is enabled, use it no matter what */
> +	return true;
> +}

This will cause both MDP/DPU probes to return -ENODEV, rather than
select the enabled one.

> +#else
> +
> +static bool prefer_mdp5 = true;
> +MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
> +module_param(prefer_mdp5, bool, 0444);
> +
> +/* list all platforms supported by both mdp5 and dpu drivers */
> +static const char *const msm_mdp5_dpu_migration[] = {
> +	NULL,
> +};
> +
> +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
> +{
> +	/* If it is not an MDP5 device, do not try MDP5 driver */
> +	if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
> +		return dpu_driver;
> +
> +	/* If it is not in the migration list, use MDP5 */
> +	if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
> +		return !dpu_driver;
> +
> +	return prefer_mdp5 ? !dpu_driver : dpu_driver;
> +}
> +#endif
> +
>   /*
>    * 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
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 01e783130054..762e13e2df74 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -563,5 +563,6 @@ int msm_drv_probe(struct device *dev,
>   	struct msm_kms *kms);
>   void msm_kms_shutdown(struct platform_device *pdev);
>   
> +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
>   
>   #endif /* __MSM_DRV_H__ */
>
Dmitry Baryshkov Jan. 6, 2024, 12:38 a.m. UTC | #2 
On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>
>
> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> > For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> > possible to support this platform via the DPU driver (e.g. to provide
> > support for DP, multirect, etc). Add a modparam to be able to switch
> > between these two drivers.
> >
> > All platforms supported by both drivers are by default handled by the
> > MDP5 driver. To let them be handled by the DPU driver pass the
> > `msm.prefer_mdp5=false` kernel param.
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
> >   drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/msm/msm_drv.h            |  1 +
> >   4 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index aa9e0ad33ebb..8f11a98491a1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1276,6 +1276,9 @@ static int dpu_dev_probe(struct platform_device *pdev)
> >       int irq;
> >       int ret = 0;
> >
> > +     if (!msm_disp_drv_should_bind(&pdev->dev, true))
> > +             return -ENODEV;
> > +
> >       dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
> >       if (!dpu_kms)
> >               return -ENOMEM;
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 0827634664ae..43d05851c54d 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -866,6 +866,9 @@ static int mdp5_dev_probe(struct platform_device *pdev)
> >
> >       DBG("");
> >
> > +     if (!msm_disp_drv_should_bind(&pdev->dev, false))
> > +             return -ENODEV;
> > +
> >       mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
> >       if (!mdp5_kms)
> >               return -ENOMEM;
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 50b65ffc24b1..ef57586fbeca 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
> >       return 0;
> >   }
> >
> > +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> > +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>
> s/mdp5_driver/dpu_driver/

Well, ignored_driver, but your suggestion is better.

>
> > +{
> > +     /* If just a single driver is enabled, use it no matter what */
> > +     return true;
> > +}
>
> This will cause both MDP/DPU probes to return -ENODEV, rather than
> select the enabled one.

No. The code (e.g. for DPU) is:

       if (!msm_disp_drv_should_bind(&pdev->dev, true))
                return -ENODEV;

So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
false. Which is logical from the function name point of view.

>
> > +#else
> > +
> > +static bool prefer_mdp5 = true;
> > +MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
> > +module_param(prefer_mdp5, bool, 0444);
> > +
> > +/* list all platforms supported by both mdp5 and dpu drivers */
> > +static const char *const msm_mdp5_dpu_migration[] = {
> > +     NULL,
> > +};
> > +
> > +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
> > +{
> > +     /* If it is not an MDP5 device, do not try MDP5 driver */
> > +     if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
> > +             return dpu_driver;
> > +
> > +     /* If it is not in the migration list, use MDP5 */
> > +     if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
> > +             return !dpu_driver;
> > +
> > +     return prefer_mdp5 ? !dpu_driver : dpu_driver;
> > +}
> > +#endif
> > +
> >   /*
> >    * 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
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 01e783130054..762e13e2df74 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -563,5 +563,6 @@ int msm_drv_probe(struct device *dev,
> >       struct msm_kms *kms);
> >   void msm_kms_shutdown(struct platform_device *pdev);
> >
> > +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
> >
> >   #endif /* __MSM_DRV_H__ */
> >
Carl Vanderlip Jan. 8, 2024, 5:57 p.m. UTC | #3 
On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
> On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>>
>>
>> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index 50b65ffc24b1..ef57586fbeca 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>>>        return 0;
>>>    }
>>>
>>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
>>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>>> +{
>>> +     /* If just a single driver is enabled, use it no matter what */
>>> +     return true;
>>> +}
>>
>> This will cause both MDP/DPU probes to return -ENODEV, rather than
>> select the enabled one.
> 
> No. The code (e.g. for DPU) is:
> 
>         if (!msm_disp_drv_should_bind(&pdev->dev, true))
>                  return -ENODEV;
> 
> So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
> false. Which is logical from the function name point of view.
> 

but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE() 
case?

at minimum the comment is incorrect since returning true causes the
driver to NOT be used.

-Carl V.
Dmitry Baryshkov Jan. 8, 2024, 7:07 p.m. UTC | #4 
On Mon, 8 Jan 2024 at 19:57, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>
>
>
> On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
> > On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
> >>
> >>
> >> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> >>> index 50b65ffc24b1..ef57586fbeca 100644
> >>> --- a/drivers/gpu/drm/msm/msm_drv.c
> >>> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
> >>>        return 0;
> >>>    }
> >>>
> >>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> >>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
> >>> +{
> >>> +     /* If just a single driver is enabled, use it no matter what */
> >>> +     return true;
> >>> +}
> >>
> >> This will cause both MDP/DPU probes to return -ENODEV, rather than
> >> select the enabled one.
> >
> > No. The code (e.g. for DPU) is:
> >
> >         if (!msm_disp_drv_should_bind(&pdev->dev, true))
> >                  return -ENODEV;
> >
> > So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
> > false. Which is logical from the function name point of view.
> >
>
> but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE()
> case?
>
> at minimum the comment is incorrect since returning true causes the
> driver to NOT be used.

No. Returning _false_ causes the driver to not be used.
Carl Vanderlip Jan. 9, 2024, 7:32 p.m. UTC | #5 
On 1/8/2024 11:07 AM, Dmitry Baryshkov wrote:
> On Mon, 8 Jan 2024 at 19:57, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>> On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
>>> On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>>>> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>>>> index 50b65ffc24b1..ef57586fbeca 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>>>>>         return 0;
>>>>>     }
>>>>>
>>>>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
>>>>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>>>>> +{
>>>>> +     /* If just a single driver is enabled, use it no matter what */
>>>>> +     return true;
>>>>> +}
>>>>
>>>> This will cause both MDP/DPU probes to return -ENODEV, rather than
>>>> select the enabled one.
>>>
>>> No. The code (e.g. for DPU) is:
>>>
>>>          if (!msm_disp_drv_should_bind(&pdev->dev, true))
>>>                   return -ENODEV;
>>>
>>> So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
>>> false. Which is logical from the function name point of view.
>>>
>>
>> but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE()
>> case?
>>
>> at minimum the comment is incorrect since returning true causes the
>> driver to NOT be used.
> 
> No. Returning _false_ causes the driver to not be used.
> 

Apologies... you are correct.

Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>

-Carl V.
Abhinav Kumar Feb. 7, 2024, 7:19 p.m. UTC | #6 
On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> possible to support this platform via the DPU driver (e.g. to provide
> support for DP, multirect, etc). Add a modparam to be able to switch
> between these two drivers.
> 
> All platforms supported by both drivers are by default handled by the
> MDP5 driver. To let them be handled by the DPU driver pass the
> `msm.prefer_mdp5=false` kernel param.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa9e0ad33ebb..8f11a98491a1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1276,6 +1276,9 @@  static int dpu_dev_probe(struct platform_device *pdev)
 	int irq;
 	int ret = 0;
 
+	if (!msm_disp_drv_should_bind(&pdev->dev, true))
+		return -ENODEV;
+
 	dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
 	if (!dpu_kms)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 0827634664ae..43d05851c54d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -866,6 +866,9 @@  static int mdp5_dev_probe(struct platform_device *pdev)
 
 	DBG("");
 
+	if (!msm_disp_drv_should_bind(&pdev->dev, false))
+		return -ENODEV;
+
 	mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
 	if (!mdp5_kms)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 50b65ffc24b1..ef57586fbeca 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -969,6 +969,37 @@  static int add_components_mdp(struct device *master_dev,
 	return 0;
 }
 
+#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
+bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
+{
+	/* If just a single driver is enabled, use it no matter what */
+	return true;
+}
+#else
+
+static bool prefer_mdp5 = true;
+MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
+module_param(prefer_mdp5, bool, 0444);
+
+/* list all platforms supported by both mdp5 and dpu drivers */
+static const char *const msm_mdp5_dpu_migration[] = {
+	NULL,
+};
+
+bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
+{
+	/* If it is not an MDP5 device, do not try MDP5 driver */
+	if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
+		return dpu_driver;
+
+	/* If it is not in the migration list, use MDP5 */
+	if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
+		return !dpu_driver;
+
+	return prefer_mdp5 ? !dpu_driver : dpu_driver;
+}
+#endif
+
 /*
  * 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
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 01e783130054..762e13e2df74 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -563,5 +563,6 @@  int msm_drv_probe(struct device *dev,
 	struct msm_kms *kms);
 void msm_kms_shutdown(struct platform_device *pdev);
 
+bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
 
 #endif /* __MSM_DRV_H__ */