[PATCH v1 0/3] Fixes for PSR

[PATCH v1 0/3] Fixes for PSR

[Vinod Polimera]

while in virtual terminal with PSR enabled, there will be
no atomic commits triggered resulting in no screen update.
Update the dirtyfb flag into plane state during atomic check 
to flush the pixel data explicitly.

Avoid scheduling PSR commits from different work queues while
running in PSR mode already.

Vinod Polimera (3):
  drm/msm/dpu: set dirty_fb flag while in self refresh mode
  msm/disp/dpu: allow atomic_check in PSR usecase
  msm: skip the atomic commit of self refresh while PSR running

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 5 ++++-
 drivers/gpu/drm/msm/msm_atomic.c         | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Vinod Polimera]

While in virtual terminal mode with PSR enabled, there will be
no atomic commits triggered without dirty_fb being set. This
will create a notion of no screen update. Allow atomic commit
when dirty_fb ioctl is issued, so that it can trigger a PSR exit
and shows update on the screen.

Reported-by: Bjorn Andersson <andersson@kernel.org>
Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ab636da..96f645e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
 	struct drm_crtc *crtc = cstate->crtc;
 	struct drm_encoder *encoder;
 
+	if (cstate->self_refresh_active)
+		return true;
+
 	drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) {
 		if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) {
 			return true;
-- 
2.7.4

Re: [PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>
> While in virtual terminal mode with PSR enabled, there will be
> no atomic commits triggered without dirty_fb being set. This
> will create a notion of no screen update. Allow atomic commit
> when dirty_fb ioctl is issued, so that it can trigger a PSR exit
> and shows update on the screen.

Will this impact non-VT workloads? If I remember correctly, we added
dirty_fb handling to prevent the framework from limiting the page
flips to vblank events (in DSI video mode).

>
> Reported-by: Bjorn Andersson <andersson@kernel.org>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index ab636da..96f645e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
>         struct drm_crtc *crtc = cstate->crtc;
>         struct drm_encoder *encoder;
>
> +       if (cstate->self_refresh_active)
> +               return true;
> +
>         drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) {
>                 if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) {
>                         return true;
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

RE: [PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Vinod Polimera]

> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com>
> wrote:
> >
> > While in virtual terminal mode with PSR enabled, there will be
> > no atomic commits triggered without dirty_fb being set. This
> > will create a notion of no screen update. Allow atomic commit
> > when dirty_fb ioctl is issued, so that it can trigger a PSR exit
> > and shows update on the screen.
> 
> Will this impact non-VT workloads? If I remember correctly, we added
> dirty_fb handling to prevent the framework from limiting the page
> flips to vblank events (in DSI video mode).
> 
From the use case referred in the commit text ( 9e4dde28e  drm/msm: Avoid dirtyfb stalls on video mode displays (v2)).
There can be an impact on the workload. I can think of two solutions:
1) Add modparam to configure PSR wait time (SELF_REFRESH_AVG_SEED_MS 200) and application can set it to "-1" if they are operating on dirty_fb
2) In msm drivers, disable psr if dirty_fb_ioctl is requested from the framework and re-enable it during regular commit.
    This will involve copying dirtyflags from drm fb ->msm_fb->dpu_plane and add atomic_check failure if ( dirty_flags &&  self_refresh_active).

Please let me know your thoughts on the above two.

Thanks,
Vinod.

> >
> > Reported-by: Bjorn Andersson <andersson@kernel.org>
> > Link:
> https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index ab636da..96f645e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct
> drm_crtc_state *cstate)
> >         struct drm_crtc *crtc = cstate->crtc;
> >         struct drm_encoder *encoder;
> >
> > +       if (cstate->self_refresh_active)
> > +               return true;
> > +
> >         drm_for_each_encoder_mask (encoder, crtc->dev, cstate-
> >encoder_mask) {
> >                 if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) {
> >                         return true;
> > --
> > 2.7.4
> >
> 
> 
> --
> With best wishes
> Dmitry

Re: [PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Dmitry Baryshkov]

On 31/03/2023 17:45, Dmitry Baryshkov wrote:
> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>>
>> While in virtual terminal mode with PSR enabled, there will be
>> no atomic commits triggered without dirty_fb being set. This
>> will create a notion of no screen update. Allow atomic commit
>> when dirty_fb ioctl is issued, so that it can trigger a PSR exit
>> and shows update on the screen.
> 
> Will this impact non-VT workloads? If I remember correctly, we added
> dirty_fb handling to prevent the framework from limiting the page
> flips to vblank events (in DSI video mode).

Actually, this is kind of stupid. If we care about the workload of this 
pipe, then it is being updated, which means it is not in SR mode, 
self_refresh_active = false.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
>>
>> Reported-by: Bjorn Andersson <andersson@kernel.org>
>> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
>> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index ab636da..96f645e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
>>          struct drm_crtc *crtc = cstate->crtc;
>>          struct drm_encoder *encoder;
>>
>> +       if (cstate->self_refresh_active)
>> +               return true;
>> +
>>          drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) {
>>                  if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) {
>>                          return true;
>> --
>> 2.7.4
>>
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode

[Doug Anderson]

Hi,

On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera
<quic_vpolimer@quicinc.com> wrote:
>
> While in virtual terminal mode with PSR enabled, there will be
> no atomic commits triggered without dirty_fb being set. This
> will create a notion of no screen update. Allow atomic commit
> when dirty_fb ioctl is issued, so that it can trigger a PSR exit
> and shows update on the screen.
>
> Reported-by: Bjorn Andersson <andersson@kernel.org>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)

I can confirm that this patch plus patch #2 fixes the typing problems
seen on VT2 on a Chromebook using PSR.

Tested-by: Douglas Anderson <dianders@chromium.org>


-Doug

[PATCH v1 2/3] msm/disp/dpu: allow atomic_check in PSR usecase

[Vinod Polimera]

Certain flags like dirty_fb will be updated into the plane state
during crtc atomic_check. Allow those updates during PSR commit.

Reported-by: Bjorn Andersson <andersson@kernel.org>
Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 96f645e..a02c7f4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1185,7 +1185,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 
 	bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
 
-	if (!crtc_state->enable || !crtc_state->active) {
+	if (!crtc_state->enable || !drm_atomic_crtc_effectively_active(crtc_state)) {
 		DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n",
 				crtc->base.id, crtc_state->enable,
 				crtc_state->active);
-- 
2.7.4

Re: [PATCH v1 2/3] msm/disp/dpu: allow atomic_check in PSR usecase

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>
> Certain flags like dirty_fb will be updated into the plane state
> during crtc atomic_check. Allow those updates during PSR commit.
>
> Reported-by: Bjorn Andersson <andersson@kernel.org>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] msm/disp/dpu: allow atomic_check in PSR usecase

[Doug Anderson]

Hi,

On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera
<quic_vpolimer@quicinc.com> wrote:
>
> Certain flags like dirty_fb will be updated into the plane state
> during crtc atomic_check. Allow those updates during PSR commit.
>
> Reported-by: Bjorn Andersson <andersson@kernel.org>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I can confirm that this patch plus patch #1 fixes the typing problems
seen on VT2 on a Chromebook using PSR.

Tested-by: Douglas Anderson <dianders@chromium.org>

[PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Vinod Polimera]

In certain CPU stress conditions, there can be a delay in scheduling commit
work and it was observed that PSR commit from a different work queue was 
scheduled. Avoid these commits as display is already in PSR mode.

Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
---
 drivers/gpu/drm/msm/msm_atomic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 645fe53..f8141bb 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 			new_crtc_state->mode_changed = true;
 			state->allow_modeset = true;
 		}
+
+		if (old_crtc_state->self_refresh_active && new_crtc_state->self_refresh_active)
+			return -EINVAL;
 	}
 
 	return drm_atomic_helper_check(dev, state);
-- 
2.7.4

Re: [PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> wrote:
>
> In certain CPU stress conditions, there can be a delay in scheduling commit
> work and it was observed that PSR commit from a different work queue was
> scheduled. Avoid these commits as display is already in PSR mode.
>
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 645fe53..f8141bb 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>                         new_crtc_state->mode_changed = true;
>                         state->allow_modeset = true;
>                 }
> +
> +               if (old_crtc_state->self_refresh_active && new_crtc_state->self_refresh_active)
> +                       return -EINVAL;

EINVAL here means that atomic_check will fail if both old and new
states are in SR mode. For example, there might be a mode set for
another CRTC (while keeping this one in SR mode). I don't think this
is correct. We should skip/shortcut the commit, that's true. But I
doubt that returning an error here is a proper way to do this. Please
correct me if I'm wrong.

>         }
>
>         return drm_atomic_helper_check(dev, state);
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

RE: [PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Vinod Polimera]

> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com>
> wrote:
> >
> > In certain CPU stress conditions, there can be a delay in scheduling commit
> > work and it was observed that PSR commit from a different work queue
> was
> > scheduled. Avoid these commits as display is already in PSR mode.
> >
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/msm_atomic.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> > index 645fe53..f8141bb 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> >                         new_crtc_state->mode_changed = true;
> >                         state->allow_modeset = true;
> >                 }
> > +
> > +               if (old_crtc_state->self_refresh_active && new_crtc_state-
> >self_refresh_active)
> > +                       return -EINVAL;
> 
> EINVAL here means that atomic_check will fail if both old and new
> states are in SR mode. For example, there might be a mode set for
> another CRTC (while keeping this one in SR mode). I don't think this
> is correct. We should skip/shortcut the commit, that's true. But I
> doubt that returning an error here is a proper way to do this. Please
> correct me if I'm wrong.

If there is a modeset on same crtc with a different connector. The new_crtc_state will not have self_refresh_active set.
Self_refresh_active is set from the helper library, which will duplicate the old_state and just adds self_refresh_active to true and active to false.
so we can be confident that if we are checking for self_refresh_active status then it should be coming from the library call.

Also the EINVAL is returned to the self_refresh library API and the function will be retired.
And self_refresh_active is cleared on every commit : https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_atomic_state_helper.c#n158

The above is true for different crtc as well.
please let me know your comments.

Thanks,
Vinod.
 

Re: [PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Dmitry Baryshkov]

On Mon, 3 Apr 2023 at 15:01, Vinod Polimera <vpolimer@qti.qualcomm.com> wrote:
>
> > On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com>
> > wrote:
> > >
> > > In certain CPU stress conditions, there can be a delay in scheduling commit
> > > work and it was observed that PSR commit from a different work queue
> > was
> > > scheduled. Avoid these commits as display is already in PSR mode.
> > >
> > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > > ---
> > >  drivers/gpu/drm/msm/msm_atomic.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > > index 645fe53..f8141bb 100644
> > > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > > @@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > >                         new_crtc_state->mode_changed = true;
> > >                         state->allow_modeset = true;
> > >                 }
> > > +
> > > +               if (old_crtc_state->self_refresh_active && new_crtc_state-
> > >self_refresh_active)
> > > +                       return -EINVAL;
> >
> > EINVAL here means that atomic_check will fail if both old and new
> > states are in SR mode. For example, there might be a mode set for
> > another CRTC (while keeping this one in SR mode). I don't think this
> > is correct. We should skip/shortcut the commit, that's true. But I
> > doubt that returning an error here is a proper way to do this. Please
> > correct me if I'm wrong.
>
> If there is a modeset on same crtc with a different connector. The new_crtc_state will not have self_refresh_active set.
> Self_refresh_active is set from the helper library, which will duplicate the old_state and just adds self_refresh_active to true and active to false.
> so we can be confident that if we are checking for self_refresh_active status then it should be coming from the library call.
>
> Also the EINVAL is returned to the self_refresh library API and the function will be retired.

Maybe I misunderstand you here. However, in this way EINVAL is
returned to drm_atomic_check_only() and not to the SR code.

> And self_refresh_active is cleared on every commit : https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_atomic_state_helper.c#n158

And this means that this check will not trigger at all, if I'm not
mistaken. You've added code to msm_atomic_check(), so
drm_self_refresh_helper_alter_state() was not called (yet) and thus
new_crtc_state->self_refresh_active is set to false, fresh after
crtc's duplicate_state.

--
With best wishes
Dmitry

Re: [PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Dmitry Baryshkov]

On 03/04/2023 19:11, Dmitry Baryshkov wrote:
> On Mon, 3 Apr 2023 at 15:01, Vinod Polimera <vpolimer@qti.qualcomm.com> wrote:
>>
>>> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com>
>>> wrote:
>>>>
>>>> In certain CPU stress conditions, there can be a delay in scheduling commit
>>>> work and it was observed that PSR commit from a different work queue
>>> was
>>>> scheduled. Avoid these commits as display is already in PSR mode.
>>>>
>>>> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/msm_atomic.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
>>> b/drivers/gpu/drm/msm/msm_atomic.c
>>>> index 645fe53..f8141bb 100644
>>>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>>>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>>>> @@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev,
>>> struct drm_atomic_state *state)
>>>>                          new_crtc_state->mode_changed = true;
>>>>                          state->allow_modeset = true;
>>>>                  }
>>>> +
>>>> +               if (old_crtc_state->self_refresh_active && new_crtc_state-
>>>> self_refresh_active)
>>>> +                       return -EINVAL;
>>>
>>> EINVAL here means that atomic_check will fail if both old and new
>>> states are in SR mode. For example, there might be a mode set for
>>> another CRTC (while keeping this one in SR mode). I don't think this
>>> is correct. We should skip/shortcut the commit, that's true. But I
>>> doubt that returning an error here is a proper way to do this. Please
>>> correct me if I'm wrong.
>>
>> If there is a modeset on same crtc with a different connector. The new_crtc_state will not have self_refresh_active set.
>> Self_refresh_active is set from the helper library, which will duplicate the old_state and just adds self_refresh_active to true and active to false.
>> so we can be confident that if we are checking for self_refresh_active status then it should be coming from the library call.
>>
>> Also the EINVAL is returned to the self_refresh library API and the function will be retired.
> 
> Maybe I misunderstand you here. However, in this way EINVAL is
> returned to drm_atomic_check_only() and not to the SR code.

Unless anybody objects, I'm going to drop this patch now. The issue 
should be solved in the framework itself.

> 
>> And self_refresh_active is cleared on every commit : https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_atomic_state_helper.c#n158
> 
> And this means that this check will not trigger at all, if I'm not
> mistaken. You've added code to msm_atomic_check(), so
> drm_self_refresh_helper_alter_state() was not called (yet) and thus
> new_crtc_state->self_refresh_active is set to false, fresh after
> crtc's duplicate_state.
> 
> --
> With best wishes
> Dmitry

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] msm: skip the atomic commit of self refresh while PSR running

[Dmitry Baryshkov]

On 31/03/2023 16:58, Vinod Polimera wrote:
> In certain CPU stress conditions, there can be a delay in scheduling commit
> work and it was observed that PSR commit from a different work queue was
> scheduled. Avoid these commits as display is already in PSR mode.
> 
> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> ---
>   drivers/gpu/drm/msm/msm_atomic.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 645fe53..f8141bb 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -192,6 +192,9 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)

The corresponding patch is not yet applied. I wonder how this was tested.

>   			new_crtc_state->mode_changed = true;
>   			state->allow_modeset = true;
>   		}
> +
> +		if (old_crtc_state->self_refresh_active && new_crtc_state->self_refresh_active)
> +			return -EINVAL;
>   	}
>   
>   	return drm_atomic_helper_check(dev, state);

-- 
With best wishes
Dmitry

Re: [PATCH v1 0/3] Fixes for PSR

[Dmitry Baryshkov]

On Fri, 31 Mar 2023 19:28:31 +0530, Vinod Polimera wrote:
> while in virtual terminal with PSR enabled, there will be
> no atomic commits triggered resulting in no screen update.
> Update the dirtyfb flag into plane state during atomic check
> to flush the pixel data explicitly.
> 
> Avoid scheduling PSR commits from different work queues while
> running in PSR mode already.
> 
> [...]

Applied, thanks!

[1/3] drm/msm/dpu: set dirty_fb flag while in self refresh mode
      https://gitlab.freedesktop.org/lumag/msm/-/commit/ddf5387d1fb7
[2/3] msm/disp/dpu: allow atomic_check in PSR usecase
      https://gitlab.freedesktop.org/lumag/msm/-/commit/86c56ba51aec

Note, patch 3, which solves a different issue (and which requires additional changes) was not applied.

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