| Message ID | 20200814170140.24917-1-Bhawanpreet.Lakha@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp_mst: Don't return error code when crtc is null | expand |
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Bhawanpreet Lakha >Sent: Friday, August 14, 2020 1:02 PM >To: mikita.lipski@amd.com; nicholas.kazlauskas@amd.com; >alexander.deucher@amd.com >Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>; dri- >devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null > >[Why] >In certain cases the crtc can be NULL and returning -EINVAL causes >atomic check to fail when it shouln't. This leads to valid >configurations failing because atomic check fails. So is this a bug fix or an exception case, or an expected possibility? From my reading of the function comments, it is not clear that pos->port->connector might be NULL for some reason. A better explanation of why this would occur would make this a much more useful commit message. My reading is that you ran into this issue an are masking it with this fix. Rather than this is a real possibility and this is the correct fix. Mike >[How] >Don't early return if crtc is null > >Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com> >--- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >b/drivers/gpu/drm/drm_dp_mst_topology.c >index 70c4b7afed12..bc90a1485699 100644 >--- a/drivers/gpu/drm/drm_dp_mst_topology.c >+++ b/drivers/gpu/drm/drm_dp_mst_topology.c >@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct >drm_atomic_state *state, struct drm > > crtc = conn_state->crtc; > >- if (WARN_ON(!crtc)) >- return -EINVAL; >+ if (!crtc) >+ continue; > > if (!drm_dp_mst_dsc_aux_for_port(pos->port)) > continue; >-- >2.17.1 > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
[AMD Official Use Only - Internal Distribution Only] pos->port->connector? This is checking the crtc not the connector. The crtc can be null if its disabled. Since it is happening after a unplug->hotplug, I guess we are missing something in the disable sequence and the old connector is still in the list. Bhawan >>-----Original Message----- >>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >>Bhawanpreet Lakha >>Sent: Friday, August 14, 2020 1:02 PM >>To: mikita.lipski@amd.com; nicholas.kazlauskas@amd.com; >>alexander.deucher@amd.com >>Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>; dri- >>devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org >>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null >> >>[Why] >>In certain cases the crtc can be NULL and returning -EINVAL causes >>atomic check to fail when it shouln't. This leads to valid >>configurations failing because atomic check fails. > >So is this a bug fix or an exception case, or an expected possibility? > >From my reading of the function comments, it is not clear that pos->port->connector >might be NULL for some reason. >A better explanation of why this would occur would make this a much more >useful commit message. > >My reading is that you ran into this issue an are masking it with this fix. > >Rather than this is a real possibility and this is the correct fix. > >Mike > >>[How] >>Don't early return if crtc is null >> >>Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com> >>--- >> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >>b/drivers/gpu/drm/drm_dp_mst_topology.c >>index 70c4b7afed12..bc90a1485699 100644 >>--- a/drivers/gpu/drm/drm_dp_mst_topology.c >>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c >>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct >>drm_atomic_state *state, struct drm >> >> crtc = conn_state->crtc; >> >>- if (WARN_ON(!crtc)) >>- return -EINVAL; >>+ if (!crtc) >>+ continue; >> >> if (!drm_dp_mst_dsc_aux_for_port(pos->port)) >> continue; >>-- >>2.17.1 >> >>_______________________________________________ >>dri-devel mailing list >>dri-devel@lists.freedesktop.org >>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7CBhawanpreet.Lakha%40amd.com%7C0f5d55c551644fef3df908d840787b3e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330233520819407&sdata=5N%2BBb0Qp3bd5zANfxovb%2BrVLAGnbP1sjyw3EeCHXj2w%3D&reserved=0
[AMD Official Use Only - Internal Distribution Only]
I took a closer look at this and there seems to be an issue in the function.
Crtc being null is a valid case here. The sequence that leads to this is, unplug -> disable crtc/release vcpi slots then hotplug. The issue is that pos->port is not guaranteed to be released in drm_dp_atomic_release_vcpi_slots() so list_for_each_entry(pos, &mst_state->vcpis, next) {} might still iterate thought it.
So, when a hotplug is done we still loop through the old port which has port! = null, crtc = null, and vpci = 0. I didn't find anything that I can use to remove the port from the list. So, a potential solution to this would be to add a check for vpci = 0 and skip that port.
Thoughts/Suggestions?
Bhawan
Reviewed-by: Lyude Paul <lyude@redhat.com> I will go ahead and push this to drm-misc-fixes, thanks! On Fri, 2020-08-14 at 13:01 -0400, Bhawanpreet Lakha wrote: > [Why] > In certain cases the crtc can be NULL and returning -EINVAL causes > atomic check to fail when it shouln't. This leads to valid > configurations failing because atomic check fails. > > [How] > Don't early return if crtc is null > > Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 70c4b7afed12..bc90a1485699 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct > drm_atomic_state *state, struct drm > > crtc = conn_state->crtc; > > - if (WARN_ON(!crtc)) > - return -EINVAL; > + if (!crtc) > + continue; > > if (!drm_dp_mst_dsc_aux_for_port(pos->port)) > continue;
Oh-just noticed this is also missing a CC for stable@vger.kernel.org. I'll add it before pushing but please make sure to follow the guidelines here when submitting fixes, since otherwise they might not get backported automatically to older kernels: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html (you can ignore the "It cannot be bigger than 100 lines, with context." part, as long as you're not trying to backport new functionality to stable and you're actually fixing something they're pretty leniant about that rule) On Mon, 2020-08-17 at 11:21 -0400, Lyude Paul wrote: > Reviewed-by: Lyude Paul <lyude@redhat.com> > > I will go ahead and push this to drm-misc-fixes, thanks! > > On Fri, 2020-08-14 at 13:01 -0400, Bhawanpreet Lakha wrote: > > [Why] > > In certain cases the crtc can be NULL and returning -EINVAL causes > > atomic check to fail when it shouln't. This leads to valid > > configurations failing because atomic check fails. > > > > [How] > > Don't early return if crtc is null > > > > Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com> > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > index 70c4b7afed12..bc90a1485699 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct > > drm_atomic_state *state, struct drm > > > > crtc = conn_state->crtc; > > > > - if (WARN_ON(!crtc)) > > - return -EINVAL; > > + if (!crtc) > > + continue; > > > > if (!drm_dp_mst_dsc_aux_for_port(pos->port)) > > continue;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 70c4b7afed12..bc90a1485699 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state, struct drm crtc = conn_state->crtc; - if (WARN_ON(!crtc)) - return -EINVAL; + if (!crtc) + continue; if (!drm_dp_mst_dsc_aux_for_port(pos->port)) continue;
[Why] In certain cases the crtc can be NULL and returning -EINVAL causes atomic check to fail when it shouln't. This leads to valid configurations failing because atomic check fails. [How] Don't early return if crtc is null Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)