[PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

This patchseries replaces drm_handle_vblank() with
drm_crtc_handle_vblank(). As a bonus result of this conversion it is
possible to drop the stored array of allocated CRTCs and use the core
CRTC iterators.

Changes since v5:
 - Clean up the event_thread->worker in case of an error to fix possible
   oops in msm_drm_uninit().

Changes since v4:
 - Removed the duplicate word 'duplicate' from the last patch'es commit
   message (noticed by Abhinav).

Changes since v3:
 - In msm_drm_init simplify the code by using an interim var for the
   event thread itself rather than just the index (suggested by Abhinav)

Changes since v2;
 - none (sent by mistake)

Changes since v1;
 - fixed uninitialized var access (LTP Robot)


Dmitry Baryshkov (4):
  drm/msm: clean event_thread->worker in case of an error
  drm/msm/mdp4: convert to drm_crtc_handle_vblank()
  drm/msm/mdp5: convert to drm_crtc_handle_vblank()
  drm/msm: stop storing the array of CRTCs in struct msm_drm_private

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c |  9 +++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c |  9 +++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.h            |  3 +-
 7 files changed, 35 insertions(+), 36 deletions(-)

-- 
2.35.1

[PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

This patchseries replaces drm_handle_vblank() with
drm_crtc_handle_vblank(). As a bonus result of this conversion it is
possible to drop the stored array of allocated CRTCs and use the core
CRTC iterators.

Changes since v5:
 - Clean up the event_thread->worker in case of an error to fix possible
   oops in msm_drm_uninit().

Changes since v4:
 - Removed the duplicate word 'duplicate' from the last patch'es commit
   message (noticed by Abhinav).

Changes since v3:
 - In msm_drm_init simplify the code by using an interim var for the
   event thread itself rather than just the index (suggested by Abhinav)

Changes since v2;
 - none (sent by mistake)

Changes since v1;
 - fixed uninitialized var access (LTP Robot)


Dmitry Baryshkov (4):
  drm/msm: clean event_thread->worker in case of an error
  drm/msm/mdp4: convert to drm_crtc_handle_vblank()
  drm/msm/mdp5: convert to drm_crtc_handle_vblank()
  drm/msm: stop storing the array of CRTCs in struct msm_drm_private

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c |  9 +++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c |  9 +++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.h            |  3 +-
 7 files changed, 35 insertions(+), 36 deletions(-)

-- 
2.35.1

[PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Dmitry Baryshkov]

If worker creation fails, nullify the event_thread->worker, so that
msm_drm_uninit() doesn't try accessing invalid memory location. While we
are at it, remove duplicate assignment to the ret variable.

Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of kthread_run")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 44485363f37a..1aab6bf86278 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 		if (IS_ERR(priv->event_thread[i].worker)) {
 			ret = PTR_ERR(priv->event_thread[i].worker);
 			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
-			ret = PTR_ERR(priv->event_thread[i].worker);
+			priv->event_thread[i].worker = NULL;
 			goto err_msm_uninit;
 		}
 
-- 
2.35.1

[PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Dmitry Baryshkov]

If worker creation fails, nullify the event_thread->worker, so that
msm_drm_uninit() doesn't try accessing invalid memory location. While we
are at it, remove duplicate assignment to the ret variable.

Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of kthread_run")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 44485363f37a..1aab6bf86278 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 		if (IS_ERR(priv->event_thread[i].worker)) {
 			ret = PTR_ERR(priv->event_thread[i].worker);
 			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
-			ret = PTR_ERR(priv->event_thread[i].worker);
+			priv->event_thread[i].worker = NULL;
 			goto err_msm_uninit;
 		}
 
-- 
2.35.1

Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Abhinav Kumar]

On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> If worker creation fails, nullify the event_thread->worker, so that
> msm_drm_uninit() doesn't try accessing invalid memory location. While we
> are at it, remove duplicate assignment to the ret variable.
> 
> Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of kthread_run")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

the change itself LGTM,

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

One minor nit below
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 44485363f37a..1aab6bf86278 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   		if (IS_ERR(priv->event_thread[i].worker)) {
>   			ret = PTR_ERR(priv->event_thread[i].worker);
>   			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");

Can we print ret in this error message?

> -			ret = PTR_ERR(priv->event_thread[i].worker);
> +			priv->event_thread[i].worker = NULL;
>   			goto err_msm_uninit;
>   		}
>   

Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Abhinav Kumar]

On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> If worker creation fails, nullify the event_thread->worker, so that
> msm_drm_uninit() doesn't try accessing invalid memory location. While we
> are at it, remove duplicate assignment to the ret variable.
> 
> Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of kthread_run")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

the change itself LGTM,

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

One minor nit below
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 44485363f37a..1aab6bf86278 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   		if (IS_ERR(priv->event_thread[i].worker)) {
>   			ret = PTR_ERR(priv->event_thread[i].worker);
>   			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");

Can we print ret in this error message?

> -			ret = PTR_ERR(priv->event_thread[i].worker);
> +			priv->event_thread[i].worker = NULL;
>   			goto err_msm_uninit;
>   		}
>   

Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Dmitry Baryshkov]

