Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* drm/msm/dpu: Correct dpu encoder spinlock initialization
@ 2019-06-24  6:27 Shubhashree Dhar
  2019-06-24 22:26 ` Jeykumar Sankaran
  2019-07-22 18:20 ` Sean Paul
  0 siblings, 2 replies; 8+ messages in thread
From: Shubhashree Dhar @ 2019-06-24  6:27 UTC (permalink / raw)
  To: dri-devel, linux-arm-msm, freedreno, devicetree
  Cc: Shubhashree Dhar, linux-kernel, robdclark, seanpaul, hoegsberg,
	abhinavk, jsanka, chandanu, nganji, jshekhar

dpu encoder spinlock should be initialized during dpu encoder
init instead of dpu encoder setup which is part of commit.
There are chances that vblank control uses the uninitialized
spinlock if not initialized during encoder init.

Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5f085b5..22938c7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	if (ret)
 		goto fail;
 
-	spin_lock_init(&dpu_enc->enc_spinlock);
-
 	atomic_set(&dpu_enc->frame_done_timeout, 0);
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
@@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
 
 	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
 
+	spin_lock_init(&dpu_enc->enc_spinlock);
 	dpu_enc->enabled = false;
 
 	return &dpu_enc->base;
-- 
1.9.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-06-24  6:27 drm/msm/dpu: Correct dpu encoder spinlock initialization Shubhashree Dhar
@ 2019-06-24 22:26 ` Jeykumar Sankaran
  2019-06-25  5:44   ` dhar
  2019-07-22 18:20 ` Sean Paul
  1 sibling, 1 reply; 8+ messages in thread
From: Jeykumar Sankaran @ 2019-06-24 22:26 UTC (permalink / raw)
  To: Shubhashree Dhar
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-06-23 23:27, Shubhashree Dhar wrote:
> dpu encoder spinlock should be initialized during dpu encoder
> init instead of dpu encoder setup which is part of commit.
> There are chances that vblank control uses the uninitialized
> spinlock if not initialized during encoder init.
Not much can be done if someone is performing a vblank operation
before encoder_setup is done.
Can you point to the path where this lock is acquired before
the encoder_setup?

Thanks
Jeykumar S.
> 
> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 5f085b5..22938c7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, 
> struct
> drm_encoder *enc,
>  	if (ret)
>  		goto fail;
> 
> -	spin_lock_init(&dpu_enc->enc_spinlock);
> -
>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>  	timer_setup(&dpu_enc->frame_done_timer,
>  			dpu_encoder_frame_done_timeout, 0);
> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
> drm_device *dev,
> 
>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> 
> +	spin_lock_init(&dpu_enc->enc_spinlock);
>  	dpu_enc->enabled = false;
> 
>  	return &dpu_enc->base;

-- 
Jeykumar S

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-06-24 22:26 ` Jeykumar Sankaran
@ 2019-06-25  5:44   ` dhar
  2019-06-25 21:40     ` Jeykumar Sankaran
  0 siblings, 1 reply; 8+ messages in thread
From: dhar @ 2019-06-25  5:44 UTC (permalink / raw)
  To: Jeykumar Sankaran
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-06-25 03:56, Jeykumar Sankaran wrote:
> On 2019-06-23 23:27, Shubhashree Dhar wrote:
>> dpu encoder spinlock should be initialized during dpu encoder
>> init instead of dpu encoder setup which is part of commit.
>> There are chances that vblank control uses the uninitialized
>> spinlock if not initialized during encoder init.
> Not much can be done if someone is performing a vblank operation
> before encoder_setup is done.
> Can you point to the path where this lock is acquired before
> the encoder_setup?
> 
> Thanks
> Jeykumar S.
>> 

When running some dp usecase, we are hitting this callstack.

Process kworker/u16:8 (pid: 215, stack limit = 0x00000000df9dd930)
Call trace:
  spin_dump+0x84/0x8c
  spin_dump+0x0/0x8c
  do_raw_spin_lock+0x80/0xb0
  _raw_spin_lock_irqsave+0x34/0x44
  dpu_encoder_toggle_vblank_for_crtc+0x8c/0xe8
  dpu_crtc_vblank+0x168/0x1a0
  dpu_kms_enable_vblank+0[   11.648998]  vblank_ctrl_worker+0x3c/0x60
  process_one_work+0x16c/0x2d8
  worker_thread+0x1d8/0x2b0
  kthread+0x124/0x134

Looks like vblank is getting enabled earlier causing this issue and we 
are using the spinlock without initializing it.

Thanks,
Shubhashree

>> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
>> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 5f085b5..22938c7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct
>> drm_encoder *enc,
>>  	if (ret)
>>  		goto fail;
>> 
>> -	spin_lock_init(&dpu_enc->enc_spinlock);
>> -
>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>>  	timer_setup(&dpu_enc->frame_done_timer,
>>  			dpu_encoder_frame_done_timeout, 0);
>> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
>> drm_device *dev,
>> 
>>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>> 
>> +	spin_lock_init(&dpu_enc->enc_spinlock);
>>  	dpu_enc->enabled = false;
>> 
>>  	return &dpu_enc->base;



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-06-25  5:44   ` dhar
@ 2019-06-25 21:40     ` Jeykumar Sankaran
  2019-07-01 10:29       ` dhar
  0 siblings, 1 reply; 8+ messages in thread
From: Jeykumar Sankaran @ 2019-06-25 21:40 UTC (permalink / raw)
  To: dhar
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-06-24 22:44, dhar@codeaurora.org wrote:
> On 2019-06-25 03:56, Jeykumar Sankaran wrote:
>> On 2019-06-23 23:27, Shubhashree Dhar wrote:
>>> dpu encoder spinlock should be initialized during dpu encoder
>>> init instead of dpu encoder setup which is part of commit.
>>> There are chances that vblank control uses the uninitialized
>>> spinlock if not initialized during encoder init.
>> Not much can be done if someone is performing a vblank operation
>> before encoder_setup is done.
>> Can you point to the path where this lock is acquired before
>> the encoder_setup?
>> 
>> Thanks
>> Jeykumar S.
>>> 
> 
> When running some dp usecase, we are hitting this callstack.
> 
> Process kworker/u16:8 (pid: 215, stack limit = 0x00000000df9dd930)
> Call trace:
>  spin_dump+0x84/0x8c
>  spin_dump+0x0/0x8c
>  do_raw_spin_lock+0x80/0xb0
>  _raw_spin_lock_irqsave+0x34/0x44
>  dpu_encoder_toggle_vblank_for_crtc+0x8c/0xe8
>  dpu_crtc_vblank+0x168/0x1a0
>  dpu_kms_enable_vblank+0[   11.648998]  vblank_ctrl_worker+0x3c/0x60
>  process_one_work+0x16c/0x2d8
>  worker_thread+0x1d8/0x2b0
>  kthread+0x124/0x134
> 
> Looks like vblank is getting enabled earlier causing this issue and we
> are using the spinlock without initializing it.
> 
> Thanks,
> Shubhashree
> 
DP calls into set_encoder_mode during hotplug before even notifying the
u/s. Can you trace out the original caller of this stack?

Even though the patch is harmless, I am not entirely convinced to move 
this
initialization. Any call which acquires the lock before encoder_setup
will be a no-op since there will not be any physical encoder to work 
with.

Thanks and Regards,
Jeykumar S.

>>> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
>>> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
>>> ---
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 5f085b5..22938c7 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>> struct
>>> drm_encoder *enc,
>>>  	if (ret)
>>>  		goto fail;
>>> 
>>> -	spin_lock_init(&dpu_enc->enc_spinlock);
>>> -
>>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>>>  	timer_setup(&dpu_enc->frame_done_timer,
>>>  			dpu_encoder_frame_done_timeout, 0);
>>> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
>>> drm_device *dev,
>>> 
>>>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>> 
>>> +	spin_lock_init(&dpu_enc->enc_spinlock);
>>>  	dpu_enc->enabled = false;
>>> 
>>>  	return &dpu_enc->base;

-- 
Jeykumar S

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-06-25 21:40     ` Jeykumar Sankaran
@ 2019-07-01 10:29       ` dhar
  2019-07-02 18:21         ` Jeykumar Sankaran
  0 siblings, 1 reply; 8+ messages in thread
From: dhar @ 2019-07-01 10:29 UTC (permalink / raw)
  To: Jeykumar Sankaran
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-06-26 03:10, Jeykumar Sankaran wrote:
> On 2019-06-24 22:44, dhar@codeaurora.org wrote:
>> On 2019-06-25 03:56, Jeykumar Sankaran wrote:
>>> On 2019-06-23 23:27, Shubhashree Dhar wrote:
>>>> dpu encoder spinlock should be initialized during dpu encoder
>>>> init instead of dpu encoder setup which is part of commit.
>>>> There are chances that vblank control uses the uninitialized
>>>> spinlock if not initialized during encoder init.
>>> Not much can be done if someone is performing a vblank operation
>>> before encoder_setup is done.
>>> Can you point to the path where this lock is acquired before
>>> the encoder_setup?
>>> 
>>> Thanks
>>> Jeykumar S.
>>>> 
>> 
>> When running some dp usecase, we are hitting this callstack.
>> 
>> Process kworker/u16:8 (pid: 215, stack limit = 0x00000000df9dd930)
>> Call trace:
>>  spin_dump+0x84/0x8c
>>  spin_dump+0x0/0x8c
>>  do_raw_spin_lock+0x80/0xb0
>>  _raw_spin_lock_irqsave+0x34/0x44
>>  dpu_encoder_toggle_vblank_for_crtc+0x8c/0xe8
>>  dpu_crtc_vblank+0x168/0x1a0
>>  dpu_kms_enable_vblank+0[   11.648998]  vblank_ctrl_worker+0x3c/0x60
>>  process_one_work+0x16c/0x2d8
>>  worker_thread+0x1d8/0x2b0
>>  kthread+0x124/0x134
>> 
>> Looks like vblank is getting enabled earlier causing this issue and we
>> are using the spinlock without initializing it.
>> 
>> Thanks,
>> Shubhashree
>> 
> DP calls into set_encoder_mode during hotplug before even notifying the
> u/s. Can you trace out the original caller of this stack?
> 
> Even though the patch is harmless, I am not entirely convinced to move 
> this
> initialization. Any call which acquires the lock before encoder_setup
> will be a no-op since there will not be any physical encoder to work 
> with.
> 
> Thanks and Regards,
> Jeykumar S.
> 
>>>> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
>>>> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
>>>> ---
>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 5f085b5..22938c7 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>>> struct
>>>> drm_encoder *enc,
>>>>  	if (ret)
>>>>  		goto fail;
>>>> 
>>>> -	spin_lock_init(&dpu_enc->enc_spinlock);
>>>> -
>>>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>>>>  	timer_setup(&dpu_enc->frame_done_timer,
>>>>  			dpu_encoder_frame_done_timeout, 0);
>>>> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
>>>> drm_device *dev,
>>>> 
>>>>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>>> 
>>>> +	spin_lock_init(&dpu_enc->enc_spinlock);
>>>>  	dpu_enc->enabled = false;
>>>> 
>>>>  	return &dpu_enc->base;

In dpu_crtc_vblank(), we are looping through all the encoders in the 
present mode_config: 
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L1082
and hence calling dpu_encoder_toggle_vblank_for_crtc() for all the 
encoders. But in dpu_encoder_toggle_vblank_for_crtc(), after acquiring 
the spinlock, we will do a early return for
the encoders which are not currently assigned to our crtc: 
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1318.
Since the encoder_setup for the secondary encoder(dp encoder in this 
case) is not called until dp hotplug, we are hitting kernel panic while 
acquiring the lock.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-07-01 10:29       ` dhar
@ 2019-07-02 18:21         ` Jeykumar Sankaran
  2019-07-02 19:15           ` Jeykumar Sankaran
  0 siblings, 1 reply; 8+ messages in thread
From: Jeykumar Sankaran @ 2019-07-02 18:21 UTC (permalink / raw)
  To: dhar
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-07-01 03:29, dhar@codeaurora.org wrote:
> On 2019-06-26 03:10, Jeykumar Sankaran wrote:
>> On 2019-06-24 22:44, dhar@codeaurora.org wrote:
>>> On 2019-06-25 03:56, Jeykumar Sankaran wrote:
>>>> On 2019-06-23 23:27, Shubhashree Dhar wrote:
>>>>> dpu encoder spinlock should be initialized during dpu encoder
>>>>> init instead of dpu encoder setup which is part of commit.
>>>>> There are chances that vblank control uses the uninitialized
>>>>> spinlock if not initialized during encoder init.
>>>> Not much can be done if someone is performing a vblank operation
>>>> before encoder_setup is done.
>>>> Can you point to the path where this lock is acquired before
>>>> the encoder_setup?
>>>> 
>>>> Thanks
>>>> Jeykumar S.
>>>>> 
>>> 
>>> When running some dp usecase, we are hitting this callstack.
>>> 
>>> Process kworker/u16:8 (pid: 215, stack limit = 0x00000000df9dd930)
>>> Call trace:
>>>  spin_dump+0x84/0x8c
>>>  spin_dump+0x0/0x8c
>>>  do_raw_spin_lock+0x80/0xb0
>>>  _raw_spin_lock_irqsave+0x34/0x44
>>>  dpu_encoder_toggle_vblank_for_crtc+0x8c/0xe8
>>>  dpu_crtc_vblank+0x168/0x1a0
>>>  dpu_kms_enable_vblank+0[   11.648998]  vblank_ctrl_worker+0x3c/0x60
>>>  process_one_work+0x16c/0x2d8
>>>  worker_thread+0x1d8/0x2b0
>>>  kthread+0x124/0x134
>>> 
>>> Looks like vblank is getting enabled earlier causing this issue and 
>>> we
>>> are using the spinlock without initializing it.
>>> 
>>> Thanks,
>>> Shubhashree
>>> 
>> DP calls into set_encoder_mode during hotplug before even notifying 
>> the
>> u/s. Can you trace out the original caller of this stack?
>> 
>> Even though the patch is harmless, I am not entirely convinced to move 
>> this
>> initialization. Any call which acquires the lock before encoder_setup
>> will be a no-op since there will not be any physical encoder to work 
>> with.
>> 
>> Thanks and Regards,
>> Jeykumar S.
>> 
>>>>> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
>>>>> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
>>>>> ---
>>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> index 5f085b5..22938c7 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>>>> struct
>>>>> drm_encoder *enc,
>>>>>  	if (ret)
>>>>>  		goto fail;
>>>>> 
>>>>> -	spin_lock_init(&dpu_enc->enc_spinlock);
>>>>> -
>>>>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>>>>>  	timer_setup(&dpu_enc->frame_done_timer,
>>>>>  			dpu_encoder_frame_done_timeout, 0);
>>>>> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
>>>>> drm_device *dev,
>>>>> 
>>>>>  	drm_encoder_helper_add(&dpu_enc->base, 
>>>>> &dpu_encoder_helper_funcs);
>>>>> 
>>>>> +	spin_lock_init(&dpu_enc->enc_spinlock);
>>>>>  	dpu_enc->enabled = false;
>>>>> 
>>>>>  	return &dpu_enc->base;
> 
> In dpu_crtc_vblank(), we are looping through all the encoders in the
> present mode_config:
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu
> 1/dpu_crtc.c#L1082
> and hence calling dpu_encoder_toggle_vblank_for_crtc() for all the
> encoders. But in dpu_encoder_toggle_vblank_for_crtc(), after acquiring
> the spinlock, we will do a early return for
> the encoders which are not currently assigned to our crtc:
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu
> 1/dpu_encoder.c#L1318.
> Since the encoder_setup for the secondary encoder(dp encoder in this
> case) is not called until dp hotplug, we are hitting kernel panic
> while acquiring the lock.
This is the sequence in which the events are expected to happen:

1) DP connector is instantiated with an inactive state
2) Hot plug on DP
3) DP connector is activated
4) User space attaches a CRTC to the activated connector
5) CRTC is enabled
6) CRTC_VBLANK_ON is called
7) dpu_crtc_vblank is called.

So can you help tracing out why dpu_crtc_vblank is called when the 
connector
is not activated yet (no hotplug)?

-- 
Jeykumar S

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-07-02 18:21         ` Jeykumar Sankaran
@ 2019-07-02 19:15           ` Jeykumar Sankaran
  0 siblings, 0 replies; 8+ messages in thread
From: Jeykumar Sankaran @ 2019-07-02 19:15 UTC (permalink / raw)
  To: dhar
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, chandanu, nganji,
	jshekhar

On 2019-07-02 11:21, Jeykumar Sankaran wrote:
> On 2019-07-01 03:29, dhar@codeaurora.org wrote:
>> On 2019-06-26 03:10, Jeykumar Sankaran wrote:
>>> On 2019-06-24 22:44, dhar@codeaurora.org wrote:
>>>> On 2019-06-25 03:56, Jeykumar Sankaran wrote:
>>>>> On 2019-06-23 23:27, Shubhashree Dhar wrote:
>>>>>> dpu encoder spinlock should be initialized during dpu encoder
>>>>>> init instead of dpu encoder setup which is part of commit.
>>>>>> There are chances that vblank control uses the uninitialized
>>>>>> spinlock if not initialized during encoder init.
>>>>> Not much can be done if someone is performing a vblank operation
>>>>> before encoder_setup is done.
>>>>> Can you point to the path where this lock is acquired before
>>>>> the encoder_setup?
>>>>> 
>>>>> Thanks
>>>>> Jeykumar S.
>>>>>> 
>>>> 
>>>> When running some dp usecase, we are hitting this callstack.
>>>> 
>>>> Process kworker/u16:8 (pid: 215, stack limit = 0x00000000df9dd930)
>>>> Call trace:
>>>>  spin_dump+0x84/0x8c
>>>>  spin_dump+0x0/0x8c
>>>>  do_raw_spin_lock+0x80/0xb0
>>>>  _raw_spin_lock_irqsave+0x34/0x44
>>>>  dpu_encoder_toggle_vblank_for_crtc+0x8c/0xe8
>>>>  dpu_crtc_vblank+0x168/0x1a0
>>>>  dpu_kms_enable_vblank+0[   11.648998]  vblank_ctrl_worker+0x3c/0x60
>>>>  process_one_work+0x16c/0x2d8
>>>>  worker_thread+0x1d8/0x2b0
>>>>  kthread+0x124/0x134
>>>> 
>>>> Looks like vblank is getting enabled earlier causing this issue and 
>>>> we
>>>> are using the spinlock without initializing it.
>>>> 
>>>> Thanks,
>>>> Shubhashree
>>>> 
>>> DP calls into set_encoder_mode during hotplug before even notifying 
>>> the
>>> u/s. Can you trace out the original caller of this stack?
>>> 
>>> Even though the patch is harmless, I am not entirely convinced to 
>>> move this
>>> initialization. Any call which acquires the lock before encoder_setup
>>> will be a no-op since there will not be any physical encoder to work 
>>> with.
>>> 
>>> Thanks and Regards,
>>> Jeykumar S.
>>> 
>>>>>> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
>>>>>> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> index 5f085b5..22938c7 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device 
>>>>>> *dev, struct
>>>>>> drm_encoder *enc,
>>>>>>  	if (ret)
>>>>>>  		goto fail;
>>>>>> 
>>>>>> -	spin_lock_init(&dpu_enc->enc_spinlock);
>>>>>> -
>>>>>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>>>>>>  	timer_setup(&dpu_enc->frame_done_timer,
>>>>>>  			dpu_encoder_frame_done_timeout, 0);
>>>>>> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct
>>>>>> drm_device *dev,
>>>>>> 
>>>>>>  	drm_encoder_helper_add(&dpu_enc->base, 
>>>>>> &dpu_encoder_helper_funcs);
>>>>>> 
>>>>>> +	spin_lock_init(&dpu_enc->enc_spinlock);
>>>>>>  	dpu_enc->enabled = false;
>>>>>> 
>>>>>>  	return &dpu_enc->base;
>> 
>> In dpu_crtc_vblank(), we are looping through all the encoders in the
>> present mode_config:
>> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu
>> 1/dpu_crtc.c#L1082
>> and hence calling dpu_encoder_toggle_vblank_for_crtc() for all the
>> encoders. But in dpu_encoder_toggle_vblank_for_crtc(), after acquiring
>> the spinlock, we will do a early return for
>> the encoders which are not currently assigned to our crtc:
>> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/disp/dpu
>> 1/dpu_encoder.c#L1318.
>> Since the encoder_setup for the secondary encoder(dp encoder in this
>> case) is not called until dp hotplug, we are hitting kernel panic
>> while acquiring the lock.
> This is the sequence in which the events are expected to happen:
> 
> 1) DP connector is instantiated with an inactive state
> 2) Hot plug on DP
> 3) DP connector is activated
> 4) User space attaches a CRTC to the activated connector
> 5) CRTC is enabled
> 6) CRTC_VBLANK_ON is called
> 7) dpu_crtc_vblank is called.
> 
> So can you help tracing out why dpu_crtc_vblank is called when the 
> connector
> is not activated yet (no hotplug)?

Overlooked the loop which iterates through *all* the encoders 
irrespective of their
activated status.

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: drm/msm/dpu: Correct dpu encoder spinlock initialization
  2019-06-24  6:27 drm/msm/dpu: Correct dpu encoder spinlock initialization Shubhashree Dhar
  2019-06-24 22:26 ` Jeykumar Sankaran
@ 2019-07-22 18:20 ` Sean Paul
  1 sibling, 0 replies; 8+ messages in thread
From: Sean Paul @ 2019-07-22 18:20 UTC (permalink / raw)
  To: Shubhashree Dhar
  Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
	robdclark, seanpaul, hoegsberg, abhinavk, jsanka, chandanu,
	nganji, jshekhar

On Mon, Jun 24, 2019 at 11:57:12AM +0530, Shubhashree Dhar wrote:
> dpu encoder spinlock should be initialized during dpu encoder
> init instead of dpu encoder setup which is part of commit.
> There are chances that vblank control uses the uninitialized
> spinlock if not initialized during encoder init.
> 
> Change-Id: I5a18b95fa47397c834a266b22abf33a517b03a4e
> Signed-off-by: Shubhashree Dhar <dhar@codeaurora.org>

Thanks for your patch.

I've resolved the conflict and tweaked the commit message a bit to reflect
current reality.

Applied to drm-misc-fixes for 5.3

Sean

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 5f085b5..22938c7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2195,8 +2195,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>  	if (ret)
>  		goto fail;
>  
> -	spin_lock_init(&dpu_enc->enc_spinlock);
> -
>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>  	timer_setup(&dpu_enc->frame_done_timer,
>  			dpu_encoder_frame_done_timeout, 0);
> @@ -2250,6 +2248,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>  
>  	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>  
> +	spin_lock_init(&dpu_enc->enc_spinlock);
>  	dpu_enc->enabled = false;
>  
>  	return &dpu_enc->base;
> -- 
> 1.9.1
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-24  6:27 drm/msm/dpu: Correct dpu encoder spinlock initialization Shubhashree Dhar
2019-06-24 22:26 ` Jeykumar Sankaran
2019-06-25  5:44   ` dhar
2019-06-25 21:40     ` Jeykumar Sankaran
2019-07-01 10:29       ` dhar
2019-07-02 18:21         ` Jeykumar Sankaran
2019-07-02 19:15           ` Jeykumar Sankaran
2019-07-22 18:20 ` Sean Paul

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox