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
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 <[email protected]>
Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
Signed-off-by: Vinod Polimera <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <[email protected]> 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 <[email protected]>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <[email protected]> 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 <[email protected]>
> ---
> 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
> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <[email protected]>
> 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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
> ---
> 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
On Mon, 3 Apr 2023 at 15:01, Vinod Polimera <[email protected]> wrote:
>
> > On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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
Hi,
On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera
<[email protected]> 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 <[email protected]>
> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
On 03/04/2023 19:11, Dmitry Baryshkov wrote:
> On Mon, 3 Apr 2023 at 15:01, Vinod Polimera <[email protected]> wrote:
>>
>>> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <[email protected]>
>>> 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 <[email protected]>
>>>> ---
>>>> 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