On 08/09/2022 03:08, Abhinav Kumar wrote:
> 
> 
> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
>> If worker creation fails, nullify the event_thread->worker, so that
>> msm_drm_uninit() doesn't try accessing invalid memory location. While we
>> are at it, remove duplicate assignment to the ret variable.
>>
>> Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of 
>> kthread_run")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> the change itself LGTM,
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> One minor nit below
>> ---
>>   drivers/gpu/drm/msm/msm_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index 44485363f37a..1aab6bf86278 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const 
>> struct drm_driver *drv)
>>           if (IS_ERR(priv->event_thread[i].worker)) {
>>               ret = PTR_ERR(priv->event_thread[i].worker);
>>               DRM_DEV_ERROR(dev, "failed to create crtc_event 
>> kthread\n");
> 
> Can we print ret in this error message?

In a separate change. I'll add it to my todo list.

> 
>> -            ret = PTR_ERR(priv->event_thread[i].worker);
>> +            priv->event_thread[i].worker = NULL;
>>               goto err_msm_uninit;
>>           }

-- 
With best wishes
Dmitry

Re: [PATCH v6 1/4] drm/msm: clean event_thread->worker in case of an error

[Dmitry Baryshkov]

On 08/09/2022 03:08, Abhinav Kumar wrote:
> 
> 
> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
>> If worker creation fails, nullify the event_thread->worker, so that
>> msm_drm_uninit() doesn't try accessing invalid memory location. While we
>> are at it, remove duplicate assignment to the ret variable.
>>
>> Fixes: 1041dee2178f ("drm/msm: use kthread_create_worker instead of 
>> kthread_run")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> the change itself LGTM,
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> One minor nit below
>> ---
>>   drivers/gpu/drm/msm/msm_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c 
>> b/drivers/gpu/drm/msm/msm_drv.c
>> index 44485363f37a..1aab6bf86278 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -436,7 +436,7 @@ static int msm_drm_init(struct device *dev, const 
>> struct drm_driver *drv)
>>           if (IS_ERR(priv->event_thread[i].worker)) {
>>               ret = PTR_ERR(priv->event_thread[i].worker);
>>               DRM_DEV_ERROR(dev, "failed to create crtc_event 
>> kthread\n");
> 
> Can we print ret in this error message?

In a separate change. I'll add it to my todo list.

> 
>> -            ret = PTR_ERR(priv->event_thread[i].worker);
>> +            priv->event_thread[i].worker = NULL;
>>               goto err_msm_uninit;
>>           }

-- 
With best wishes
Dmitry

[PATCH v6 2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

Stop using deprecated drm_handle_vblank(), use drm_crtc_handle_vblank()
instead.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
index 4d49f3ba6a96..ddcdd5e87853 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
@@ -69,8 +69,7 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
 	struct drm_device *dev = mdp4_kms->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	unsigned int id;
+	struct drm_crtc *crtc;
 	uint32_t status, enable;
 
 	enable = mdp4_read(mdp4_kms, REG_MDP4_INTR_ENABLE);
@@ -81,9 +80,9 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
 
 	mdp_dispatch_irqs(mdp_kms, status);
 
-	for (id = 0; id < priv->num_crtcs; id++)
-		if (status & mdp4_crtc_vblank(priv->crtcs[id]))
-			drm_handle_vblank(dev, id);
+	drm_for_each_crtc(crtc, dev)
+		if (status & mdp4_crtc_vblank(crtc))
+			drm_crtc_handle_vblank(crtc);
 
 	return IRQ_HANDLED;
 }
-- 
2.35.1

[PATCH v6 2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

Stop using deprecated drm_handle_vblank(), use drm_crtc_handle_vblank()
instead.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
index 4d49f3ba6a96..ddcdd5e87853 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_irq.c
@@ -69,8 +69,7 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(mdp_kms);
 	struct drm_device *dev = mdp4_kms->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	unsigned int id;
+	struct drm_crtc *crtc;
 	uint32_t status, enable;
 
 	enable = mdp4_read(mdp4_kms, REG_MDP4_INTR_ENABLE);
@@ -81,9 +80,9 @@ irqreturn_t mdp4_irq(struct msm_kms *kms)
 
 	mdp_dispatch_irqs(mdp_kms, status);
 
-	for (id = 0; id < priv->num_crtcs; id++)
-		if (status & mdp4_crtc_vblank(priv->crtcs[id]))
-			drm_handle_vblank(dev, id);
+	drm_for_each_crtc(crtc, dev)
+		if (status & mdp4_crtc_vblank(crtc))
+			drm_crtc_handle_vblank(crtc);
 
 	return IRQ_HANDLED;
 }
-- 
2.35.1

[PATCH v6 3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

Stop using deprecated drm_handle_vblank(), use drm_crtc_handle_vblank()
instead.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
index 9b4c8d92ff32..43443a435d59 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
@@ -82,8 +82,7 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
 	struct drm_device *dev = mdp5_kms->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	unsigned int id;
+	struct drm_crtc *crtc;
 	uint32_t status, enable;
 
 	enable = mdp5_read(mdp5_kms, REG_MDP5_INTR_EN);
@@ -94,9 +93,9 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
 
 	mdp_dispatch_irqs(mdp_kms, status);
 
-	for (id = 0; id < priv->num_crtcs; id++)
-		if (status & mdp5_crtc_vblank(priv->crtcs[id]))
-			drm_handle_vblank(dev, id);
+	drm_for_each_crtc(crtc, dev)
+		if (status & mdp5_crtc_vblank(crtc))
+			drm_crtc_handle_vblank(crtc);
 
 	return IRQ_HANDLED;
 }
-- 
2.35.1

[PATCH v6 3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

Stop using deprecated drm_handle_vblank(), use drm_crtc_handle_vblank()
instead.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
index 9b4c8d92ff32..43443a435d59 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_irq.c
@@ -82,8 +82,7 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
 	struct mdp_kms *mdp_kms = to_mdp_kms(kms);
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
 	struct drm_device *dev = mdp5_kms->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	unsigned int id;
+	struct drm_crtc *crtc;
 	uint32_t status, enable;
 
 	enable = mdp5_read(mdp5_kms, REG_MDP5_INTR_EN);
@@ -94,9 +93,9 @@ irqreturn_t mdp5_irq(struct msm_kms *kms)
 
 	mdp_dispatch_irqs(mdp_kms, status);
 
-	for (id = 0; id < priv->num_crtcs; id++)
-		if (status & mdp5_crtc_vblank(priv->crtcs[id]))
-			drm_handle_vblank(dev, id);
+	drm_for_each_crtc(crtc, dev)
+		if (status & mdp5_crtc_vblank(crtc))
+			drm_crtc_handle_vblank(crtc);
 
 	return IRQ_HANDLED;
 }
-- 
2.35.1

[PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Dmitry Baryshkov]

The array of CRTC in the struct msm_drm_private duplicates a list of
CRTCs in the drm_device. Drop it and use the existing list for CRTC
enumeration.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.h            |  3 +-
 5 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e23e2552e802..e79f0a8817ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 			ret = PTR_ERR(crtc);
 			return ret;
 		}
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/* All CRTCs are compatible with all encoders */
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index fb48c8c19ec3..7449c1693e45 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
 			goto fail;
 		}
 
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 3d5621a68f85..36808990f840 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
 			DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
 			goto fail;
 		}
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 1aab6bf86278..567e77dae43b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
 
 struct msm_vblank_work {
 	struct work_struct work;
-	int crtc_id;
+	struct drm_crtc *crtc;
 	bool enable;
 	struct msm_drm_private *priv;
 };
@@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
 	struct msm_kms *kms = priv->kms;
 
 	if (vbl_work->enable)
-		kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
+		kms->funcs->enable_vblank(kms, vbl_work->crtc);
 	else
-		kms->funcs->disable_vblank(kms,	priv->crtcs[vbl_work->crtc_id]);
+		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
 
 	kfree(vbl_work);
 }
 
 static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
-					int crtc_id, bool enable)
+					struct drm_crtc *crtc, bool enable)
 {
 	struct msm_vblank_work *vbl_work;
 
@@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
 
 	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
 
-	vbl_work->crtc_id = crtc_id;
+	vbl_work->crtc = crtc;
 	vbl_work->enable = enable;
 	vbl_work->priv = priv;
 
@@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
 	struct drm_device *ddev;
 	struct msm_kms *kms;
-	int ret, i;
+	struct drm_crtc *crtc;
+	int ret;
 
 	if (drm_firmware_drivers_only())
 		return -ENODEV;
@@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
-	for (i = 0; i < priv->num_crtcs; i++) {
+	drm_for_each_crtc(crtc, ddev) {
+		struct msm_drm_thread *ev_thread;
+
 		/* initialize event thread */
-		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
-		priv->event_thread[i].dev = ddev;
-		priv->event_thread[i].worker = kthread_create_worker(0,
-			"crtc_event:%d", priv->event_thread[i].crtc_id);
-		if (IS_ERR(priv->event_thread[i].worker)) {
-			ret = PTR_ERR(priv->event_thread[i].worker);
+		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
+		ev_thread->crtc = crtc;
+		ev_thread->dev = ddev;
+		ev_thread->worker = kthread_create_worker(0,
+			"crtc_event:%d", ev_thread->crtc->base.id);
+		if (IS_ERR(ev_thread->worker)) {
+			ret = PTR_ERR(ev_thread->worker);
 			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
-			priv->event_thread[i].worker = NULL;
+			ev_thread->worker = NULL;
 			goto err_msm_uninit;
 		}
 
-		sched_set_fifo(priv->event_thread[i].worker->task);
+		sched_set_fifo(ev_thread->worker->task);
 	}
 
 	ret = drm_vblank_init(ddev, priv->num_crtcs);
@@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
 int msm_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	unsigned int pipe = crtc->index;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	if (!kms)
 		return -ENXIO;
-	drm_dbg_vbl(dev, "crtc=%u", pipe);
-	return vblank_ctrl_queue_work(priv, pipe, true);
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	return vblank_ctrl_queue_work(priv, crtc, true);
 }
 
 void msm_crtc_disable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	unsigned int pipe = crtc->index;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	if (!kms)
 		return;
-	drm_dbg_vbl(dev, "crtc=%u", pipe);
-	vblank_ctrl_queue_work(priv, pipe, false);
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	vblank_ctrl_queue_work(priv, crtc, false);
 }
 
 /*
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 08388d742d65..0e98b6f161df 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -102,7 +102,7 @@ struct msm_display_topology {
 /* Commit/Event thread specific structure */
 struct msm_drm_thread {
 	struct drm_device *dev;
-	unsigned int crtc_id;
+	struct drm_crtc *crtc;
 	struct kthread_worker *worker;
 };
 
@@ -178,7 +178,6 @@ struct msm_drm_private {
 	struct workqueue_struct *wq;
 
 	unsigned int num_crtcs;
-	struct drm_crtc *crtcs[MAX_CRTCS];
 
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
-- 
2.35.1

[PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Dmitry Baryshkov]

The array of CRTC in the struct msm_drm_private duplicates a list of
CRTCs in the drm_device. Drop it and use the existing list for CRTC
enumeration.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.h            |  3 +-
 5 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e23e2552e802..e79f0a8817ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 			ret = PTR_ERR(crtc);
 			return ret;
 		}
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/* All CRTCs are compatible with all encoders */
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index fb48c8c19ec3..7449c1693e45 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
 			goto fail;
 		}
 
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 3d5621a68f85..36808990f840 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
 			DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
 			goto fail;
 		}
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		priv->num_crtcs++;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 1aab6bf86278..567e77dae43b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
 
 struct msm_vblank_work {
 	struct work_struct work;
-	int crtc_id;
+	struct drm_crtc *crtc;
 	bool enable;
 	struct msm_drm_private *priv;
 };
@@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
 	struct msm_kms *kms = priv->kms;
 
 	if (vbl_work->enable)
-		kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
+		kms->funcs->enable_vblank(kms, vbl_work->crtc);
 	else
-		kms->funcs->disable_vblank(kms,	priv->crtcs[vbl_work->crtc_id]);
+		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
 
 	kfree(vbl_work);
 }
 
 static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
-					int crtc_id, bool enable)
+					struct drm_crtc *crtc, bool enable)
 {
 	struct msm_vblank_work *vbl_work;
 
@@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
 
 	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
 
-	vbl_work->crtc_id = crtc_id;
+	vbl_work->crtc = crtc;
 	vbl_work->enable = enable;
 	vbl_work->priv = priv;
 
@@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
 	struct drm_device *ddev;
 	struct msm_kms *kms;
-	int ret, i;
+	struct drm_crtc *crtc;
+	int ret;
 
 	if (drm_firmware_drivers_only())
 		return -ENODEV;
@@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
-	for (i = 0; i < priv->num_crtcs; i++) {
+	drm_for_each_crtc(crtc, ddev) {
+		struct msm_drm_thread *ev_thread;
+
 		/* initialize event thread */
-		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
-		priv->event_thread[i].dev = ddev;
-		priv->event_thread[i].worker = kthread_create_worker(0,
-			"crtc_event:%d", priv->event_thread[i].crtc_id);
-		if (IS_ERR(priv->event_thread[i].worker)) {
-			ret = PTR_ERR(priv->event_thread[i].worker);
+		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
+		ev_thread->crtc = crtc;
+		ev_thread->dev = ddev;
+		ev_thread->worker = kthread_create_worker(0,
+			"crtc_event:%d", ev_thread->crtc->base.id);
+		if (IS_ERR(ev_thread->worker)) {
+			ret = PTR_ERR(ev_thread->worker);
 			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
-			priv->event_thread[i].worker = NULL;
+			ev_thread->worker = NULL;
 			goto err_msm_uninit;
 		}
 
-		sched_set_fifo(priv->event_thread[i].worker->task);
+		sched_set_fifo(ev_thread->worker->task);
 	}
 
 	ret = drm_vblank_init(ddev, priv->num_crtcs);
@@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
 int msm_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	unsigned int pipe = crtc->index;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	if (!kms)
 		return -ENXIO;
-	drm_dbg_vbl(dev, "crtc=%u", pipe);
-	return vblank_ctrl_queue_work(priv, pipe, true);
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	return vblank_ctrl_queue_work(priv, crtc, true);
 }
 
 void msm_crtc_disable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	unsigned int pipe = crtc->index;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	if (!kms)
 		return;
-	drm_dbg_vbl(dev, "crtc=%u", pipe);
-	vblank_ctrl_queue_work(priv, pipe, false);
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	vblank_ctrl_queue_work(priv, crtc, false);
 }
 
 /*
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 08388d742d65..0e98b6f161df 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -102,7 +102,7 @@ struct msm_display_topology {
 /* Commit/Event thread specific structure */
 struct msm_drm_thread {
 	struct drm_device *dev;
-	unsigned int crtc_id;
+	struct drm_crtc *crtc;
 	struct kthread_worker *worker;
 };
 
@@ -178,7 +178,6 @@ struct msm_drm_private {
 	struct workqueue_struct *wq;
 
 	unsigned int num_crtcs;
-	struct drm_crtc *crtcs[MAX_CRTCS];
 
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
-- 
2.35.1

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Abhinav Kumar]

On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> The array of CRTC in the struct msm_drm_private duplicates a list of
> CRTCs in the drm_device. Drop it and use the existing list for CRTC
> enumeration.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
>   drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
>   drivers/gpu/drm/msm/msm_drv.h            |  3 +-
>   5 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e23e2552e802..e79f0a8817ac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   			ret = PTR_ERR(crtc);
>   			return ret;
>   		}
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/* All CRTCs are compatible with all encoders */
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index fb48c8c19ec3..7449c1693e45 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
>   			goto fail;
>   		}
>   
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 3d5621a68f85..36808990f840 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>   			DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
>   			goto fail;
>   		}
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 1aab6bf86278..567e77dae43b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
>   
>   struct msm_vblank_work {
>   	struct work_struct work;
> -	int crtc_id;
> +	struct drm_crtc *crtc;
>   	bool enable;
>   	struct msm_drm_private *priv;
>   };
> @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
>   	struct msm_kms *kms = priv->kms;
>   

Is there any chance of vbl_work->crtc becoming NULL before this gets 
executed?

So do we need to protect this like

if (vbl_work->enable && vbl_work->crtc)

Because the layers below this dont seem to have NULL protection.


>   	if (vbl_work->enable)
> -		kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> +		kms->funcs->enable_vblank(kms, vbl_work->crtc);
>   	else
> -		kms->funcs->disable_vblank(kms,	priv->crtcs[vbl_work->crtc_id]);
> +		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
>   
>   	kfree(vbl_work);
>   }
>   
>   static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> -					int crtc_id, bool enable)
> +					struct drm_crtc *crtc, bool enable)
>   {
>   	struct msm_vblank_work *vbl_work;
>   
> @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>   
>   	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
>   
> -	vbl_work->crtc_id = crtc_id;
> +	vbl_work->crtc = crtc;
>   	vbl_work->enable = enable;
>   	vbl_work->priv = priv;
>   
> @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	struct msm_drm_private *priv = dev_get_drvdata(dev);
>   	struct drm_device *ddev;
>   	struct msm_kms *kms;
> -	int ret, i;
> +	struct drm_crtc *crtc;
> +	int ret;
>   
>   	if (drm_firmware_drivers_only())
>   		return -ENODEV;
> @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	ddev->mode_config.funcs = &mode_config_funcs;
>   	ddev->mode_config.helper_private = &mode_config_helper_funcs;
>   
> -	for (i = 0; i < priv->num_crtcs; i++) {
> +	drm_for_each_crtc(crtc, ddev) {
> +		struct msm_drm_thread *ev_thread;
> +
>   		/* initialize event thread */
> -		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
> -		priv->event_thread[i].dev = ddev;
> -		priv->event_thread[i].worker = kthread_create_worker(0,
> -			"crtc_event:%d", priv->event_thread[i].crtc_id);
> -		if (IS_ERR(priv->event_thread[i].worker)) {
> -			ret = PTR_ERR(priv->event_thread[i].worker);
> +		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
> +		ev_thread->crtc = crtc;
> +		ev_thread->dev = ddev;
> +		ev_thread->worker = kthread_create_worker(0,
> +			"crtc_event:%d", ev_thread->crtc->base.id);

Please correct me if wrong.

Today, other than just populating the name for the worker is the 
ev_thread->crtc used anywhere?

If so, can we just drop crtc from msm_drm_thread and while creating the 
worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);

> +		if (IS_ERR(ev_thread->worker)) {
> +			ret = PTR_ERR(ev_thread->worker);
>   			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
> -			priv->event_thread[i].worker = NULL;
> +			ev_thread->worker = NULL;
>   			goto err_msm_uninit;
>   		}
>   
> -		sched_set_fifo(priv->event_thread[i].worker->task);
> +		sched_set_fifo(ev_thread->worker->task);
>   	}
>   
>   	ret = drm_vblank_init(ddev, priv->num_crtcs);
> @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
>   int msm_crtc_enable_vblank(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> -	unsigned int pipe = crtc->index;
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	if (!kms)
>   		return -ENXIO;
> -	drm_dbg_vbl(dev, "crtc=%u", pipe);
> -	return vblank_ctrl_queue_work(priv, pipe, true);
> +	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> +	return vblank_ctrl_queue_work(priv, crtc, true);
>   }
>   
>   void msm_crtc_disable_vblank(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> -	unsigned int pipe = crtc->index;
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	if (!kms)
>   		return;
> -	drm_dbg_vbl(dev, "crtc=%u", pipe);
> -	vblank_ctrl_queue_work(priv, pipe, false);
> +	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> +	vblank_ctrl_queue_work(priv, crtc, false);
>   }
>   
>   /*
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 08388d742d65..0e98b6f161df 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -102,7 +102,7 @@ struct msm_display_topology {
>   /* Commit/Event thread specific structure */
>   struct msm_drm_thread {
>   	struct drm_device *dev;
> -	unsigned int crtc_id;
> +	struct drm_crtc *crtc;
>   	struct kthread_worker *worker;
>   };
>   
> @@ -178,7 +178,6 @@ struct msm_drm_private {
>   	struct workqueue_struct *wq;
>   
>   	unsigned int num_crtcs;
> -	struct drm_crtc *crtcs[MAX_CRTCS];
>   
>   	struct msm_drm_thread event_thread[MAX_CRTCS];
>   

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Abhinav Kumar]

On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> The array of CRTC in the struct msm_drm_private duplicates a list of
> CRTCs in the drm_device. Drop it and use the existing list for CRTC
> enumeration.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
>   drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
>   drivers/gpu/drm/msm/msm_drv.h            |  3 +-
>   5 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e23e2552e802..e79f0a8817ac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   			ret = PTR_ERR(crtc);
>   			return ret;
>   		}
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/* All CRTCs are compatible with all encoders */
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index fb48c8c19ec3..7449c1693e45 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
>   			goto fail;
>   		}
>   
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 3d5621a68f85..36808990f840 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>   			DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
>   			goto fail;
>   		}
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		priv->num_crtcs++;
>   	}
>   
>   	/*
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 1aab6bf86278..567e77dae43b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
>   
>   struct msm_vblank_work {
>   	struct work_struct work;
> -	int crtc_id;
> +	struct drm_crtc *crtc;
>   	bool enable;
>   	struct msm_drm_private *priv;
>   };
> @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
>   	struct msm_kms *kms = priv->kms;
>   

Is there any chance of vbl_work->crtc becoming NULL before this gets 
executed?

So do we need to protect this like

if (vbl_work->enable && vbl_work->crtc)

Because the layers below this dont seem to have NULL protection.


>   	if (vbl_work->enable)
> -		kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> +		kms->funcs->enable_vblank(kms, vbl_work->crtc);
>   	else
> -		kms->funcs->disable_vblank(kms,	priv->crtcs[vbl_work->crtc_id]);
> +		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
>   
>   	kfree(vbl_work);
>   }
>   
>   static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> -					int crtc_id, bool enable)
> +					struct drm_crtc *crtc, bool enable)
>   {
>   	struct msm_vblank_work *vbl_work;
>   
> @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>   
>   	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
>   
> -	vbl_work->crtc_id = crtc_id;
> +	vbl_work->crtc = crtc;
>   	vbl_work->enable = enable;
>   	vbl_work->priv = priv;
>   
> @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	struct msm_drm_private *priv = dev_get_drvdata(dev);
>   	struct drm_device *ddev;
>   	struct msm_kms *kms;
> -	int ret, i;
> +	struct drm_crtc *crtc;
> +	int ret;
>   
>   	if (drm_firmware_drivers_only())
>   		return -ENODEV;
> @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>   	ddev->mode_config.funcs = &mode_config_funcs;
>   	ddev->mode_config.helper_private = &mode_config_helper_funcs;
>   
> -	for (i = 0; i < priv->num_crtcs; i++) {
> +	drm_for_each_crtc(crtc, ddev) {
> +		struct msm_drm_thread *ev_thread;
> +
>   		/* initialize event thread */
> -		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
> -		priv->event_thread[i].dev = ddev;
> -		priv->event_thread[i].worker = kthread_create_worker(0,
> -			"crtc_event:%d", priv->event_thread[i].crtc_id);
> -		if (IS_ERR(priv->event_thread[i].worker)) {
> -			ret = PTR_ERR(priv->event_thread[i].worker);
> +		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
> +		ev_thread->crtc = crtc;
> +		ev_thread->dev = ddev;
> +		ev_thread->worker = kthread_create_worker(0,
> +			"crtc_event:%d", ev_thread->crtc->base.id);

Please correct me if wrong.

Today, other than just populating the name for the worker is the 
ev_thread->crtc used anywhere?

If so, can we just drop crtc from msm_drm_thread and while creating the 
worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);

> +		if (IS_ERR(ev_thread->worker)) {
> +			ret = PTR_ERR(ev_thread->worker);
>   			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
> -			priv->event_thread[i].worker = NULL;
> +			ev_thread->worker = NULL;
>   			goto err_msm_uninit;
>   		}
>   
> -		sched_set_fifo(priv->event_thread[i].worker->task);
> +		sched_set_fifo(ev_thread->worker->task);
>   	}
>   
>   	ret = drm_vblank_init(ddev, priv->num_crtcs);
> @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
>   int msm_crtc_enable_vblank(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> -	unsigned int pipe = crtc->index;
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	if (!kms)
>   		return -ENXIO;
> -	drm_dbg_vbl(dev, "crtc=%u", pipe);
> -	return vblank_ctrl_queue_work(priv, pipe, true);
> +	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> +	return vblank_ctrl_queue_work(priv, crtc, true);
>   }
>   
>   void msm_crtc_disable_vblank(struct drm_crtc *crtc)
>   {
>   	struct drm_device *dev = crtc->dev;
> -	unsigned int pipe = crtc->index;
>   	struct msm_drm_private *priv = dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	if (!kms)
>   		return;
> -	drm_dbg_vbl(dev, "crtc=%u", pipe);
> -	vblank_ctrl_queue_work(priv, pipe, false);
> +	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> +	vblank_ctrl_queue_work(priv, crtc, false);
>   }
>   
>   /*
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 08388d742d65..0e98b6f161df 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -102,7 +102,7 @@ struct msm_display_topology {
>   /* Commit/Event thread specific structure */
>   struct msm_drm_thread {
>   	struct drm_device *dev;
> -	unsigned int crtc_id;
> +	struct drm_crtc *crtc;
>   	struct kthread_worker *worker;
>   };
>   
> @@ -178,7 +178,6 @@ struct msm_drm_private {
>   	struct workqueue_struct *wq;
>   
>   	unsigned int num_crtcs;
> -	struct drm_crtc *crtcs[MAX_CRTCS];
>   
>   	struct msm_drm_thread event_thread[MAX_CRTCS];
>   

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Dmitry Baryshkov]

On Wed, 25 Jan 2023 at 04:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> > The array of CRTC in the struct msm_drm_private duplicates a list of
> > CRTCs in the drm_device. Drop it and use the existing list for CRTC
> > enumeration.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
> >   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
> >   drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
> >   drivers/gpu/drm/msm/msm_drv.h            |  3 +-
> >   5 files changed, 27 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index e23e2552e802..e79f0a8817ac 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> >                       ret = PTR_ERR(crtc);
> >                       return ret;
> >               }
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /* All CRTCs are compatible with all encoders */
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > index fb48c8c19ec3..7449c1693e45 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
> >                       goto fail;
> >               }
> >
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /*
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 3d5621a68f85..36808990f840 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> >                       DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
> >                       goto fail;
> >               }
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /*
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 1aab6bf86278..567e77dae43b 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
> >
> >   struct msm_vblank_work {
> >       struct work_struct work;
> > -     int crtc_id;
> > +     struct drm_crtc *crtc;
> >       bool enable;
> >       struct msm_drm_private *priv;
> >   };
> > @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
> >       struct msm_kms *kms = priv->kms;
> >
>
> Is there any chance of vbl_work->crtc becoming NULL before this gets
> executed?

No. The worker is created in vblank_ctrl_queue_work. The
vbl_work->crtc is filled at the time of creation.

> So do we need to protect this like
>
> if (vbl_work->enable && vbl_work->crtc)
>
> Because the layers below this dont seem to have NULL protection.
>
>
> >       if (vbl_work->enable)
> > -             kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> > +             kms->funcs->enable_vblank(kms, vbl_work->crtc);
> >       else
> > -             kms->funcs->disable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> > +             kms->funcs->disable_vblank(kms, vbl_work->crtc);
> >
> >       kfree(vbl_work);
> >   }
> >
> >   static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> > -                                     int crtc_id, bool enable)
> > +                                     struct drm_crtc *crtc, bool enable)
> >   {
> >       struct msm_vblank_work *vbl_work;
> >
> > @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> >
> >       INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
> >
> > -     vbl_work->crtc_id = crtc_id;
> > +     vbl_work->crtc = crtc;
> >       vbl_work->enable = enable;
> >       vbl_work->priv = priv;
> >
> > @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> >       struct msm_drm_private *priv = dev_get_drvdata(dev);
> >       struct drm_device *ddev;
> >       struct msm_kms *kms;
> > -     int ret, i;
> > +     struct drm_crtc *crtc;
> > +     int ret;
> >
> >       if (drm_firmware_drivers_only())
> >               return -ENODEV;
> > @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> >       ddev->mode_config.funcs = &mode_config_funcs;
> >       ddev->mode_config.helper_private = &mode_config_helper_funcs;
> >
> > -     for (i = 0; i < priv->num_crtcs; i++) {
> > +     drm_for_each_crtc(crtc, ddev) {
> > +             struct msm_drm_thread *ev_thread;
> > +
> >               /* initialize event thread */
> > -             priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
> > -             priv->event_thread[i].dev = ddev;
> > -             priv->event_thread[i].worker = kthread_create_worker(0,
> > -                     "crtc_event:%d", priv->event_thread[i].crtc_id);
> > -             if (IS_ERR(priv->event_thread[i].worker)) {
> > -                     ret = PTR_ERR(priv->event_thread[i].worker);
> > +             ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
> > +             ev_thread->crtc = crtc;
> > +             ev_thread->dev = ddev;
> > +             ev_thread->worker = kthread_create_worker(0,
> > +                     "crtc_event:%d", ev_thread->crtc->base.id);
>
> Please correct me if wrong.
>
> Today, other than just populating the name for the worker is the
> ev_thread->crtc used anywhere?
>
> If so, can we just drop crtc from msm_drm_thread and while creating the
> worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);

It seems so. I'll take a look for v2.

However your questions actually raised another question in my head. I
went on looking for the reason for such complex vblank handling. It
was added by Hai Li in the commit 78b1d470d57d ("drm/msm: Enable
clocks during enable/disable_vblank() callbacks"). However I don't
fully understand why the code will toggle vblank handling while the
DPU/MDP5/MDP4 device is not resumed already. Maybe I just missed
something here. Do you know the story behind the change?

>
> > +             if (IS_ERR(ev_thread->worker)) {
> > +                     ret = PTR_ERR(ev_thread->worker);
> >                       DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
> > -                     priv->event_thread[i].worker = NULL;
> > +                     ev_thread->worker = NULL;
> >                       goto err_msm_uninit;
> >               }
> >
> > -             sched_set_fifo(priv->event_thread[i].worker->task);
> > +             sched_set_fifo(ev_thread->worker->task);
> >       }
> >
> >       ret = drm_vblank_init(ddev, priv->num_crtcs);
> > @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
> >   int msm_crtc_enable_vblank(struct drm_crtc *crtc)
> >   {
> >       struct drm_device *dev = crtc->dev;
> > -     unsigned int pipe = crtc->index;
> >       struct msm_drm_private *priv = dev->dev_private;
> >       struct msm_kms *kms = priv->kms;
> >       if (!kms)
> >               return -ENXIO;
> > -     drm_dbg_vbl(dev, "crtc=%u", pipe);
> > -     return vblank_ctrl_queue_work(priv, pipe, true);
> > +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> > +     return vblank_ctrl_queue_work(priv, crtc, true);
> >   }
> >
> >   void msm_crtc_disable_vblank(struct drm_crtc *crtc)
> >   {
> >       struct drm_device *dev = crtc->dev;
> > -     unsigned int pipe = crtc->index;
> >       struct msm_drm_private *priv = dev->dev_private;
> >       struct msm_kms *kms = priv->kms;
> >       if (!kms)
> >               return;
> > -     drm_dbg_vbl(dev, "crtc=%u", pipe);
> > -     vblank_ctrl_queue_work(priv, pipe, false);
> > +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> > +     vblank_ctrl_queue_work(priv, crtc, false);
> >   }
> >
> >   /*
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 08388d742d65..0e98b6f161df 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -102,7 +102,7 @@ struct msm_display_topology {
> >   /* Commit/Event thread specific structure */
> >   struct msm_drm_thread {
> >       struct drm_device *dev;
> > -     unsigned int crtc_id;
> > +     struct drm_crtc *crtc;
> >       struct kthread_worker *worker;
> >   };
> >
> > @@ -178,7 +178,6 @@ struct msm_drm_private {
> >       struct workqueue_struct *wq;
> >
> >       unsigned int num_crtcs;
> > -     struct drm_crtc *crtcs[MAX_CRTCS];
> >
> >       struct msm_drm_thread event_thread[MAX_CRTCS];
> >



-- 
With best wishes
Dmitry

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Dmitry Baryshkov]

On Wed, 25 Jan 2023 at 04:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
> > The array of CRTC in the struct msm_drm_private duplicates a list of
> > CRTCs in the drm_device. Drop it and use the existing list for CRTC
> > enumeration.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
> >   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
> >   drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
> >   drivers/gpu/drm/msm/msm_drv.h            |  3 +-
> >   5 files changed, 27 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index e23e2552e802..e79f0a8817ac 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> >                       ret = PTR_ERR(crtc);
> >                       return ret;
> >               }
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /* All CRTCs are compatible with all encoders */
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > index fb48c8c19ec3..7449c1693e45 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
> >                       goto fail;
> >               }
> >
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /*
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 3d5621a68f85..36808990f840 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> >                       DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
> >                       goto fail;
> >               }
> > -             priv->crtcs[priv->num_crtcs++] = crtc;
> > +             priv->num_crtcs++;
> >       }
> >
> >       /*
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 1aab6bf86278..567e77dae43b 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
> >
> >   struct msm_vblank_work {
> >       struct work_struct work;
> > -     int crtc_id;
> > +     struct drm_crtc *crtc;
> >       bool enable;
> >       struct msm_drm_private *priv;
> >   };
> > @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
> >       struct msm_kms *kms = priv->kms;
> >
>
> Is there any chance of vbl_work->crtc becoming NULL before this gets
> executed?

No. The worker is created in vblank_ctrl_queue_work. The
vbl_work->crtc is filled at the time of creation.

> So do we need to protect this like
>
> if (vbl_work->enable && vbl_work->crtc)
>
> Because the layers below this dont seem to have NULL protection.
>
>
> >       if (vbl_work->enable)
> > -             kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> > +             kms->funcs->enable_vblank(kms, vbl_work->crtc);
> >       else
> > -             kms->funcs->disable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
> > +             kms->funcs->disable_vblank(kms, vbl_work->crtc);
> >
> >       kfree(vbl_work);
> >   }
> >
> >   static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> > -                                     int crtc_id, bool enable)
> > +                                     struct drm_crtc *crtc, bool enable)
> >   {
> >       struct msm_vblank_work *vbl_work;
> >
> > @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
> >
> >       INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
> >
> > -     vbl_work->crtc_id = crtc_id;
> > +     vbl_work->crtc = crtc;
> >       vbl_work->enable = enable;
> >       vbl_work->priv = priv;
> >
> > @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> >       struct msm_drm_private *priv = dev_get_drvdata(dev);
> >       struct drm_device *ddev;
> >       struct msm_kms *kms;
> > -     int ret, i;
> > +     struct drm_crtc *crtc;
> > +     int ret;
> >
> >       if (drm_firmware_drivers_only())
> >               return -ENODEV;
> > @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
> >       ddev->mode_config.funcs = &mode_config_funcs;
> >       ddev->mode_config.helper_private = &mode_config_helper_funcs;
> >
> > -     for (i = 0; i < priv->num_crtcs; i++) {
> > +     drm_for_each_crtc(crtc, ddev) {
> > +             struct msm_drm_thread *ev_thread;
> > +
> >               /* initialize event thread */
> > -             priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
> > -             priv->event_thread[i].dev = ddev;
> > -             priv->event_thread[i].worker = kthread_create_worker(0,
> > -                     "crtc_event:%d", priv->event_thread[i].crtc_id);
> > -             if (IS_ERR(priv->event_thread[i].worker)) {
> > -                     ret = PTR_ERR(priv->event_thread[i].worker);
> > +             ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
> > +             ev_thread->crtc = crtc;
> > +             ev_thread->dev = ddev;
> > +             ev_thread->worker = kthread_create_worker(0,
> > +                     "crtc_event:%d", ev_thread->crtc->base.id);
>
> Please correct me if wrong.
>
> Today, other than just populating the name for the worker is the
> ev_thread->crtc used anywhere?
>
> If so, can we just drop crtc from msm_drm_thread and while creating the
> worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);

