[Intel-gfx] [PATCH] drm/i915/dp: Fix DP MST error after unplugging TypeC cable

[S, Srinivasan]

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 at linux.intel.com>
> Cc: Navare, Manasi D <manasi.d.navare at intel.com>; 'intel-
> gfx at lists.freedesktop.org' <intel-gfx at lists.freedesktop.org>; 'dri-
> devel at lists.freedesktop.org' <dri-devel at 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 at linux.intel.com>
> Cc: Navare, Manasi D <manasi.d.navare at intel.com>; 'intel-
> gfx at lists.freedesktop.org' <intel-gfx at lists.freedesktop.org>; 'dri-
> devel at lists.freedesktop.org' <dri-devel at 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 at linux.intel.com>
> Sent: Thursday, September 19, 2019 5:34 PM
> To: S, Srinivasan <srinivasan.s at intel.com>
> Cc: Navare, Manasi D <manasi.d.navare at intel.com>; intel-
> gfx at lists.freedesktop.org; dri-devel at 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 at lists.freedesktop.org> On Behalf Of
> > Manasi Navare
> > Sent: Wednesday, September 18, 2019 11:55 PM
> > To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: S, Srinivasan <srinivasan.s at intel.com>;
> > intel-gfx at lists.freedesktop.org; dri-devel at 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 at intel.com
> wrote:
> > > > > From: Srinivasan S <srinivasan.s at 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 at 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Ville Syrjälä
> Intel