Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
On Fri, Nov 18, 2022 at 2:53 PM Lyude Paul wrote: > > JFYI - I'm not sure of the correct commit ID to add for the Fixes: tag since > it's in your branch, so I'll omit that and let you add it into the patch Yeah, I'll add that. Many thanks! Alex > > On Fri, 2022-11-18 at 14:47 -0500, Lyude Paul wrote: > > of course, will do in just a moment > > > > On Fri, 2022-11-18 at 14:46 -0500, Alex Deucher wrote: > > > I've already picked this up. Can you send a follow up patch with just > > > the coverity fix? > > > > > > Alex > > > > > > On Fri, Nov 18, 2022 at 2:17 PM Lyude Paul wrote: > > > > > > > > JFYI, Coverity pointed out one more issue with this series so I'm going > > > > to > > > > send out a respin real quick to fix it. It's just a missing variable > > > > assignment (we leave ret unassigned by mistake in > > > > pre_compute_mst_dsc_configs()) so I will carry over your r-b on it. > > > > > > > > On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > > > > > [Public] > > > > > > > > > > All the patch set looks good to me. Feel free to add: > > > > > Reviewed-by: Wayne Lin > > > > > > > > > > Again, thank you Lyude for helping on this!!! > > > > > > > > > > Regards, > > > > > Wayne > > > > > > -Original Message- > > > > > > From: Lyude Paul > > > > > > Sent: Tuesday, November 15, 2022 6:18 AM > > > > > > To: amd-gfx@lists.freedesktop.org > > > > > > Cc: Wentland, Harry ; > > > > > > sta...@vger.kernel.org; > > > > > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > > > > > ; Deucher, Alexander > > > > > > ; Koenig, Christian > > > > > > ; Pan, Xinhui ; David > > > > > > Airlie ; Daniel Vetter ; > > > > > > Kazlauskas, > > > > > > Nicholas ; Pillai, Aurabindo > > > > > > ; Li, Roman ; Zuo, Jerry > > > > > > ; Wu, Hersen ; Lin, Wayne > > > > > > ; Thomas Zimmermann ; > > > > > > Mahfooz, Hamza ; Hung, Alex > > > > > > ; Mikita Lipski ; Liu, > > > > > > Wenjing ; Francis, David > > > > > > ; open list:DRM DRIVERS > > > > > de...@lists.freedesktop.org>; open list > > > > > > > > > > > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes > > > > > > and > > > > > > deadlocking > > > > > > > > > > > > It appears that amdgpu makes the mistake of completely ignoring the > > > > > > return > > > > > > values from the DP MST helpers, and instead just returns a simple > > > > > > true/false. > > > > > > In this case, it seems to have come back to bite us because as a > > > > > > result of > > > > > > simply returning false from compute_mst_dsc_configs_for_state(), > > > > > > amdgpu > > > > > > had no way of telling when a deadlock happened from these helpers. > > > > > > This > > > > > > could definitely result in some kernel splats. > > > > > > > > > > > > V2: > > > > > > * Address Wayne's comments (fix another bunch of spots where we > > > > > > weren't > > > > > > passing down return codes) > > > > > > > > > > > > Signed-off-by: Lyude Paul > > > > > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > > > > > Cc: Harry Wentland > > > > > > Cc: # v5.6+ > > > > > > --- > > > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 > > > > > > ++-- > > > > > > -- > > > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > > > > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > > @@ -6462,7 +6462,7 @@ static int > > > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > > > struct drm_connector_state *new_con_state; > > > > > > struct amdgpu_dm_connector *aconnector; > > > > > > struct dm_connector_state *dm_conn_state; > > > > > > - int i, j; > > > > > > + int i, j, ret; > > > > > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > > > > > > > > > for_each_new_connector_in_state(state, connector, > > > > > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > > > dm_conn_state->pbn = pbn; > > > > > > dm_conn_state->vcpi_slots = slot_num; > > > > > > > > > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > > > > > port, dm_conn_state->pbn, > > > > > > -false); > > > > > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > > > > > aconnector->port, > > > > > > + > > > > > > dm_conn_state- > > > > > > > pbn, false); > > > > > > + if (ret < 0) > > > > > > +
Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
JFYI - I'm not sure of the correct commit ID to add for the Fixes: tag since it's in your branch, so I'll omit that and let you add it into the patch On Fri, 2022-11-18 at 14:47 -0500, Lyude Paul wrote: > of course, will do in just a moment > > On Fri, 2022-11-18 at 14:46 -0500, Alex Deucher wrote: > > I've already picked this up. Can you send a follow up patch with just > > the coverity fix? > > > > Alex > > > > On Fri, Nov 18, 2022 at 2:17 PM Lyude Paul wrote: > > > > > > JFYI, Coverity pointed out one more issue with this series so I'm going to > > > send out a respin real quick to fix it. It's just a missing variable > > > assignment (we leave ret unassigned by mistake in > > > pre_compute_mst_dsc_configs()) so I will carry over your r-b on it. > > > > > > On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > > > > [Public] > > > > > > > > All the patch set looks good to me. Feel free to add: > > > > Reviewed-by: Wayne Lin > > > > > > > > Again, thank you Lyude for helping on this!!! > > > > > > > > Regards, > > > > Wayne > > > > > -Original Message- > > > > > From: Lyude Paul > > > > > Sent: Tuesday, November 15, 2022 6:18 AM > > > > > To: amd-gfx@lists.freedesktop.org > > > > > Cc: Wentland, Harry ; sta...@vger.kernel.org; > > > > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > > > > ; Deucher, Alexander > > > > > ; Koenig, Christian > > > > > ; Pan, Xinhui ; David > > > > > Airlie ; Daniel Vetter ; > > > > > Kazlauskas, > > > > > Nicholas ; Pillai, Aurabindo > > > > > ; Li, Roman ; Zuo, Jerry > > > > > ; Wu, Hersen ; Lin, Wayne > > > > > ; Thomas Zimmermann ; > > > > > Mahfooz, Hamza ; Hung, Alex > > > > > ; Mikita Lipski ; Liu, > > > > > Wenjing ; Francis, David > > > > > ; open list:DRM DRIVERS > > > > de...@lists.freedesktop.org>; open list > > > > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > > > > > deadlocking > > > > > > > > > > It appears that amdgpu makes the mistake of completely ignoring the > > > > > return > > > > > values from the DP MST helpers, and instead just returns a simple > > > > > true/false. > > > > > In this case, it seems to have come back to bite us because as a > > > > > result of > > > > > simply returning false from compute_mst_dsc_configs_for_state(), > > > > > amdgpu > > > > > had no way of telling when a deadlock happened from these helpers. > > > > > This > > > > > could definitely result in some kernel splats. > > > > > > > > > > V2: > > > > > * Address Wayne's comments (fix another bunch of spots where we > > > > > weren't > > > > > passing down return codes) > > > > > > > > > > Signed-off-by: Lyude Paul > > > > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > > > > Cc: Harry Wentland > > > > > Cc: # v5.6+ > > > > > --- > > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > > > > > -- > > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > > > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > @@ -6462,7 +6462,7 @@ static int > > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > > struct drm_connector_state *new_con_state; > > > > > struct amdgpu_dm_connector *aconnector; > > > > > struct dm_connector_state *dm_conn_state; > > > > > - int i, j; > > > > > + int i, j, ret; > > > > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > > > > > > > for_each_new_connector_in_state(state, connector, > > > > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > > dm_conn_state->pbn = pbn; > > > > > dm_conn_state->vcpi_slots = slot_num; > > > > > > > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > > > > port, dm_conn_state->pbn, > > > > > -false); > > > > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > > > > aconnector->port, > > > > > + dm_conn_state- > > > > > > pbn, false); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > continue; > > > > > } > > > > > > > > > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > > > > > drm_device *dev, > > > > > > > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > > > > if (dc_resource_is_dsc_encoding_supported(dc)) { > > > > > - if (!pre_validate_dsc(state, &
Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
of course, will do in just a moment On Fri, 2022-11-18 at 14:46 -0500, Alex Deucher wrote: > I've already picked this up. Can you send a follow up patch with just > the coverity fix? > > Alex > > On Fri, Nov 18, 2022 at 2:17 PM Lyude Paul wrote: > > > > JFYI, Coverity pointed out one more issue with this series so I'm going to > > send out a respin real quick to fix it. It's just a missing variable > > assignment (we leave ret unassigned by mistake in > > pre_compute_mst_dsc_configs()) so I will carry over your r-b on it. > > > > On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > > > [Public] > > > > > > All the patch set looks good to me. Feel free to add: > > > Reviewed-by: Wayne Lin > > > > > > Again, thank you Lyude for helping on this!!! > > > > > > Regards, > > > Wayne > > > > -Original Message- > > > > From: Lyude Paul > > > > Sent: Tuesday, November 15, 2022 6:18 AM > > > > To: amd-gfx@lists.freedesktop.org > > > > Cc: Wentland, Harry ; sta...@vger.kernel.org; > > > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > > > ; Deucher, Alexander > > > > ; Koenig, Christian > > > > ; Pan, Xinhui ; David > > > > Airlie ; Daniel Vetter ; Kazlauskas, > > > > Nicholas ; Pillai, Aurabindo > > > > ; Li, Roman ; Zuo, Jerry > > > > ; Wu, Hersen ; Lin, Wayne > > > > ; Thomas Zimmermann ; > > > > Mahfooz, Hamza ; Hung, Alex > > > > ; Mikita Lipski ; Liu, > > > > Wenjing ; Francis, David > > > > ; open list:DRM DRIVERS > > > de...@lists.freedesktop.org>; open list > > > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > > > > deadlocking > > > > > > > > It appears that amdgpu makes the mistake of completely ignoring the > > > > return > > > > values from the DP MST helpers, and instead just returns a simple > > > > true/false. > > > > In this case, it seems to have come back to bite us because as a result > > > > of > > > > simply returning false from compute_mst_dsc_configs_for_state(), amdgpu > > > > had no way of telling when a deadlock happened from these helpers. This > > > > could definitely result in some kernel splats. > > > > > > > > V2: > > > > * Address Wayne's comments (fix another bunch of spots where we weren't > > > > passing down return codes) > > > > > > > > Signed-off-by: Lyude Paul > > > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > > > Cc: Harry Wentland > > > > Cc: # v5.6+ > > > > --- > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > > > > -- > > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > @@ -6462,7 +6462,7 @@ static int > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > struct drm_connector_state *new_con_state; > > > > struct amdgpu_dm_connector *aconnector; > > > > struct dm_connector_state *dm_conn_state; > > > > - int i, j; > > > > + int i, j, ret; > > > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > > > > > for_each_new_connector_in_state(state, connector, > > > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > > dm_conn_state->pbn = pbn; > > > > dm_conn_state->vcpi_slots = slot_num; > > > > > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > > > port, dm_conn_state->pbn, > > > > -false); > > > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > > > aconnector->port, > > > > + dm_conn_state- > > > > > pbn, false); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > continue; > > > > } > > > > > > > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > > > > drm_device *dev, > > > > > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > > > if (dc_resource_is_dsc_encoding_supported(dc)) { > > > > - if (!pre_validate_dsc(state, &dm_state, vars)) { > > > > - ret = -EINVAL; > > > > + ret = pre_validate_dsc(state, &dm_state, vars); > > > > + if (ret != 0) > > > > goto fail; > > > > - } > > > > } > > > > #endif > > > > > > > > @@ -9621,9 +9623,9 @@ static int amdgpu_dm_atomic_check(struct > > > > drm_device *dev, > > > > } > > > > > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > > > - if (!compute_ms
Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
I've already picked this up. Can you send a follow up patch with just the coverity fix? Alex On Fri, Nov 18, 2022 at 2:17 PM Lyude Paul wrote: > > JFYI, Coverity pointed out one more issue with this series so I'm going to > send out a respin real quick to fix it. It's just a missing variable > assignment (we leave ret unassigned by mistake in > pre_compute_mst_dsc_configs()) so I will carry over your r-b on it. > > On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > > [Public] > > > > All the patch set looks good to me. Feel free to add: > > Reviewed-by: Wayne Lin > > > > Again, thank you Lyude for helping on this!!! > > > > Regards, > > Wayne > > > -Original Message- > > > From: Lyude Paul > > > Sent: Tuesday, November 15, 2022 6:18 AM > > > To: amd-gfx@lists.freedesktop.org > > > Cc: Wentland, Harry ; sta...@vger.kernel.org; > > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > > ; Deucher, Alexander > > > ; Koenig, Christian > > > ; Pan, Xinhui ; David > > > Airlie ; Daniel Vetter ; Kazlauskas, > > > Nicholas ; Pillai, Aurabindo > > > ; Li, Roman ; Zuo, Jerry > > > ; Wu, Hersen ; Lin, Wayne > > > ; Thomas Zimmermann ; > > > Mahfooz, Hamza ; Hung, Alex > > > ; Mikita Lipski ; Liu, > > > Wenjing ; Francis, David > > > ; open list:DRM DRIVERS > > de...@lists.freedesktop.org>; open list > > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > > > deadlocking > > > > > > It appears that amdgpu makes the mistake of completely ignoring the return > > > values from the DP MST helpers, and instead just returns a simple > > > true/false. > > > In this case, it seems to have come back to bite us because as a result of > > > simply returning false from compute_mst_dsc_configs_for_state(), amdgpu > > > had no way of telling when a deadlock happened from these helpers. This > > > could definitely result in some kernel splats. > > > > > > V2: > > > * Address Wayne's comments (fix another bunch of spots where we weren't > > > passing down return codes) > > > > > > Signed-off-by: Lyude Paul > > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > > Cc: Harry Wentland > > > Cc: # v5.6+ > > > --- > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > > > -- > > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > @@ -6462,7 +6462,7 @@ static int > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > struct drm_connector_state *new_con_state; > > > struct amdgpu_dm_connector *aconnector; > > > struct dm_connector_state *dm_conn_state; > > > - int i, j; > > > + int i, j, ret; > > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > > > for_each_new_connector_in_state(state, connector, > > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > > dm_conn_state->pbn = pbn; > > > dm_conn_state->vcpi_slots = slot_num; > > > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > > port, dm_conn_state->pbn, > > > -false); > > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > > aconnector->port, > > > + dm_conn_state- > > > > pbn, false); > > > + if (ret < 0) > > > + return ret; > > > + > > > continue; > > > } > > > > > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > > > drm_device *dev, > > > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > > if (dc_resource_is_dsc_encoding_supported(dc)) { > > > - if (!pre_validate_dsc(state, &dm_state, vars)) { > > > - ret = -EINVAL; > > > + ret = pre_validate_dsc(state, &dm_state, vars); > > > + if (ret != 0) > > > goto fail; > > > - } > > > } > > > #endif > > > > > > @@ -9621,9 +9623,9 @@ static int amdgpu_dm_atomic_check(struct > > > drm_device *dev, > > > } > > > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > > - if (!compute_mst_dsc_configs_for_state(state, dm_state- > > > > context, vars)) { > > > + ret = compute_mst_dsc_configs_for_state(state, dm_state- > > > > context, vars); > > > + if (ret) { > > > > > > DRM_DEBUG_DRIVER("compute_mst_dsc_configs_for_state() > > > failed\n"); > > > - ret = -EINVAL; > > > goto fail; > >
Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
JFYI, Coverity pointed out one more issue with this series so I'm going to send out a respin real quick to fix it. It's just a missing variable assignment (we leave ret unassigned by mistake in pre_compute_mst_dsc_configs()) so I will carry over your r-b on it. On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > [Public] > > All the patch set looks good to me. Feel free to add: > Reviewed-by: Wayne Lin > > Again, thank you Lyude for helping on this!!! > > Regards, > Wayne > > -Original Message- > > From: Lyude Paul > > Sent: Tuesday, November 15, 2022 6:18 AM > > To: amd-gfx@lists.freedesktop.org > > Cc: Wentland, Harry ; sta...@vger.kernel.org; > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > ; Deucher, Alexander > > ; Koenig, Christian > > ; Pan, Xinhui ; David > > Airlie ; Daniel Vetter ; Kazlauskas, > > Nicholas ; Pillai, Aurabindo > > ; Li, Roman ; Zuo, Jerry > > ; Wu, Hersen ; Lin, Wayne > > ; Thomas Zimmermann ; > > Mahfooz, Hamza ; Hung, Alex > > ; Mikita Lipski ; Liu, > > Wenjing ; Francis, David > > ; open list:DRM DRIVERS > de...@lists.freedesktop.org>; open list > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > > deadlocking > > > > It appears that amdgpu makes the mistake of completely ignoring the return > > values from the DP MST helpers, and instead just returns a simple > > true/false. > > In this case, it seems to have come back to bite us because as a result of > > simply returning false from compute_mst_dsc_configs_for_state(), amdgpu > > had no way of telling when a deadlock happened from these helpers. This > > could definitely result in some kernel splats. > > > > V2: > > * Address Wayne's comments (fix another bunch of spots where we weren't > > passing down return codes) > > > > Signed-off-by: Lyude Paul > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > Cc: Harry Wentland > > Cc: # v5.6+ > > --- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > > -- > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -6462,7 +6462,7 @@ static int > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > struct drm_connector_state *new_con_state; > > struct amdgpu_dm_connector *aconnector; > > struct dm_connector_state *dm_conn_state; > > - int i, j; > > + int i, j, ret; > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > for_each_new_connector_in_state(state, connector, > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > dm_conn_state->pbn = pbn; > > dm_conn_state->vcpi_slots = slot_num; > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > port, dm_conn_state->pbn, > > -false); > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > aconnector->port, > > + dm_conn_state- > > > pbn, false); > > + if (ret < 0) > > + return ret; > > + > > continue; > > } > > > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > > drm_device *dev, > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > if (dc_resource_is_dsc_encoding_supported(dc)) { > > - if (!pre_validate_dsc(state, &dm_state, vars)) { > > - ret = -EINVAL; > > + ret = pre_validate_dsc(state, &dm_state, vars); > > + if (ret != 0) > > goto fail; > > - } > > } > > #endif > > > > @@ -9621,9 +9623,9 @@ static int amdgpu_dm_atomic_check(struct > > drm_device *dev, > > } > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > - if (!compute_mst_dsc_configs_for_state(state, dm_state- > > > context, vars)) { > > + ret = compute_mst_dsc_configs_for_state(state, dm_state- > > > context, vars); > > + if (ret) { > > > > DRM_DEBUG_DRIVER("compute_mst_dsc_configs_for_state() > > failed\n"); > > - ret = -EINVAL; > > goto fail; > > } > > > > diff --git > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > index 6ff96b4bdda5c..bba2e8aaa2c20 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > +++ > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > @@ -703,13 +703,13
Re: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
On Wed, 2022-11-16 at 04:39 +, Lin, Wayne wrote: > [Public] > > All the patch set looks good to me. Feel free to add: > Reviewed-by: Wayne Lin > > Again, thank you Lyude for helping on this!!! No problem! I was the one who introduced the bug anyway :P, I'm just glad we were able to fix this on time. Harry, Alex - feel free to merge this on whatever branch you want (I'm fine with the mst-helper bits going through amd's branch, especially since AMD is the only driver using the dsc stuff right now) > > Regards, > Wayne > > -Original Message- > > From: Lyude Paul > > Sent: Tuesday, November 15, 2022 6:18 AM > > To: amd-gfx@lists.freedesktop.org > > Cc: Wentland, Harry ; sta...@vger.kernel.org; > > Li, Sun peng (Leo) ; Siqueira, Rodrigo > > ; Deucher, Alexander > > ; Koenig, Christian > > ; Pan, Xinhui ; David > > Airlie ; Daniel Vetter ; Kazlauskas, > > Nicholas ; Pillai, Aurabindo > > ; Li, Roman ; Zuo, Jerry > > ; Wu, Hersen ; Lin, Wayne > > ; Thomas Zimmermann ; > > Mahfooz, Hamza ; Hung, Alex > > ; Mikita Lipski ; Liu, > > Wenjing ; Francis, David > > ; open list:DRM DRIVERS > de...@lists.freedesktop.org>; open list > > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > > deadlocking > > > > It appears that amdgpu makes the mistake of completely ignoring the return > > values from the DP MST helpers, and instead just returns a simple > > true/false. > > In this case, it seems to have come back to bite us because as a result of > > simply returning false from compute_mst_dsc_configs_for_state(), amdgpu > > had no way of telling when a deadlock happened from these helpers. This > > could definitely result in some kernel splats. > > > > V2: > > * Address Wayne's comments (fix another bunch of spots where we weren't > > passing down return codes) > > > > Signed-off-by: Lyude Paul > > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > > Cc: Harry Wentland > > Cc: # v5.6+ > > --- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > > -- > > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > > 3 files changed, 147 insertions(+), 118 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > index 0db2a88cd4d7b..852a2100c6b38 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -6462,7 +6462,7 @@ static int > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > struct drm_connector_state *new_con_state; > > struct amdgpu_dm_connector *aconnector; > > struct dm_connector_state *dm_conn_state; > > - int i, j; > > + int i, j, ret; > > int vcpi, pbn_div, pbn, slot_num = 0; > > > > for_each_new_connector_in_state(state, connector, > > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > > dm_conn_state->pbn = pbn; > > dm_conn_state->vcpi_slots = slot_num; > > > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > > > port, dm_conn_state->pbn, > > -false); > > + ret = drm_dp_mst_atomic_enable_dsc(state, > > aconnector->port, > > + dm_conn_state- > > > pbn, false); > > + if (ret < 0) > > + return ret; > > + > > continue; > > } > > > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > > drm_device *dev, > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > if (dc_resource_is_dsc_encoding_supported(dc)) { > > - if (!pre_validate_dsc(state, &dm_state, vars)) { > > - ret = -EINVAL; > > + ret = pre_validate_dsc(state, &dm_state, vars); > > + if (ret != 0) > > goto fail; > > - } > > } > > #endif > > > > @@ -9621,9 +9623,9 @@ static int amdgpu_dm_atomic_check(struct > > drm_device *dev, > > } > > > > #if defined(CONFIG_DRM_AMD_DC_DCN) > > - if (!compute_mst_dsc_configs_for_state(state, dm_state- > > > context, vars)) { > > + ret = compute_mst_dsc_configs_for_state(state, dm_state- > > > context, vars); > > + if (ret) { > > > > DRM_DEBUG_DRIVER("compute_mst_dsc_configs_for_state() > > failed\n"); > > - ret = -EINVAL; > > goto fail; > > } > > > > diff --git > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > index 6ff96b4bdda5c..bba2e8aaa2c20 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > +++ > > b/drivers/gpu/drm/amd/display/amdgpu
RE: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and deadlocking
[Public] All the patch set looks good to me. Feel free to add: Reviewed-by: Wayne Lin Again, thank you Lyude for helping on this!!! Regards, Wayne > -Original Message- > From: Lyude Paul > Sent: Tuesday, November 15, 2022 6:18 AM > To: amd-gfx@lists.freedesktop.org > Cc: Wentland, Harry ; sta...@vger.kernel.org; > Li, Sun peng (Leo) ; Siqueira, Rodrigo > ; Deucher, Alexander > ; Koenig, Christian > ; Pan, Xinhui ; David > Airlie ; Daniel Vetter ; Kazlauskas, > Nicholas ; Pillai, Aurabindo > ; Li, Roman ; Zuo, Jerry > ; Wu, Hersen ; Lin, Wayne > ; Thomas Zimmermann ; > Mahfooz, Hamza ; Hung, Alex > ; Mikita Lipski ; Liu, > Wenjing ; Francis, David > ; open list:DRM DRIVERS de...@lists.freedesktop.org>; open list > Subject: [PATCH v2 1/4] drm/amdgpu/mst: Stop ignoring error codes and > deadlocking > > It appears that amdgpu makes the mistake of completely ignoring the return > values from the DP MST helpers, and instead just returns a simple true/false. > In this case, it seems to have come back to bite us because as a result of > simply returning false from compute_mst_dsc_configs_for_state(), amdgpu > had no way of telling when a deadlock happened from these helpers. This > could definitely result in some kernel splats. > > V2: > * Address Wayne's comments (fix another bunch of spots where we weren't > passing down return codes) > > Signed-off-by: Lyude Paul > Fixes: 8c20a1ed9b4f ("drm/amd/display: MST DSC compute fair share") > Cc: Harry Wentland > Cc: # v5.6+ > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +- > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 235 ++-- > -- > .../display/amdgpu_dm/amdgpu_dm_mst_types.h | 12 +- > 3 files changed, 147 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 0db2a88cd4d7b..852a2100c6b38 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -6462,7 +6462,7 @@ static int > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > struct drm_connector_state *new_con_state; > struct amdgpu_dm_connector *aconnector; > struct dm_connector_state *dm_conn_state; > - int i, j; > + int i, j, ret; > int vcpi, pbn_div, pbn, slot_num = 0; > > for_each_new_connector_in_state(state, connector, > new_con_state, i) { @@ -6509,8 +6509,11 @@ static int > dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > dm_conn_state->pbn = pbn; > dm_conn_state->vcpi_slots = slot_num; > > - drm_dp_mst_atomic_enable_dsc(state, aconnector- > >port, dm_conn_state->pbn, > - false); > + ret = drm_dp_mst_atomic_enable_dsc(state, > aconnector->port, > +dm_conn_state- > >pbn, false); > + if (ret < 0) > + return ret; > + > continue; > } > > @@ -9523,10 +9526,9 @@ static int amdgpu_dm_atomic_check(struct > drm_device *dev, > > #if defined(CONFIG_DRM_AMD_DC_DCN) > if (dc_resource_is_dsc_encoding_supported(dc)) { > - if (!pre_validate_dsc(state, &dm_state, vars)) { > - ret = -EINVAL; > + ret = pre_validate_dsc(state, &dm_state, vars); > + if (ret != 0) > goto fail; > - } > } > #endif > > @@ -9621,9 +9623,9 @@ static int amdgpu_dm_atomic_check(struct > drm_device *dev, > } > > #if defined(CONFIG_DRM_AMD_DC_DCN) > - if (!compute_mst_dsc_configs_for_state(state, dm_state- > >context, vars)) { > + ret = compute_mst_dsc_configs_for_state(state, dm_state- > >context, vars); > + if (ret) { > > DRM_DEBUG_DRIVER("compute_mst_dsc_configs_for_state() > failed\n"); > - ret = -EINVAL; > goto fail; > } > > diff --git > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > index 6ff96b4bdda5c..bba2e8aaa2c20 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > +++ > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > @@ -703,13 +703,13 @@ static int bpp_x16_from_pbn(struct > dsc_mst_fairness_params param, int pbn) > return dsc_config.bits_per_pixel; > } > > -static bool increase_dsc_bpp(struct drm_atomic_state *state, > - struct drm_dp_mst_topology_state *mst_state, > - struct dc_link *dc_link, > - struct dsc_mst_fairness_params *params, > - struct dsc_mst_fairness_vars *vars, > - int count, > -