* [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-26 23:11 ` Stephen Boyd
` (2 more replies)
2021-07-25 4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
` (5 subsequent siblings)
6 siblings, 3 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
As the Qualcomm DisplayPort driver only supports a single instance of
the driver the commonly used struct dp_display is kept in a global
variable. As we introduce additional instances this obviously doesn't
work.
Replace this with a combination of existing references to adjacent
objects and drvdata.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
1 file changed, 37 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 70b319a8fe83..8696b36d30e4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
#include "dp_audio.h"
#include "dp_debug.h"
-static struct msm_dp *g_dp_display;
#define HPD_STRING_SIZE 30
enum {
@@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
{}
};
+static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
+{
+ struct msm_dp *dp = dev_get_drvdata(dev);
+
+ return container_of(dp, struct dp_display_private, dp_display);
+}
+
static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
u32 data, u32 delay)
{
@@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
struct drm_device *drm;
struct msm_drm_private *priv;
drm = dev_get_drvdata(master);
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ if (!dp) {
+ DRM_ERROR("DP driver bind failed. Invalid driver data\n");
+ return -EINVAL;
+ }
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
@@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev, struct device *master,
static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ if (!dp) {
+ DRM_ERROR("Invalid DP driver data\n");
+ return;
+ }
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
@@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
static int dp_display_usbpd_configure_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ rc = -ENODEV;
goto end;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_display_host_init(dp, false);
rc = dp_display_process_hpd_high(dp);
@@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ rc = -ENODEV;
return rc;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
return rc;
@@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
+ struct dp_usbpd *hpd;
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- return -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ return -ENODEV;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ hpd = dp->usbpd;
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
/* signal the disconnect event early to ensure proper teardown */
- dp_display_handle_plugged_change(g_dp_display, false);
+ dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
@@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
static int dp_display_enable(struct dp_display_private *dp, u32 data)
{
int rc = 0;
- struct msm_dp *dp_display;
-
- dp_display = g_dp_display;
+ struct msm_dp *dp_display = &dp->dp_display;
if (dp_display->power_on) {
DRM_DEBUG_DP("Link already setup, return\n");
@@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
static int dp_display_disable(struct dp_display_private *dp, u32 data)
{
- struct msm_dp *dp_display;
-
- dp_display = g_dp_display;
+ struct msm_dp *dp_display = &dp->dp_display;
if (!dp_display->power_on)
return 0;
@@ -1229,14 +1229,13 @@ static int dp_display_probe(struct platform_device *pdev)
}
mutex_init(&dp->event_mutex);
- g_dp_display = &dp->dp_display;
/* Store DP audio handle inside DP display */
- g_dp_display->dp_audio = dp->audio;
+ dp->dp_display.dp_audio = dp->audio;
init_completion(&dp->audio_comp);
- platform_set_drvdata(pdev, g_dp_display);
+ platform_set_drvdata(pdev, &dp->dp_display);
rc = component_add(&pdev->dev, &dp_display_comp_ops);
if (rc) {
@@ -1249,10 +1248,7 @@ static int dp_display_probe(struct platform_device *pdev)
static int dp_display_remove(struct platform_device *pdev)
{
- struct dp_display_private *dp;
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = platform_get_drvdata(pdev);
dp_display_deinit_sub_modules(dp);
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
2021-07-25 4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
@ 2021-07-26 23:11 ` Stephen Boyd
2021-07-27 18:15 ` Dmitry Baryshkov
2021-07-30 23:37 ` [Freedreno] " abhinavk
2 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:11 UTC (permalink / raw)
To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
Sean Paul
Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Bjorn Andersson (2021-07-24 21:24:31)
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Thanks for removing the global.
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
How can it be NULL? dev_to_dp_display_private() returns container_of()
pointer so it doesn't look possible.
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
2021-07-25 4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-07-26 23:11 ` Stephen Boyd
@ 2021-07-27 18:15 ` Dmitry Baryshkov
2021-07-30 23:37 ` [Freedreno] " abhinavk
2 siblings, 0 replies; 16+ messages in thread
From: Dmitry Baryshkov @ 2021-07-27 18:15 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, David Airlie,
Daniel Vetter, Abhinav Kumar, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
On 25/07/2021 07:24, Bjorn Andersson wrote:
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
dev_get_dp_display_private() ?
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
As a matter of preference, it might be cleaner to inline dev_get_drvdata
and then define msm_dp_get_private to convert from msm_dp to
dp_display_private, see below.
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
This is not correct, if I'm not mistaken. you wanted to check if dev's
private data is NULL (correct check), but ended up checking whether the
result of container_of is NULL (incorrect).
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
> @@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev, struct device *master,
> static void dp_display_unbind(struct device *dev, struct device *master,
> void *data)
> {
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("Invalid DP driver data\n");
> + return;
> + }
>
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
> static int dp_display_usbpd_configure_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> goto end;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_display_host_init(dp, false);
>
> rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
> static int dp_display_usbpd_disconnect_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
`!dev` check should remain in place. And dp should be fetched
afterwards. This applies to all the checks in the patch.
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> return rc;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>
> return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
> {
> int rc = 0;
> u32 sink_request;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> + struct dp_usbpd *hpd;
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - return -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + return -ENODEV;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + hpd = dp->usbpd;
>
> /* check for any test request issued by sink */
> rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>
> /* signal the disconnect event early to ensure proper teardown */
> - dp_display_handle_plugged_change(g_dp_display, false);
> + dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> @@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
> static int dp_display_enable(struct dp_display_private *dp, u32 data)
> {
> int rc = 0;
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (dp_display->power_on) {
> DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
>
> static int dp_display_disable(struct dp_display_private *dp, u32 data)
> {
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (!dp_display->power_on)
> return 0;
> @@ -1229,14 +1229,13 @@ static int dp_display_probe(struct platform_device *pdev)
> }
>
> mutex_init(&dp->event_mutex);
> - g_dp_display = &dp->dp_display;
>
> /* Store DP audio handle inside DP display */
> - g_dp_display->dp_audio = dp->audio;
> + dp->dp_display.dp_audio = dp->audio;
>
> init_completion(&dp->audio_comp);
>
> - platform_set_drvdata(pdev, g_dp_display);
> + platform_set_drvdata(pdev, &dp->dp_display);
>
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> @@ -1249,10 +1248,7 @@ static int dp_display_probe(struct platform_device *pdev)
>
> static int dp_display_remove(struct platform_device *pdev)
> {
> - struct dp_display_private *dp;
> -
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + struct dp_display_private *dp = platform_get_drvdata(pdev);
dev_to_dp_display_private() rather than just get_drvdata?
>
> dp_display_deinit_sub_modules(dp);
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Freedreno] [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
2021-07-25 4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-07-26 23:11 ` Stephen Boyd
2021-07-27 18:15 ` Dmitry Baryshkov
@ 2021-07-30 23:37 ` abhinavk
2 siblings, 0 replies; 16+ messages in thread
From: abhinavk @ 2021-07-30 23:37 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, linux-arm-msm,
linux-kernel, dri-devel, Stephen Boyd, Rob Herring, freedreno
On 2021-07-24 21:24, Bjorn Andersson wrote:
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct
> device *dev)
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
> @@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
> static void dp_display_unbind(struct device *dev, struct device
> *master,
> void *data)
> {
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("Invalid DP driver data\n");
> + return;
> + }
>
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct
> dp_display_private *dp)
> static int dp_display_usbpd_configure_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> goto end;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_display_host_init(dp, false);
>
> rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct
> device *dev)
> static int dp_display_usbpd_disconnect_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> return rc;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>
> return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct
> device *dev)
> {
> int rc = 0;
> u32 sink_request;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> + struct dp_usbpd *hpd;
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - return -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + return -ENODEV;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + hpd = dp->usbpd;
hpd is unused here. It was removed with
https://patches.linaro.org/patch/416670/
>
> /* check for any test request issued by sink */
> rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct
> dp_display_private *dp, u32 data)
> dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
> DP_TIMEOUT_5_SECOND);
>
> /* signal the disconnect event early to ensure proper teardown */
> - dp_display_handle_plugged_change(g_dp_display, false);
> + dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK,
> true);
> @@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
> static int dp_display_enable(struct dp_display_private *dp, u32 data)
> {
> int rc = 0;
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (dp_display->power_on) {
> DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp
> *dp_display)
>
> static int dp_display_disable(struct dp_display_private *dp, u32 data)
> {
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (!dp_display->power_on)
> return 0;
> @@ -1229,14 +1229,13 @@ static int dp_display_probe(struct
> platform_device *pdev)
> }
>
> mutex_init(&dp->event_mutex);
> - g_dp_display = &dp->dp_display;
>
> /* Store DP audio handle inside DP display */
> - g_dp_display->dp_audio = dp->audio;
> + dp->dp_display.dp_audio = dp->audio;
>
> init_completion(&dp->audio_comp);
>
> - platform_set_drvdata(pdev, g_dp_display);
> + platform_set_drvdata(pdev, &dp->dp_display);
>
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> @@ -1249,10 +1248,7 @@ static int dp_display_probe(struct
> platform_device *pdev)
>
> static int dp_display_remove(struct platform_device *pdev)
> {
> - struct dp_display_private *dp;
> -
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + struct dp_display_private *dp = platform_get_drvdata(pdev);
>
> dp_display_deinit_sub_modules(dp);
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
2021-07-25 4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-26 23:55 ` Stephen Boyd
2021-07-27 18:42 ` Dmitry Baryshkov
2021-07-25 4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
` (4 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
Functions in the DisplayPort code that relates to individual instances
(encoders) are passed both the struct msm_dp and the struct drm_encoder. But
in a situation where multiple DP instances would exist this means that
the caller need to resolve which struct msm_dp relates to the struct
drm_encoder at hand.
The information for doing this lookup is available inside the DP driver,
so update the API to take the struct msm_drm_private and the struct
drm_encoder and have the DP code figure out which struct msm_dp the
operation relates to.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++----
drivers/gpu/drm/msm/msm_drv.h | 31 +++++++++--------
3 files changed, 56 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1c04b7cce43e..0d64ef0819af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1002,8 +1002,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
- msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+ msm_dp_display_mode_set(priv, drm_enc, mode, adj_mode);
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
@@ -1184,9 +1184,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- ret = msm_dp_display_enable(priv->dp,
- drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ ret = msm_dp_display_enable(priv, drm_enc);
if (ret) {
DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
ret);
@@ -1226,8 +1225,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_pre_disable(priv, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
}
@@ -1255,8 +1254,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_disable(priv, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 8696b36d30e4..59ffd6c8f41f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
return 0;
}
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
+static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
+{
+ if (priv->dp && priv->dp->encoder == encoder)
+ return priv->dp;
+
+ return NULL;
+}
+
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
int rc = 0;
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
u32 state;
+ if (!dp)
+ return -EINVAL;
+
dp_display = container_of(dp, struct dp_display_private, dp_display);
if (!dp_display->dp_mode.drm_mode.clock) {
DRM_ERROR("invalid params\n");
@@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return 0;
dp_display = container_of(dp, struct dp_display_private, dp_display);
@@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
return 0;
}
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
int rc = 0;
u32 state;
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return 0;
dp_display = container_of(dp, struct dp_display_private, dp_display);
@@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return;
dp_display = container_of(dp, struct dp_display_private, dp_display);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9bfd37855969..e9232032b266 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -388,12 +388,13 @@ int __init msm_dp_register(void);
void __exit msm_dp_unregister(void);
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder);
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode);
void msm_dp_irq_postinstall(struct msm_dp *dp_display);
void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
@@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
{
return -EINVAL;
}
-static inline int msm_dp_display_enable(struct msm_dp *dp,
+static inline int msm_dp_display_enable(struct msm_drm_private *priv,
struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline int msm_dp_display_disable(struct msm_dp *dp,
- struct drm_encoder *encoder)
+static inline int msm_dp_display_disable(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
- struct drm_encoder *encoder)
+static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline void msm_dp_display_mode_set(struct msm_dp *dp,
- struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
}
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
2021-07-25 4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
@ 2021-07-26 23:55 ` Stephen Boyd
2021-07-27 18:42 ` Dmitry Baryshkov
1 sibling, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:55 UTC (permalink / raw)
To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
Sean Paul
Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Bjorn Andersson (2021-07-24 21:24:32)
> Functions in the DisplayPort code that relates to individual instances
> (encoders) are passed both the struct msm_dp and the struct drm_encoder. But
> in a situation where multiple DP instances would exist this means that
> the caller need to resolve which struct msm_dp relates to the struct
> drm_encoder at hand.
>
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
2021-07-25 4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
2021-07-26 23:55 ` Stephen Boyd
@ 2021-07-27 18:42 ` Dmitry Baryshkov
1 sibling, 0 replies; 16+ messages in thread
From: Dmitry Baryshkov @ 2021-07-27 18:42 UTC (permalink / raw)
To: Bjorn Andersson, Rob Clark, Sean Paul, David Airlie,
Daniel Vetter, Abhinav Kumar, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
On 25/07/2021 07:24, Bjorn Andersson wrote:
> Functions in the DisplayPort code that relates to individual instances
> (encoders) are passed both the struct msm_dp and the struct drm_encoder. But
> in a situation where multiple DP instances would exist this means that
> the caller need to resolve which struct msm_dp relates to the struct
> drm_encoder at hand.
>
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.
Initially I thought to propose moving encoder->dp lookup into dpu code
by adding msm_dp_display_get_encoder() function. However as I was
writing that, I remembered that at some point I had to refactor my own
patchset in the way to get rid of calling msm_FOO_get_encoder().
I'd propose simpler solution. In dpu_encoder_setup() you have the DP
index and the encoder. So you can store valid msm_dp pointer in the
dpu_encoder_virt and remove all the lookups. Then you can replace all
priv->dp with bare dpu_enc->dp accesses. Will this work for you?
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
> drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++----
> drivers/gpu/drm/msm/msm_drv.h | 31 +++++++++--------
> 3 files changed, 56 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1c04b7cce43e..0d64ef0819af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1002,8 +1002,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>
> trace_dpu_enc_mode_set(DRMID(drm_enc));
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
> - msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
> + msm_dp_display_mode_set(priv, drm_enc, mode, adj_mode);
>
> list_for_each_entry(conn_iter, connector_list, head)
> if (conn_iter->encoder == drm_enc)
> @@ -1184,9 +1184,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>
> _dpu_encoder_virt_enable_helper(drm_enc);
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - ret = msm_dp_display_enable(priv->dp,
> - drm_enc);
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + ret = msm_dp_display_enable(priv, drm_enc);
> if (ret) {
> DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
> ret);
> @@ -1226,8 +1225,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> /* wait for idle */
> dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - if (msm_dp_display_pre_disable(priv->dp, drm_enc))
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + if (msm_dp_display_pre_disable(priv, drm_enc))
> DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
> }
>
> @@ -1255,8 +1254,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - if (msm_dp_display_disable(priv->dp, drm_enc))
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + if (msm_dp_display_disable(priv, drm_enc))
> DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8696b36d30e4..59ffd6c8f41f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> return 0;
> }
>
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> +static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> +{
> + if (priv->dp && priv->dp->encoder == encoder)
> + return priv->dp;
> +
> + return NULL;
> +}
> +
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> int rc = 0;
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> u32 state;
>
> + if (!dp)
> + return -EINVAL;
> +
> dp_display = container_of(dp, struct dp_display_private, dp_display);
> if (!dp_display->dp_mode.drm_mode.clock) {
> DRM_ERROR("invalid params\n");
> @@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return 0;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> @@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> return 0;
> }
>
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> int rc = 0;
> u32 state;
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return 0;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> @@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9bfd37855969..e9232032b266 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -388,12 +388,13 @@ int __init msm_dp_register(void);
> void __exit msm_dp_unregister(void);
> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder);
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode);
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode);
> void msm_dp_irq_postinstall(struct msm_dp *dp_display);
> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>
> @@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_enable(struct msm_dp *dp,
> +static inline int msm_dp_display_enable(struct msm_drm_private *priv,
> struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_disable(struct msm_dp *dp,
> - struct drm_encoder *encoder)
> +static inline int msm_dp_display_disable(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
> - struct drm_encoder *encoder)
> +static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline void msm_dp_display_mode_set(struct msm_dp *dp,
> - struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> }
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
2021-07-25 4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-07-25 4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-26 23:51 ` Stephen Boyd
2021-07-25 4:24 ` [PATCH 4/4] drm/msm/dp: Add sc8180x " Bjorn Andersson
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
Based on the removal of the g_dp_display and the movement of the
priv->dp lookup into the DP code it's now possible to have multiple
DP instances.
In line with the other controllers in the MSM driver, introduce a
per-compatible list of base addresses which is used to resolve the
"instance id" for the given DP controller. This instance id is used as
index in the priv->dp[] array.
Then extend the initialization code to initialize struct drm_encoder for
each of the registered priv->dp[] and update the logic for finding a
struct msm_dp from a struct drm_encoder.
Lastly, bump the number of struct msm_dp instances carries by priv->dp
to 3, the currently known maximum number of controllers found in a
Qualcomm SoC.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++++++++++--------
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 59 ++++++++++++++++--
drivers/gpu/drm/msm/msm_drv.h | 2 +-
4 files changed, 95 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f655adbc2421..a793cc8a007e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -188,6 +188,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
struct dentry *entry;
struct drm_device *dev;
struct msm_drm_private *priv;
+ int i;
if (!p)
return -EINVAL;
@@ -203,8 +204,8 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
dpu_debugfs_vbif_init(dpu_kms, entry);
dpu_debugfs_core_irq_init(dpu_kms, entry);
- if (priv->dp)
- msm_dp_debugfs_init(priv->dp, minor);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
+ msm_dp_debugfs_init(priv->dp[i], minor);
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -545,33 +546,40 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
int rc = 0;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
- if (IS_ERR(encoder)) {
- DPU_ERROR("encoder init failed for dsi display\n");
- return PTR_ERR(encoder);
- }
+ encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+ if (IS_ERR(encoder)) {
+ DPU_ERROR("encoder init failed for dsi display\n");
+ return PTR_ERR(encoder);
+ }
- memset(&info, 0, sizeof(info));
- rc = msm_dp_modeset_init(priv->dp, dev, encoder);
- if (rc) {
- DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
- drm_encoder_cleanup(encoder);
- return rc;
- }
+ memset(&info, 0, sizeof(info));
+ rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
+ if (rc) {
+ DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
+ drm_encoder_cleanup(encoder);
+ return rc;
+ }
- priv->encoders[priv->num_encoders++] = encoder;
+ priv->encoders[priv->num_encoders++] = encoder;
+
+ info.num_of_h_tiles = 1;
+ info.h_tile_instance[0] = i;
+ info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
+ info.intf_type = encoder->encoder_type;
+ rc = dpu_encoder_setup(dev, encoder, &info);
+ if (rc) {
+ DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+ encoder->base.id, rc);
+ return rc;
+ }
+ }
- info.num_of_h_tiles = 1;
- info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
- info.intf_type = encoder->encoder_type;
- rc = dpu_encoder_setup(dev, encoder, &info);
- if (rc)
- DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
- encoder->base.id, rc);
return rc;
}
@@ -792,6 +800,7 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
{
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+ int i;
if (!dpu_kms || !dpu_kms->dev)
return -EINVAL;
@@ -800,7 +809,8 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
if (!priv)
return -EINVAL;
- msm_dp_irq_postinstall(priv->dp);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
+ msm_dp_irq_postinstall(priv->dp[i]);
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index cabe15190ec1..2e1acb1bc390 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -126,8 +126,12 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
priv = drm_dev->dev_private;
kms = priv->kms;
- if (priv->dp)
- msm_dp_snapshot(disp_state, priv->dp);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
+
+ msm_dp_snapshot(disp_state, priv->dp[i]);
+ }
for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
if (!priv->dsi[i])
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 59ffd6c8f41f..92b7646a1bb7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -79,6 +79,8 @@ struct dp_display_private {
char *name;
int irq;
+ int id;
+
/* state variables */
bool core_initialized;
bool hpd_irq_on;
@@ -116,8 +118,19 @@ struct dp_display_private {
struct dp_audio *audio;
};
+
+struct msm_dp_config {
+ phys_addr_t io_start[3];
+ size_t num_dp;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+ .io_start = { 0x0ae90000 },
+ .num_dp = 1,
+};
+
static const struct of_device_id dp_dt_match[] = {
- {.compatible = "qcom,sc7180-dp"},
+ { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
{}
};
@@ -217,7 +230,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
- priv->dp = &(dp->dp_display);
+ priv->dp[dp->id] = &(dp->dp_display);
rc = dp->parser->parse(dp->parser);
if (rc) {
@@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
}
rc = dp_register_audio_driver(dev, dp->audio);
- if (rc)
+ if (rc) {
DRM_ERROR("Audio registration Dp failed\n");
+ goto end;
+ }
+
end:
return rc;
@@ -259,7 +275,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
- priv->dp = NULL;
+ priv->dp[dp->id] = NULL;
}
static const struct component_ops dp_display_comp_ops = {
@@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
return 0;
}
+static int dp_display_get_id(struct platform_device *pdev)
+{
+ const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
+ struct resource *res;
+ int i;
+
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ for (i = 0; i < cfg->num_dp; i++) {
+ if (cfg->io_start[i] == res->start)
+ return i;
+ }
+
+ dev_err(&pdev->dev, "unknown displayport instance\n");
+ return -EINVAL;
+}
+
static int dp_display_probe(struct platform_device *pdev)
{
int rc = 0;
@@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
if (!dp)
return -ENOMEM;
+ dp->id = dp_display_get_id(pdev);
+ if (dp->id < 0)
+ return -EINVAL;
+
dp->pdev = pdev;
dp->name = "drm_dp";
@@ -1386,6 +1426,9 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
struct device *dev;
int rc;
+ if (!dp_display)
+ return;
+
dp = container_of(dp_display, struct dp_display_private, dp_display);
dev = &dp->pdev->dev;
@@ -1435,8 +1478,12 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
struct drm_encoder *encoder)
{
- if (priv->dp && priv->dp->encoder == encoder)
- return priv->dp;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (priv->dp[i] && priv->dp[i]->encoder == encoder)
+ return priv->dp[i];
+ }
return NULL;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e9232032b266..62d54ef6c2c4 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -161,7 +161,7 @@ struct msm_drm_private {
/* DSI is shared by mdp4 and mdp5 */
struct msm_dsi *dsi[2];
- struct msm_dp *dp;
+ struct msm_dp *dp[3];
/* when we have more than one 'msm_gpu' these need to be an array: */
struct msm_gpu *gpu;
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers
2021-07-25 4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
@ 2021-07-26 23:51 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:51 UTC (permalink / raw)
To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
Sean Paul
Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Bjorn Andersson (2021-07-24 21:24:33)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59ffd6c8f41f..92b7646a1bb7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
> }
>
> rc = dp_register_audio_driver(dev, dp->audio);
> - if (rc)
> + if (rc) {
> DRM_ERROR("Audio registration Dp failed\n");
> + goto end;
> + }
> +
>
> end:
> return rc;
This hunk looks useless. Drop it?
> @@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> return 0;
> }
>
> +static int dp_display_get_id(struct platform_device *pdev)
> +{
> + const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> + struct resource *res;
> + int i;
> +
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + for (i = 0; i < cfg->num_dp; i++) {
> + if (cfg->io_start[i] == res->start)
> + return i;
> + }
> +
> + dev_err(&pdev->dev, "unknown displayport instance\n");
> + return -EINVAL;
> +}
> +
> static int dp_display_probe(struct platform_device *pdev)
> {
> int rc = 0;
> @@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
> if (!dp)
> return -ENOMEM;
>
> + dp->id = dp_display_get_id(pdev);
> + if (dp->id < 0)
> + return -EINVAL;
> +
> dp->pdev = pdev;
> dp->name = "drm_dp";
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index e9232032b266..62d54ef6c2c4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -161,7 +161,7 @@ struct msm_drm_private {
> /* DSI is shared by mdp4 and mdp5 */
> struct msm_dsi *dsi[2];
>
> - struct msm_dp *dp;
> + struct msm_dp *dp[3];
It would be nice to either make this dynamically sized (probably little
gain), somehow make a BUILD_BUG_ON(), or have a WARN_ON if
ARRAY_SIZE(dp) is less than a num_dp so we know somebody messed up.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/4] drm/msm/dp: Add sc8180x DP controllers
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
` (2 preceding siblings ...)
2021-07-25 4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-25 4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_dp = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae90000, 0xae98000, 0 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+ .io_start = { 0, 0, 0xae9a000 },
+ .num_dp = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
{}
};
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
` (3 preceding siblings ...)
2021-07-25 4:24 ` [PATCH 4/4] drm/msm/dp: Add sc8180x " Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-26 23:52 ` Stephen Boyd
2021-07-25 4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
2021-07-31 1:21 ` [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x abhinavk
6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
compatibles for these to the msm/dp binding.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
.../devicetree/bindings/display/msm/dp-controller.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index a6e41be038fc..c6056e0b0845 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -17,6 +17,8 @@ properties:
compatible:
enum:
- qcom,sc7180-dp
+ - qcom,sc8180x-dp
+ - qcom,sc8180x-edp
reg:
items:
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
2021-07-25 4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
@ 2021-07-26 23:52 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:52 UTC (permalink / raw)
To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
Sean Paul
Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Bjorn Andersson (2021-07-24 21:24:35)
> The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
> compatibles for these to the msm/dp binding.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
` (4 preceding siblings ...)
2021-07-25 4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
@ 2021-07-25 4:24 ` Bjorn Andersson
2021-07-26 23:54 ` Stephen Boyd
2021-07-31 1:21 ` [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x abhinavk
6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25 4:24 UTC (permalink / raw)
To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
linux-kernel
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_dp = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae90000, 0xae98000, 0 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+ .io_start = { 0, 0, 0xae9a000 },
+ .num_dp = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
{}
};
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers
2021-07-25 4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
@ 2021-07-26 23:54 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:54 UTC (permalink / raw)
To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
Sean Paul
Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Bjorn Andersson (2021-07-24 21:24:36)
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
> DP driver.
>
> Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 92b7646a1bb7..c26805cfcdd1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> .num_dp = 1,
> };
>
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> + .io_start = { 0xae90000, 0xae98000, 0 },
> + .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> + .io_start = { 0, 0, 0xae9a000 },
> + .num_dp = 3,
> +};
Can the two structs not be combined into one struct and set as .data for
either compatible?
> +
> static const struct of_device_id dp_dt_match[] = {
> { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
> + { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
> + { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
> {}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x
2021-07-25 4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
` (5 preceding siblings ...)
2021-07-25 4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
@ 2021-07-31 1:21 ` abhinavk
6 siblings, 0 replies; 16+ messages in thread
From: abhinavk @ 2021-07-31 1:21 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Herring,
Stephen Boyd, linux-arm-msm, dri-devel, freedreno, linux-kernel
Hi Bjorn
On 2021-07-24 21:24, Bjorn Andersson wrote:
> The current implementation supports a single DP instance and the DPU
> code will
> only match it against INTF_DP instance 0. These patches extends this to
> allow
> multiple DP instances and support for matching against DP instances
> beyond 0.
>
> This is based on v4 of Dmitry's work on multiple DSI interfaces:
> https://lore.kernel.org/linux-arm-msm/20210717124016.316020-1-dmitry.baryshkov@linaro.org/
>
> With that in place add SC8180x DP and eDP controllers.
Thanks for posting the changes.
I dont have major concerns on the series as such apart from minor
comments which i will post in a day or two
but I will check and get back if this has been validated on sc7280
without any concerns.
One question i had is not directly related to this series but related to
multi-DP in general.
Does audio work fine across both the DPs when both are connected?
The reason I ask this question is that, I dont know how two hdmi-codec
instances are handled today.
So we will register twice with hdmi-codec so there should be two audio
streams.
But I am not sure if this works correctly in todays design with
hdmi-codec.
Any chance you had validated this?
>
> Bjorn Andersson (5):
> drm/msm/dp: Remove global g_dp_display variable
> drm/msm/dp: Modify prototype of encoder based API
> drm/msm/dp: Support up to 3 DP controllers
> dt-bindings: msm/dp: Add SC8180x compatibles
> drm/msm/dp: Add sc8180x DP controllers
>
> .../bindings/display/msm/dp-controller.yaml | 2 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++---
> .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 +-
> drivers/gpu/drm/msm/dp/dp_display.c | 183 +++++++++++++-----
> drivers/gpu/drm/msm/msm_drv.h | 33 ++--
> 6 files changed, 200 insertions(+), 103 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread