Message ID | 1568770783-169735-1-git-send-email-srinivasan.s@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915/dp: Fix DP MST error after unplugging TypeC cable | expand |
On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > From: Srinivasan S <srinivasan.s@intel.com> > > This patch avoids DP MST payload error message in dmesg, as it is trying > to read the payload from the disconnected DP MST device. After the unplug > the connector status is disconnected and we should not be looking for the > payload and hence remove the error and throw the warning. > > This details can be found in: > https://bugs.freedesktop.org/show_bug.cgi?id=111632 Please add this link as Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign off statement > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index eeeb3f933aa4..5b2278fdf675 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > if (ret) { > - DRM_ERROR("failed to update payload %d\n", ret); > + if (!connector || > + connector->base.status != connector_status_connected) { > + DRM_WARN("DP MST disconnect\n"); May be adding this check before calling drm_dp_update_payload_part1() is a better idea? If the connector is disconnected, why update payload? Jani, Ville, thoughts? Regards Manasi > + } else { > + DRM_ERROR("failed to update payload %d\n", ret); > + } > } > if (old_crtc_state->has_audio) > intel_audio_codec_disable(encoder, > -- > 2.7.4 >
On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > From: Srinivasan S <srinivasan.s@intel.com> > > > > This patch avoids DP MST payload error message in dmesg, as it is trying > > to read the payload from the disconnected DP MST device. After the unplug > > the connector status is disconnected and we should not be looking for the > > payload and hence remove the error and throw the warning. > > > > This details can be found in: > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > Please add this link as Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign off > statement > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index eeeb3f933aa4..5b2278fdf675 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > if (ret) { > > - DRM_ERROR("failed to update payload %d\n", ret); > > + if (!connector || > > + connector->base.status != connector_status_connected) { > > + DRM_WARN("DP MST disconnect\n"); > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > If the connector is disconnected, why update payload? > > Jani, Ville, thoughts? Or just convert it to a debug? > > Regards > Manasi > > > + } else { > > + DRM_ERROR("failed to update payload %d\n", ret); > > + } > > } > > if (old_crtc_state->has_audio) > > intel_audio_codec_disable(encoder, > > -- > > 2.7.4 > >
On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > This patch avoids DP MST payload error message in dmesg, as it is trying > > > to read the payload from the disconnected DP MST device. After the unplug > > > the connector status is disconnected and we should not be looking for the > > > payload and hence remove the error and throw the warning. > > > > > > This details can be found in: > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > Please add this link as Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign off > > statement > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > if (ret) { > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > + if (!connector || > > > + connector->base.status != connector_status_connected) { > > > + DRM_WARN("DP MST disconnect\n"); > > > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > > If the connector is disconnected, why update payload? > > > > Jani, Ville, thoughts? > > Or just convert it to a debug? Sure that will work, but do we really want to update the payload if the connector status is disconnected. So shouldnt checking that before calling the function be a better fix? Manasi > > > > > Regards > > Manasi > > > > > + } else { > > > + DRM_ERROR("failed to update payload %d\n", ret); > > > + } > > > } > > > if (old_crtc_state->has_audio) > > > intel_audio_codec_disable(encoder, > > > -- > > > 2.7.4 > > > > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Would the following be appropriate fix? if (connector || connector->base.status == connector_status_connected) { ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); if (ret) { DRM_ERROR("failed to update payload %d\n", ret); } } Regards, -----Original Message----- From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Manasi Navare Sent: Wednesday, September 18, 2019 11:55 PM To: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: S, Srinivasan <srinivasan.s@intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > This patch avoids DP MST payload error message in dmesg, as it is > > > trying to read the payload from the disconnected DP MST device. > > > After the unplug the connector status is disconnected and we > > > should not be looking for the payload and hence remove the error and throw the warning. > > > > > > This details can be found in: > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > Please add this link as Bugzilla: > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign > > off statement > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct > > > intel_encoder *encoder, > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > if (ret) { > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > + if (!connector || > > > + connector->base.status != connector_status_connected) { > > > + DRM_WARN("DP MST disconnect\n"); > > > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > > If the connector is disconnected, why update payload? > > > > Jani, Ville, thoughts? > > Or just convert it to a debug? Sure that will work, but do we really want to update the payload if the connector status is disconnected. So shouldnt checking that before calling the function be a better fix? Manasi > > > > > Regards > > Manasi > > > > > + } else { > > > + DRM_ERROR("failed to update payload %d\n", ret); > > > + } > > > } > > > if (old_crtc_state->has_audio) > > > intel_audio_codec_disable(encoder, > > > -- > > > 2.7.4 > > > > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 19, 2019 at 07:23:30AM +0000, S, Srinivasan wrote: > Would the following be appropriate fix? > > if (connector || connector->base.status == connector_status_connected) { > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > if (ret) { > DRM_ERROR("failed to update payload %d\n", ret); > } > } The whole connector->status check is racy. IMO don't do it. > > Regards, > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Manasi Navare > Sent: Wednesday, September 18, 2019 11:55 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: S, Srinivasan <srinivasan.s@intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable > > On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > > > This patch avoids DP MST payload error message in dmesg, as it is > > > > trying to read the payload from the disconnected DP MST device. > > > > After the unplug the connector status is disconnected and we > > > > should not be looking for the payload and hence remove the error and throw the warning. > > > > > > > > This details can be found in: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > > > Please add this link as Bugzilla: > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign > > > off statement > > > > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct > > > > intel_encoder *encoder, > > > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > if (ret) { > > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > > + if (!connector || > > > > + connector->base.status != connector_status_connected) { > > > > + DRM_WARN("DP MST disconnect\n"); > > > > > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > > > If the connector is disconnected, why update payload? > > > > > > Jani, Ville, thoughts? > > > > Or just convert it to a debug? > > Sure that will work, but do we really want to update the payload if the connector status is disconnected. > So shouldnt checking that before calling the function be a better fix? > > Manasi > > > > > > > > > Regards > > > Manasi > > > > > > > + } else { > > > > + DRM_ERROR("failed to update payload %d\n", ret); > > > > + } > > > > } > > > > if (old_crtc_state->has_audio) > > > > intel_audio_codec_disable(encoder, > > > > -- > > > > 2.7.4 > > > > > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 18 Sep 2019, Manasi Navare <manasi.d.navare@intel.com> wrote: > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: >> From: Srinivasan S <srinivasan.s@intel.com> >> >> This patch avoids DP MST payload error message in dmesg, as it is trying >> to read the payload from the disconnected DP MST device. After the unplug >> the connector status is disconnected and we should not be looking for the >> payload and hence remove the error and throw the warning. >> >> This details can be found in: >> https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > Please add this link as Bugzilla: > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign off > statement ITYM *before* Signed-off-by. But yeah, use the Bugzilla: tag, and please use git log to see plenty of examples. BR, Jani. > >> >> Signed-off-by: Srinivasan S <srinivasan.s@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index eeeb3f933aa4..5b2278fdf675 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, >> >> ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); >> if (ret) { >> - DRM_ERROR("failed to update payload %d\n", ret); >> + if (!connector || >> + connector->base.status != connector_status_connected) { >> + DRM_WARN("DP MST disconnect\n"); > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > If the connector is disconnected, why update payload? > > Jani, Ville, thoughts? > > Regards > Manasi > >> + } else { >> + DRM_ERROR("failed to update payload %d\n", ret); >> + } >> } >> if (old_crtc_state->has_audio) >> intel_audio_codec_disable(encoder, >> -- >> 2.7.4 >>
Then it's better that, could we change it to DRM_DEBUG_KMS("failed to update payload %d\n", ret); instead of DRM_ERROR("failed to update payload %d\n", ret);, without any connector status check, would that be fine? Regards, -----Original Message----- From: Ville Syrjälä <ville.syrjala@linux.intel.com> Sent: Thursday, September 19, 2019 5:34 PM To: S, Srinivasan <srinivasan.s@intel.com> Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable On Thu, Sep 19, 2019 at 07:23:30AM +0000, S, Srinivasan wrote: > Would the following be appropriate fix? > > if (connector || connector->base.status == connector_status_connected) { > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > if (ret) { > DRM_ERROR("failed to update payload %d\n", ret); > } > } The whole connector->status check is racy. IMO don't do it. > > Regards, > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > Manasi Navare > Sent: Wednesday, September 18, 2019 11:55 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: S, Srinivasan <srinivasan.s@intel.com>; > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging > TypeC cable > > On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > > > This patch avoids DP MST payload error message in dmesg, as it > > > > is trying to read the payload from the disconnected DP MST device. > > > > After the unplug the connector status is disconnected and we > > > > should not be looking for the payload and hence remove the error and throw the warning. > > > > > > > > This details can be found in: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > > > Please add this link as Bugzilla: > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign > > > off statement > > > > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct > > > > intel_encoder *encoder, > > > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > if (ret) { > > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > > + if (!connector || > > > > + connector->base.status != connector_status_connected) { > > > > + DRM_WARN("DP MST disconnect\n"); > > > > > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > > > If the connector is disconnected, why update payload? > > > > > > Jani, Ville, thoughts? > > > > Or just convert it to a debug? > > Sure that will work, but do we really want to update the payload if the connector status is disconnected. > So shouldnt checking that before calling the function be a better fix? > > Manasi > > > > > > > > > Regards > > > Manasi > > > > > > > + } else { > > > > + DRM_ERROR("failed to update payload %d\n", ret); > > > > + } > > > > } > > > > if (old_crtc_state->has_audio) > > > > intel_audio_codec_disable(encoder, > > > > -- > > > > 2.7.4 > > > > > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel
Hi Ville, I have revised the patch from DRM_ERROR to DRM_DEBUG, could you please review? https://patchwork.freedesktop.org/patch/332806/?series=66837&rev=3 Thanks, -----Original Message----- From: S, Srinivasan Sent: Thursday, September 19, 2019 7:22 PM To: 'Ville Syrjälä' <ville.syrjala@linux.intel.com> Cc: Navare, Manasi D <manasi.d.navare@intel.com>; 'intel-gfx@lists.freedesktop.org' <intel-gfx@lists.freedesktop.org>; 'dri-devel@lists.freedesktop.org' <dri-devel@lists.freedesktop.org> Subject: RE: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable Then it's better that, could we change it to DRM_DEBUG_KMS("failed to update payload %d\n", ret); instead of DRM_ERROR("failed to update payload %d\n", ret);, without any connector status check, would that be fine? Regards, -----Original Message----- From: Ville Syrjälä <ville.syrjala@linux.intel.com> Sent: Thursday, September 19, 2019 5:34 PM To: S, Srinivasan <srinivasan.s@intel.com> Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable On Thu, Sep 19, 2019 at 07:23:30AM +0000, S, Srinivasan wrote: > Would the following be appropriate fix? > > if (connector || connector->base.status == connector_status_connected) { > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > if (ret) { > DRM_ERROR("failed to update payload %d\n", ret); > } > } The whole connector->status check is racy. IMO don't do it. > > Regards, > -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > Manasi Navare > Sent: Wednesday, September 18, 2019 11:55 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: S, Srinivasan <srinivasan.s@intel.com>; > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging > TypeC cable > > On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com wrote: > > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > > > This patch avoids DP MST payload error message in dmesg, as it > > > > is trying to read the payload from the disconnected DP MST device. > > > > After the unplug the connector status is disconnected and we > > > > should not be looking for the payload and hence remove the error and throw the warning. > > > > > > > > This details can be found in: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > > > Please add this link as Bugzilla: > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign > > > off statement > > > > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct > > > > intel_encoder *encoder, > > > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > if (ret) { > > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > > + if (!connector || > > > > + connector->base.status != connector_status_connected) { > > > > + DRM_WARN("DP MST disconnect\n"); > > > > > > May be adding this check before calling drm_dp_update_payload_part1() is a better idea? > > > If the connector is disconnected, why update payload? > > > > > > Jani, Ville, thoughts? > > > > Or just convert it to a debug? > > Sure that will work, but do we really want to update the payload if the connector status is disconnected. > So shouldnt checking that before calling the function be a better fix? > > Manasi > > > > > > > > > Regards > > > Manasi > > > > > > > + } else { > > > > + DRM_ERROR("failed to update payload %d\n", ret); > > > > + } > > > > } > > > > if (old_crtc_state->has_audio) > > > > intel_audio_codec_disable(encoder, > > > > -- > > > > 2.7.4 > > > > > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel
Could anyone please review the patch below & let me know if any other ideas please? https://patchwork.freedesktop.org/patch/332806/?series=66837&rev=3 Thanks, > -----Original Message----- > From: S, Srinivasan > Sent: Wednesday, September 25, 2019 8:33 PM > To: 'Ville Syrjälä' <ville.syrjala@linux.intel.com> > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; 'intel- > gfx@lists.freedesktop.org' <intel-gfx@lists.freedesktop.org>; 'dri- > devel@lists.freedesktop.org' <dri-devel@lists.freedesktop.org> > Subject: RE: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC > cable > > Hi Ville, > > I have revised the patch from DRM_ERROR to DRM_DEBUG, could you please > review? > > https://patchwork.freedesktop.org/patch/332806/?series=66837&rev=3 > > Thanks, > > -----Original Message----- > From: S, Srinivasan > Sent: Thursday, September 19, 2019 7:22 PM > To: 'Ville Syrjälä' <ville.syrjala@linux.intel.com> > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; 'intel- > gfx@lists.freedesktop.org' <intel-gfx@lists.freedesktop.org>; 'dri- > devel@lists.freedesktop.org' <dri-devel@lists.freedesktop.org> > Subject: RE: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC > cable > > Then it's better that, could we change it to DRM_DEBUG_KMS("failed to update > payload %d\n", ret); instead of DRM_ERROR("failed to update payload %d\n", > ret);, without any connector status check, would that be fine? > > Regards, > -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Thursday, September 19, 2019 5:34 PM > To: S, Srinivasan <srinivasan.s@intel.com> > Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC > cable > > On Thu, Sep 19, 2019 at 07:23:30AM +0000, S, Srinivasan wrote: > > Would the following be appropriate fix? > > > > if (connector || connector->base.status == connector_status_connected) > { > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > if (ret) { > > DRM_ERROR("failed to update payload %d\n", ret); > > } > > } > > The whole connector->status check is racy. IMO don't do it. > > > > > Regards, > > -----Original Message----- > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of > > Manasi Navare > > Sent: Wednesday, September 18, 2019 11:55 PM > > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: S, Srinivasan <srinivasan.s@intel.com>; > > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > > Subject: Re: [PATCH] drm/i915/dp: Fix DP MST error after unplugging > > TypeC cable > > > > On Wed, Sep 18, 2019 at 09:11:36PM +0300, Ville Syrjälä wrote: > > > On Wed, Sep 18, 2019 at 10:50:39AM -0700, Manasi Navare wrote: > > > > On Wed, Sep 18, 2019 at 07:09:43AM +0530, srinivasan.s@intel.com > wrote: > > > > > From: Srinivasan S <srinivasan.s@intel.com> > > > > > > > > > > This patch avoids DP MST payload error message in dmesg, as it > > > > > is trying to read the payload from the disconnected DP MST device. > > > > > After the unplug the connector status is disconnected and we > > > > > should not be looking for the payload and hence remove the error and > throw the warning. > > > > > > > > > > This details can be found in: > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 > > > > > > > > Please add this link as Bugzilla: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=111632 after the Sign > > > > off statement > > > > > > > > > > > > > > Signed-off-by: Srinivasan S <srinivasan.s@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > index eeeb3f933aa4..5b2278fdf675 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct > > > > > intel_encoder *encoder, > > > > > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > > if (ret) { > > > > > - DRM_ERROR("failed to update payload %d\n", ret); > > > > > + if (!connector || > > > > > + connector->base.status != > connector_status_connected) { > > > > > + DRM_WARN("DP MST disconnect\n"); > > > > > > > > May be adding this check before calling drm_dp_update_payload_part1() is > a better idea? > > > > If the connector is disconnected, why update payload? > > > > > > > > Jani, Ville, thoughts? > > > > > > Or just convert it to a debug? > > > > Sure that will work, but do we really want to update the payload if the > connector status is disconnected. > > So shouldnt checking that before calling the function be a better fix? > > > > Manasi > > > > > > > > > > > > > Regards > > > > Manasi > > > > > > > > > + } else { > > > > > + DRM_ERROR("failed to update payload %d\n", > ret); > > > > > + } > > > > > } > > > > > if (old_crtc_state->has_audio) > > > > > intel_audio_codec_disable(encoder, > > > > > -- > > > > > 2.7.4 > > > > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index eeeb3f933aa4..5b2278fdf675 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -215,7 +215,12 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); if (ret) { - DRM_ERROR("failed to update payload %d\n", ret); + if (!connector || + connector->base.status != connector_status_connected) { + DRM_WARN("DP MST disconnect\n"); + } else { + DRM_ERROR("failed to update payload %d\n", ret); + } } if (old_crtc_state->has_audio) intel_audio_codec_disable(encoder,