It seems so. I'll take a look for v2.

However your questions actually raised another question in my head. I
went on looking for the reason for such complex vblank handling. It
was added by Hai Li in the commit 78b1d470d57d ("drm/msm: Enable
clocks during enable/disable_vblank() callbacks"). However I don't
fully understand why the code will toggle vblank handling while the
DPU/MDP5/MDP4 device is not resumed already. Maybe I just missed
something here. Do you know the story behind the change?

>
> > +             if (IS_ERR(ev_thread->worker)) {
> > +                     ret = PTR_ERR(ev_thread->worker);
> >                       DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
> > -                     priv->event_thread[i].worker = NULL;
> > +                     ev_thread->worker = NULL;
> >                       goto err_msm_uninit;
> >               }
> >
> > -             sched_set_fifo(priv->event_thread[i].worker->task);
> > +             sched_set_fifo(ev_thread->worker->task);
> >       }
> >
> >       ret = drm_vblank_init(ddev, priv->num_crtcs);
> > @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
> >   int msm_crtc_enable_vblank(struct drm_crtc *crtc)
> >   {
> >       struct drm_device *dev = crtc->dev;
> > -     unsigned int pipe = crtc->index;
> >       struct msm_drm_private *priv = dev->dev_private;
> >       struct msm_kms *kms = priv->kms;
> >       if (!kms)
> >               return -ENXIO;
> > -     drm_dbg_vbl(dev, "crtc=%u", pipe);
> > -     return vblank_ctrl_queue_work(priv, pipe, true);
> > +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> > +     return vblank_ctrl_queue_work(priv, crtc, true);
> >   }
> >
> >   void msm_crtc_disable_vblank(struct drm_crtc *crtc)
> >   {
> >       struct drm_device *dev = crtc->dev;
> > -     unsigned int pipe = crtc->index;
> >       struct msm_drm_private *priv = dev->dev_private;
> >       struct msm_kms *kms = priv->kms;
> >       if (!kms)
> >               return;
> > -     drm_dbg_vbl(dev, "crtc=%u", pipe);
> > -     vblank_ctrl_queue_work(priv, pipe, false);
> > +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
> > +     vblank_ctrl_queue_work(priv, crtc, false);
> >   }
> >
> >   /*
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 08388d742d65..0e98b6f161df 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -102,7 +102,7 @@ struct msm_display_topology {
> >   /* Commit/Event thread specific structure */
> >   struct msm_drm_thread {
> >       struct drm_device *dev;
> > -     unsigned int crtc_id;
> > +     struct drm_crtc *crtc;
> >       struct kthread_worker *worker;
> >   };
> >
> > @@ -178,7 +178,6 @@ struct msm_drm_private {
> >       struct workqueue_struct *wq;
> >
> >       unsigned int num_crtcs;
> > -     struct drm_crtc *crtcs[MAX_CRTCS];
> >
> >       struct msm_drm_thread event_thread[MAX_CRTCS];
> >



-- 
With best wishes
Dmitry

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Abhinav Kumar]

