| Message ID | 1675091494-13988-14-git-send-email-quic_vpolimer@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PSR support for eDP | expand |
On 30/01/2023 17:11, Vinod Polimera wrote: > Enable PSR on eDP interface using drm self-refresh librabry. > This patch uses a trigger from self-refresh library to enter/exit > into PSR, when there are no updates from framework. > > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 ++++++++++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > 3 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index f29a339..60e5984 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -21,6 +21,7 @@ > #include <drm/drm_probe_helper.h> > #include <drm/drm_rect.h> > #include <drm/drm_vblank.h> > +#include <drm/drm_self_refresh_helper.h> > > #include "dpu_kms.h" > #include "dpu_hw_lm.h" > @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > + if (old_crtc_state->self_refresh_active) > + return; > + I have been looking at the crtc_needs_disable(). It explicitly mentions that 'We also need to run through the crtc_funcs->disable() function [..] if it's transitioning to self refresh mode...'. Don't we need to perform some cleanup here (like disabling the vblank irq handling, freeing the bandwidth, etc)? > /* Disable/save vblank irq handling */ > drm_crtc_vblank_off(crtc); > > @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, > { > struct drm_crtc *crtc = NULL; > struct dpu_crtc *dpu_crtc = NULL; > - int i; > + int i, ret; > > dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); > if (!dpu_crtc) > @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, > /* initialize event handling */ > spin_lock_init(&dpu_crtc->event_lock); > > + ret = drm_self_refresh_helper_init(crtc); > + if (ret) { > + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", > + crtc->name, ret); > + return ERR_PTR(ret); > + } > + > DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name); > return crtc; > } > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 01b7509..450abb1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -12,6 +12,7 @@ > #include <linux/kthread.h> > #include <linux/seq_file.h> > > +#include <drm/drm_atomic.h> > #include <drm/drm_crtc.h> > #include <drm/drm_file.h> > #include <drm/drm_probe_helper.h> > @@ -1212,11 +1213,24 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, > struct drm_atomic_state *state) > { > struct dpu_encoder_virt *dpu_enc = NULL; > + struct drm_crtc *crtc; > + struct drm_crtc_state *old_state = NULL; > int i = 0; > > dpu_enc = to_dpu_encoder_virt(drm_enc); > DPU_DEBUG_ENC(dpu_enc, "\n"); > > + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); > + if (crtc) > + old_state = drm_atomic_get_old_crtc_state(state, crtc); > + > + /* > + * The encoder is already disabled if self refresh mode was set earlier, > + * in the old_state for the corresponding crtc. > + */ > + if (old_state && old_state->self_refresh_active) > + return; > + Again the same question here, doesn't crtc_needs_disable() take care of this clause? I might be missing something in the PSR state transitions. Could you please add some explanation here? > mutex_lock(&dpu_enc->enc_lock); > dpu_enc->enabled = false; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index a683bd9..681dd2e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -491,7 +491,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, > return; > } > > - if (!crtc->state->active) { > + if (!drm_atomic_crtc_effectively_active(crtc->state)) { > DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); > return; > }
> -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Tuesday, January 31, 2023 6:29 PM > To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- > devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; > freedreno@lists.freedesktop.org; devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; > dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) > <quic_kalyant@quicinc.com>; Kuogee Hsieh (QUIC) > <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC) > <quic_vproddut@quicinc.com>; Bjorn Andersson (QUIC) > <quic_bjorande@quicinc.com>; Abhinav Kumar (QUIC) > <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC) > <quic_sbillaka@quicinc.com> > Subject: Re: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP > interface in dpu driver > > > On 30/01/2023 17:11, Vinod Polimera wrote: > > Enable PSR on eDP interface using drm self-refresh librabry. > > This patch uses a trigger from self-refresh library to enter/exit > > into PSR, when there are no updates from framework. > > > > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 ++++++++++++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > > 3 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index f29a339..60e5984 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -21,6 +21,7 @@ > > #include <drm/drm_probe_helper.h> > > #include <drm/drm_rect.h> > > #include <drm/drm_vblank.h> > > +#include <drm/drm_self_refresh_helper.h> > > > > #include "dpu_kms.h" > > #include "dpu_hw_lm.h" > > @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > > > + if (old_crtc_state->self_refresh_active) > > + return; > > + > > I have been looking at the crtc_needs_disable(). It explicitly mentions > that 'We also need to run through the crtc_funcs->disable() function > [..] if it's transitioning to self refresh mode...'. Don't we need to > perform some cleanup here (like disabling the vblank irq handling, > freeing the bandwidth, etc)? When self refresh active is enabled, then we will clean up irq handling and bandwidth etc. The above case is to handle display off commit triggered when we are in psr as all the resources are already cleaned up . we just need to do an early return. > > > /* Disable/save vblank irq handling */ > > drm_crtc_vblank_off(crtc); > > > > @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device > *dev, struct drm_plane *plane, > > { > > struct drm_crtc *crtc = NULL; > > struct dpu_crtc *dpu_crtc = NULL; > > - int i; > > + int i, ret; > > > > dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); > > if (!dpu_crtc) > > @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct > drm_device *dev, struct drm_plane *plane, > > /* initialize event handling */ > > spin_lock_init(&dpu_crtc->event_lock); > > > > + ret = drm_self_refresh_helper_init(crtc); > > + if (ret) { > > + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", > > + crtc->name, ret); > > + return ERR_PTR(ret); > > + } > > + > > DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc- > >name); > > return crtc; > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 01b7509..450abb1 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -12,6 +12,7 @@ > > #include <linux/kthread.h> > > #include <linux/seq_file.h> > > > > +#include <drm/drm_atomic.h> > > #include <drm/drm_crtc.h> > > #include <drm/drm_file.h> > > #include <drm/drm_probe_helper.h> > > @@ -1212,11 +1213,24 @@ static void > dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, > > struct drm_atomic_state *state) > > { > > struct dpu_encoder_virt *dpu_enc = NULL; > > + struct drm_crtc *crtc; > > + struct drm_crtc_state *old_state = NULL; > > int i = 0; > > > > dpu_enc = to_dpu_encoder_virt(drm_enc); > > DPU_DEBUG_ENC(dpu_enc, "\n"); > > > > + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); > > + if (crtc) > > + old_state = drm_atomic_get_old_crtc_state(state, crtc); > > + > > + /* > > + * The encoder is already disabled if self refresh mode was set earlier, > > + * in the old_state for the corresponding crtc. > > + */ > > + if (old_state && old_state->self_refresh_active) > > + return; > > + > > Again the same question here, doesn't crtc_needs_disable() take care of > this clause? I might be missing something in the PSR state transitions. > Could you please add some explanation here? Same usecase as above, applies to encoder disable also when triggered via disable commit When driver is in psr state. > > > mutex_lock(&dpu_enc->enc_lock); > > dpu_enc->enabled = false; > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index a683bd9..681dd2e 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -491,7 +491,7 @@ static void dpu_kms_wait_for_commit_done(struct > msm_kms *kms, > > return; > > } > > > > - if (!crtc->state->active) { > > + if (!drm_atomic_crtc_effectively_active(crtc->state)) { > > DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); > > return; > > } > > -- > With best wishes > Dmitry Thanks, Vinod P.
On 07/02/2023 16:26, Vinod Polimera wrote: > > >> -----Original Message----- >> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Sent: Tuesday, January 31, 2023 6:29 PM >> To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- >> devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; >> freedreno@lists.freedesktop.org; devicetree@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; >> dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) >> <quic_kalyant@quicinc.com>; Kuogee Hsieh (QUIC) >> <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC) >> <quic_vproddut@quicinc.com>; Bjorn Andersson (QUIC) >> <quic_bjorande@quicinc.com>; Abhinav Kumar (QUIC) >> <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC) >> <quic_sbillaka@quicinc.com> >> Subject: Re: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP >> interface in dpu driver >> >> >> On 30/01/2023 17:11, Vinod Polimera wrote: >>> Enable PSR on eDP interface using drm self-refresh librabry. >>> This patch uses a trigger from self-refresh library to enter/exit >>> into PSR, when there are no updates from framework. >>> >>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 ++++++++++++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- >>> 3 files changed, 27 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> index f29a339..60e5984 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> @@ -21,6 +21,7 @@ >>> #include <drm/drm_probe_helper.h> >>> #include <drm/drm_rect.h> >>> #include <drm/drm_vblank.h> >>> +#include <drm/drm_self_refresh_helper.h> >>> >>> #include "dpu_kms.h" >>> #include "dpu_hw_lm.h" >>> @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, >>> >>> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); >>> >>> + if (old_crtc_state->self_refresh_active) >>> + return; >>> + >> >> I have been looking at the crtc_needs_disable(). It explicitly mentions >> that 'We also need to run through the crtc_funcs->disable() function >> [..] if it's transitioning to self refresh mode...'. Don't we need to >> perform some cleanup here (like disabling the vblank irq handling, >> freeing the bandwidth, etc)? > > When self refresh active is enabled, then we will clean up irq handling and bandwidth etc. > The above case is to handle display off commit triggered when we are in psr as all > the resources are already cleaned up . we just need to do an early return. >> >>> /* Disable/save vblank irq handling */ >>> drm_crtc_vblank_off(crtc); >>> >>> @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device >> *dev, struct drm_plane *plane, >>> { >>> struct drm_crtc *crtc = NULL; >>> struct dpu_crtc *dpu_crtc = NULL; >>> - int i; >>> + int i, ret; >>> >>> dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); >>> if (!dpu_crtc) >>> @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct >> drm_device *dev, struct drm_plane *plane, >>> /* initialize event handling */ >>> spin_lock_init(&dpu_crtc->event_lock); >>> >>> + ret = drm_self_refresh_helper_init(crtc); >>> + if (ret) { >>> + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", >>> + crtc->name, ret); >>> + return ERR_PTR(ret); >>> + } >>> + >>> DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc- >>> name); >>> return crtc; >>> } >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 01b7509..450abb1 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -12,6 +12,7 @@ >>> #include <linux/kthread.h> >>> #include <linux/seq_file.h> >>> >>> +#include <drm/drm_atomic.h> >>> #include <drm/drm_crtc.h> >>> #include <drm/drm_file.h> >>> #include <drm/drm_probe_helper.h> >>> @@ -1212,11 +1213,24 @@ static void >> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, >>> struct drm_atomic_state *state) >>> { >>> struct dpu_encoder_virt *dpu_enc = NULL; >>> + struct drm_crtc *crtc; >>> + struct drm_crtc_state *old_state = NULL; >>> int i = 0; >>> >>> dpu_enc = to_dpu_encoder_virt(drm_enc); >>> DPU_DEBUG_ENC(dpu_enc, "\n"); >>> >>> + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); >>> + if (crtc) >>> + old_state = drm_atomic_get_old_crtc_state(state, crtc); >>> + >>> + /* >>> + * The encoder is already disabled if self refresh mode was set earlier, >>> + * in the old_state for the corresponding crtc. >>> + */ >>> + if (old_state && old_state->self_refresh_active) >>> + return; >>> + >> >> Again the same question here, doesn't crtc_needs_disable() take care of >> this clause? I might be missing something in the PSR state transitions. >> Could you please add some explanation here? > Same usecase as above, applies to encoder disable also when triggered via disable commit > When driver is in psr state. Ack, thank you for the explanations. I'd like to take another glance later today, but generally it look good to me.
On 07/02/2023 17:25, Dmitry Baryshkov wrote: > On 07/02/2023 16:26, Vinod Polimera wrote: >> >> >>> -----Original Message----- >>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Sent: Tuesday, January 31, 2023 6:29 PM >>> To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- >>> devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; >>> freedreno@lists.freedesktop.org; devicetree@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; >>> dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) >>> <quic_kalyant@quicinc.com>; Kuogee Hsieh (QUIC) >>> <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC) >>> <quic_vproddut@quicinc.com>; Bjorn Andersson (QUIC) >>> <quic_bjorande@quicinc.com>; Abhinav Kumar (QUIC) >>> <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC) >>> <quic_sbillaka@quicinc.com> >>> Subject: Re: [PATCH v12 13/14] drm/msm/disp/dpu: add PSR support for eDP >>> interface in dpu driver >>> >>> >>> On 30/01/2023 17:11, Vinod Polimera wrote: >>>> Enable PSR on eDP interface using drm self-refresh librabry. >>>> This patch uses a trigger from self-refresh library to enter/exit >>>> into PSR, when there are no updates from framework. >>>> >>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>>> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 ++++++++++++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- >>>> 3 files changed, 27 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> index f29a339..60e5984 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> @@ -21,6 +21,7 @@ >>>> #include <drm/drm_probe_helper.h> >>>> #include <drm/drm_rect.h> >>>> #include <drm/drm_vblank.h> >>>> +#include <drm/drm_self_refresh_helper.h> >>>> >>>> #include "dpu_kms.h" >>>> #include "dpu_hw_lm.h" >>>> @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc >>>> *crtc, >>>> >>>> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); >>>> >>>> + if (old_crtc_state->self_refresh_active) >>>> + return; >>>> + >>> >>> I have been looking at the crtc_needs_disable(). It explicitly mentions >>> that 'We also need to run through the crtc_funcs->disable() function >>> [..] if it's transitioning to self refresh mode...'. Don't we need to >>> perform some cleanup here (like disabling the vblank irq handling, >>> freeing the bandwidth, etc)? >> >> When self refresh active is enabled, then we will clean up irq >> handling and bandwidth etc. >> The above case is to handle display off commit triggered when we are >> in psr as all >> the resources are already cleaned up . we just need to do an early >> return. >>> >>>> /* Disable/save vblank irq handling */ >>>> drm_crtc_vblank_off(crtc); >>>> >>>> @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device >>> *dev, struct drm_plane *plane, >>>> { >>>> struct drm_crtc *crtc = NULL; >>>> struct dpu_crtc *dpu_crtc = NULL; >>>> - int i; >>>> + int i, ret; >>>> >>>> dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); >>>> if (!dpu_crtc) >>>> @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct >>> drm_device *dev, struct drm_plane *plane, >>>> /* initialize event handling */ >>>> spin_lock_init(&dpu_crtc->event_lock); >>>> >>>> + ret = drm_self_refresh_helper_init(crtc); >>>> + if (ret) { >>>> + DPU_ERROR("Failed to initialize %s with self-refresh >>>> helpers %d\n", >>>> + crtc->name, ret); >>>> + return ERR_PTR(ret); >>>> + } >>>> + >>>> DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc- >>>> name); >>>> return crtc; >>>> } >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 01b7509..450abb1 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -12,6 +12,7 @@ >>>> #include <linux/kthread.h> >>>> #include <linux/seq_file.h> >>>> >>>> +#include <drm/drm_atomic.h> >>>> #include <drm/drm_crtc.h> >>>> #include <drm/drm_file.h> >>>> #include <drm/drm_probe_helper.h> >>>> @@ -1212,11 +1213,24 @@ static void >>> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, >>>> struct drm_atomic_state *state) >>>> { >>>> struct dpu_encoder_virt *dpu_enc = NULL; >>>> + struct drm_crtc *crtc; >>>> + struct drm_crtc_state *old_state = NULL; >>>> int i = 0; >>>> >>>> dpu_enc = to_dpu_encoder_virt(drm_enc); >>>> DPU_DEBUG_ENC(dpu_enc, "\n"); >>>> >>>> + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); >>>> + if (crtc) >>>> + old_state = drm_atomic_get_old_crtc_state(state, crtc); >>>> + >>>> + /* >>>> + * The encoder is already disabled if self refresh mode was >>>> set earlier, >>>> + * in the old_state for the corresponding crtc. >>>> + */ >>>> + if (old_state && old_state->self_refresh_active) >>>> + return; >>>> + >>> >>> Again the same question here, doesn't crtc_needs_disable() take care of >>> this clause? I might be missing something in the PSR state transitions. >>> Could you please add some explanation here? >> Same usecase as above, applies to encoder disable also when triggered >> via disable commit >> When driver is in psr state. > > Ack, thank you for the explanations. I'd like to take another glance > later today, but generally it look good to me. After another glance it still looks good to me. Please send the last iteration of the series: - moving all core patches to the first place, as it was asked previously. This will help us get them merged to drm core repo first - dropping the patch 09 as agreed.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index f29a339..60e5984 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -21,6 +21,7 @@ #include <drm/drm_probe_helper.h> #include <drm/drm_rect.h> #include <drm/drm_vblank.h> +#include <drm/drm_self_refresh_helper.h> #include "dpu_kms.h" #include "dpu_hw_lm.h" @@ -1021,6 +1022,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); + if (old_crtc_state->self_refresh_active) + return; + /* Disable/save vblank irq handling */ drm_crtc_vblank_off(crtc); @@ -1577,7 +1581,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, { struct drm_crtc *crtc = NULL; struct dpu_crtc *dpu_crtc = NULL; - int i; + int i, ret; dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); if (!dpu_crtc) @@ -1614,6 +1618,13 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, /* initialize event handling */ spin_lock_init(&dpu_crtc->event_lock); + ret = drm_self_refresh_helper_init(crtc); + if (ret) { + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", + crtc->name, ret); + return ERR_PTR(ret); + } + DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name); return crtc; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 01b7509..450abb1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -12,6 +12,7 @@ #include <linux/kthread.h> #include <linux/seq_file.h> +#include <drm/drm_atomic.h> #include <drm/drm_crtc.h> #include <drm/drm_file.h> #include <drm/drm_probe_helper.h> @@ -1212,11 +1213,24 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, struct drm_atomic_state *state) { struct dpu_encoder_virt *dpu_enc = NULL; + struct drm_crtc *crtc; + struct drm_crtc_state *old_state = NULL; int i = 0; dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n"); + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); + if (crtc) + old_state = drm_atomic_get_old_crtc_state(state, crtc); + + /* + * The encoder is already disabled if self refresh mode was set earlier, + * in the old_state for the corresponding crtc. + */ + if (old_state && old_state->self_refresh_active) + return; + mutex_lock(&dpu_enc->enc_lock); dpu_enc->enabled = false; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index a683bd9..681dd2e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -491,7 +491,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, return; } - if (!crtc->state->active) { + if (!drm_atomic_crtc_effectively_active(crtc->state)) { DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); return; }