| Message ID | 20231201014101.15802-2-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Stabilize use of vblank_refcount | expand |
On Fri, 1 Dec 2023 at 03:41, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > When the irq callback returns a value other than zero, > modify vblank_refcount by performing the inverse > operation of its corresponding if-else condition. I think it might be better to follow Bjorn's suggestion: once we have the lock, we don't need atomics at all. Then you rearrange the code to set the new value after getting return code from dpu_core_irq_register_callback() / dpu_core_irq_unregister_callback(). > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +++++++-- > 2 files changed, 14 insertions(+), 4 deletions(-)
On 11/30/2023 11:45 PM, Dmitry Baryshkov wrote: > On Fri, 1 Dec 2023 at 03:41, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> When the irq callback returns a value other than zero, >> modify vblank_refcount by performing the inverse >> operation of its corresponding if-else condition. > > I think it might be better to follow Bjorn's suggestion: once we have > the lock, we don't need atomics at all. > Then you rearrange the code to set the new value after getting return > code from dpu_core_irq_register_callback() / > dpu_core_irq_unregister_callback(). > Even if we drop the atomics, we will have to replace it with a simple refcount. The refcount checks will be before calling dpu_core_irq_register_callback() / dpu_core_irq_unregister_callback(). So we will still need the corresponding inverse refcount when either of those calls fail but just that they wont be atomics. Am I missing something here? >> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +++++++-- >> 2 files changed, 14 insertions(+), 4 deletions(-) >
On Fri, 1 Dec 2023 at 21:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 11/30/2023 11:45 PM, Dmitry Baryshkov wrote: > > On Fri, 1 Dec 2023 at 03:41, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >> > >> When the irq callback returns a value other than zero, > >> modify vblank_refcount by performing the inverse > >> operation of its corresponding if-else condition. > > > > I think it might be better to follow Bjorn's suggestion: once we have > > the lock, we don't need atomics at all. > > Then you rearrange the code to set the new value after getting return > > code from dpu_core_irq_register_callback() / > > dpu_core_irq_unregister_callback(). > > > > Even if we drop the atomics, we will have to replace it with a simple > refcount. The refcount checks will be before calling > dpu_core_irq_register_callback() / dpu_core_irq_unregister_callback(). > > So we will still need the corresponding inverse refcount when either of > those calls fail but just that they wont be atomics. Within the critical section you can check the value before register_callback and increment it afterwards if registration succeeds. > > Am I missing something here? > > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++-- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +++++++-- > >> 2 files changed, 14 insertions(+), 4 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 be185fe69793b..25babfe1f001a 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 @@ -260,14 +260,19 @@ static int dpu_encoder_phys_cmd_control_vblank_irq( phys_enc->hw_pp->idx - PINGPONG_0, enable ? "true" : "false", refcount); - if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1) + if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1) { ret = dpu_core_irq_register_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_RDPTR], dpu_encoder_phys_cmd_te_rd_ptr_irq, phys_enc); - else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0) + if (ret) + atomic_dec(&phys_enc->vblank_refcount); + } else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0) { ret = dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_RDPTR]); + if (ret) + atomic_inc(&phys_enc->vblank_refcount); + } end: if (ret) { 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 a01fda7118835..8e905d7267f9f 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 @@ -379,14 +379,19 @@ static int dpu_encoder_phys_vid_control_vblank_irq( DRM_DEBUG_VBL("id:%u enable=%d/%d\n", DRMID(phys_enc->parent), enable, atomic_read(&phys_enc->vblank_refcount)); - if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1) + if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1) { ret = dpu_core_irq_register_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_VSYNC], dpu_encoder_phys_vid_vblank_irq, phys_enc); - else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0) + if (ret) + atomic_dec(&phys_enc->vblank_refcount); + } else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0) { ret = dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_VSYNC]); + if (ret) + atomic_inc(&phys_enc->vblank_refcount); + } end: if (ret) {
When the irq callback returns a value other than zero, modify vblank_refcount by performing the inverse operation of its corresponding if-else condition. Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++-- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-)