Re: [PATCH] drm/msm: dpu: Don't enable vblank interrupts in irq_control

[Abhinav Kumar <abhinavk@codeaurora.org>]

On 2018-12-07 13:50, Sean Paul wrote:
> On Fri, Dec 07, 2018 at 01:16:54PM -0800, Abhinav Kumar wrote:
>> On 2018-12-07 09:16, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > The irq_control function is called upon encoder enable/disable and turns
>> > on/off the vblank interrupts. Unfortunately, it enables them when the
>> > drm code is not expecting them to be on. As a result, we can get into
>> > nasty locking situations.
>> >
>> > vblank interrupts should be solely managed by the drm core via
>> > control_vblank_irq(). This patch removes the vblank irq handling from
>> > irq_control.
>> >
>> > Here's some tracing before this patch:
>> > -- CRTC is enabled, irq_control is called, drm_vblank reference is 0
>> >    25.078455: dpu_crtc_enable: id:46 enable:true state{enabled:false}
>> > -- vblank irqs incorrectly firing, drm_crtc_handle_vblank is called from
>> > them
>> >    25.290067: dpu_crtc_vblank_cb: id=46
>> >    25.306691: dpu_crtc_vblank_cb: id=46
>> > -- drm core enables vblank interrupts, drm_vblank_reference > 0
>> >    25.311527: dpu_crtc_vblank: id:46 enable:true state{enabled:true}
>> > -- vblank irqs continue (correctly)
>> >    25.323351: dpu_crtc_vblank_cb: id=46
>> >    25.340033: dpu_crtc_vblank_cb: id=46
>> >    25.356701: dpu_crtc_vblank_cb: id=46
>> >
>> > And after this patch:
>> > -- CRTC is enabled, irq_control does not enable vblank interrupts
>> >   123.861353: dpu_crtc_enable: id:46 enable:true state{enabled:false}
>> > -- drm core enables vblank, we enable the irq
>> >   124.192916: dpu_crtc_vblank: id:46 enable:true state{enabled:true}
>> > -- vblank irqs start at the correct time
>> >   124.200548: dpu_crtc_vblank_cb: id=46
>> >   124.217195: dpu_crtc_vblank_cb: id=46
>> >   124.233848: dpu_crtc_vblank_cb: id=46
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> 
>> With this change "FROMLIST: drm/msm: Grab a vblank reference when 
>> waiting
>> for commit_done"
>> in place, this patch looks fine to me as we are protected to disable 
>> vblank
>> right after commit hence cases such as idle power collapse for smart 
>> panels
>> ideally
>> shouldnt get impacted.
>> 
>> However, since we are not validating smart panels do we want to remove
>> changes to dpu_encoder_phys_cmd.c and just keep it for video mode?
> 
> We really don't want the vblank handler running when it's not supposed
> to. So having the irq always enabled may be covering up another bug in
> cmd mode, but I'd much rather fix this now and deal with regressions
> if they surface than leave a known issue in the code.
> 
> Sean
Alright, in that case no further concerns from my side
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> 
>> 
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 --
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 6 ------
>> >  2 files changed, 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> > index 99ab5ca9bed3..c9eaa3516e9a 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> > @@ -339,7 +339,6 @@ static void
>> > dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
>> >  	if (enable) {
>> >  		dpu_encoder_helper_register_irq(phys_enc, INTR_IDX_PINGPONG);
>> >  		dpu_encoder_helper_register_irq(phys_enc, INTR_IDX_UNDERRUN);
>> > -		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, true);
>> >
>> >  		if (dpu_encoder_phys_cmd_is_master(phys_enc))
>> >  			dpu_encoder_helper_register_irq(phys_enc,
>> > @@ -350,7 +349,6 @@ static void
>> > dpu_encoder_phys_cmd_irq_control(struct dpu_encoder_phys *phys_enc,
>> >  					INTR_IDX_CTL_START);
>> >
>> >  		dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_UNDERRUN);
>> > -		dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
>> >  		dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_PINGPONG);
>> >  	}
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > index acdab5b0db18..e3125a1ab3dc 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > @@ -706,7 +706,6 @@ static void
>> > dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
>> >  		bool enable)
>> >  {
>> >  	struct dpu_encoder_phys_vid *vid_enc;
>> > -	int ret;
>> >
>> >  	if (!phys_enc)
>> >  		return;
>> > @@ -719,13 +718,8 @@ static void
>> > dpu_encoder_phys_vid_irq_control(struct dpu_encoder_phys *phys_enc,
>> >  			    atomic_read(&phys_enc->vblank_refcount));
>> >
>> >  	if (enable) {
>> > -		ret = dpu_encoder_phys_vid_control_vblank_irq(phys_enc, true);
>> > -		if (ret)
>> > -			return;
>> > -
>> >  		dpu_encoder_helper_register_irq(phys_enc, INTR_IDX_UNDERRUN);
>> >  	} else {
>> > -		dpu_encoder_phys_vid_control_vblank_irq(phys_enc, false);
>> >  		dpu_encoder_helper_unregister_irq(phys_enc, INTR_IDX_UNDERRUN);
>> >  	}
>> >  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel