Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 3:15 PM Dmitry Baryshkov
wrote:
>
> On 06/05/2022 00:24, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
> > wrote:
> >>
> >> On Thu, 5 May 2022 at 23:21, Doug Anderson wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> >>> wrote:
>
> On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
> >
> > Hi,
> >
> > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > wrote:
> >>
> >> On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> >>> wrote:
>
> On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> >> wrote:
> >>>
> >>> On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> When doing DP AUX transfers there are two actors that need to be
> powered in order for the DP AUX transfer to work: the DP source
> and
> the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> requirement on side-channel operations") added some documentation
> saying that the DP source is required to power itself up (if
> needed)
> to do AUX transfers. However, that commit doesn't talk anything
> about
> the DP sink.
>
> For full fledged DP the sink isn't really a problem. It's
> expected
> that if an external DP monitor isn't plugged in that attempting
> to do
> AUX transfers won't work. It's also expected that if a DP
> monitor is
> plugged in (and thus asserting HPD) that it AUX transfers will
> work.
>
> When we're looking at eDP, however, things are less obvious.
> Let's add
> some documentation about expectations. Here's what we'll say:
>
> 1. We don't expect the DP AUX transfer function to power on an
> eDP
> panel. If an eDP panel is physically connected but powered off
> then it
> makes sense for the transfer to fail.
> >>>
> >>> I don't agree with this. I think the panel should just get powred
> >>> up
> >>> for AUX transfers.
> >>
> >> That's definitely a fair thing to think about and I have at times
> >> thought about trying to make it work that way. It always ends up
> >> hitting a roadblock.
>
> How do you even probe the panel initially if you can't power it on
> without doing some kind of full modeset/etc.?
> >>>
> >>> It's not that we can't power it on without a full modeset. It' that at
> >>> panel probe time all the DRM components haven't been hooked together
> >>> yet, so the bridge chain isn't available yet. The panel can power
> >>> itself on, though. This is why the documentation I added says: "if a
> >>> panel driver is initiating a DP AUX transfer it may power itself up
> >>> however it wants"
> >>>
> >>>
> >> The biggest roadblock that I recall is that to make this work then
> >> you'd have to somehow ensure that the bridge chain's pre_enable()
> >> call
> >> was made as part of the AUX transfer, right? Since the transfer
> >> function can be called in any context at all, we have to coordinate
> >> this with DRM. If, for instance, DRM is mid way through powering
> >> the
> >> panel down then we need to wait for DRM to fully finish powering
> >> down,
> >> then we need to power the panel back up. I don't believe that we
> >> can
> >> just force the panel to stay on if DRM is turning it off because of
> >> panel power sequencing requirements. At least I know it would have
> >> the
> >> potential to break "samsung-atna33xc20.c" which absolutely needs to
> >> see the panel power off after it's been disabled.
> >>
> >> We also, I believe, need to handle the fact that the bridge chain
> >> may
> >> not have even been created yet. We do AUX transfers to read the
> >> EDID
> >> and also to setup the backlight in the probe function of
> >> panel-edp. At
> >> that point the panel hasn't been linked into the chain. We had
> >> _long_
> >> discussions [1] about moving these out of probe and decided that we
> >> could move the EDID read to be later but that it was going to
> >> really
> >> ugly to move the AUX backlight later. The
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On 06/05/2022 00:24, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
wrote:
On Thu, 5 May 2022 at 23:21, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
wrote:
On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
wrote:
On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
Hi,
On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
wrote:
On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
Hi,
On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
wrote:
On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
When doing DP AUX transfers there are two actors that need to be
powered in order for the DP AUX transfer to work: the DP source and
the DP sync. Commit bacbab58f09d ("drm: Mention the power state
requirement on side-channel operations") added some documentation
saying that the DP source is required to power itself up (if needed)
to do AUX transfers. However, that commit doesn't talk anything about
the DP sink.
For full fledged DP the sink isn't really a problem. It's expected
that if an external DP monitor isn't plugged in that attempting to do
AUX transfers won't work. It's also expected that if a DP monitor is
plugged in (and thus asserting HPD) that it AUX transfers will work.
When we're looking at eDP, however, things are less obvious. Let's add
some documentation about expectations. Here's what we'll say:
1. We don't expect the DP AUX transfer function to power on an eDP
panel. If an eDP panel is physically connected but powered off then it
makes sense for the transfer to fail.
I don't agree with this. I think the panel should just get powred up
for AUX transfers.
That's definitely a fair thing to think about and I have at times
thought about trying to make it work that way. It always ends up
hitting a roadblock.
How do you even probe the panel initially if you can't power it on
without doing some kind of full modeset/etc.?
It's not that we can't power it on without a full modeset. It' that at
panel probe time all the DRM components haven't been hooked together
yet, so the bridge chain isn't available yet. The panel can power
itself on, though. This is why the documentation I added says: "if a
panel driver is initiating a DP AUX transfer it may power itself up
however it wants"
The biggest roadblock that I recall is that to make this work then
you'd have to somehow ensure that the bridge chain's pre_enable() call
was made as part of the AUX transfer, right? Since the transfer
function can be called in any context at all, we have to coordinate
this with DRM. If, for instance, DRM is mid way through powering the
panel down then we need to wait for DRM to fully finish powering down,
then we need to power the panel back up. I don't believe that we can
just force the panel to stay on if DRM is turning it off because of
panel power sequencing requirements. At least I know it would have the
potential to break "samsung-atna33xc20.c" which absolutely needs to
see the panel power off after it's been disabled.
We also, I believe, need to handle the fact that the bridge chain may
not have even been created yet. We do AUX transfers to read the EDID
and also to setup the backlight in the probe function of panel-edp. At
that point the panel hasn't been linked into the chain. We had _long_
discussions [1] about moving these out of probe and decided that we
could move the EDID read to be later but that it was going to really
ugly to move the AUX backlight later. The backlight would end up
popping up at some point in time later (the first call to panel
prepare() or maybe get_modes()) and that seemed weird.
[1]
https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
Otherwise you can't trust that eg. the /dev/aux
stuff is actually usable.
Yeah, it's been on my mind to talk more about /dev/aux. I think
/dev/aux has some problems, at least with eDP. Specifically:
1. Even if we somehow figure out how to power the panel on as part of
the aux transfer, we actually _still_ not guaranteed to be able to
talk to it as far as I understand. My colleague reported to me that on
a system he was working with that had PSR (panel self refresh) that
when the panel was powered on but in PSR mode that it wouldn't talk
over AUX. Assuming that this is correct then I guess we'd also have to
do even more coordination with DRM to exit PSR and block future
transitions of PSR. (NOTE: it's always possible that my colleague ran
into some other bug and that panels are _supposed_ to be able to talk
in PSR. If you think this is the case, I can always try to dig more).
TBH - the coordination with drm I don't think would be the difficult part, as
we'd just need to add some sort of property (ideally invisible to userspace)
that can be used in an atomic commit to disable PSR - similar to how we enab
Re: [Freedreno] [PATCH v3 2/2] drm/msm/mdp5: Return error code in mdp5_mixer_release when deadlock is detected
On Thu, May 5, 2022 at 2:41 PM Jessica Zhang wrote:
>
> There is a possibility for mdp5_get_global_state to return
> -EDEADLK when acquiring the modeset lock, but currently global_state in
> mdp5_mixer_release doesn't check for if an error is returned.
>
> To avoid a NULL dereference error, let's have mdp5_mixer_release
> check if an error is returned and propagate that error.
>
> Reported-by: Tomeu Vizoso
> Signed-off-by: Jessica Zhang
Fixes: 7907a0d77cb4 ("drm/msm/mdp5: Use the new private_obj state")
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 --
> drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 15 +++
> drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h | 4 ++--
> 3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index b966cd69f99d..fe2922c8d21b 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -612,9 +612,15 @@ static int mdp5_crtc_setup_pipeline(struct drm_crtc
> *crtc,
> if (ret)
> return ret;
>
> - mdp5_mixer_release(new_crtc_state->state, old_mixer);
> + ret = mdp5_mixer_release(new_crtc_state->state, old_mixer);
> + if (ret)
> + return ret;
> +
> if (old_r_mixer) {
> - mdp5_mixer_release(new_crtc_state->state,
> old_r_mixer);
> + ret = mdp5_mixer_release(new_crtc_state->state,
> old_r_mixer);
> + if (ret)
> + return ret;
> +
> if (!need_right_mixer)
> pipeline->r_mixer = NULL;
> }
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
> index 954db683ae44..2536def2a000 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
> @@ -116,21 +116,28 @@ int mdp5_mixer_assign(struct drm_atomic_state *s,
> struct drm_crtc *crtc,
> return 0;
> }
>
> -void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer
> *mixer)
> +int mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer
> *mixer)
> {
> struct mdp5_global_state *global_state = mdp5_get_global_state(s);
> - struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer;
> + struct mdp5_hw_mixer_state *new_state;
>
> if (!mixer)
> - return;
> + return 0;
> +
> + if (IS_ERR(global_state))
> + return PTR_ERR(global_state);
> +
> + new_state = &global_state->hwmixer;
>
> if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx]))
> - return;
> + return -EINVAL;
>
> DBG("%s: release from crtc %s", mixer->name,
> new_state->hwmixer_to_crtc[mixer->idx]->name);
>
> new_state->hwmixer_to_crtc[mixer->idx] = NULL;
> +
> + return 0;
> }
>
> void mdp5_mixer_destroy(struct mdp5_hw_mixer *mixer)
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
> index 43c9ba43ce18..545ee223b9d7 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
> @@ -30,7 +30,7 @@ void mdp5_mixer_destroy(struct mdp5_hw_mixer *lm);
> int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc,
> uint32_t caps, struct mdp5_hw_mixer **mixer,
> struct mdp5_hw_mixer **r_mixer);
> -void mdp5_mixer_release(struct drm_atomic_state *s,
> - struct mdp5_hw_mixer *mixer);
> +int mdp5_mixer_release(struct drm_atomic_state *s,
> + struct mdp5_hw_mixer *mixer);
>
> #endif /* __MDP5_LM_H__ */
> --
> 2.35.1
>
Re: [Freedreno] [PATCH v3 1/2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected
On Thu, May 5, 2022 at 2:41 PM Jessica Zhang wrote:
>
> mdp5_get_global_state runs the risk of hitting a -EDEADLK when acquiring
> the modeset lock, but currently mdp5_pipe_release doesn't check for if
> an error is returned. Because of this, there is a possibility of
> mdp5_pipe_release hitting a NULL dereference error.
>
> To avoid this, let's have mdp5_pipe_release check if
> mdp5_get_global_state returns an error and propogate that error.
>
> Changes since v1:
> - Separated declaration and initialization of *new_state to avoid
> compiler warning
> - Fixed some spelling mistakes in commit message
>
> Changes since v2:
> - Return 0 in case where hwpipe is NULL as this is considered normal
> behavior
> - Added 2nd patch in series to fix a similar NULL dereference issue in
> mdp5_mixer_release
>
> Reported-by: Tomeu Vizoso
> Signed-off-by: Jessica Zhang
Fixes: 7907a0d77cb4 ("drm/msm/mdp5: Use the new private_obj state")
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 15 +++
> drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h | 2 +-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 20
> 3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> index ba6695963aa6..a4f5cb90f3e8 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> @@ -119,18 +119,23 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
> drm_plane *plane,
> return 0;
> }
>
> -void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe)
> +int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe)
> {
> struct msm_drm_private *priv = s->dev->dev_private;
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> struct mdp5_global_state *state = mdp5_get_global_state(s);
> - struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
> + struct mdp5_hw_pipe_state *new_state;
>
> if (!hwpipe)
> - return;
> + return 0;
> +
> + if (IS_ERR(state))
> + return PTR_ERR(state);
> +
> + new_state = &state->hwpipe;
>
> if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
> - return;
> + return -EINVAL;
>
> DBG("%s: release from plane %s", hwpipe->name,
> new_state->hwpipe_to_plane[hwpipe->idx]->name);
> @@ -141,6 +146,8 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct
> mdp5_hw_pipe *hwpipe)
> }
>
> new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
> +
> + return 0;
> }
>
> void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> index 9b26d0761bd4..cca67938cab2 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> @@ -37,7 +37,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
> drm_plane *plane,
> uint32_t caps, uint32_t blkcfg,
> struct mdp5_hw_pipe **hwpipe,
> struct mdp5_hw_pipe **r_hwpipe);
> -void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe);
> +int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> *hwpipe);
>
> struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
> uint32_t reg_offset, uint32_t caps);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 228b22830970..979458482841 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -311,12 +311,24 @@ static int mdp5_plane_atomic_check_with_state(struct
> drm_crtc_state *crtc_state,
> mdp5_state->r_hwpipe = NULL;
>
>
> - mdp5_pipe_release(state->state, old_hwpipe);
> - mdp5_pipe_release(state->state, old_right_hwpipe);
> + ret = mdp5_pipe_release(state->state, old_hwpipe);
> + if (ret)
> + return ret;
> +
> + ret = mdp5_pipe_release(state->state,
> old_right_hwpipe);
> + if (ret)
> + return ret;
> +
> }
> } else {
> - mdp5_pipe_release(state->state, mdp5_state->hwpipe);
> - mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
> + ret = mdp5_pipe_release(state->state, mdp5_state->hwpipe);
> + if (ret)
> + return ret;
> +
> + ret = mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
> + if (ret)
> + return ret;
> +
>
[Freedreno] [PATCH v3 2/2] drm/msm/mdp5: Return error code in mdp5_mixer_release when deadlock is detected
There is a possibility for mdp5_get_global_state to return
-EDEADLK when acquiring the modeset lock, but currently global_state in
mdp5_mixer_release doesn't check for if an error is returned.
To avoid a NULL dereference error, let's have mdp5_mixer_release
check if an error is returned and propagate that error.
Reported-by: Tomeu Vizoso
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 10 --
drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c | 15 +++
drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h | 4 ++--
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index b966cd69f99d..fe2922c8d21b 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -612,9 +612,15 @@ static int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
if (ret)
return ret;
- mdp5_mixer_release(new_crtc_state->state, old_mixer);
+ ret = mdp5_mixer_release(new_crtc_state->state, old_mixer);
+ if (ret)
+ return ret;
+
if (old_r_mixer) {
- mdp5_mixer_release(new_crtc_state->state, old_r_mixer);
+ ret = mdp5_mixer_release(new_crtc_state->state,
old_r_mixer);
+ if (ret)
+ return ret;
+
if (!need_right_mixer)
pipeline->r_mixer = NULL;
}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
index 954db683ae44..2536def2a000 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.c
@@ -116,21 +116,28 @@ int mdp5_mixer_assign(struct drm_atomic_state *s, struct
drm_crtc *crtc,
return 0;
}
-void mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer
*mixer)
+int mdp5_mixer_release(struct drm_atomic_state *s, struct mdp5_hw_mixer *mixer)
{
struct mdp5_global_state *global_state = mdp5_get_global_state(s);
- struct mdp5_hw_mixer_state *new_state = &global_state->hwmixer;
+ struct mdp5_hw_mixer_state *new_state;
if (!mixer)
- return;
+ return 0;
+
+ if (IS_ERR(global_state))
+ return PTR_ERR(global_state);
+
+ new_state = &global_state->hwmixer;
if (WARN_ON(!new_state->hwmixer_to_crtc[mixer->idx]))
- return;
+ return -EINVAL;
DBG("%s: release from crtc %s", mixer->name,
new_state->hwmixer_to_crtc[mixer->idx]->name);
new_state->hwmixer_to_crtc[mixer->idx] = NULL;
+
+ return 0;
}
void mdp5_mixer_destroy(struct mdp5_hw_mixer *mixer)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
index 43c9ba43ce18..545ee223b9d7 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mixer.h
@@ -30,7 +30,7 @@ void mdp5_mixer_destroy(struct mdp5_hw_mixer *lm);
int mdp5_mixer_assign(struct drm_atomic_state *s, struct drm_crtc *crtc,
uint32_t caps, struct mdp5_hw_mixer **mixer,
struct mdp5_hw_mixer **r_mixer);
-void mdp5_mixer_release(struct drm_atomic_state *s,
- struct mdp5_hw_mixer *mixer);
+int mdp5_mixer_release(struct drm_atomic_state *s,
+ struct mdp5_hw_mixer *mixer);
#endif /* __MDP5_LM_H__ */
--
2.35.1
[Freedreno] [PATCH v3 1/2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected
mdp5_get_global_state runs the risk of hitting a -EDEADLK when acquiring
the modeset lock, but currently mdp5_pipe_release doesn't check for if
an error is returned. Because of this, there is a possibility of
mdp5_pipe_release hitting a NULL dereference error.
To avoid this, let's have mdp5_pipe_release check if
mdp5_get_global_state returns an error and propogate that error.
Changes since v1:
- Separated declaration and initialization of *new_state to avoid
compiler warning
- Fixed some spelling mistakes in commit message
Changes since v2:
- Return 0 in case where hwpipe is NULL as this is considered normal
behavior
- Added 2nd patch in series to fix a similar NULL dereference issue in
mdp5_mixer_release
Reported-by: Tomeu Vizoso
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 15 +++
drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h | 2 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 20
3 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
index ba6695963aa6..a4f5cb90f3e8 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
@@ -119,18 +119,23 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
drm_plane *plane,
return 0;
}
-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
{
struct msm_drm_private *priv = s->dev->dev_private;
struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
struct mdp5_global_state *state = mdp5_get_global_state(s);
- struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
+ struct mdp5_hw_pipe_state *new_state;
if (!hwpipe)
- return;
+ return 0;
+
+ if (IS_ERR(state))
+ return PTR_ERR(state);
+
+ new_state = &state->hwpipe;
if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
- return;
+ return -EINVAL;
DBG("%s: release from plane %s", hwpipe->name,
new_state->hwpipe_to_plane[hwpipe->idx]->name);
@@ -141,6 +146,8 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct
mdp5_hw_pipe *hwpipe)
}
new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
+
+ return 0;
}
void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
index 9b26d0761bd4..cca67938cab2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
@@ -37,7 +37,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
drm_plane *plane,
uint32_t caps, uint32_t blkcfg,
struct mdp5_hw_pipe **hwpipe,
struct mdp5_hw_pipe **r_hwpipe);
-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
*hwpipe);
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe);
struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
uint32_t reg_offset, uint32_t caps);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 228b22830970..979458482841 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -311,12 +311,24 @@ static int mdp5_plane_atomic_check_with_state(struct
drm_crtc_state *crtc_state,
mdp5_state->r_hwpipe = NULL;
- mdp5_pipe_release(state->state, old_hwpipe);
- mdp5_pipe_release(state->state, old_right_hwpipe);
+ ret = mdp5_pipe_release(state->state, old_hwpipe);
+ if (ret)
+ return ret;
+
+ ret = mdp5_pipe_release(state->state, old_right_hwpipe);
+ if (ret)
+ return ret;
+
}
} else {
- mdp5_pipe_release(state->state, mdp5_state->hwpipe);
- mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+ ret = mdp5_pipe_release(state->state, mdp5_state->hwpipe);
+ if (ret)
+ return ret;
+
+ ret = mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+ if (ret)
+ return ret;
+
mdp5_state->hwpipe = mdp5_state->r_hwpipe = NULL;
}
--
2.35.1
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 1:56 PM Dmitry Baryshkov
wrote:
>
> On Thu, 5 May 2022 at 23:21, Doug Anderson wrote:
> >
> > Hi,
> >
> > On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> > wrote:
> > >
> > > On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > > > wrote:
> > > > >
> > > > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson
> > > > > > > > > > wrote:
> > > > > > > > > > > When doing DP AUX transfers there are two actors that
> > > > > > > > > > > need to be
> > > > > > > > > > > powered in order for the DP AUX transfer to work: the DP
> > > > > > > > > > > source and
> > > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power
> > > > > > > > > > > state
> > > > > > > > > > > requirement on side-channel operations") added some
> > > > > > > > > > > documentation
> > > > > > > > > > > saying that the DP source is required to power itself up
> > > > > > > > > > > (if needed)
> > > > > > > > > > > to do AUX transfers. However, that commit doesn't talk
> > > > > > > > > > > anything about
> > > > > > > > > > > the DP sink.
> > > > > > > > > > >
> > > > > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > > > > expected
> > > > > > > > > > > that if an external DP monitor isn't plugged in that
> > > > > > > > > > > attempting to do
> > > > > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > > > > monitor is
> > > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers
> > > > > > > > > > > will work.
> > > > > > > > > > >
> > > > > > > > > > > When we're looking at eDP, however, things are less
> > > > > > > > > > > obvious. Let's add
> > > > > > > > > > > some documentation about expectations. Here's what we'll
> > > > > > > > > > > say:
> > > > > > > > > > >
> > > > > > > > > > > 1. We don't expect the DP AUX transfer function to power
> > > > > > > > > > > on an eDP
> > > > > > > > > > > panel. If an eDP panel is physically connected but
> > > > > > > > > > > powered off then it
> > > > > > > > > > > makes sense for the transfer to fail.
> > > > > > > > > >
> > > > > > > > > > I don't agree with this. I think the panel should just get
> > > > > > > > > > powred up
> > > > > > > > > > for AUX transfers.
> > > > > > > > >
> > > > > > > > > That's definitely a fair thing to think about and I have at
> > > > > > > > > times
> > > > > > > > > thought about trying to make it work that way. It always ends
> > > > > > > > > up
> > > > > > > > > hitting a roadblock.
> > > > > > >
> > > > > > > How do you even probe the panel initially if you can't power it on
> > > > > > > without doing some kind of full modeset/etc.?
> > > > > >
> > > > > > It's not that we can't power it on without a full modeset. It' that
> > > > > > at
> > > > > > panel probe time all the DRM components haven't been hooked together
> > > > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > > > itself on, though. This is why the documentation I added says: "if a
> > > > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > > > however it wants"
> > > > > >
> > > > > >
> > > > > > > > > The biggest roadblock that I recall is that to make this work
> > > > > > > > > then
> > > > > > > > > you'd have to somehow ensure that the bridge chain's
> > > > > > > > > pre_enable() call
> > > > > > > > > was made as part of the AUX transfer, right? Since the
> > > > > > > > > transfer
> > > > > > > > > function can be called in any context at all, we have to
> > > > > > > > > coordinate
> > > > > > > > > this with DRM. If, for instance, DRM is mid way through
> > > > > > > > > powering the
> > > > > > > > > panel down then we need to wait for DRM to fully finish
> > > > > > > > > powering down,
> > > > > > > > > then we need to power the panel back up. I don't believe that
> > > > > > > > > we can
> > > > > > > > > just force the panel to stay on if DRM is turning it off
> > > > > > > > > because of
> > > > > > > > > panel power sequencing requirements. At least I know it would
> > > > > > > > > have the
> > > > > > > > > potential to break "samsung-atna33xc20.c" which absolutely
> > > > > > > > > needs to
> > > > > > > > > see the panel power off after it's been disabled.
> > > > > > > > >
> > > > > > > > > We also, I believe, need to handle the fact that the bridge
> > > > > >
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, 5 May 2022 at 23:21, Doug Anderson wrote:
>
> Hi,
>
> On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
> wrote:
> >
> > On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
> > >
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > > wrote:
> > > > > >
> > > > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson
> > > > > > > > > wrote:
> > > > > > > > > > When doing DP AUX transfers there are two actors that need
> > > > > > > > > > to be
> > > > > > > > > > powered in order for the DP AUX transfer to work: the DP
> > > > > > > > > > source and
> > > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power
> > > > > > > > > > state
> > > > > > > > > > requirement on side-channel operations") added some
> > > > > > > > > > documentation
> > > > > > > > > > saying that the DP source is required to power itself up
> > > > > > > > > > (if needed)
> > > > > > > > > > to do AUX transfers. However, that commit doesn't talk
> > > > > > > > > > anything about
> > > > > > > > > > the DP sink.
> > > > > > > > > >
> > > > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > > > expected
> > > > > > > > > > that if an external DP monitor isn't plugged in that
> > > > > > > > > > attempting to do
> > > > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > > > monitor is
> > > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers
> > > > > > > > > > will work.
> > > > > > > > > >
> > > > > > > > > > When we're looking at eDP, however, things are less
> > > > > > > > > > obvious. Let's add
> > > > > > > > > > some documentation about expectations. Here's what we'll
> > > > > > > > > > say:
> > > > > > > > > >
> > > > > > > > > > 1. We don't expect the DP AUX transfer function to power on
> > > > > > > > > > an eDP
> > > > > > > > > > panel. If an eDP panel is physically connected but powered
> > > > > > > > > > off then it
> > > > > > > > > > makes sense for the transfer to fail.
> > > > > > > > >
> > > > > > > > > I don't agree with this. I think the panel should just get
> > > > > > > > > powred up
> > > > > > > > > for AUX transfers.
> > > > > > > >
> > > > > > > > That's definitely a fair thing to think about and I have at
> > > > > > > > times
> > > > > > > > thought about trying to make it work that way. It always ends up
> > > > > > > > hitting a roadblock.
> > > > > >
> > > > > > How do you even probe the panel initially if you can't power it on
> > > > > > without doing some kind of full modeset/etc.?
> > > > >
> > > > > It's not that we can't power it on without a full modeset. It' that at
> > > > > panel probe time all the DRM components haven't been hooked together
> > > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > > itself on, though. This is why the documentation I added says: "if a
> > > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > > however it wants"
> > > > >
> > > > >
> > > > > > > > The biggest roadblock that I recall is that to make this work
> > > > > > > > then
> > > > > > > > you'd have to somehow ensure that the bridge chain's
> > > > > > > > pre_enable() call
> > > > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > > > function can be called in any context at all, we have to
> > > > > > > > coordinate
> > > > > > > > this with DRM. If, for instance, DRM is mid way through
> > > > > > > > powering the
> > > > > > > > panel down then we need to wait for DRM to fully finish
> > > > > > > > powering down,
> > > > > > > > then we need to power the panel back up. I don't believe that
> > > > > > > > we can
> > > > > > > > just force the panel to stay on if DRM is turning it off
> > > > > > > > because of
> > > > > > > > panel power sequencing requirements. At least I know it would
> > > > > > > > have the
> > > > > > > > potential to break "samsung-atna33xc20.c" which absolutely
> > > > > > > > needs to
> > > > > > > > see the panel power off after it's been disabled.
> > > > > > > >
> > > > > > > > We also, I believe, need to handle the fact that the bridge
> > > > > > > > chain may
> > > > > > > > not have even been created yet. We do AUX transfers to read the
> > > > > > > > EDID
> > > > > > > > and also to setup the backlight in the probe function of
> > > > > > > > panel-edp. At
> > > > > > > > that point the panel hasn't been linked into the chain. We had
> > > > > > > > _long_
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 1:10 PM Dmitry Baryshkov
wrote:
>
> On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
> >
> > Hi,
> >
> > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > wrote:
> > >
> > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > wrote:
> > > > >
> > > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson
> > > > > > > > wrote:
> > > > > > > > > When doing DP AUX transfers there are two actors that need to
> > > > > > > > > be
> > > > > > > > > powered in order for the DP AUX transfer to work: the DP
> > > > > > > > > source and
> > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power
> > > > > > > > > state
> > > > > > > > > requirement on side-channel operations") added some
> > > > > > > > > documentation
> > > > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > > > needed)
> > > > > > > > > to do AUX transfers. However, that commit doesn't talk
> > > > > > > > > anything about
> > > > > > > > > the DP sink.
> > > > > > > > >
> > > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > > expected
> > > > > > > > > that if an external DP monitor isn't plugged in that
> > > > > > > > > attempting to do
> > > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > > monitor is
> > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers
> > > > > > > > > will work.
> > > > > > > > >
> > > > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > > > Let's add
> > > > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > > > >
> > > > > > > > > 1. We don't expect the DP AUX transfer function to power on
> > > > > > > > > an eDP
> > > > > > > > > panel. If an eDP panel is physically connected but powered
> > > > > > > > > off then it
> > > > > > > > > makes sense for the transfer to fail.
> > > > > > > >
> > > > > > > > I don't agree with this. I think the panel should just get
> > > > > > > > powred up
> > > > > > > > for AUX transfers.
> > > > > > >
> > > > > > > That's definitely a fair thing to think about and I have at times
> > > > > > > thought about trying to make it work that way. It always ends up
> > > > > > > hitting a roadblock.
> > > > >
> > > > > How do you even probe the panel initially if you can't power it on
> > > > > without doing some kind of full modeset/etc.?
> > > >
> > > > It's not that we can't power it on without a full modeset. It' that at
> > > > panel probe time all the DRM components haven't been hooked together
> > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > itself on, though. This is why the documentation I added says: "if a
> > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > however it wants"
> > > >
> > > >
> > > > > > > The biggest roadblock that I recall is that to make this work then
> > > > > > > you'd have to somehow ensure that the bridge chain's pre_enable()
> > > > > > > call
> > > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > > function can be called in any context at all, we have to
> > > > > > > coordinate
> > > > > > > this with DRM. If, for instance, DRM is mid way through powering
> > > > > > > the
> > > > > > > panel down then we need to wait for DRM to fully finish powering
> > > > > > > down,
> > > > > > > then we need to power the panel back up. I don't believe that we
> > > > > > > can
> > > > > > > just force the panel to stay on if DRM is turning it off because
> > > > > > > of
> > > > > > > panel power sequencing requirements. At least I know it would
> > > > > > > have the
> > > > > > > potential to break "samsung-atna33xc20.c" which absolutely needs
> > > > > > > to
> > > > > > > see the panel power off after it's been disabled.
> > > > > > >
> > > > > > > We also, I believe, need to handle the fact that the bridge chain
> > > > > > > may
> > > > > > > not have even been created yet. We do AUX transfers to read the
> > > > > > > EDID
> > > > > > > and also to setup the backlight in the probe function of
> > > > > > > panel-edp. At
> > > > > > > that point the panel hasn't been linked into the chain. We had
> > > > > > > _long_
> > > > > > > discussions [1] about moving these out of probe and decided that
> > > > > > > we
> > > > > > > could move the EDID read to be later but that it was going to
> > > > > > > really
> > > > > > > ugly to move the AUX backlight later. The backlight would end up
> > > > > > > popping up at some point in time
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 12:19 PM Ville Syrjälä
wrote:
>
> On Thu, May 05, 2022 at 08:53:12AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> > wrote:
> > >
> > > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > > wrote:
> > > > >
> > > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson
> > > > > > > > wrote:
> > > > > > > > > When doing DP AUX transfers there are two actors that need to
> > > > > > > > > be
> > > > > > > > > powered in order for the DP AUX transfer to work: the DP
> > > > > > > > > source and
> > > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power
> > > > > > > > > state
> > > > > > > > > requirement on side-channel operations") added some
> > > > > > > > > documentation
> > > > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > > > needed)
> > > > > > > > > to do AUX transfers. However, that commit doesn't talk
> > > > > > > > > anything about
> > > > > > > > > the DP sink.
> > > > > > > > >
> > > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > > expected
> > > > > > > > > that if an external DP monitor isn't plugged in that
> > > > > > > > > attempting to do
> > > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > > monitor is
> > > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers
> > > > > > > > > will work.
> > > > > > > > >
> > > > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > > > Let's add
> > > > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > > > >
> > > > > > > > > 1. We don't expect the DP AUX transfer function to power on
> > > > > > > > > an eDP
> > > > > > > > > panel. If an eDP panel is physically connected but powered
> > > > > > > > > off then it
> > > > > > > > > makes sense for the transfer to fail.
> > > > > > > >
> > > > > > > > I don't agree with this. I think the panel should just get
> > > > > > > > powred up
> > > > > > > > for AUX transfers.
> > > > > > >
> > > > > > > That's definitely a fair thing to think about and I have at times
> > > > > > > thought about trying to make it work that way. It always ends up
> > > > > > > hitting a roadblock.
> > > > >
> > > > > How do you even probe the panel initially if you can't power it on
> > > > > without doing some kind of full modeset/etc.?
> > > >
> > > > It's not that we can't power it on without a full modeset. It' that at
> > > > panel probe time all the DRM components haven't been hooked together
> > > > yet, so the bridge chain isn't available yet. The panel can power
> > > > itself on, though. This is why the documentation I added says: "if a
> > > > panel driver is initiating a DP AUX transfer it may power itself up
> > > > however it wants"
> > > >
> > > >
> > > > > > > The biggest roadblock that I recall is that to make this work then
> > > > > > > you'd have to somehow ensure that the bridge chain's pre_enable()
> > > > > > > call
> > > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > > function can be called in any context at all, we have to
> > > > > > > coordinate
> > > > > > > this with DRM. If, for instance, DRM is mid way through powering
> > > > > > > the
> > > > > > > panel down then we need to wait for DRM to fully finish powering
> > > > > > > down,
> > > > > > > then we need to power the panel back up. I don't believe that we
> > > > > > > can
> > > > > > > just force the panel to stay on if DRM is turning it off because
> > > > > > > of
> > > > > > > panel power sequencing requirements. At least I know it would
> > > > > > > have the
> > > > > > > potential to break "samsung-atna33xc20.c" which absolutely needs
> > > > > > > to
> > > > > > > see the panel power off after it's been disabled.
> > > > > > >
> > > > > > > We also, I believe, need to handle the fact that the bridge chain
> > > > > > > may
> > > > > > > not have even been created yet. We do AUX transfers to read the
> > > > > > > EDID
> > > > > > > and also to setup the backlight in the probe function of
> > > > > > > panel-edp. At
> > > > > > > that point the panel hasn't been linked into the chain. We had
> > > > > > > _long_
> > > > > > > discussions [1] about moving these out of probe and decided that
> > > > > > > we
> > > > > > > could move the EDID read to be later but that it was going to
> > > > > > > really
> > > > > > > ugly to move the AUX backlight later. The backlight would end up
> > > > > > > popping up at some point in
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, 5 May 2022 at 18:53, Doug Anderson wrote:
>
> Hi,
>
> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> wrote:
> >
> > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > wrote:
> > > > > > >
> > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > > > powered in order for the DP AUX transfer to work: the DP source
> > > > > > > > and
> > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > > > requirement on side-channel operations") added some
> > > > > > > > documentation
> > > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > > needed)
> > > > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > > > about
> > > > > > > > the DP sink.
> > > > > > > >
> > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > expected
> > > > > > > > that if an external DP monitor isn't plugged in that attempting
> > > > > > > > to do
> > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > monitor is
> > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will
> > > > > > > > work.
> > > > > > > >
> > > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > > Let's add
> > > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > > >
> > > > > > > > 1. We don't expect the DP AUX transfer function to power on an
> > > > > > > > eDP
> > > > > > > > panel. If an eDP panel is physically connected but powered off
> > > > > > > > then it
> > > > > > > > makes sense for the transfer to fail.
> > > > > > >
> > > > > > > I don't agree with this. I think the panel should just get powred
> > > > > > > up
> > > > > > > for AUX transfers.
> > > > > >
> > > > > > That's definitely a fair thing to think about and I have at times
> > > > > > thought about trying to make it work that way. It always ends up
> > > > > > hitting a roadblock.
> > > >
> > > > How do you even probe the panel initially if you can't power it on
> > > > without doing some kind of full modeset/etc.?
> > >
> > > It's not that we can't power it on without a full modeset. It' that at
> > > panel probe time all the DRM components haven't been hooked together
> > > yet, so the bridge chain isn't available yet. The panel can power
> > > itself on, though. This is why the documentation I added says: "if a
> > > panel driver is initiating a DP AUX transfer it may power itself up
> > > however it wants"
> > >
> > >
> > > > > > The biggest roadblock that I recall is that to make this work then
> > > > > > you'd have to somehow ensure that the bridge chain's pre_enable()
> > > > > > call
> > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > function can be called in any context at all, we have to coordinate
> > > > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > > > panel down then we need to wait for DRM to fully finish powering
> > > > > > down,
> > > > > > then we need to power the panel back up. I don't believe that we can
> > > > > > just force the panel to stay on if DRM is turning it off because of
> > > > > > panel power sequencing requirements. At least I know it would have
> > > > > > the
> > > > > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > > > > see the panel power off after it's been disabled.
> > > > > >
> > > > > > We also, I believe, need to handle the fact that the bridge chain
> > > > > > may
> > > > > > not have even been created yet. We do AUX transfers to read the EDID
> > > > > > and also to setup the backlight in the probe function of panel-edp.
> > > > > > At
> > > > > > that point the panel hasn't been linked into the chain. We had
> > > > > > _long_
> > > > > > discussions [1] about moving these out of probe and decided that we
> > > > > > could move the EDID read to be later but that it was going to really
> > > > > > ugly to move the AUX backlight later. The backlight would end up
> > > > > > popping up at some point in time later (the first call to panel
> > > > > > prepare() or maybe get_modes()) and that seemed weird.
> > > > > >
> > > > > > [1]
> > > > > > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> > > > > >
> > > > > >
> > > > > > > Otherwise you can't trust that eg. the /dev/aux
> > > > > > > stuff is actually usable.
> > > > > >
> > > > > > Yeah, it's been on my mind to talk more about /
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
So I think if Ville is OK with it (an ack at least) I'm fine with this
documentation change. I think it's worth me noting for other reviewers I made
this decision based on the fact that the documentation is for the transfer()
function - which I agree shouldn't need to be responsible for powering the
panel on.
Since that doesn't actually specify what we expect the behavior for userspace
accesses to be (since we could in theory add more behavior in those codepaths
around the transfer() calls that don't exist for the driver's own AUX usages)
I think this is totally fine
Reviewed-by: Lyude Paul
On Tue, 2022-05-03 at 16:21 -0700, Douglas Anderson wrote:
> When doing DP AUX transfers there are two actors that need to be
> powered in order for the DP AUX transfer to work: the DP source and
> the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> requirement on side-channel operations") added some documentation
> saying that the DP source is required to power itself up (if needed)
> to do AUX transfers. However, that commit doesn't talk anything about
> the DP sink.
>
> For full fledged DP the sink isn't really a problem. It's expected
> that if an external DP monitor isn't plugged in that attempting to do
> AUX transfers won't work. It's also expected that if a DP monitor is
> plugged in (and thus asserting HPD) that it AUX transfers will work.
>
> When we're looking at eDP, however, things are less obvious. Let's add
> some documentation about expectations. Here's what we'll say:
>
> 1. We don't expect the DP AUX transfer function to power on an eDP
> panel. If an eDP panel is physically connected but powered off then it
> makes sense for the transfer to fail.
>
> 2. We'll document that the official way to power on a panel is via the
> bridge chain, specifically by making sure that the panel's prepare
> function has been called (which is called by
> panel_bridge_pre_enable()). It's already specified in the kernel doc
> of drm_panel_prepare() that this is the way to power the panel on and
> also that after this call "it is possible to communicate with any
> integrated circuitry via a command bus."
>
> 3. We'll also document that for code running in the panel driver
> itself that it is legal for the panel driver to power itself up
> however it wants (it doesn't need to officially call
> drm_panel_pre_enable()) and then it can do AUX bus transfers. This is
> currently the way that edp-panel works when it's running atop the DP
> AUX bus.
>
> Signed-off-by: Douglas Anderson
> ---
>
> include/drm/display/drm_dp_helper.h | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index dca40a045dd6..e5165b708a40 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -370,9 +370,17 @@ struct drm_dp_aux {
> * helpers assume this is the case.
> *
> * Also note that this callback can be called no matter the
> - * state @dev is in. Drivers that need that device to be powered
> - * to perform this operation will first need to make sure it's
> - * been properly enabled.
> + * state @dev is in and also no matter what state the panel is
> + * in. It's expected:
> + * - If the @dev providing the AUX bus is currently unpowered then
> + * it will power itself up for the transfer.
> + * - If we're on eDP and the panel is not in a state where it can
> + * respond (it's not powered or it's in a low power state) then
> this
> + * function will return an error (but not crash). Note that if a
> + * panel driver is initiating a DP AUX transfer it may power
> itself
> + * up however it wants. All other code should ensure that the
> + * pre_enable() bridge chain (which eventually calls the panel
> + * prepare function) has powered the panel.
> */
> ssize_t (*transfer)(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg);
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, May 05, 2022 at 08:53:12AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> wrote:
> >
> > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > wrote:
> > > > > > >
> > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > > > powered in order for the DP AUX transfer to work: the DP source
> > > > > > > > and
> > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > > > requirement on side-channel operations") added some
> > > > > > > > documentation
> > > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > > needed)
> > > > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > > > about
> > > > > > > > the DP sink.
> > > > > > > >
> > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > expected
> > > > > > > > that if an external DP monitor isn't plugged in that attempting
> > > > > > > > to do
> > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > monitor is
> > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will
> > > > > > > > work.
> > > > > > > >
> > > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > > Let's add
> > > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > > >
> > > > > > > > 1. We don't expect the DP AUX transfer function to power on an
> > > > > > > > eDP
> > > > > > > > panel. If an eDP panel is physically connected but powered off
> > > > > > > > then it
> > > > > > > > makes sense for the transfer to fail.
> > > > > > >
> > > > > > > I don't agree with this. I think the panel should just get powred
> > > > > > > up
> > > > > > > for AUX transfers.
> > > > > >
> > > > > > That's definitely a fair thing to think about and I have at times
> > > > > > thought about trying to make it work that way. It always ends up
> > > > > > hitting a roadblock.
> > > >
> > > > How do you even probe the panel initially if you can't power it on
> > > > without doing some kind of full modeset/etc.?
> > >
> > > It's not that we can't power it on without a full modeset. It' that at
> > > panel probe time all the DRM components haven't been hooked together
> > > yet, so the bridge chain isn't available yet. The panel can power
> > > itself on, though. This is why the documentation I added says: "if a
> > > panel driver is initiating a DP AUX transfer it may power itself up
> > > however it wants"
> > >
> > >
> > > > > > The biggest roadblock that I recall is that to make this work then
> > > > > > you'd have to somehow ensure that the bridge chain's pre_enable()
> > > > > > call
> > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > function can be called in any context at all, we have to coordinate
> > > > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > > > panel down then we need to wait for DRM to fully finish powering
> > > > > > down,
> > > > > > then we need to power the panel back up. I don't believe that we can
> > > > > > just force the panel to stay on if DRM is turning it off because of
> > > > > > panel power sequencing requirements. At least I know it would have
> > > > > > the
> > > > > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > > > > see the panel power off after it's been disabled.
> > > > > >
> > > > > > We also, I believe, need to handle the fact that the bridge chain
> > > > > > may
> > > > > > not have even been created yet. We do AUX transfers to read the EDID
> > > > > > and also to setup the backlight in the probe function of panel-edp.
> > > > > > At
> > > > > > that point the panel hasn't been linked into the chain. We had
> > > > > > _long_
> > > > > > discussions [1] about moving these out of probe and decided that we
> > > > > > could move the EDID read to be later but that it was going to really
> > > > > > ugly to move the AUX backlight later. The backlight would end up
> > > > > > popping up at some point in time later (the first call to panel
> > > > > > prepare() or maybe get_modes()) and that seemed weird.
> > > > > >
> > > > > > [1]
> > > > > > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> > > > > >
> > > > > >
> > > > > > > Otherwise you can't trust that eg. the /dev/aux
> > > > > > > stuff is actually usable.
> > > > > >
> > > > > > Yeah, it's been on my mind to talk m
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
Quoting Sankeerth Billakanti (QUIC) (2022-05-05 11:47:20) > >Quoting Sankeerth Billakanti (2022-05-05 11:02:36) > >> > >> Our internal power grid documents list the regulators as VDD_A_*_1P2 > >> and VDD_A_*_0P9 for all the platforms. > > > >Do your internal power grid documents indicate what these supplies are > >powering? The question is if these supplies power any of the logic inside the > >eDP controller or if they only supply power to the analog circuits in the eDP > >phy. If it's the eDP phy only then the regulator usage in the eDP driver > >should > >be removed. I would suspect this is the case because the controller is > >probably all digital logic and runs at the typical 1.8V that the rest of the > >SoC > >uses. Similarly, these are voltage references which sound like a PLL > >reference > >voltage. > > > >Please clarify this further. > > > > For the DP driver using the usb-dp combo phy, there were cases where the usb > driver > was turning off the phy and pll regulators whenever usb-dp concurrent mode > need not be supported. > This caused phy and pll to be powered down causing aux transaction failures > and display blankouts. > From then on, it became a practice for the controller driver to vote for the > phy and pll regulators also. > That sounds like USB-DP combo phy driver had improper regulator power management where aux transactions from DP didn't keep the power on to the phy. Where does the power physically go? If the power isn't physically going to the DP controller it shouldn't be controlled from the DP controller driver. If the aux bus needs the DP phy enabled, the DP controller driver should enable the phy power (via phy_power_on()?).
Re: [Freedreno] [PATCH 2/2] dt-bindings: phy: List supplies for qcom, edp-phy
>We're supposed to list the supplies in the dt bindings but there are none in >the eDP PHY bindings. > >Looking at the driver in Linux, I can see that there seem to be two relevant >supplies: "vdda-phy" and "vdda-pll". Let's add those to the bindings. > >NOTE: from looking at the Qualcomm datasheet for sc7280, it's not >immediately clear how to figure out how to fill in these supplies. The only two >eDP related supplies are simply described as "power for eDP 0.9V circuits" and >"power for eDP 1.2V circuits". From guessing and from comparing how a >similar PHY is hooked up on other similar Qualcomm boards, I'll make the >educated guess that the 1.2V supply goes to "vdda-phy" and the 0.9V supply >goes to "vdda-pll" and I'll use that in the example here. > >Signed-off-by: Douglas Anderson >--- > > Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml | 6 ++ > 1 file changed, 6 insertions(+) > >diff --git a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >index a5850ff529f8..cf9e9b8011cb 100644 >--- a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >+++ b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >@@ -41,6 +41,9 @@ properties: > "#phy-cells": > const: 0 > >+ vdda-phy-supply: true >+ vdda-pll-supply: true >+ > required: > - compatible > - reg >@@ -65,5 +68,8 @@ examples: > > #clock-cells = <1>; > #phy-cells = <0>; >+ >+ vdda-phy-supply = <&vdd_a_edp_0_1p2>; >+ vdda-pll-supply = <&vdd_a_edp_0_0p9>; > }; > ... >-- >2.36.0.rc2.479.g8af0fa9b8e-goog Reviewed-by: Sankeerth Billakanti
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
>Quoting Sankeerth Billakanti (2022-05-05 11:02:36) >> >> >> >> Quoting Douglas Anderson (2022-04-25 14:06:42) >> >> >> >> Having 'a' in 'vdda' typically means 'analog' for 'analog' >> >> circuits, so I'd expect this to only matter for the phy that >> >> contains the analog circuitry. It would be great to remove the >> >> regulator code from drivers/gpu/drm/msm/dp/dp_power.c and move >the >> >> regulator_set_load() call to the phy driver if possible. Hopefully >> >> qcom folks can help clarify here. >> > >> >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A". >> >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the >> >schematic in front of me someone labeled these pins on the sc7280 >> >with the "A". ...and the driver looks for a supply with the "a". :-/ >> > >> >It would be good to get clarification from someone with better >information. >> > >> >-Doug >> >> Our internal power grid documents list the regulators as VDD_A_*_1P2 >> and VDD_A_*_0P9 for all the platforms. > >Do your internal power grid documents indicate what these supplies are >powering? The question is if these supplies power any of the logic inside the >eDP controller or if they only supply power to the analog circuits in the eDP >phy. If it's the eDP phy only then the regulator usage in the eDP driver should >be removed. I would suspect this is the case because the controller is >probably all digital logic and runs at the typical 1.8V that the rest of the >SoC >uses. Similarly, these are voltage references which sound like a PLL reference >voltage. > >Please clarify this further. > For the DP driver using the usb-dp combo phy, there were cases where the usb driver was turning off the phy and pll regulators whenever usb-dp concurrent mode need not be supported. This caused phy and pll to be powered down causing aux transaction failures and display blankouts. From then on, it became a practice for the controller driver to vote for the phy and pll regulators also. >> >> So, as a practice, we put the same name in the DT files. Hence, >> >> Reviewed-by: Sankeerth Billakanti >>
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
Quoting Sankeerth Billakanti (2022-05-05 11:02:36) > >> > >> Quoting Douglas Anderson (2022-04-25 14:06:42) > >> > >> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, > >> so I'd expect this to only matter for the phy that contains the analog > >> circuitry. It would be great to remove the regulator code from > >> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() > >> call to the phy driver if possible. Hopefully qcom folks can help > >> clarify here. > > > >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A". > >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the > >schematic in front of me someone labeled these pins on the sc7280 with the > >"A". ...and the driver looks for a supply with the "a". :-/ > > > >It would be good to get clarification from someone with better information. > > > >-Doug > > Our internal power grid documents list the regulators as VDD_A_*_1P2 and > VDD_A_*_0P9 > for all the platforms. Do your internal power grid documents indicate what these supplies are powering? The question is if these supplies power any of the logic inside the eDP controller or if they only supply power to the analog circuits in the eDP phy. If it's the eDP phy only then the regulator usage in the eDP driver should be removed. I would suspect this is the case because the controller is probably all digital logic and runs at the typical 1.8V that the rest of the SoC uses. Similarly, these are voltage references which sound like a PLL reference voltage. Please clarify this further. > > So, as a practice, we put the same name in the DT files. Hence, > > Reviewed-by: Sankeerth Billakanti >
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
Hi Doug, >> >> Quoting Douglas Anderson (2022-04-25 14:06:42) >> > We're supposed to list the supplies in the dt bindings but there are >> > none in the DP controller bindings. Looking at the Linux driver and >> > existing device trees, we can see that two supplies are expected: >> > - vdda-0p9-supply >> > - vdda-1p2-supply >> > >> > Let's list them both in the bindings. Note that the datasheet for >> > sc7280 doesn't describe these supplies very verbosely. For the 0p9 >> > supply, for instance, it says "Power for eDP 0.9 V circuits". This >> > this is obvious from the property name, we don't bother cluttering >> > the bindings with a description. >> > >> > Signed-off-by: Douglas Anderson >> > --- >> > >> > .../devicetree/bindings/display/msm/dp-controller.yaml | 6 ++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git >> > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> > index cd05cfd76536..dba31108db51 100644 >> > --- >> > a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.ya >> > +++ ml >> > @@ -76,6 +76,9 @@ properties: >> >"#sound-dai-cells": >> > const: 0 >> > >> > + vdda-0p9-supply: true >> > + vdda-1p2-supply: true >> > + >> >ports: >> > $ref: /schemas/graph.yaml#/properties/ports >> > properties: >> > @@ -137,6 +140,9 @@ examples: >> > >> > power-domains = <&rpmhpd SC7180_CX>; >> > >> > +vdda-0p9-supply = <&vdda_usb_ss_dp_core>; >> >> Having 'a' in 'vdda' typically means 'analog' for 'analog' circuits, >> so I'd expect this to only matter for the phy that contains the analog >> circuitry. It would be great to remove the regulator code from >> drivers/gpu/drm/msm/dp/dp_power.c and move the regulator_set_load() >> call to the phy driver if possible. Hopefully qcom folks can help >> clarify here. > >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A". >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the >schematic in front of me someone labeled these pins on the sc7280 with the >"A". ...and the driver looks for a supply with the "a". :-/ > >It would be good to get clarification from someone with better information. > >-Doug Our internal power grid documents list the regulators as VDD_A_*_1P2 and VDD_A_*_0P9 for all the platforms. So, as a practice, we put the same name in the DT files. Hence, Reviewed-by: Sankeerth Billakanti Thank you, Sankeerth
Re: [Freedreno] [PATCH v2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected
On 5/5/2022 1:48 AM, Dmitry Baryshkov wrote:
On Thu, 5 May 2022 at 05:06, Rob Clark wrote:
On Wed, May 4, 2022 at 6:55 PM Jessica Zhang wrote:
mdp5_get_global_state runs the risk of hitting a -EDEADLK when acquiring
the modeset lock, but currently mdp5_pipe_release doesn't check for if
an error is returned. Because of this, there is a possibility of
mdp5_pipe_release hitting a NULL dereference error.
To avoid this, let's have mdp5_pipe_release check if
mdp5_get_global_state returns an error and propogate that error.
Changes since v1:
- Separated declaration and initialization of *new_state to avoid
compiler warning
- Fixed some spelling mistakes in commit message
Note that mdp5_mixer_release() needs the same treatment.. one more comment below
Got it, will submit a fix for that.
Signed-off-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 15 +++
drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h | 2 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 20
3 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
index ba6695963aa6..97887a2be082 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
@@ -119,18 +119,23 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
drm_plane *plane,
return 0;
}
-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe)
{
struct msm_drm_private *priv = s->dev->dev_private;
struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
struct mdp5_global_state *state = mdp5_get_global_state(s);
- struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
+ struct mdp5_hw_pipe_state *new_state;
if (!hwpipe)
- return;
+ return -EINVAL;
At least per the current code, !hwpipe is "normal".. I think that fits
the model of things like kfree(NULL), so lets make this just return 0
Especially since we release the r_hwpipe w/o additional check. And
r_hwpipe frequently is NULL.
Noted.
Thanks,
Jessica Zhang
+
+ if (IS_ERR(state))
+ return PTR_ERR(state);
+
+ new_state = &state->hwpipe;
if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
- return;
+ return -EINVAL;
DBG("%s: release from plane %s", hwpipe->name,
new_state->hwpipe_to_plane[hwpipe->idx]->name);
@@ -141,6 +146,8 @@ void mdp5_pipe_release(struct drm_atomic_state *s, struct
mdp5_hw_pipe *hwpipe)
}
new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
+
+ return 0;
}
void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
index 9b26d0761bd4..cca67938cab2 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
@@ -37,7 +37,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
drm_plane *plane,
uint32_t caps, uint32_t blkcfg,
struct mdp5_hw_pipe **hwpipe,
struct mdp5_hw_pipe **r_hwpipe);
-void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
*hwpipe);
+int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe *hwpipe);
struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
uint32_t reg_offset, uint32_t caps);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 228b22830970..979458482841 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -311,12 +311,24 @@ static int mdp5_plane_atomic_check_with_state(struct
drm_crtc_state *crtc_state,
mdp5_state->r_hwpipe = NULL;
- mdp5_pipe_release(state->state, old_hwpipe);
- mdp5_pipe_release(state->state, old_right_hwpipe);
+ ret = mdp5_pipe_release(state->state, old_hwpipe);
+ if (ret)
+ return ret;
+
+ ret = mdp5_pipe_release(state->state, old_right_hwpipe);
+ if (ret)
+ return ret;
+
}
} else {
- mdp5_pipe_release(state->state, mdp5_state->hwpipe);
- mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+ ret = mdp5_pipe_release(state->state, mdp5_state->hwpipe);
+ if (ret)
+ return ret;
+
+ ret = mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
+ if (ret)
+ return ret;
+
mdp5_state->
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
wrote:
>
> On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > wrote:
> > >
> > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > wrote:
> > > > > >
> > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > > powered in order for the DP AUX transfer to work: the DP source
> > > > > > > and
> > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > > requirement on side-channel operations") added some documentation
> > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > needed)
> > > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > > about
> > > > > > > the DP sink.
> > > > > > >
> > > > > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > > > > that if an external DP monitor isn't plugged in that attempting
> > > > > > > to do
> > > > > > > AUX transfers won't work. It's also expected that if a DP monitor
> > > > > > > is
> > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will
> > > > > > > work.
> > > > > > >
> > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > Let's add
> > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > >
> > > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > > > > panel. If an eDP panel is physically connected but powered off
> > > > > > > then it
> > > > > > > makes sense for the transfer to fail.
> > > > > >
> > > > > > I don't agree with this. I think the panel should just get powred up
> > > > > > for AUX transfers.
> > > > >
> > > > > That's definitely a fair thing to think about and I have at times
> > > > > thought about trying to make it work that way. It always ends up
> > > > > hitting a roadblock.
> > >
> > > How do you even probe the panel initially if you can't power it on
> > > without doing some kind of full modeset/etc.?
> >
> > It's not that we can't power it on without a full modeset. It' that at
> > panel probe time all the DRM components haven't been hooked together
> > yet, so the bridge chain isn't available yet. The panel can power
> > itself on, though. This is why the documentation I added says: "if a
> > panel driver is initiating a DP AUX transfer it may power itself up
> > however it wants"
> >
> >
> > > > > The biggest roadblock that I recall is that to make this work then
> > > > > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > function can be called in any context at all, we have to coordinate
> > > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > > panel down then we need to wait for DRM to fully finish powering down,
> > > > > then we need to power the panel back up. I don't believe that we can
> > > > > just force the panel to stay on if DRM is turning it off because of
> > > > > panel power sequencing requirements. At least I know it would have the
> > > > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > > > see the panel power off after it's been disabled.
> > > > >
> > > > > We also, I believe, need to handle the fact that the bridge chain may
> > > > > not have even been created yet. We do AUX transfers to read the EDID
> > > > > and also to setup the backlight in the probe function of panel-edp. At
> > > > > that point the panel hasn't been linked into the chain. We had _long_
> > > > > discussions [1] about moving these out of probe and decided that we
> > > > > could move the EDID read to be later but that it was going to really
> > > > > ugly to move the AUX backlight later. The backlight would end up
> > > > > popping up at some point in time later (the first call to panel
> > > > > prepare() or maybe get_modes()) and that seemed weird.
> > > > >
> > > > > [1]
> > > > > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> > > > >
> > > > >
> > > > > > Otherwise you can't trust that eg. the /dev/aux
> > > > > > stuff is actually usable.
> > > > >
> > > > > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > > > > /dev/aux has some problems, at least with eDP. Specifically:
> > > > >
> > > > > 1. Even if we somehow figure out how to power the panel on as part of
> > > > > the aux transfer, we actually _still_ not guaranteed to be able to
> > > > > talk to it as far as I understand. My colleague reported to me that on
> > > > > a system he was working with that had PS
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> wrote:
> >
> > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > wrote:
> > > > >
> > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > requirement on side-channel operations") added some documentation
> > > > > > saying that the DP source is required to power itself up (if needed)
> > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > about
> > > > > > the DP sink.
> > > > > >
> > > > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > > > that if an external DP monitor isn't plugged in that attempting to
> > > > > > do
> > > > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > > > >
> > > > > > When we're looking at eDP, however, things are less obvious. Let's
> > > > > > add
> > > > > > some documentation about expectations. Here's what we'll say:
> > > > > >
> > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > > > panel. If an eDP panel is physically connected but powered off then
> > > > > > it
> > > > > > makes sense for the transfer to fail.
> > > > >
> > > > > I don't agree with this. I think the panel should just get powred up
> > > > > for AUX transfers.
> > > >
> > > > That's definitely a fair thing to think about and I have at times
> > > > thought about trying to make it work that way. It always ends up
> > > > hitting a roadblock.
> >
> > How do you even probe the panel initially if you can't power it on
> > without doing some kind of full modeset/etc.?
>
> It's not that we can't power it on without a full modeset. It' that at
> panel probe time all the DRM components haven't been hooked together
> yet, so the bridge chain isn't available yet. The panel can power
> itself on, though. This is why the documentation I added says: "if a
> panel driver is initiating a DP AUX transfer it may power itself up
> however it wants"
>
>
> > > > The biggest roadblock that I recall is that to make this work then
> > > > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > > > was made as part of the AUX transfer, right? Since the transfer
> > > > function can be called in any context at all, we have to coordinate
> > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > panel down then we need to wait for DRM to fully finish powering down,
> > > > then we need to power the panel back up. I don't believe that we can
> > > > just force the panel to stay on if DRM is turning it off because of
> > > > panel power sequencing requirements. At least I know it would have the
> > > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > > see the panel power off after it's been disabled.
> > > >
> > > > We also, I believe, need to handle the fact that the bridge chain may
> > > > not have even been created yet. We do AUX transfers to read the EDID
> > > > and also to setup the backlight in the probe function of panel-edp. At
> > > > that point the panel hasn't been linked into the chain. We had _long_
> > > > discussions [1] about moving these out of probe and decided that we
> > > > could move the EDID read to be later but that it was going to really
> > > > ugly to move the AUX backlight later. The backlight would end up
> > > > popping up at some point in time later (the first call to panel
> > > > prepare() or maybe get_modes()) and that seemed weird.
> > > >
> > > > [1]
> > > > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> > > >
> > > >
> > > > > Otherwise you can't trust that eg. the /dev/aux
> > > > > stuff is actually usable.
> > > >
> > > > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > > > /dev/aux has some problems, at least with eDP. Specifically:
> > > >
> > > > 1. Even if we somehow figure out how to power the panel on as part of
> > > > the aux transfer, we actually _still_ not guaranteed to be able to
> > > > talk to it as far as I understand. My colleague reported to me that on
> > > > a system he was working with that had PSR (panel self refresh) that
> > > > when the panel was powered on but in PSR mode that it wouldn't talk
> > > > over AUX. Assuming that this is correct then I guess we'd also have to
> > > > do even more coordination with DRM to exit PSR and block future
> > > > transitions of PSR. (NOTE: it's alway
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
wrote:
>
> On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > requirement on side-channel operations") added some documentation
> > > > > saying that the DP source is required to power itself up (if needed)
> > > > > to do AUX transfers. However, that commit doesn't talk anything about
> > > > > the DP sink.
> > > > >
> > > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > > that if an external DP monitor isn't plugged in that attempting to do
> > > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > > >
> > > > > When we're looking at eDP, however, things are less obvious. Let's add
> > > > > some documentation about expectations. Here's what we'll say:
> > > > >
> > > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > > panel. If an eDP panel is physically connected but powered off then it
> > > > > makes sense for the transfer to fail.
> > > >
> > > > I don't agree with this. I think the panel should just get powred up
> > > > for AUX transfers.
> > >
> > > That's definitely a fair thing to think about and I have at times
> > > thought about trying to make it work that way. It always ends up
> > > hitting a roadblock.
>
> How do you even probe the panel initially if you can't power it on
> without doing some kind of full modeset/etc.?
It's not that we can't power it on without a full modeset. It' that at
panel probe time all the DRM components haven't been hooked together
yet, so the bridge chain isn't available yet. The panel can power
itself on, though. This is why the documentation I added says: "if a
panel driver is initiating a DP AUX transfer it may power itself up
however it wants"
> > > The biggest roadblock that I recall is that to make this work then
> > > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > > was made as part of the AUX transfer, right? Since the transfer
> > > function can be called in any context at all, we have to coordinate
> > > this with DRM. If, for instance, DRM is mid way through powering the
> > > panel down then we need to wait for DRM to fully finish powering down,
> > > then we need to power the panel back up. I don't believe that we can
> > > just force the panel to stay on if DRM is turning it off because of
> > > panel power sequencing requirements. At least I know it would have the
> > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > see the panel power off after it's been disabled.
> > >
> > > We also, I believe, need to handle the fact that the bridge chain may
> > > not have even been created yet. We do AUX transfers to read the EDID
> > > and also to setup the backlight in the probe function of panel-edp. At
> > > that point the panel hasn't been linked into the chain. We had _long_
> > > discussions [1] about moving these out of probe and decided that we
> > > could move the EDID read to be later but that it was going to really
> > > ugly to move the AUX backlight later. The backlight would end up
> > > popping up at some point in time later (the first call to panel
> > > prepare() or maybe get_modes()) and that seemed weird.
> > >
> > > [1]
> > > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> > >
> > >
> > > > Otherwise you can't trust that eg. the /dev/aux
> > > > stuff is actually usable.
> > >
> > > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > > /dev/aux has some problems, at least with eDP. Specifically:
> > >
> > > 1. Even if we somehow figure out how to power the panel on as part of
> > > the aux transfer, we actually _still_ not guaranteed to be able to
> > > talk to it as far as I understand. My colleague reported to me that on
> > > a system he was working with that had PSR (panel self refresh) that
> > > when the panel was powered on but in PSR mode that it wouldn't talk
> > > over AUX. Assuming that this is correct then I guess we'd also have to
> > > do even more coordination with DRM to exit PSR and block future
> > > transitions of PSR. (NOTE: it's always possible that my colleague ran
> > > into some other bug and that panels are _supposed_ to be able to talk
> > > in PSR. If you think this is the case, I can always try to dig more).
> >
> > TBH - the coordination with drm I don't think would be the difficult part,
> > as
> > we'd just need to
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
Hi,
On Wed, May 4, 2022 at 11:10 AM Lyude Paul wrote:
>
> On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > wrote:
> > >
> > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > When doing DP AUX transfers there are two actors that need to be
> > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > requirement on side-channel operations") added some documentation
> > > > saying that the DP source is required to power itself up (if needed)
> > > > to do AUX transfers. However, that commit doesn't talk anything about
> > > > the DP sink.
> > > >
> > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > that if an external DP monitor isn't plugged in that attempting to do
> > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > >
> > > > When we're looking at eDP, however, things are less obvious. Let's add
> > > > some documentation about expectations. Here's what we'll say:
> > > >
> > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > panel. If an eDP panel is physically connected but powered off then it
> > > > makes sense for the transfer to fail.
> > >
> > > I don't agree with this. I think the panel should just get powred up
> > > for AUX transfers.
> >
> > That's definitely a fair thing to think about and I have at times
> > thought about trying to make it work that way. It always ends up
> > hitting a roadblock.
> >
> > The biggest roadblock that I recall is that to make this work then
> > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > was made as part of the AUX transfer, right? Since the transfer
> > function can be called in any context at all, we have to coordinate
> > this with DRM. If, for instance, DRM is mid way through powering the
> > panel down then we need to wait for DRM to fully finish powering down,
> > then we need to power the panel back up. I don't believe that we can
> > just force the panel to stay on if DRM is turning it off because of
> > panel power sequencing requirements. At least I know it would have the
> > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > see the panel power off after it's been disabled.
> >
> > We also, I believe, need to handle the fact that the bridge chain may
> > not have even been created yet. We do AUX transfers to read the EDID
> > and also to setup the backlight in the probe function of panel-edp. At
> > that point the panel hasn't been linked into the chain. We had _long_
> > discussions [1] about moving these out of probe and decided that we
> > could move the EDID read to be later but that it was going to really
> > ugly to move the AUX backlight later. The backlight would end up
> > popping up at some point in time later (the first call to panel
> > prepare() or maybe get_modes()) and that seemed weird.
> >
> > [1]
> > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> >
> >
> > > Otherwise you can't trust that eg. the /dev/aux
> > > stuff is actually usable.
> >
> > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > /dev/aux has some problems, at least with eDP. Specifically:
> >
> > 1. Even if we somehow figure out how to power the panel on as part of
> > the aux transfer, we actually _still_ not guaranteed to be able to
> > talk to it as far as I understand. My colleague reported to me that on
> > a system he was working with that had PSR (panel self refresh) that
> > when the panel was powered on but in PSR mode that it wouldn't talk
> > over AUX. Assuming that this is correct then I guess we'd also have to
> > do even more coordination with DRM to exit PSR and block future
> > transitions of PSR. (NOTE: it's always possible that my colleague ran
> > into some other bug and that panels are _supposed_ to be able to talk
> > in PSR. If you think this is the case, I can always try to dig more).
>
> TBH - the coordination with drm I don't think would be the difficult part, as
> we'd just need to add some sort of property (ideally invisible to userspace)
> that can be used in an atomic commit to disable PSR - similar to how we enable
> CRC readback from sysfs in the majority of DRM drivers. That being said
> though, I think we can just leave the work of solving this problem up to
> whoever ends up needing this to work.
>
> >
> > 2. I'm not totally convinced that it's a great idea, at least for eDP,
> > for userspace to be mucking with /dev/aux. For DP's case I guess
> > /dev/aux is essentially enabling userspace drivers to do things like
> > update firmware on DP monitors or play with the backlight. I guess we
> > decided that we didn't want to add drivers in the kernel to handle
> > th
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > wrote:
> > >
> > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > When doing DP AUX transfers there are two actors that need to be
> > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > requirement on side-channel operations") added some documentation
> > > > saying that the DP source is required to power itself up (if needed)
> > > > to do AUX transfers. However, that commit doesn't talk anything about
> > > > the DP sink.
> > > >
> > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > that if an external DP monitor isn't plugged in that attempting to do
> > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > >
> > > > When we're looking at eDP, however, things are less obvious. Let's add
> > > > some documentation about expectations. Here's what we'll say:
> > > >
> > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > panel. If an eDP panel is physically connected but powered off then it
> > > > makes sense for the transfer to fail.
> > >
> > > I don't agree with this. I think the panel should just get powred up
> > > for AUX transfers.
> >
> > That's definitely a fair thing to think about and I have at times
> > thought about trying to make it work that way. It always ends up
> > hitting a roadblock.
How do you even probe the panel initially if you can't power it on
without doing some kind of full modeset/etc.?
> >
> > The biggest roadblock that I recall is that to make this work then
> > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > was made as part of the AUX transfer, right? Since the transfer
> > function can be called in any context at all, we have to coordinate
> > this with DRM. If, for instance, DRM is mid way through powering the
> > panel down then we need to wait for DRM to fully finish powering down,
> > then we need to power the panel back up. I don't believe that we can
> > just force the panel to stay on if DRM is turning it off because of
> > panel power sequencing requirements. At least I know it would have the
> > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > see the panel power off after it's been disabled.
> >
> > We also, I believe, need to handle the fact that the bridge chain may
> > not have even been created yet. We do AUX transfers to read the EDID
> > and also to setup the backlight in the probe function of panel-edp. At
> > that point the panel hasn't been linked into the chain. We had _long_
> > discussions [1] about moving these out of probe and decided that we
> > could move the EDID read to be later but that it was going to really
> > ugly to move the AUX backlight later. The backlight would end up
> > popping up at some point in time later (the first call to panel
> > prepare() or maybe get_modes()) and that seemed weird.
> >
> > [1]
> > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> >
> >
> > > Otherwise you can't trust that eg. the /dev/aux
> > > stuff is actually usable.
> >
> > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > /dev/aux has some problems, at least with eDP. Specifically:
> >
> > 1. Even if we somehow figure out how to power the panel on as part of
> > the aux transfer, we actually _still_ not guaranteed to be able to
> > talk to it as far as I understand. My colleague reported to me that on
> > a system he was working with that had PSR (panel self refresh) that
> > when the panel was powered on but in PSR mode that it wouldn't talk
> > over AUX. Assuming that this is correct then I guess we'd also have to
> > do even more coordination with DRM to exit PSR and block future
> > transitions of PSR. (NOTE: it's always possible that my colleague ran
> > into some other bug and that panels are _supposed_ to be able to talk
> > in PSR. If you think this is the case, I can always try to dig more).
>
> TBH - the coordination with drm I don't think would be the difficult part, as
> we'd just need to add some sort of property (ideally invisible to userspace)
> that can be used in an atomic commit to disable PSR - similar to how we enable
> CRC readback from sysfs in the majority of DRM drivers. That being said
> though, I think we can just leave the work of solving this problem up to
> whoever ends up needing this to work.
The driver should just disable/prevent PSR when doing AUX if the hardware
can't guarantee the PSR and AUX won't interfere with each other.
For i915 we have no problems with powering the panel on for AUX, but
there is still a race with PSR v
[Freedreno] [PATCH 2/2] drm/msm/mdp4: get rid of struct mdp4_platform_config
Struct mdp4_platform_config is a relict from the DT-conversion time.
Move the max_clk field to the mdp4_kms_init(), the place where it is
used and drop the struct mdp4_platform_config and the mdp4_get_config()
function.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 21 ++---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 5 -
2 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 1fba6ab06eb1..ccde710c63fa 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -13,8 +13,6 @@
#include "msm_mmu.h"
#include "mdp4_kms.h"
-static struct mdp4_platform_config *mdp4_get_config(struct platform_device
*dev);
-
static int mdp4_hw_init(struct msm_kms *kms)
{
struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
@@ -384,7 +382,6 @@ static void read_mdp_hw_revision(struct mdp4_kms *mdp4_kms,
static int mdp4_kms_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
- struct mdp4_platform_config *config = mdp4_get_config(pdev);
struct msm_drm_private *priv = dev->dev_private;
struct mdp4_kms *mdp4_kms;
struct msm_kms *kms = NULL;
@@ -392,6 +389,10 @@ static int mdp4_kms_init(struct drm_device *dev)
struct msm_gem_address_space *aspace;
int irq, ret;
u32 major, minor;
+ unsigned long max_clk;
+
+ /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
+ max_clk = 27000;
mdp4_kms = kzalloc(sizeof(*mdp4_kms), GFP_KERNEL);
if (!mdp4_kms) {
@@ -459,7 +460,7 @@ static int mdp4_kms_init(struct drm_device *dev)
goto fail;
}
- clk_set_rate(mdp4_kms->clk, config->max_clk);
+ clk_set_rate(mdp4_kms->clk, max_clk);
read_mdp_hw_revision(mdp4_kms, &major, &minor);
@@ -479,7 +480,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = PTR_ERR(mdp4_kms->lut_clk);
goto fail;
}
- clk_set_rate(mdp4_kms->lut_clk, config->max_clk);
+ clk_set_rate(mdp4_kms->lut_clk, max_clk);
}
pm_runtime_enable(dev->dev);
@@ -552,16 +553,6 @@ static int mdp4_kms_init(struct drm_device *dev)
return ret;
}
-static struct mdp4_platform_config *mdp4_get_config(struct platform_device
*dev)
-{
- static struct mdp4_platform_config config = {};
-
- /* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
- config.max_clk = 27000;
-
- return &config;
-}
-
static const struct dev_pm_ops mdp4_pm_ops = {
.prepare = msm_pm_prepare,
.complete = msm_pm_complete,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
index 7cc549b6a82b..01179e764a29 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
@@ -42,11 +42,6 @@ struct mdp4_kms {
};
#define to_mdp4_kms(x) container_of(x, struct mdp4_kms, base)
-/* platform config data (ie. from DT, or pdata) */
-struct mdp4_platform_config {
- uint32_t max_clk;
-};
-
static inline void mdp4_write(struct mdp4_kms *mdp4_kms, u32 reg, u32 data)
{
msm_writel(data, mdp4_kms->mmio + reg);
--
2.35.1
[Freedreno] [PATCH 1/2] drm/msm/mdp4: move iommu_domain_alloc() call close to its usage
Move iommu_domain_alloc() in front of adress space/IOMMU initialization.
This allows us to drop it from struct mdp4_cfg_platform which
remained from the pre-DT days.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 8
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 1 -
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index fb48c8c19ec3..1fba6ab06eb1 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -388,6 +388,7 @@ static int mdp4_kms_init(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct mdp4_kms *mdp4_kms;
struct msm_kms *kms = NULL;
+ struct iommu_domain *iommu;
struct msm_gem_address_space *aspace;
int irq, ret;
u32 major, minor;
@@ -495,9 +496,9 @@ static int mdp4_kms_init(struct drm_device *dev)
mdp4_disable(mdp4_kms);
mdelay(16);
- if (config->iommu) {
- struct msm_mmu *mmu = msm_iommu_new(&pdev->dev,
- config->iommu);
+ iommu = iommu_domain_alloc(pdev->dev.bus);
+ if (iommu) {
+ struct msm_mmu *mmu = msm_iommu_new(&pdev->dev, iommu);
aspace = msm_gem_address_space_create(mmu,
"mdp4", 0x1000, 0x1 - 0x1000);
@@ -557,7 +558,6 @@ static struct mdp4_platform_config *mdp4_get_config(struct
platform_device *dev)
/* TODO: Chips that aren't apq8064 have a 200 Mhz max_clk */
config.max_clk = 27000;
- config.iommu = iommu_domain_alloc(&platform_bus_type);
return &config;
}
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
index e8ee92ab7956..7cc549b6a82b 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h
@@ -44,7 +44,6 @@ struct mdp4_kms {
/* platform config data (ie. from DT, or pdata) */
struct mdp4_platform_config {
- struct iommu_domain *iommu;
uint32_t max_clk;
};
--
2.35.1
[Freedreno] [PATCH 0/2] drm/msm/mpd4: get rid of struct mdp4_platform_config
The struct mdp4_platform_config is a remnant from pre-DT times. It serves no particular purpose nowadays. So let's get rid of it. Dmitry Baryshkov (2): drm/msm/mdp4: move iommu_domain_alloc() call close to its usage drm/msm/mdp4: get rid of struct mdp4_platform_config drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 29 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 6 - 2 files changed, 10 insertions(+), 25 deletions(-) -- 2.35.1
Re: [Freedreno] [PATCH] device property: Fix recent breakage of fwnode_get_next_parent_dev()
On Sun, May 1, 2022 at 9:50 AM Andy Shevchenko
wrote:
>
> On Sat, Apr 30, 2022 at 3:00 PM Douglas Anderson
> wrote:
> >
> > Due to a subtle typo, instead of commit 87ffea09470d ("device
> > property: Introduce fwnode_for_each_parent_node()") being a no-op
> > change, it ended up causing the display on my sc7180-trogdor-lazor
> > device from coming up unless I added "fw_devlink=off" to my kernel
> > command line. Fix the typo.
>
> Sorry and merci pour la fix!
> Reviewed-by: Andy Shevchenko
Applied, thanks!
> > Fixes: 87ffea09470d ("device property: Introduce
> > fwnode_for_each_parent_node()")
> > Signed-off-by: Douglas Anderson
> > ---
> >
> > drivers/base/property.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 36401cfe432c..52e85dcb20b5 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -600,7 +600,7 @@ struct device *fwnode_get_next_parent_dev(struct
> > fwnode_handle *fwnode)
> > struct device *dev;
> >
> > fwnode_for_each_parent_node(fwnode, parent) {
> > - dev = get_dev_from_fwnode(fwnode);
> > + dev = get_dev_from_fwnode(parent);
> > if (dev) {
> > fwnode_handle_put(parent);
> > return dev;
> > --
> > 2.36.0.464.gb9c8b46e94-goog
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [Freedreno] [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()
On Thu, 5 May 2022 at 13:27, Robin Murphy wrote:
>
> On 2022-05-05 01:16, Dmitry Baryshkov wrote:
> > Change msm_kms_init_aspace() to use generic function
> > device_iommu_mapped() instead of the fwnode-specific interface
> > dev_iommu_fwspec_get(). While we are at it, stop referencing
> > platform_bus_type directly and use the bus of the IOMMU device.
>
> FWIW, I'd have squashed these changes across the previous patches, such
> that the dodgy fwspec calls are never introduced in the first place, but
> it's your driver, and if that's the way you want to work it and Rob's
> happy with it too, then fine by me.
I thought about this. But as the calls were already there (in the
mdp5), it was easier for me to merge the code and to update it
afterwards.
>
> For the end result,
>
> Reviewed-by: Robin Murphy
>
> I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well,
> but unless there's any other reason to respin this series, that's
> something we could do as a follow-up. Thanks for sorting this out!
Not really. MDP4 doesn't have the parent MDSS device, so it doesn't
need all these troubles.
>
> Robin.
>
> > Suggested-by: Robin Murphy
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 98ae0036ab57..2fc3f820cd59 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -272,21 +272,21 @@ struct msm_gem_address_space
> > *msm_kms_init_aspace(struct drm_device *dev)
> > struct device *mdss_dev = mdp_dev->parent;
> > struct device *iommu_dev;
> >
> > - domain = iommu_domain_alloc(&platform_bus_type);
> > - if (!domain) {
> > - drm_info(dev, "no IOMMU, fallback to phys contig buffers for
> > scanout\n");
> > - return NULL;
> > - }
> > -
> > /*
> >* IOMMUs can be a part of MDSS device tree binding, or the
> >* MDP/DPU device.
> >*/
> > - if (dev_iommu_fwspec_get(mdp_dev))
> > + if (device_iommu_mapped(mdp_dev))
> > iommu_dev = mdp_dev;
> > else
> > iommu_dev = mdss_dev;
> >
> > + domain = iommu_domain_alloc(iommu_dev->bus);
> > + if (!domain) {
> > + drm_info(dev, "no IOMMU, fallback to phys contig buffers for
> > scanout\n");
> > + return NULL;
> > + }
> > +
> > mmu = msm_iommu_new(iommu_dev, domain);
> > if (IS_ERR(mmu)) {
> > iommu_domain_free(domain);
--
With best wishes
Dmitry
[Freedreno] [PATCH] drm/msm: return an error pointer in msm_gem_prime_get_sg_table()
The msm_gem_prime_get_sg_table() needs to return error pointers on
error. This is called from drm_gem_map_dma_buf() and returning a
NULL will lead to a crash in that function.
Fixes: ac45146733b0 ("drm/msm: fix msm_gem_prime_get_sg_table()")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_gem_prime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c
b/drivers/gpu/drm/msm/msm_gem_prime.c
index e8f1b7a2ca9c..94ab705e9b8a 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -17,7 +17,7 @@ struct sg_table *msm_gem_prime_get_sg_table(struct
drm_gem_object *obj)
int npages = obj->size >> PAGE_SHIFT;
if (WARN_ON(!msm_obj->pages)) /* should have already pinned! */
- return NULL;
+ return ERR_PTR(-ENOMEM);
return drm_prime_pages_to_sg(obj->dev, msm_obj->pages, npages);
}
--
2.35.1
Re: [Freedreno] [PATCH v2 5/5] drm/msm: switch msm_kms_init_aspace() to use device_iommu_mapped()
On 2022-05-05 01:16, Dmitry Baryshkov wrote:
Change msm_kms_init_aspace() to use generic function
device_iommu_mapped() instead of the fwnode-specific interface
dev_iommu_fwspec_get(). While we are at it, stop referencing
platform_bus_type directly and use the bus of the IOMMU device.
FWIW, I'd have squashed these changes across the previous patches, such
that the dodgy fwspec calls are never introduced in the first place, but
it's your driver, and if that's the way you want to work it and Rob's
happy with it too, then fine by me.
For the end result,
Reviewed-by: Robin Murphy
I'm guessing MDP4 could probably use msm_kms_init_aspace() now as well,
but unless there's any other reason to respin this series, that's
something we could do as a follow-up. Thanks for sorting this out!
Robin.
Suggested-by: Robin Murphy
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_drv.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 98ae0036ab57..2fc3f820cd59 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -272,21 +272,21 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct
drm_device *dev)
struct device *mdss_dev = mdp_dev->parent;
struct device *iommu_dev;
- domain = iommu_domain_alloc(&platform_bus_type);
- if (!domain) {
- drm_info(dev, "no IOMMU, fallback to phys contig buffers for
scanout\n");
- return NULL;
- }
-
/*
* IOMMUs can be a part of MDSS device tree binding, or the
* MDP/DPU device.
*/
- if (dev_iommu_fwspec_get(mdp_dev))
+ if (device_iommu_mapped(mdp_dev))
iommu_dev = mdp_dev;
else
iommu_dev = mdss_dev;
+ domain = iommu_domain_alloc(iommu_dev->bus);
+ if (!domain) {
+ drm_info(dev, "no IOMMU, fallback to phys contig buffers for
scanout\n");
+ return NULL;
+ }
+
mmu = msm_iommu_new(iommu_dev, domain);
if (IS_ERR(mmu)) {
iommu_domain_free(domain);
Re: [Freedreno] [PATCH v2] drm/msm/mdp5: Return error code in mdp5_pipe_release when deadlock is detected
On Thu, 5 May 2022 at 05:06, Rob Clark wrote:
>
> On Wed, May 4, 2022 at 6:55 PM Jessica Zhang
> wrote:
> >
> > mdp5_get_global_state runs the risk of hitting a -EDEADLK when acquiring
> > the modeset lock, but currently mdp5_pipe_release doesn't check for if
> > an error is returned. Because of this, there is a possibility of
> > mdp5_pipe_release hitting a NULL dereference error.
> >
> > To avoid this, let's have mdp5_pipe_release check if
> > mdp5_get_global_state returns an error and propogate that error.
> >
> > Changes since v1:
> > - Separated declaration and initialization of *new_state to avoid
> > compiler warning
> > - Fixed some spelling mistakes in commit message
> >
>
> Note that mdp5_mixer_release() needs the same treatment.. one more comment
> below
>
> > Signed-off-by: Jessica Zhang
> > ---
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c | 15 +++
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h | 2 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 20
> > 3 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> > index ba6695963aa6..97887a2be082 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c
> > @@ -119,18 +119,23 @@ int mdp5_pipe_assign(struct drm_atomic_state *s,
> > struct drm_plane *plane,
> > return 0;
> > }
> >
> > -void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> > *hwpipe)
> > +int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> > *hwpipe)
> > {
> > struct msm_drm_private *priv = s->dev->dev_private;
> > struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> > struct mdp5_global_state *state = mdp5_get_global_state(s);
> > - struct mdp5_hw_pipe_state *new_state = &state->hwpipe;
> > + struct mdp5_hw_pipe_state *new_state;
> >
> > if (!hwpipe)
> > - return;
> > + return -EINVAL;
>
> At least per the current code, !hwpipe is "normal".. I think that fits
> the model of things like kfree(NULL), so lets make this just return 0
Especially since we release the r_hwpipe w/o additional check. And
r_hwpipe frequently is NULL.
>
> > +
> > + if (IS_ERR(state))
> > + return PTR_ERR(state);
> > +
> > + new_state = &state->hwpipe;
> >
> > if (WARN_ON(!new_state->hwpipe_to_plane[hwpipe->idx]))
> > - return;
> > + return -EINVAL;
> >
> > DBG("%s: release from plane %s", hwpipe->name,
> > new_state->hwpipe_to_plane[hwpipe->idx]->name);
> > @@ -141,6 +146,8 @@ void mdp5_pipe_release(struct drm_atomic_state *s,
> > struct mdp5_hw_pipe *hwpipe)
> > }
> >
> > new_state->hwpipe_to_plane[hwpipe->idx] = NULL;
> > +
> > + return 0;
> > }
> >
> > void mdp5_pipe_destroy(struct mdp5_hw_pipe *hwpipe)
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> > index 9b26d0761bd4..cca67938cab2 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.h
> > @@ -37,7 +37,7 @@ int mdp5_pipe_assign(struct drm_atomic_state *s, struct
> > drm_plane *plane,
> > uint32_t caps, uint32_t blkcfg,
> > struct mdp5_hw_pipe **hwpipe,
> > struct mdp5_hw_pipe **r_hwpipe);
> > -void mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> > *hwpipe);
> > +int mdp5_pipe_release(struct drm_atomic_state *s, struct mdp5_hw_pipe
> > *hwpipe);
> >
> > struct mdp5_hw_pipe *mdp5_pipe_init(enum mdp5_pipe pipe,
> > uint32_t reg_offset, uint32_t caps);
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > index 228b22830970..979458482841 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> > @@ -311,12 +311,24 @@ static int mdp5_plane_atomic_check_with_state(struct
> > drm_crtc_state *crtc_state,
> > mdp5_state->r_hwpipe = NULL;
> >
> >
> > - mdp5_pipe_release(state->state, old_hwpipe);
> > - mdp5_pipe_release(state->state, old_right_hwpipe);
> > + ret = mdp5_pipe_release(state->state, old_hwpipe);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mdp5_pipe_release(state->state,
> > old_right_hwpipe);
> > + if (ret)
> > + return ret;
> > +
> > }
> > } else {
> > - mdp5_pipe_release(state->state, mdp5_state->hwpipe);
> > - mdp5_pipe_release(state->state, mdp5_state->r_hwpipe);
> > + r