On 1/24/2023 11:29 PM, Dmitry Baryshkov wrote:
> On Wed, 25 Jan 2023 at 04:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
>>> The array of CRTC in the struct msm_drm_private duplicates a list of
>>> CRTCs in the drm_device. Drop it and use the existing list for CRTC
>>> enumeration.
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
>>>    drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
>>>    drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
>>>    drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
>>>    drivers/gpu/drm/msm/msm_drv.h            |  3 +-
>>>    5 files changed, 27 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index e23e2552e802..e79f0a8817ac 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>>                        ret = PTR_ERR(crtc);
>>>                        return ret;
>>>                }
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /* All CRTCs are compatible with all encoders */
>>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> index fb48c8c19ec3..7449c1693e45 100644
>>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
>>>                        goto fail;
>>>                }
>>>
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /*
>>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> index 3d5621a68f85..36808990f840 100644
>>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>>>                        DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
>>>                        goto fail;
>>>                }
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /*
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index 1aab6bf86278..567e77dae43b 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
>>>
>>>    struct msm_vblank_work {
>>>        struct work_struct work;
>>> -     int crtc_id;
>>> +     struct drm_crtc *crtc;
>>>        bool enable;
>>>        struct msm_drm_private *priv;
>>>    };
>>> @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
>>>        struct msm_kms *kms = priv->kms;
>>>
>>
>> Is there any chance of vbl_work->crtc becoming NULL before this gets
>> executed?
> 
> No. The worker is created in vblank_ctrl_queue_work. The
> vbl_work->crtc is filled at the time of creation.
> 
>> So do we need to protect this like
>>
>> if (vbl_work->enable && vbl_work->crtc)
>>
>> Because the layers below this dont seem to have NULL protection.
>>
>>
>>>        if (vbl_work->enable)
>>> -             kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
>>> +             kms->funcs->enable_vblank(kms, vbl_work->crtc);
>>>        else
>>> -             kms->funcs->disable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
>>> +             kms->funcs->disable_vblank(kms, vbl_work->crtc);
>>>
>>>        kfree(vbl_work);
>>>    }
>>>
>>>    static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>>> -                                     int crtc_id, bool enable)
>>> +                                     struct drm_crtc *crtc, bool enable)
>>>    {
>>>        struct msm_vblank_work *vbl_work;
>>>
>>> @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>>>
>>>        INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
>>>
>>> -     vbl_work->crtc_id = crtc_id;
>>> +     vbl_work->crtc = crtc;
>>>        vbl_work->enable = enable;
>>>        vbl_work->priv = priv;
>>>
>>> @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>>>        struct msm_drm_private *priv = dev_get_drvdata(dev);
>>>        struct drm_device *ddev;
>>>        struct msm_kms *kms;
>>> -     int ret, i;
>>> +     struct drm_crtc *crtc;
>>> +     int ret;
>>>
>>>        if (drm_firmware_drivers_only())
>>>                return -ENODEV;
>>> @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>>>        ddev->mode_config.funcs = &mode_config_funcs;
>>>        ddev->mode_config.helper_private = &mode_config_helper_funcs;
>>>
>>> -     for (i = 0; i < priv->num_crtcs; i++) {
>>> +     drm_for_each_crtc(crtc, ddev) {
>>> +             struct msm_drm_thread *ev_thread;
>>> +
>>>                /* initialize event thread */
>>> -             priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
>>> -             priv->event_thread[i].dev = ddev;
>>> -             priv->event_thread[i].worker = kthread_create_worker(0,
>>> -                     "crtc_event:%d", priv->event_thread[i].crtc_id);
>>> -             if (IS_ERR(priv->event_thread[i].worker)) {
>>> -                     ret = PTR_ERR(priv->event_thread[i].worker);
>>> +             ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
>>> +             ev_thread->crtc = crtc;
>>> +             ev_thread->dev = ddev;
>>> +             ev_thread->worker = kthread_create_worker(0,
>>> +                     "crtc_event:%d", ev_thread->crtc->base.id);
>>
>> Please correct me if wrong.
>>
>> Today, other than just populating the name for the worker is the
>> ev_thread->crtc used anywhere?
>>
>> If so, can we just drop crtc from msm_drm_thread and while creating the
>> worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);
> 
> It seems so. I'll take a look for v2.
> 
> However your questions actually raised another question in my head. I
> went on looking for the reason for such complex vblank handling. It
> was added by Hai Li in the commit 78b1d470d57d ("drm/msm: Enable
> clocks during enable/disable_vblank() callbacks"). However I don't
> fully understand why the code will toggle vblank handling while the
> DPU/MDP5/MDP4 device is not resumed already. Maybe I just missed
> something here. Do you know the story behind the change?
> 
I dont know the history as it pre-dates my work in upstream.
But one use-case I can think of is for command mode panel.

Typically, command mode panel will disable the clocks during idle screen.

Now, if usermode wants to enable vsync, it will crash in the driver.

So driver needs to make sure what whenever hw vsync is toggled on/off, 
the required clocks need to be enabled.
>>
>>> +             if (IS_ERR(ev_thread->worker)) {
>>> +                     ret = PTR_ERR(ev_thread->worker);
>>>                        DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
>>> -                     priv->event_thread[i].worker = NULL;
>>> +                     ev_thread->worker = NULL;
>>>                        goto err_msm_uninit;
>>>                }
>>>
>>> -             sched_set_fifo(priv->event_thread[i].worker->task);
>>> +             sched_set_fifo(ev_thread->worker->task);
>>>        }
>>>
>>>        ret = drm_vblank_init(ddev, priv->num_crtcs);
>>> @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
>>>    int msm_crtc_enable_vblank(struct drm_crtc *crtc)
>>>    {
>>>        struct drm_device *dev = crtc->dev;
>>> -     unsigned int pipe = crtc->index;
>>>        struct msm_drm_private *priv = dev->dev_private;
>>>        struct msm_kms *kms = priv->kms;
>>>        if (!kms)
>>>                return -ENXIO;
>>> -     drm_dbg_vbl(dev, "crtc=%u", pipe);
>>> -     return vblank_ctrl_queue_work(priv, pipe, true);
>>> +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
>>> +     return vblank_ctrl_queue_work(priv, crtc, true);
>>>    }
>>>
>>>    void msm_crtc_disable_vblank(struct drm_crtc *crtc)
>>>    {
>>>        struct drm_device *dev = crtc->dev;
>>> -     unsigned int pipe = crtc->index;
>>>        struct msm_drm_private *priv = dev->dev_private;
>>>        struct msm_kms *kms = priv->kms;
>>>        if (!kms)
>>>                return;
>>> -     drm_dbg_vbl(dev, "crtc=%u", pipe);
>>> -     vblank_ctrl_queue_work(priv, pipe, false);
>>> +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
>>> +     vblank_ctrl_queue_work(priv, crtc, false);
>>>    }
>>>
>>>    /*
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>>> index 08388d742d65..0e98b6f161df 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>> @@ -102,7 +102,7 @@ struct msm_display_topology {
>>>    /* Commit/Event thread specific structure */
>>>    struct msm_drm_thread {
>>>        struct drm_device *dev;
>>> -     unsigned int crtc_id;
>>> +     struct drm_crtc *crtc;
>>>        struct kthread_worker *worker;
>>>    };
>>>
>>> @@ -178,7 +178,6 @@ struct msm_drm_private {
>>>        struct workqueue_struct *wq;
>>>
>>>        unsigned int num_crtcs;
>>> -     struct drm_crtc *crtcs[MAX_CRTCS];
>>>
>>>        struct msm_drm_thread event_thread[MAX_CRTCS];
>>>
> 
> 
> 

Re: [PATCH v6 4/4] drm/msm: stop storing the array of CRTCs in struct msm_drm_private

[Abhinav Kumar]

On 1/24/2023 11:29 PM, Dmitry Baryshkov wrote:
> On Wed, 25 Jan 2023 at 04:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/17/2022 4:33 PM, Dmitry Baryshkov wrote:
>>> The array of CRTC in the struct msm_drm_private duplicates a list of
>>> CRTCs in the drm_device. Drop it and use the existing list for CRTC
>>> enumeration.
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  2 +-
>>>    drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  2 +-
>>>    drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  2 +-
>>>    drivers/gpu/drm/msm/msm_drv.c            | 44 +++++++++++++-----------
>>>    drivers/gpu/drm/msm/msm_drv.h            |  3 +-
>>>    5 files changed, 27 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index e23e2552e802..e79f0a8817ac 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -806,7 +806,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>>                        ret = PTR_ERR(crtc);
>>>                        return ret;
>>>                }
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /* All CRTCs are compatible with all encoders */
>>> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> index fb48c8c19ec3..7449c1693e45 100644
>>> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
>>> @@ -337,7 +337,7 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
>>>                        goto fail;
>>>                }
>>>
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /*
>>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> index 3d5621a68f85..36808990f840 100644
>>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>>> @@ -497,7 +497,7 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>>>                        DRM_DEV_ERROR(dev->dev, "failed to construct crtc %d (%d)\n", i, ret);
>>>                        goto fail;
>>>                }
>>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>>> +             priv->num_crtcs++;
>>>        }
>>>
>>>        /*
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index 1aab6bf86278..567e77dae43b 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -149,7 +149,7 @@ static void msm_irq_uninstall(struct drm_device *dev)
>>>
>>>    struct msm_vblank_work {
>>>        struct work_struct work;
>>> -     int crtc_id;
>>> +     struct drm_crtc *crtc;
>>>        bool enable;
>>>        struct msm_drm_private *priv;
>>>    };
>>> @@ -162,15 +162,15 @@ static void vblank_ctrl_worker(struct work_struct *work)
>>>        struct msm_kms *kms = priv->kms;
>>>
>>
>> Is there any chance of vbl_work->crtc becoming NULL before this gets
>> executed?
> 
> No. The worker is created in vblank_ctrl_queue_work. The
> vbl_work->crtc is filled at the time of creation.
> 
>> So do we need to protect this like
>>
>> if (vbl_work->enable && vbl_work->crtc)
>>
>> Because the layers below this dont seem to have NULL protection.
>>
>>
>>>        if (vbl_work->enable)
>>> -             kms->funcs->enable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
>>> +             kms->funcs->enable_vblank(kms, vbl_work->crtc);
>>>        else
>>> -             kms->funcs->disable_vblank(kms, priv->crtcs[vbl_work->crtc_id]);
>>> +             kms->funcs->disable_vblank(kms, vbl_work->crtc);
>>>
>>>        kfree(vbl_work);
>>>    }
>>>
>>>    static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>>> -                                     int crtc_id, bool enable)
>>> +                                     struct drm_crtc *crtc, bool enable)
>>>    {
>>>        struct msm_vblank_work *vbl_work;
>>>
>>> @@ -180,7 +180,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
>>>
>>>        INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
>>>
>>> -     vbl_work->crtc_id = crtc_id;
>>> +     vbl_work->crtc = crtc;
>>>        vbl_work->enable = enable;
>>>        vbl_work->priv = priv;
>>>
>>> @@ -354,7 +354,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>>>        struct msm_drm_private *priv = dev_get_drvdata(dev);
>>>        struct drm_device *ddev;
>>>        struct msm_kms *kms;
>>> -     int ret, i;
>>> +     struct drm_crtc *crtc;
>>> +     int ret;
>>>
>>>        if (drm_firmware_drivers_only())
>>>                return -ENODEV;
>>> @@ -427,20 +428,23 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>>>        ddev->mode_config.funcs = &mode_config_funcs;
>>>        ddev->mode_config.helper_private = &mode_config_helper_funcs;
>>>
>>> -     for (i = 0; i < priv->num_crtcs; i++) {
>>> +     drm_for_each_crtc(crtc, ddev) {
>>> +             struct msm_drm_thread *ev_thread;
>>> +
>>>                /* initialize event thread */
>>> -             priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
>>> -             priv->event_thread[i].dev = ddev;
>>> -             priv->event_thread[i].worker = kthread_create_worker(0,
>>> -                     "crtc_event:%d", priv->event_thread[i].crtc_id);
>>> -             if (IS_ERR(priv->event_thread[i].worker)) {
>>> -                     ret = PTR_ERR(priv->event_thread[i].worker);
>>> +             ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
>>> +             ev_thread->crtc = crtc;
>>> +             ev_thread->dev = ddev;
>>> +             ev_thread->worker = kthread_create_worker(0,
>>> +                     "crtc_event:%d", ev_thread->crtc->base.id);
>>
>> Please correct me if wrong.
>>
>> Today, other than just populating the name for the worker is the
>> ev_thread->crtc used anywhere?
>>
>> If so, can we just drop crtc from msm_drm_thread and while creating the
>> worker just use kthread_create_worker(0, "crtc_event:%d", crtc->base.id);
> 
> It seems so. I'll take a look for v2.
> 
> However your questions actually raised another question in my head. I
> went on looking for the reason for such complex vblank handling. It
> was added by Hai Li in the commit 78b1d470d57d ("drm/msm: Enable
> clocks during enable/disable_vblank() callbacks"). However I don't
> fully understand why the code will toggle vblank handling while the
> DPU/MDP5/MDP4 device is not resumed already. Maybe I just missed
> something here. Do you know the story behind the change?
> 
I dont know the history as it pre-dates my work in upstream.
But one use-case I can think of is for command mode panel.

Typically, command mode panel will disable the clocks during idle screen.

Now, if usermode wants to enable vsync, it will crash in the driver.

So driver needs to make sure what whenever hw vsync is toggled on/off, 
the required clocks need to be enabled.
>>
>>> +             if (IS_ERR(ev_thread->worker)) {
>>> +                     ret = PTR_ERR(ev_thread->worker);
>>>                        DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
>>> -                     priv->event_thread[i].worker = NULL;
>>> +                     ev_thread->worker = NULL;
>>>                        goto err_msm_uninit;
>>>                }
>>>
>>> -             sched_set_fifo(priv->event_thread[i].worker->task);
>>> +             sched_set_fifo(ev_thread->worker->task);
>>>        }
>>>
>>>        ret = drm_vblank_init(ddev, priv->num_crtcs);
>>> @@ -563,25 +567,23 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
>>>    int msm_crtc_enable_vblank(struct drm_crtc *crtc)
>>>    {
>>>        struct drm_device *dev = crtc->dev;
>>> -     unsigned int pipe = crtc->index;
>>>        struct msm_drm_private *priv = dev->dev_private;
>>>        struct msm_kms *kms = priv->kms;
>>>        if (!kms)
>>>                return -ENXIO;
>>> -     drm_dbg_vbl(dev, "crtc=%u", pipe);
>>> -     return vblank_ctrl_queue_work(priv, pipe, true);
>>> +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
>>> +     return vblank_ctrl_queue_work(priv, crtc, true);
>>>    }
>>>
>>>    void msm_crtc_disable_vblank(struct drm_crtc *crtc)
>>>    {
>>>        struct drm_device *dev = crtc->dev;
>>> -     unsigned int pipe = crtc->index;
>>>        struct msm_drm_private *priv = dev->dev_private;
>>>        struct msm_kms *kms = priv->kms;
>>>        if (!kms)
>>>                return;
>>> -     drm_dbg_vbl(dev, "crtc=%u", pipe);
>>> -     vblank_ctrl_queue_work(priv, pipe, false);
>>> +     drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
>>> +     vblank_ctrl_queue_work(priv, crtc, false);
>>>    }
>>>
>>>    /*
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>>> index 08388d742d65..0e98b6f161df 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>> @@ -102,7 +102,7 @@ struct msm_display_topology {
>>>    /* Commit/Event thread specific structure */
>>>    struct msm_drm_thread {
>>>        struct drm_device *dev;
>>> -     unsigned int crtc_id;
>>> +     struct drm_crtc *crtc;
>>>        struct kthread_worker *worker;
>>>    };
>>>
>>> @@ -178,7 +178,6 @@ struct msm_drm_private {
>>>        struct workqueue_struct *wq;
>>>
>>>        unsigned int num_crtcs;
>>> -     struct drm_crtc *crtcs[MAX_CRTCS];
>>>
>>>        struct msm_drm_thread event_thread[MAX_CRTCS];
>>>
> 
> 
> 

Re: [PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

On Sat, 18 Jun 2022 02:33:24 +0300, Dmitry Baryshkov wrote:
> This patchseries replaces drm_handle_vblank() with
> drm_crtc_handle_vblank(). As a bonus result of this conversion it is
> possible to drop the stored array of allocated CRTCs and use the core
> CRTC iterators.
> 
> Changes since v5:
>  - Clean up the event_thread->worker in case of an error to fix possible
>    oops in msm_drm_uninit().
> 
> [...]

Applied, thanks!

[1/4] drm/msm: clean event_thread->worker in case of an error
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c79bb6b92def
[2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/6606a96ab1ce
[3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/e96c08e91726

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

On Sat, 18 Jun 2022 02:33:24 +0300, Dmitry Baryshkov wrote:
> This patchseries replaces drm_handle_vblank() with
> drm_crtc_handle_vblank(). As a bonus result of this conversion it is
> possible to drop the stored array of allocated CRTCs and use the core
> CRTC iterators.
> 
> Changes since v5:
>  - Clean up the event_thread->worker in case of an error to fix possible
>    oops in msm_drm_uninit().
> 
> [...]

Applied, thanks!

[1/4] drm/msm: clean event_thread->worker in case of an error
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c79bb6b92def
[2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/6606a96ab1ce
[3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/e96c08e91726

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

On Sat, 18 Jun 2022 02:33:24 +0300, Dmitry Baryshkov wrote:
> This patchseries replaces drm_handle_vblank() with
> drm_crtc_handle_vblank(). As a bonus result of this conversion it is
> possible to drop the stored array of allocated CRTCs and use the core
> CRTC iterators.
> 
> Changes since v5:
>  - Clean up the event_thread->worker in case of an error to fix possible
>    oops in msm_drm_uninit().
> 
> [...]

Applied, thanks!

[1/4] drm/msm: clean event_thread->worker in case of an error
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c79bb6b92def
[2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/6606a96ab1ce
[3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/e96c08e91726

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH v6 0/4] drm/msm: convet to drm_crtc_handle_vblank()

[Dmitry Baryshkov]

On Sat, 18 Jun 2022 02:33:24 +0300, Dmitry Baryshkov wrote:
> This patchseries replaces drm_handle_vblank() with
> drm_crtc_handle_vblank(). As a bonus result of this conversion it is
> possible to drop the stored array of allocated CRTCs and use the core
> CRTC iterators.
> 
> Changes since v5:
>  - Clean up the event_thread->worker in case of an error to fix possible
>    oops in msm_drm_uninit().
> 
> [...]

Applied, thanks!

[1/4] drm/msm: clean event_thread->worker in case of an error
      https://gitlab.freedesktop.org/lumag/msm/-/commit/c79bb6b92def
[2/4] drm/msm/mdp4: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/6606a96ab1ce
[3/4] drm/msm/mdp5: convert to drm_crtc_handle_vblank()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/e96c08e91726

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>