I've been experiencing some intermittent crashes down in the display
driver code. The symptoms are ususally a line like this in dmesg:
amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port 000000006d3a3885: -5
...followed by an Oops due to a NULL pointer dereference.
The real bug is probably in the caller of this function, which is
passing it a NULL state pointer, but this patch at least keeps my
machine from oopsing when this occurs.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
Signed-off-by: Jeff Layton <[email protected]>
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 38dab76ae69e..87ad406c50f9 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
/* Skip failed payloads */
if (payload->vc_start_slot == -1) {
- drm_dbg_kms(state->dev, "Part 1 of payload creation for %s failed, skipping part 2\n",
+ drm_dbg_kms(state ? state->dev : NULL,
+ "Part 1 of payload creation for %s failed, skipping part 2\n",
payload->port->connector->name);
return -EIO;
}
--
2.39.2
On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> I've been experiencing some intermittent crashes down in the display
> driver code. The symptoms are ususally a line like this in dmesg:
>
> amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port 000000006d3a3885: -5
>
> ...followed by an Oops due to a NULL pointer dereference.
>
> The real bug is probably in the caller of this function, which is
> passing it a NULL state pointer, but this patch at least keeps my
> machine from oopsing when this occurs.
My fear is that papering over this makes the root cause harder to find.
Cc: Harry, Alex
BR,
Jani.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 38dab76ae69e..87ad406c50f9 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
>
> /* Skip failed payloads */
> if (payload->vc_start_slot == -1) {
> - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s failed, skipping part 2\n",
> + drm_dbg_kms(state ? state->dev : NULL,
> + "Part 1 of payload creation for %s failed, skipping part 2\n",
> payload->port->connector->name);
> return -EIO;
> }
--
Jani Nikula, Intel Open Source Graphics Center
On Thu, 2023-04-13 at 15:31 +0300, Jani Nikula wrote:
> On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> > I've been experiencing some intermittent crashes down in the display
> > driver code. The symptoms are ususally a line like this in dmesg:
> >
> > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port 000000006d3a3885: -5
> >
> > ...followed by an Oops due to a NULL pointer dereference.
> >
> > The real bug is probably in the caller of this function, which is
> > passing it a NULL state pointer, but this patch at least keeps my
> > machine from oopsing when this occurs.
>
> My fear is that papering over this makes the root cause harder to find.
>
> Cc: Harry, Alex
>
>
> BR,
> Jani.
>
>
I'm happy to help track down the root cause. Display drivers are
somewhat outside my wheelhouse though.
Maybe we can throw a WARNING when this happens? I'd just like it to not
crash my machine.
> >
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 38dab76ae69e..87ad406c50f9 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> >
> > /* Skip failed payloads */
> > if (payload->vc_start_slot == -1) {
> > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s failed, skipping part 2\n",
> > + drm_dbg_kms(state ? state->dev : NULL,
> > + "Part 1 of payload creation for %s failed, skipping part 2\n",
> > payload->port->connector->name);
> > return -EIO;
> > }
>
--
Jeff Layton <[email protected]>
+ Wayne
On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]> wrote:
>
> On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> > I've been experiencing some intermittent crashes down in the display
> > driver code. The symptoms are ususally a line like this in dmesg:
> >
> > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port 000000006d3a3885: -5
> >
> > ...followed by an Oops due to a NULL pointer dereference.
> >
> > The real bug is probably in the caller of this function, which is
> > passing it a NULL state pointer, but this patch at least keeps my
> > machine from oopsing when this occurs.
>
> My fear is that papering over this makes the root cause harder to find.
>
> Cc: Harry, Alex
>
>
> BR,
> Jani.
>
>
> >
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 38dab76ae69e..87ad406c50f9 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
> >
> > /* Skip failed payloads */
> > if (payload->vc_start_slot == -1) {
> > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s failed, skipping part 2\n",
> > + drm_dbg_kms(state ? state->dev : NULL,
> > + "Part 1 of payload creation for %s failed, skipping part 2\n",
> > payload->port->connector->name);
> > return -EIO;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
[Public]
Hi Jeff,
Thanks. I might need more information to understand why we can't retrieve
the drm atomic state. Also , "Failed to create MST payload for port" indicates
error while configuring DPCD payload ID table. Could you help to provide log
with KMS + ATOMIC + DP debug on please? Thanks in advance!
Regards,
Wayne
> -----Original Message-----
> From: Alex Deucher <[email protected]>
> Sent: Thursday, April 13, 2023 8:59 PM
> To: Jani Nikula <[email protected]>; Lin, Wayne
> <[email protected]>
> Cc: Jeff Layton <[email protected]>; David Airlie <[email protected]>;
> Daniel Vetter <[email protected]>; Deucher, Alexander
> <[email protected]>; [email protected]; dri-
> [email protected]
> Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
> handle NULL state pointer
>
> + Wayne
>
> On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]>
> wrote:
> >
> > On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > I've been experiencing some intermittent crashes down in the display
> > > driver code. The symptoms are ususally a line like this in dmesg:
> > >
> > > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port
> > > 000000006d3a3885: -5
> > >
> > > ...followed by an Oops due to a NULL pointer dereference.
> > >
> > > The real bug is probably in the caller of this function, which is
> > > passing it a NULL state pointer, but this patch at least keeps my
> > > machine from oopsing when this occurs.
> >
> > My fear is that papering over this makes the root cause harder to find.
> >
> > Cc: Harry, Alex
> >
> >
> > BR,
> > Jani.
> >
> >
> > >
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > index 38dab76ae69e..87ad406c50f9 100644
> > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >
> > > /* Skip failed payloads */
> > > if (payload->vc_start_slot == -1) {
> > > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s
> failed, skipping part 2\n",
> > > + drm_dbg_kms(state ? state->dev : NULL,
> > > + "Part 1 of payload creation for %s failed,
> > > + skipping part 2\n",
> > > payload->port->connector->name);
> > > return -EIO;
> > > }
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> [Public]
>
> Hi Jeff,
>
> Thanks. I might need more information to understand why we can't retrieve
> the drm atomic state. Also , "Failed to create MST payload for port" indicates
> error while configuring DPCD payload ID table. Could you help to provide log
> with KMS + ATOMIC + DP debug on please? Thanks in advance!
>
> Regards,
> Wayne
>
Possibly. I'm not that familiar with display driver debugging. Can you
send me some directions on how to crank up that sort of debug logging?
Note that this problem is _very_ intermittent too: I went about 2 weeks
between crashes, and then I got 3 in one day. I'd rather not run with a
lot of debug logging for a long time if that's what this is going to
require, as this is my main workstation.
The last time I got this log message, my proposed patch did prevent the
box from oopsing, so I'd really like to see it go in unless it's just
categorically wrong for the caller to pass down a NULL state pointer to
drm_dp_add_payload_part2.
> > -----Original Message-----
> > From: Alex Deucher <[email protected]>
> > Sent: Thursday, April 13, 2023 8:59 PM
> > To: Jani Nikula <[email protected]>; Lin, Wayne
> > <[email protected]>
> > Cc: Jeff Layton <[email protected]>; David Airlie <[email protected]>;
> > Daniel Vetter <[email protected]>; Deucher, Alexander
> > <[email protected]>; [email protected]; dri-
> > [email protected]
> > Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
> > handle NULL state pointer
> >
> > + Wayne
> >
> > On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]>
> > wrote:
> > >
> > > On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > > I've been experiencing some intermittent crashes down in the display
> > > > driver code. The symptoms are ususally a line like this in dmesg:
> > > >
> > > > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port
> > > > 000000006d3a3885: -5
> > > >
> > > > ...followed by an Oops due to a NULL pointer dereference.
> > > >
> > > > The real bug is probably in the caller of this function, which is
> > > > passing it a NULL state pointer, but this patch at least keeps my
> > > > machine from oopsing when this occurs.
> > >
> > > My fear is that papering over this makes the root cause harder to find.
> > >
> > > Cc: Harry, Alex
> > >
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > >
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > index 38dab76ae69e..87ad406c50f9 100644
> > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct
> > > > drm_dp_mst_topology_mgr *mgr,
> > > >
> > > > /* Skip failed payloads */
> > > > if (payload->vc_start_slot == -1) {
> > > > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s
> > failed, skipping part 2\n",
> > > > + drm_dbg_kms(state ? state->dev : NULL,
> > > > + "Part 1 of payload creation for %s failed,
> > > > + skipping part 2\n",
> > > > payload->port->connector->name);
> > > > return -EIO;
> > > > }
> > >
> > > --
> > > Jani Nikula, Intel Open Source Graphics Center
--
Jeff Layton <[email protected]>
On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
>> [Public]
>>
>> Hi Jeff,
>>
>> Thanks. I might need more information to understand why we can't retrieve
>> the drm atomic state. Also , "Failed to create MST payload for port" indicates
>> error while configuring DPCD payload ID table. Could you help to provide log
>> with KMS + ATOMIC + DP debug on please? Thanks in advance!
>>
>> Regards,
>> Wayne
>>
>
> Possibly. I'm not that familiar with display driver debugging. Can you
> send me some directions on how to crank up that sort of debug logging?
>
> Note that this problem is _very_ intermittent too: I went about 2 weeks
> between crashes, and then I got 3 in one day. I'd rather not run with a
> lot of debug logging for a long time if that's what this is going to
> require, as this is my main workstation.
>
> The last time I got this log message, my proposed patch did prevent the
> box from oopsing, so I'd really like to see it go in unless it's just
> categorically wrong for the caller to pass down a NULL state pointer to
> drm_dp_add_payload_part2.
Cc: Lyude.
Looks like the state parameter was added in commit 4d07b0bc4034
("drm/display/dp_mst: Move all payload info into the atomic state") and
its only use is to get at state->dev for debug logging.
What's the plan for the parameter? Surely something more than that! :)
Instead of "state ? state->dev : NULL" I guess we could use mgr->dev
like the other logging calls do. It's papering over the NULL parameter
too, but perhaps in a slightly cleaner way...
BR,
Jani.
>
>> > -----Original Message-----
>> > From: Alex Deucher <[email protected]>
>> > Sent: Thursday, April 13, 2023 8:59 PM
>> > To: Jani Nikula <[email protected]>; Lin, Wayne
>> > <[email protected]>
>> > Cc: Jeff Layton <[email protected]>; David Airlie <[email protected]>;
>> > Daniel Vetter <[email protected]>; Deucher, Alexander
>> > <[email protected]>; [email protected]; dri-
>> > [email protected]
>> > Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
>> > handle NULL state pointer
>> >
>> > + Wayne
>> >
>> > On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]>
>> > wrote:
>> > >
>> > > On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
>> > > > I've been experiencing some intermittent crashes down in the display
>> > > > driver code. The symptoms are ususally a line like this in dmesg:
>> > > >
>> > > > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port
>> > > > 000000006d3a3885: -5
>> > > >
>> > > > ...followed by an Oops due to a NULL pointer dereference.
>> > > >
>> > > > The real bug is probably in the caller of this function, which is
>> > > > passing it a NULL state pointer, but this patch at least keeps my
>> > > > machine from oopsing when this occurs.
>> > >
>> > > My fear is that papering over this makes the root cause harder to find.
>> > >
>> > > Cc: Harry, Alex
>> > >
>> > >
>> > > BR,
>> > > Jani.
>> > >
>> > >
>> > > >
>> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
>> > > > Signed-off-by: Jeff Layton <[email protected]>
>> > > > ---
>> > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
>> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > index 38dab76ae69e..87ad406c50f9 100644
>> > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct
>> > > > drm_dp_mst_topology_mgr *mgr,
>> > > >
>> > > > /* Skip failed payloads */
>> > > > if (payload->vc_start_slot == -1) {
>> > > > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s
>> > failed, skipping part 2\n",
>> > > > + drm_dbg_kms(state ? state->dev : NULL,
>> > > > + "Part 1 of payload creation for %s failed,
>> > > > + skipping part 2\n",
>> > > > payload->port->connector->name);
>> > > > return -EIO;
>> > > > }
>> > >
>> > > --
>> > > Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
> On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> > > [Public]
> > >
> > > Hi Jeff,
> > >
> > > Thanks. I might need more information to understand why we can't retrieve
> > > the drm atomic state. Also , "Failed to create MST payload for port" indicates
> > > error while configuring DPCD payload ID table. Could you help to provide log
> > > with KMS + ATOMIC + DP debug on please? Thanks in advance!
> > >
> > > Regards,
> > > Wayne
> > >
> >
> > Possibly. I'm not that familiar with display driver debugging. Can you
> > send me some directions on how to crank up that sort of debug logging?
> >
> > Note that this problem is _very_ intermittent too: I went about 2 weeks
> > between crashes, and then I got 3 in one day. I'd rather not run with a
> > lot of debug logging for a long time if that's what this is going to
> > require, as this is my main workstation.
> >
> > The last time I got this log message, my proposed patch did prevent the
> > box from oopsing, so I'd really like to see it go in unless it's just
> > categorically wrong for the caller to pass down a NULL state pointer to
> > drm_dp_add_payload_part2.
>
> Cc: Lyude.
>
> Looks like the state parameter was added in commit 4d07b0bc4034
> ("drm/display/dp_mst: Move all payload info into the atomic state") and
> its only use is to get at state->dev for debug logging.
>
> What's the plan for the parameter? Surely something more than that! :)
I don't think there was any plan for that, or at least I certainly don't even
remember adding that D:. It must totally have been by mistake and snuck by
review, if that's the only thing that we're using it for I'd say it's
definitely fine to just drop it entirely
>
> Instead of "state ? state->dev : NULL" I guess we could use mgr->dev
> like the other logging calls do. It's papering over the NULL parameter
> too, but perhaps in a slightly cleaner way...
>
>
> BR,
> Jani.
>
>
> >
> > > > -----Original Message-----
> > > > From: Alex Deucher <[email protected]>
> > > > Sent: Thursday, April 13, 2023 8:59 PM
> > > > To: Jani Nikula <[email protected]>; Lin, Wayne
> > > > <[email protected]>
> > > > Cc: Jeff Layton <[email protected]>; David Airlie <[email protected]>;
> > > > Daniel Vetter <[email protected]>; Deucher, Alexander
> > > > <[email protected]>; [email protected]; dri-
> > > > [email protected]
> > > > Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
> > > > handle NULL state pointer
> > > >
> > > > + Wayne
> > > >
> > > > On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]>
> > > > wrote:
> > > > >
> > > > > On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > > > > I've been experiencing some intermittent crashes down in the display
> > > > > > driver code. The symptoms are ususally a line like this in dmesg:
> > > > > >
> > > > > > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port
> > > > > > 000000006d3a3885: -5
> > > > > >
> > > > > > ...followed by an Oops due to a NULL pointer dereference.
> > > > > >
> > > > > > The real bug is probably in the caller of this function, which is
> > > > > > passing it a NULL state pointer, but this patch at least keeps my
> > > > > > machine from oopsing when this occurs.
> > > > >
> > > > > My fear is that papering over this makes the root cause harder to find.
> > > > >
> > > > > Cc: Harry, Alex
> > > > >
> > > > >
> > > > > BR,
> > > > > Jani.
> > > > >
> > > > >
> > > > > >
> > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
> > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > ---
> > > > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > index 38dab76ae69e..87ad406c50f9 100644
> > > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct
> > > > > > drm_dp_mst_topology_mgr *mgr,
> > > > > >
> > > > > > /* Skip failed payloads */
> > > > > > if (payload->vc_start_slot == -1) {
> > > > > > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s
> > > > failed, skipping part 2\n",
> > > > > > + drm_dbg_kms(state ? state->dev : NULL,
> > > > > > + "Part 1 of payload creation for %s failed,
> > > > > > + skipping part 2\n",
> > > > > > payload->port->connector->name);
> > > > > > return -EIO;
> > > > > > }
> > > > >
> > > > > --
> > > > > Jani Nikula, Intel Open Source Graphics Center
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
> On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
>> On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
>> > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
>> > > [Public]
>> > >
>> > > Hi Jeff,
>> > >
>> > > Thanks. I might need more information to understand why we can't retrieve
>> > > the drm atomic state. Also , "Failed to create MST payload for port" indicates
>> > > error while configuring DPCD payload ID table. Could you help to provide log
>> > > with KMS + ATOMIC + DP debug on please? Thanks in advance!
>> > >
>> > > Regards,
>> > > Wayne
>> > >
>> >
>> > Possibly. I'm not that familiar with display driver debugging. Can you
>> > send me some directions on how to crank up that sort of debug logging?
>> >
>> > Note that this problem is _very_ intermittent too: I went about 2 weeks
>> > between crashes, and then I got 3 in one day. I'd rather not run with a
>> > lot of debug logging for a long time if that's what this is going to
>> > require, as this is my main workstation.
>> >
>> > The last time I got this log message, my proposed patch did prevent the
>> > box from oopsing, so I'd really like to see it go in unless it's just
>> > categorically wrong for the caller to pass down a NULL state pointer to
>> > drm_dp_add_payload_part2.
>>
>> Cc: Lyude.
>>
>> Looks like the state parameter was added in commit 4d07b0bc4034
>> ("drm/display/dp_mst: Move all payload info into the atomic state") and
>> its only use is to get at state->dev for debug logging.
>>
>> What's the plan for the parameter? Surely something more than that! :)
>
> I don't think there was any plan for that, or at least I certainly don't even
> remember adding that D:. It must totally have been by mistake and snuck by
> review, if that's the only thing that we're using it for I'd say it's
> definitely fine to just drop it entirely
I guess we could use two patches then, first replace state->dev with
mgr->dev as something that can be backported as needed, and second drop
the state parameter altogether.
Jeff, up for it? At least the first one?
BR,
Jani.
>
>>
>> Instead of "state ? state->dev : NULL" I guess we could use mgr->dev
>> like the other logging calls do. It's papering over the NULL parameter
>> too, but perhaps in a slightly cleaner way...
>>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > > > -----Original Message-----
>> > > > From: Alex Deucher <[email protected]>
>> > > > Sent: Thursday, April 13, 2023 8:59 PM
>> > > > To: Jani Nikula <[email protected]>; Lin, Wayne
>> > > > <[email protected]>
>> > > > Cc: Jeff Layton <[email protected]>; David Airlie <[email protected]>;
>> > > > Daniel Vetter <[email protected]>; Deucher, Alexander
>> > > > <[email protected]>; [email protected]; dri-
>> > > > [email protected]
>> > > > Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
>> > > > handle NULL state pointer
>> > > >
>> > > > + Wayne
>> > > >
>> > > > On Thu, Apr 13, 2023 at 8:31 AM Jani Nikula <[email protected]>
>> > > > wrote:
>> > > > >
>> > > > > On Thu, 13 Apr 2023, Jeff Layton <[email protected]> wrote:
>> > > > > > I've been experiencing some intermittent crashes down in the display
>> > > > > > driver code. The symptoms are ususally a line like this in dmesg:
>> > > > > >
>> > > > > > amdgpu 0000:30:00.0: [drm] Failed to create MST payload for port
>> > > > > > 000000006d3a3885: -5
>> > > > > >
>> > > > > > ...followed by an Oops due to a NULL pointer dereference.
>> > > > > >
>> > > > > > The real bug is probably in the caller of this function, which is
>> > > > > > passing it a NULL state pointer, but this patch at least keeps my
>> > > > > > machine from oopsing when this occurs.
>> > > > >
>> > > > > My fear is that papering over this makes the root cause harder to find.
>> > > > >
>> > > > > Cc: Harry, Alex
>> > > > >
>> > > > >
>> > > > > BR,
>> > > > > Jani.
>> > > > >
>> > > > >
>> > > > > >
>> > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2184855
>> > > > > > Signed-off-by: Jeff Layton <[email protected]>
>> > > > > > ---
>> > > > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 ++-
>> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > > > index 38dab76ae69e..87ad406c50f9 100644
>> > > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > > > > > @@ -3404,7 +3404,8 @@ int drm_dp_add_payload_part2(struct
>> > > > > > drm_dp_mst_topology_mgr *mgr,
>> > > > > >
>> > > > > > /* Skip failed payloads */
>> > > > > > if (payload->vc_start_slot == -1) {
>> > > > > > - drm_dbg_kms(state->dev, "Part 1 of payload creation for %s
>> > > > failed, skipping part 2\n",
>> > > > > > + drm_dbg_kms(state ? state->dev : NULL,
>> > > > > > + "Part 1 of payload creation for %s failed,
>> > > > > > + skipping part 2\n",
>> > > > > > payload->port->connector->name);
>> > > > > > return -EIO;
>> > > > > > }
>> > > > >
>> > > > > --
>> > > > > Jani Nikula, Intel Open Source Graphics Center
>>
--
Jani Nikula, Intel Open Source Graphics Center
On Mon, 2023-04-17 at 11:44 +0300, Jani Nikula wrote:
> On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
> > On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
> > > On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> > > > > [Public]
> > > > >
> > > > > Hi Jeff,
> > > > >
> > > > > Thanks. I might need more information to understand why we can't retrieve
> > > > > the drm atomic state. Also , "Failed to create MST payload for port" indicates
> > > > > error while configuring DPCD payload ID table. Could you help to provide log
> > > > > with KMS + ATOMIC + DP debug on please? Thanks in advance!
> > > > >
> > > > > Regards,
> > > > > Wayne
> > > > >
> > > >
> > > > Possibly. I'm not that familiar with display driver debugging. Can you
> > > > send me some directions on how to crank up that sort of debug logging?
> > > >
> > > > Note that this problem is _very_ intermittent too: I went about 2 weeks
> > > > between crashes, and then I got 3 in one day. I'd rather not run with a
> > > > lot of debug logging for a long time if that's what this is going to
> > > > require, as this is my main workstation.
> > > >
> > > > The last time I got this log message, my proposed patch did prevent the
> > > > box from oopsing, so I'd really like to see it go in unless it's just
> > > > categorically wrong for the caller to pass down a NULL state pointer to
> > > > drm_dp_add_payload_part2.
> > >
> > > Cc: Lyude.
> > >
> > > Looks like the state parameter was added in commit 4d07b0bc4034
> > > ("drm/display/dp_mst: Move all payload info into the atomic state") and
> > > its only use is to get at state->dev for debug logging.
> > >
> > > What's the plan for the parameter? Surely something more than that! :)
> >
> > I don't think there was any plan for that, or at least I certainly don't even
> > remember adding that D:. It must totally have been by mistake and snuck by
> > review, if that's the only thing that we're using it for I'd say it's
> > definitely fine to just drop it entirely
>
> I guess we could use two patches then, first replace state->dev with
> mgr->dev as something that can be backported as needed, and second drop
> the state parameter altogether.
>
> Jeff, up for it? At least the first one?
>
>
> BR,
> Jani.
>
Sure. I'm happy to test patches if you send them along.
Thanks,
--
Jeff Layton <[email protected]>
On Mon, 17 Apr 2023, Jeff Layton <[email protected]> wrote:
> On Mon, 2023-04-17 at 11:44 +0300, Jani Nikula wrote:
>> On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
>> > On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
>> > > On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
>> > > > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
>> > > > > [Public]
>> > > > >
>> > > > > Hi Jeff,
>> > > > >
>> > > > > Thanks. I might need more information to understand why we can't retrieve
>> > > > > the drm atomic state. Also , "Failed to create MST payload for port" indicates
>> > > > > error while configuring DPCD payload ID table. Could you help to provide log
>> > > > > with KMS + ATOMIC + DP debug on please? Thanks in advance!
>> > > > >
>> > > > > Regards,
>> > > > > Wayne
>> > > > >
>> > > >
>> > > > Possibly. I'm not that familiar with display driver debugging. Can you
>> > > > send me some directions on how to crank up that sort of debug logging?
>> > > >
>> > > > Note that this problem is _very_ intermittent too: I went about 2 weeks
>> > > > between crashes, and then I got 3 in one day. I'd rather not run with a
>> > > > lot of debug logging for a long time if that's what this is going to
>> > > > require, as this is my main workstation.
>> > > >
>> > > > The last time I got this log message, my proposed patch did prevent the
>> > > > box from oopsing, so I'd really like to see it go in unless it's just
>> > > > categorically wrong for the caller to pass down a NULL state pointer to
>> > > > drm_dp_add_payload_part2.
>> > >
>> > > Cc: Lyude.
>> > >
>> > > Looks like the state parameter was added in commit 4d07b0bc4034
>> > > ("drm/display/dp_mst: Move all payload info into the atomic state") and
>> > > its only use is to get at state->dev for debug logging.
>> > >
>> > > What's the plan for the parameter? Surely something more than that! :)
>> >
>> > I don't think there was any plan for that, or at least I certainly don't even
>> > remember adding that D:. It must totally have been by mistake and snuck by
>> > review, if that's the only thing that we're using it for I'd say it's
>> > definitely fine to just drop it entirely
>>
>> I guess we could use two patches then, first replace state->dev with
>> mgr->dev as something that can be backported as needed, and second drop
>> the state parameter altogether.
>>
>> Jeff, up for it? At least the first one?
>>
>>
>> BR,
>> Jani.
>>
>
> Sure. I'm happy to test patches if you send them along.
I was hoping to lure you into sending patches. ;)
Anyway, I'm not working on this.
BR,
Jani.
>
> Thanks,
--
Jani Nikula, Intel Open Source Graphics Center
[AMD Official Use Only - General]
> -----Original Message-----
> From: Jani Nikula <[email protected]>
> Sent: Monday, April 17, 2023 6:30 PM
> To: Jeff Layton <[email protected]>; Lyude Paul <[email protected]>; Lin,
> Wayne <[email protected]>; Alex Deucher <[email protected]>
> Cc: David Airlie <[email protected]>; Daniel Vetter <[email protected]>;
> Deucher, Alexander <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
> handle NULL state pointer
>
> On Mon, 17 Apr 2023, Jeff Layton <[email protected]> wrote:
> > On Mon, 2023-04-17 at 11:44 +0300, Jani Nikula wrote:
> >> On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
> >> > On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
> >> > > On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> >> > > > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> >> > > > > [Public]
> >> > > > >
> >> > > > > Hi Jeff,
> >> > > > >
> >> > > > > Thanks. I might need more information to understand why we
> >> > > > > can't retrieve the drm atomic state. Also , "Failed to create
> >> > > > > MST payload for port" indicates error while configuring DPCD
> >> > > > > payload ID table. Could you help to provide log with KMS +
> ATOMIC + DP debug on please? Thanks in advance!
> >> > > > >
> >> > > > > Regards,
> >> > > > > Wayne
> >> > > > >
> >> > > >
> >> > > > Possibly. I'm not that familiar with display driver debugging.
> >> > > > Can you send me some directions on how to crank up that sort of
> debug logging?
> >> > > >
> >> > > > Note that this problem is _very_ intermittent too: I went about
> >> > > > 2 weeks between crashes, and then I got 3 in one day. I'd
> >> > > > rather not run with a lot of debug logging for a long time if
> >> > > > that's what this is going to require, as this is my main workstation.
> >> > > >
> >> > > > The last time I got this log message, my proposed patch did
> >> > > > prevent the box from oopsing, so I'd really like to see it go
> >> > > > in unless it's just categorically wrong for the caller to pass
> >> > > > down a NULL state pointer to drm_dp_add_payload_part2.
> >> > >
> >> > > Cc: Lyude.
> >> > >
> >> > > Looks like the state parameter was added in commit 4d07b0bc4034
> >> > > ("drm/display/dp_mst: Move all payload info into the atomic
> >> > > state") and its only use is to get at state->dev for debug logging.
> >> > >
> >> > > What's the plan for the parameter? Surely something more than
> >> > > that! :)
> >> >
> >> > I don't think there was any plan for that, or at least I certainly
> >> > don't even remember adding that D:. It must totally have been by
> >> > mistake and snuck by review, if that's the only thing that we're
> >> > using it for I'd say it's definitely fine to just drop it entirely
> >>
> >> I guess we could use two patches then, first replace state->dev with
> >> mgr->dev as something that can be backported as needed, and second
> >> mgr->drop
> >> the state parameter altogether.
> >>
> >> Jeff, up for it? At least the first one?
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >
> > Sure. I'm happy to test patches if you send them along.
>
> I was hoping to lure you into sending patches. ;)
>
> Anyway, I'm not working on this.
>
>
Hi Jeff,
I probably know the root cause.
But it doesn't need to use the state in the end, I will just rely on the patch
that Jani suggested to fix it. I can help to provide the patch later : )
> BR,
> Jani.
>
> >
> > Thanks,
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Regards,
Wayne
On Mon, 2023-04-17 at 13:29 +0300, Jani Nikula wrote:
> On Mon, 17 Apr 2023, Jeff Layton <[email protected]> wrote:
> > On Mon, 2023-04-17 at 11:44 +0300, Jani Nikula wrote:
> > > On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
> > > > On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
> > > > > On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > > > > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> > > > > > > [Public]
> > > > > > >
> > > > > > > Hi Jeff,
> > > > > > >
> > > > > > > Thanks. I might need more information to understand why we can't retrieve
> > > > > > > the drm atomic state. Also , "Failed to create MST payload for port" indicates
> > > > > > > error while configuring DPCD payload ID table. Could you help to provide log
> > > > > > > with KMS + ATOMIC + DP debug on please? Thanks in advance!
> > > > > > >
> > > > > > > Regards,
> > > > > > > Wayne
> > > > > > >
> > > > > >
> > > > > > Possibly. I'm not that familiar with display driver debugging. Can you
> > > > > > send me some directions on how to crank up that sort of debug logging?
> > > > > >
> > > > > > Note that this problem is _very_ intermittent too: I went about 2 weeks
> > > > > > between crashes, and then I got 3 in one day. I'd rather not run with a
> > > > > > lot of debug logging for a long time if that's what this is going to
> > > > > > require, as this is my main workstation.
> > > > > >
> > > > > > The last time I got this log message, my proposed patch did prevent the
> > > > > > box from oopsing, so I'd really like to see it go in unless it's just
> > > > > > categorically wrong for the caller to pass down a NULL state pointer to
> > > > > > drm_dp_add_payload_part2.
> > > > >
> > > > > Cc: Lyude.
> > > > >
> > > > > Looks like the state parameter was added in commit 4d07b0bc4034
> > > > > ("drm/display/dp_mst: Move all payload info into the atomic state") and
> > > > > its only use is to get at state->dev for debug logging.
> > > > >
> > > > > What's the plan for the parameter? Surely something more than that! :)
> > > >
> > > > I don't think there was any plan for that, or at least I certainly don't even
> > > > remember adding that D:. It must totally have been by mistake and snuck by
> > > > review, if that's the only thing that we're using it for I'd say it's
> > > > definitely fine to just drop it entirely
> > >
> > > I guess we could use two patches then, first replace state->dev with
> > > mgr->dev as something that can be backported as needed, and second drop
> > > the state parameter altogether.
> > >
> > > Jeff, up for it? At least the first one?
> > >
> > >
> > > BR,
> > > Jani.
> > >
> >
> > Sure. I'm happy to test patches if you send them along.
>
> I was hoping to lure you into sending patches. ;)
>
> Anyway, I'm not working on this.
>
>
Ok. I misunderstood, I'll change the patch I've been using to use mgr-
>dev instead and will send along a v2 here in a few days (after some
testing).
--
Jeff Layton <[email protected]>
On Mon, 2023-04-17 at 10:58 +0000, Lin, Wayne wrote:
> [AMD Official Use Only - General]
>
>
>
> > -----Original Message-----
> > From: Jani Nikula <[email protected]>
> > Sent: Monday, April 17, 2023 6:30 PM
> > To: Jeff Layton <[email protected]>; Lyude Paul <[email protected]>; Lin,
> > Wayne <[email protected]>; Alex Deucher <[email protected]>
> > Cc: David Airlie <[email protected]>; Daniel Vetter <[email protected]>;
> > Deucher, Alexander <[email protected]>; linux-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH] drm: make drm_dp_add_payload_part2 gracefully
> > handle NULL state pointer
> >
> > On Mon, 17 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > On Mon, 2023-04-17 at 11:44 +0300, Jani Nikula wrote:
> > > > On Fri, 14 Apr 2023, Lyude Paul <[email protected]> wrote:
> > > > > On Fri, 2023-04-14 at 13:35 +0300, Jani Nikula wrote:
> > > > > > On Fri, 14 Apr 2023, Jeff Layton <[email protected]> wrote:
> > > > > > > On Fri, 2023-04-14 at 04:40 +0000, Lin, Wayne wrote:
> > > > > > > > [Public]
> > > > > > > >
> > > > > > > > Hi Jeff,
> > > > > > > >
> > > > > > > > Thanks. I might need more information to understand why we
> > > > > > > > can't retrieve the drm atomic state. Also , "Failed to create
> > > > > > > > MST payload for port" indicates error while configuring DPCD
> > > > > > > > payload ID table. Could you help to provide log with KMS +
> > ATOMIC + DP debug on please? Thanks in advance!
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Wayne
> > > > > > > >
> > > > > > >
> > > > > > > Possibly. I'm not that familiar with display driver debugging.
> > > > > > > Can you send me some directions on how to crank up that sort of
> > debug logging?
> > > > > > >
> > > > > > > Note that this problem is _very_ intermittent too: I went about
> > > > > > > 2 weeks between crashes, and then I got 3 in one day. I'd
> > > > > > > rather not run with a lot of debug logging for a long time if
> > > > > > > that's what this is going to require, as this is my main workstation.
> > > > > > >
> > > > > > > The last time I got this log message, my proposed patch did
> > > > > > > prevent the box from oopsing, so I'd really like to see it go
> > > > > > > in unless it's just categorically wrong for the caller to pass
> > > > > > > down a NULL state pointer to drm_dp_add_payload_part2.
> > > > > >
> > > > > > Cc: Lyude.
> > > > > >
> > > > > > Looks like the state parameter was added in commit 4d07b0bc4034
> > > > > > ("drm/display/dp_mst: Move all payload info into the atomic
> > > > > > state") and its only use is to get at state->dev for debug logging.
> > > > > >
> > > > > > What's the plan for the parameter? Surely something more than
> > > > > > that! :)
> > > > >
> > > > > I don't think there was any plan for that, or at least I certainly
> > > > > don't even remember adding that D:. It must totally have been by
> > > > > mistake and snuck by review, if that's the only thing that we're
> > > > > using it for I'd say it's definitely fine to just drop it entirely
> > > >
> > > > I guess we could use two patches then, first replace state->dev with
> > > > mgr->dev as something that can be backported as needed, and second
> > > > mgr->drop
> > > > the state parameter altogether.
> > > >
> > > > Jeff, up for it? At least the first one?
> > > >
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > >
> > > Sure. I'm happy to test patches if you send them along.
> >
> > I was hoping to lure you into sending patches. ;)
> >
> > Anyway, I'm not working on this.
> >
> >
> Hi Jeff,
>
> I probably know the root cause.
> But it doesn't need to use the state in the end, I will just rely on the patch
> that Jani suggested to fix it. I can help to provide the patch later : )
>
>
Sounds good. If you want to send me a patch to solve the root cause,
I'll put it in the kernel with the other one I'm testing.
Thanks,
--
Jeff Layton <[email protected]>