Re: [PATCH 0/2] drm: link status property and DP link training failure handling

[Manasi Navare <manasi.d.navare@intel.com>]

On Tue, Dec 20, 2016 at 10:30:17AM +0100, Daniel Vetter wrote:
> On Mon, Dec 19, 2016 at 11:15:40PM +0000, Pandiyan, Dhinakaran wrote:
> > On Sun, 2016-12-18 at 14:43 +0100, Daniel Vetter wrote:
> > > On Sat, Dec 17, 2016 at 05:47:56AM +0000, Pandiyan, Dhinakaran wrote:
> > > > On Fri, 2016-12-16 at 16:47 +0200, Jani Nikula wrote:
> > > > > On Fri, 16 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > On Fri, Dec 16, 2016 at 12:29:05PM +0200, Jani Nikula wrote:
> > > > > >> The two remaining patches from [1], rebased.
> > > > > >> 
> > > > > >> BR,
> > > > > >> Jani.
> > > > > >> 
> > > > > >> 
> > > > > >> [1] http://mid.mail-archive.com/1480984058-552-1-git-send-email-manasi.d.navare@intel.com
> > > > > >
> > > > > > Just for the record, I think the only thing missing here is the Xorg
> > > > > > review on the -modesetting patch. As soon as we have that I can vacuum
> > > > > > this up (probably best through drm-misc, but not sure).
> > > > > 
> > > > > Yeah I rebased this (and provided a debug hack privately) so Martin can
> > > > > test the modesetting changes.
> > > > > 
> > > > > BR,
> > > > > Jani.
> > > > > 
> > > > > 
> > > > 
> > > > I tested the -modesetting patch, which Martin had provided to Manasi,
> > > > with a compliance testing device (DPR-120) that can simulate link
> > > > training failure. The link rate correctly lowered after the link_status
> > > > property was set to BAD by the kernel and the userspace responded with a
> > > > modeset. 
> > > > 
> > > > One thing that was not straight forward to figure out was I had to boot
> > > > with i915.nuclear_pageflip=1. Is it documented somewhere that the
> > > > property needs DRIVER_ATOMIC to be set, or is it implicit?
> > > 
> > > It should work without DRIVER_ATOMIC. At least the property should be
> > > exposed ... If this does only work with DRIVER_ATOMIC set then we have a
> > > bug somewhere. Can you pls try to figure out why it doesn't work?
> > > 
> > 
> > The property is exposed even without DRIVER_ATOMIC set, but the value is
> > always GOOD (0).
> > We set connector->state->link_status to BAD when link training fails but
> > the getconnector() ioctl ends up reading obj->properties->values[i] if
> > DRIVER_ATOMIC is NOT set. But with DRIVER_ATOMIC set, getconnector()
> > calls into drm_atomic_connector_get_property() and retrieves the value
> > stored in connector->state->link_status.
> 
> That sounds like a bug in the getconnector code. This needs the same
> treatment as other places, see e.g.
> 
> commit d3a46183db97536a53df5de6b556bd61197842b2
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Jun 8 14:19:15 2016 +0200
> 
>     drm: Replace fb_helper->atomic with mode_config->atomic_commit
> 
> I think it'd be good to extract this check into a
> drm_drv_uses_atomic_modeset to better self-document the code, roll it out
> to all existing places that check for atomic_commit and then also roll it
> out to the getproperty functions (for connectors, planes and crtcs).
> 
> > > > The other thing I had trouble with -modesetting was, there was no
> > > > modeset following a long pulse from the sink at the begging of the test.
> > > > I had to force a modeset by changing the resolution so that the link
> > > > training path is executed. However, the link training failure induced a
> > > > modeset without any intervention.


This seems little strange because in case of SNA driver, it does respond
with a full reprobe and a modeset following a long pulse from the sink at the
beginning of the test and no forcing through xrandr was required.
I think the modesetting driver should also behave the same and should not
need any forcing of the modeset, but should be able to respond to the
hotplug/long pulse at the beginning of each test which is trying to simulate
the connect/disconnect of the DP cable.
My guess is that the modesetting driver was probably not built with hotplug support
which would cause it to not respond to the long pulse at the beginning
of the test.

Chris/Martin/Daniel , any thoughts?

Manasi


> > > 
> > > Sounds roughly like how it's supposed to work. For real mode configuration
> > > changes the desktop environment is supposed to set the mode it wants, by
> > > listening to the xrandr hotplug event. That's not the same as the udev
> > > hotplug event. You can listen for the xrandr hotplug event using
> > > 
> > > $ xev -event randr
> > > 
> > 
> > Got it, -modesetting does indeed send out the hotplug events upon
> > hotplug.
> 
> Excellent, so at least that's all working well.
> -Daniel
> 
> > 
> > -DK
> > 
> > 
> > > Cheers, Daniel
> > > 
> > > > 
> > > > -DK
> > > > 
> > > > 
> > > > > > -Daniel
> > > > > >
> > > > > >> 
> > > > > >> 
> > > > > >> Manasi Navare (2):
> > > > > >>   drm: Add a new connector atomic property for link status
> > > > > >>   drm/i915: Implement Link Rate fallback on Link training failure
> > > > > >> 
> > > > > >>  drivers/gpu/drm/drm_atomic.c                  | 16 +++++++++
> > > > > >>  drivers/gpu/drm/drm_atomic_helper.c           | 15 ++++++++
> > > > > >>  drivers/gpu/drm/drm_connector.c               | 52 +++++++++++++++++++++++++++
> > > > > >>  drivers/gpu/drm/i915/intel_dp.c               | 27 ++++++++++++++
> > > > > >>  drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++--
> > > > > >>  drivers/gpu/drm/i915/intel_drv.h              |  3 ++
> > > > > >>  include/drm/drm_connector.h                   | 19 ++++++++++
> > > > > >>  include/drm/drm_mode_config.h                 |  5 +++
> > > > > >>  include/uapi/drm/drm_mode.h                   |  4 +++
> > > > > >>  9 files changed, 161 insertions(+), 2 deletions(-)
> > > > > >> 
> > > > > >> -- 
> > > > > >> 2.1.4
> > > > > >> 
> > > > > >> _______________________________________________
> > > > > >> dri-devel mailing list
> > > > > >> dri-devel@lists.freedesktop.org
> > > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > 
> > > > 
> > > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx