[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Tue, Nov 22, 2016 at 05:15:59PM -0800, Manasi Navare wrote:
> On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst
> > > > > > > > > wrote:
> > > > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten
> > > > > > > > > > > Lankhorst wrote:
> > > > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > > > >>> if connector link status property has changed. This
> > > > > > > > > > >>> will tell the
> > > > > > > > > > >>> driver to do a complete modeset due to change in
> > > > > > > > > > >>> connector property.
> > > > > > > > > > >>>
> > > > > > > > > > >>> Acked-by: Harry Wentland
> > > > > > > > > > >>> Acked-by: Tony Cheng
> > > > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > > > >>> Cc: Jani Nikula
> > > > > > > > > > >>> Cc: Daniel Vetter
> > > > > > > > > > >>> Cc: Ville Syrjala
> > > > > > > > > > >>> Signed-off-by: Manasi Navare > > > > > > > > > >>> intel.com>
> > > > > > > > > > >>> ---
> > > > > > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > > > > > >>>
> > > > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state
> > > > > > > > > > >>> *state,
> > > > > > > > > > >>>
> > > > > > > > > > >>> connector_state);
> > > > > > > > > > >>> if (ret)
> > > > > > > > > > >>> return ret;
> > > > > > > > > > >>> +
> > > > > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > > > > >>> + crtc_state =
> > > > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > > > >>> +
> > > > > > > > > > >>> connector->state->crtc);
> > > > > > > > > > >>> + if (connector->link_status ==
> > > > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > > > >>> +
> > > > > > > > > > >>> crtc_state->connectors_changed = true;
> > > > > > > > > > >>> + }
> > > > > > > > > > >>> }
> > > > > > > > > > >>>
> > > > > > > > > > >>> /*
> > > > > > > > > > >> This will cause ordinary atomic commits that happen to
> > > > > > > > > > >> change connector flags to potentially fail with -EINVAL
> > > > > > > > > > >> if ALLOW_MODESET is not set.
> > > > > > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > > > > > >> automatically by the kernel. Could we add add a retrain
> > > > > > > > > > >> link property instead, that
> > > > > > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > > > Or just check for allow_modeset before setting
> > > > > > > > > > > connectors_changed=true here?
> > > > > > > > > >
> > > > > > > > > > I don't think modesets should be done automatically like
> > > > > > > > > > that, even if ALLOW_MODESET is set a modeset may not be
> > > > > > > > > > expected by userspace.
> > > > > > > > >
> > > > > > > > > Presumably userspace would want a picture on the screen using
> > > > > > > > > any means
> > > > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset
> > > > > > > > > it should
> > > > > > > > > probably say as much?
> > > > > > > >
> > > > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have
> > > > > > > > to do a
> > > > > > > > modeset to recover it, otherwise you'll be forever stuck with a
> > > > > > > > bad
> > > > > > > > screen.
> > > > > > > >
> > > > > > > > What we could try is to gate this of whether userspace touches
> > > > > > > > the mode
> > > > > > > > property on the corresponding CRTC. I.e. if that's touched
> > > > > > > >
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst
> > > > > > > > wrote:
> > > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst
> > > > > > > > > > wrote:
> > > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > > >>> if connector link status property has changed. This will
> > > > > > > > > >>> tell the
> > > > > > > > > >>> driver to do a complete modeset due to change in
> > > > > > > > > >>> connector property.
> > > > > > > > > >>>
> > > > > > > > > >>> Acked-by: Harry Wentland
> > > > > > > > > >>> Acked-by: Tony Cheng
> > > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > > >>> Cc: Jani Nikula
> > > > > > > > > >>> Cc: Daniel Vetter
> > > > > > > > > >>> Cc: Ville Syrjala
> > > > > > > > > >>> Signed-off-by: Manasi Navare > > > > > > > > >>> intel.com>
> > > > > > > > > >>> ---
> > > > > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > > > > >>>
> > > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state
> > > > > > > > > >>> *state,
> > > > > > > > > >>> connector_state);
> > > > > > > > > >>> if (ret)
> > > > > > > > > >>> return ret;
> > > > > > > > > >>> +
> > > > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > > > >>> + crtc_state =
> > > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > > >>> +
> > > > > > > > > >>> connector->state->crtc);
> > > > > > > > > >>> + if (connector->link_status ==
> > > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > > >>> + crtc_state->connectors_changed
> > > > > > > > > >>> = true;
> > > > > > > > > >>> + }
> > > > > > > > > >>> }
> > > > > > > > > >>>
> > > > > > > > > >>> /*
> > > > > > > > > >> This will cause ordinary atomic commits that happen to
> > > > > > > > > >> change connector flags to potentially fail with -EINVAL if
> > > > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > > > > >> automatically by the kernel. Could we add add a retrain
> > > > > > > > > >> link property instead, that
> > > > > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > > Or just check for allow_modeset before setting
> > > > > > > > > > connectors_changed=true here?
> > > > > > > > >
> > > > > > > > > I don't think modesets should be done automatically like
> > > > > > > > > that, even if ALLOW_MODESET is set a modeset may not be
> > > > > > > > > expected by userspace.
> > > > > > > >
> > > > > > > > Presumably userspace would want a picture on the screen using
> > > > > > > > any means
> > > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset
> > > > > > > > it should
> > > > > > > > probably say as much?
> > > > > > >
> > > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have
> > > > > > > to do a
> > > > > > > modeset to recover it, otherwise you'll be forever stuck with a
> > > > > > > bad
> > > > > > > screen.
> > > > > > >
> > > > > > > What we could try is to gate this of whether userspace touches
> > > > > > > the mode
> > > > > > > property on the corresponding CRTC. I.e. if that's touched (even
> > > > > > > if it's
> > > > > > > the same mode), and a link is bad in one of the connectors in the
> > > > > > > state
> > > > > > > then we do a full modeset to recover.
> > > > > > >
> > > > > > > Another option would be to make the link status writeable. Trying
> > > > > > > to
> > > > > > > change it from bad->good would force
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 11:00:52AM -0800, Manasi Navare wrote: > On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote: > > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote: > > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote: > > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote: > > > > > - Another fallout is that legacy clients will no longer see the > > > > > link-status property. And they won't be able to set it through the > > > > > SETCRTC ioctl, which would kinda defaut the point. I think the best > > > > > solution would be to check for link_status == BAD in > > > > > drm_atomic_helper_set_config, and reset it to good automatically for > > > > > legacy clients. > > > > > > > > Then how do they know that the kernel demands the modeset? Both a legacy > > > > and atomic property? > > > > > > I guess we could avoid the filtering of the property for legacy clients. > > > Definitely not 2 properties, that's silly. Or we teach userspace to go > > > look for atomic properties. > > > > Well, now that I flushed the gunk out of my brain with some work-out it's > > a lot easier: ATOMIC on properties is only to hide them from legacy > > userspace, it doesn't control how it's implement. Which means we can > > implement it as described above, and non-atomic userspace can still read > > it. Setting would also work, but since we want to do that as part of > > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a > > modeset will happen or not, automagic in there seems reasonable. > > Thanks Daniel for providing the solution alternatives here. > So after we make it atomic, we would solve the problem of updating the > connector_changed > in atomic_helper_check_modeset function. So in this, who resets the property > to GOOD? > Would this happen in drm_atomic_helper_set_config in both atomic and non > atomic cases? > > And in case of non atomic userspace, will it still be able to read > link-status as BAD in userspace > to decide whether it needs to call setcrtc? > > Chris, will any implementation in your patch for link _status change if this > is made atomic? So long at the property remains visible via the GETCONNECTOR ioctl, no. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst
> > > > > > > > > wrote:
> > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > >>> if connector link status property has changed. This will
> > > > > > > > >>> tell the
> > > > > > > > >>> driver to do a complete modeset due to change in connector
> > > > > > > > >>> property.
> > > > > > > > >>>
> > > > > > > > >>> Acked-by: Harry Wentland
> > > > > > > > >>> Acked-by: Tony Cheng
> > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > >>> Cc: Jani Nikula
> > > > > > > > >>> Cc: Daniel Vetter
> > > > > > > > >>> Cc: Ville Syrjala
> > > > > > > > >>> Signed-off-by: Manasi Navare
> > > > > > > > >>> ---
> > > > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > > > >>>
> > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > > > > >>>connector_state);
> > > > > > > > >>> if (ret)
> > > > > > > > >>> return ret;
> > > > > > > > >>> +
> > > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > > >>> + crtc_state =
> > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > >>> +
> > > > > > > > >>> connector->state->crtc);
> > > > > > > > >>> + if (connector->link_status ==
> > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > >>> + crtc_state->connectors_changed
> > > > > > > > >>> = true;
> > > > > > > > >>> + }
> > > > > > > > >>> }
> > > > > > > > >>>
> > > > > > > > >>> /*
> > > > > > > > >> This will cause ordinary atomic commits that happen to
> > > > > > > > >> change connector flags to potentially fail with -EINVAL if
> > > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > > > >> automatically by the kernel. Could we add add a retrain link
> > > > > > > > >> property instead, that
> > > > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > Or just check for allow_modeset before setting
> > > > > > > > > connectors_changed=true here?
> > > > > > > >
> > > > > > > > I don't think modesets should be done automatically like that,
> > > > > > > > even if ALLOW_MODESET is set a modeset may not be expected by
> > > > > > > > userspace.
> > > > > > >
> > > > > > > Presumably userspace would want a picture on the screen using any
> > > > > > > means
> > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it
> > > > > > > should
> > > > > > > probably say as much?
> > > > > >
> > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have to
> > > > > > do a
> > > > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > > > screen.
> > > > > >
> > > > > > What we could try is to gate this of whether userspace touches the
> > > > > > mode
> > > > > > property on the corresponding CRTC. I.e. if that's touched (even if
> > > > > > it's
> > > > > > the same mode), and a link is bad in one of the connectors in the
> > > > > > state
> > > > > > then we do a full modeset to recover.
> > > > > >
> > > > > > Another option would be to make the link status writeable. Trying to
> > > > > > change it from bad->good would force the modeset. That would be
> > > > > > 100% clear
> > > > > > to userspace, not special hacks needed with checking for
> > > > > > allow_modeset,
> > > > > > no magic property that auto-changes its value. And 100% backwards
> > > > > > compat
> > > > > > because existing userspace should never touch properties it
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 08:46:19PM +, Chris Wilson wrote: > On Mon, Nov 21, 2016 at 11:00:52AM -0800, Manasi Navare wrote: > > On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote: > > > On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote: > > > > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote: > > > > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote: > > > > > > - Another fallout is that legacy clients will no longer see the > > > > > > link-status property. And they won't be able to set it through the > > > > > > SETCRTC ioctl, which would kinda defaut the point. I think the > > > > > > best > > > > > > solution would be to check for link_status == BAD in > > > > > > drm_atomic_helper_set_config, and reset it to good automatically > > > > > > for > > > > > > legacy clients. > > > > > > > > > > Then how do they know that the kernel demands the modeset? Both a > > > > > legacy > > > > > and atomic property? > > > > > > > > I guess we could avoid the filtering of the property for legacy clients. > > > > Definitely not 2 properties, that's silly. Or we teach userspace to go > > > > look for atomic properties. > > > > > > Well, now that I flushed the gunk out of my brain with some work-out it's > > > a lot easier: ATOMIC on properties is only to hide them from legacy > > > userspace, it doesn't control how it's implement. Which means we can > > > implement it as described above, and non-atomic userspace can still read > > > it. Setting would also work, but since we want to do that as part of > > > SETCRTC anyway, and since legacy SETCRTC doesn't specifiy whether a > > > modeset will happen or not, automagic in there seems reasonable. > > > > Thanks Daniel for providing the solution alternatives here. > > So after we make it atomic, we would solve the problem of updating the > > connector_changed > > in atomic_helper_check_modeset function. So in this, who resets the > > property to GOOD? > > Would this happen in drm_atomic_helper_set_config in both atomic and non > > atomic cases? > > > > And in case of non atomic userspace, will it still be able to read > > link-status as BAD in userspace > > to decide whether it needs to call setcrtc? > > > > Chris, will any implementation in your patch for link _status change if > > this is made atomic? > > So long at the property remains visible via the GETCONNECTOR ioctl, no. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre If it is made an atomic property, will it be visible to userspace through GETCONNECTOR? Manasi
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst
> > > > > > > > wrote:
> > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > >>> if connector link status property has changed. This will tell
> > > > > > > >>> the
> > > > > > > >>> driver to do a complete modeset due to change in connector
> > > > > > > >>> property.
> > > > > > > >>>
> > > > > > > >>> Acked-by: Harry Wentland
> > > > > > > >>> Acked-by: Tony Cheng
> > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > >>> Cc: Jani Nikula
> > > > > > > >>> Cc: Daniel Vetter
> > > > > > > >>> Cc: Ville Syrjala
> > > > > > > >>> Signed-off-by: Manasi Navare
> > > > > > > >>> ---
> > > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > > >>>
> > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > > > >>> connector_state);
> > > > > > > >>> if (ret)
> > > > > > > >>> return ret;
> > > > > > > >>> +
> > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > >>> + crtc_state =
> > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > >>> +
> > > > > > > >>> connector->state->crtc);
> > > > > > > >>> + if (connector->link_status ==
> > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > >>> + crtc_state->connectors_changed
> > > > > > > >>> = true;
> > > > > > > >>> + }
> > > > > > > >>> }
> > > > > > > >>>
> > > > > > > >>> /*
> > > > > > > >> This will cause ordinary atomic commits that happen to change
> > > > > > > >> connector flags to potentially fail with -EINVAL if
> > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > > >> automatically by the kernel. Could we add add a retrain link
> > > > > > > >> property instead, that
> > > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > Or just check for allow_modeset before setting
> > > > > > > > connectors_changed=true here?
> > > > > > >
> > > > > > > I don't think modesets should be done automatically like that,
> > > > > > > even if ALLOW_MODESET is set a modeset may not be expected by
> > > > > > > userspace.
> > > > > >
> > > > > > Presumably userspace would want a picture on the screen using any
> > > > > > means
> > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it
> > > > > > should
> > > > > > probably say as much?
> > > > >
> > > > > Yeah, agreed. Also, if the link is bad then we pretty much have to do
> > > > > a
> > > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > > screen.
> > > > >
> > > > > What we could try is to gate this of whether userspace touches the
> > > > > mode
> > > > > property on the corresponding CRTC. I.e. if that's touched (even if
> > > > > it's
> > > > > the same mode), and a link is bad in one of the connectors in the
> > > > > state
> > > > > then we do a full modeset to recover.
> > > > >
> > > > > Another option would be to make the link status writeable. Trying to
> > > > > change it from bad->good would force the modeset. That would be 100%
> > > > > clear
> > > > > to userspace, not special hacks needed with checking for
> > > > > allow_modeset,
> > > > > no magic property that auto-changes its value. And 100% backwards
> > > > > compat
> > > > > because existing userspace should never touch properties it doesn't
> > > > > understand (except when restoring a mode, and then it should allow a
> > > > > full
> > > > > modeset). And if someone does try a good->bad transition, we just
> > > > > silently
> > > > > keep it at good.
> > > >
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 04:48:07PM +0100, Daniel Vetter wrote:
> On Mon, Nov 21, 2016 at 11:10:45AM +0100, Daniel Vetter wrote:
> > On Mon, Nov 21, 2016 at 09:42:57AM +, Chris Wilson wrote:
> > > On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > > > > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst
> > > > > > > > wrote:
> > > > > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst
> > > > > > > > > > wrote:
> > > > > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > > > > >>> if connector link status property has changed. This will
> > > > > > > > > >>> tell the
> > > > > > > > > >>> driver to do a complete modeset due to change in
> > > > > > > > > >>> connector property.
> > > > > > > > > >>>
> > > > > > > > > >>> Acked-by: Harry Wentland
> > > > > > > > > >>> Acked-by: Tony Cheng
> > > > > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > > > > >>> Cc: Jani Nikula
> > > > > > > > > >>> Cc: Daniel Vetter
> > > > > > > > > >>> Cc: Ville Syrjala
> > > > > > > > > >>> Signed-off-by: Manasi Navare > > > > > > > > >>> intel.com>
> > > > > > > > > >>> ---
> > > > > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > > > > >>>
> > > > > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state
> > > > > > > > > >>> *state,
> > > > > > > > > >>> connector_state);
> > > > > > > > > >>> if (ret)
> > > > > > > > > >>> return ret;
> > > > > > > > > >>> +
> > > > > > > > > >>> + if (connector->state->crtc) {
> > > > > > > > > >>> + crtc_state =
> > > > > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > > > > >>> +
> > > > > > > > > >>> connector->state->crtc);
> > > > > > > > > >>> + if (connector->link_status ==
> > > > > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > > > > >>> + crtc_state->connectors_changed
> > > > > > > > > >>> = true;
> > > > > > > > > >>> + }
> > > > > > > > > >>> }
> > > > > > > > > >>>
> > > > > > > > > >>> /*
> > > > > > > > > >> This will cause ordinary atomic commits that happen to
> > > > > > > > > >> change connector flags to potentially fail with -EINVAL if
> > > > > > > > > >> ALLOW_MODESET is not set.
> > > > > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > > > > >> automatically by the kernel. Could we add add a retrain
> > > > > > > > > >> link property instead, that
> > > > > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > > > > Or just check for allow_modeset before setting
> > > > > > > > > > connectors_changed=true here?
> > > > > > > > >
> > > > > > > > > I don't think modesets should be done automatically like
> > > > > > > > > that, even if ALLOW_MODESET is set a modeset may not be
> > > > > > > > > expected by userspace.
> > > > > > > >
> > > > > > > > Presumably userspace would want a picture on the screen using
> > > > > > > > any means
> > > > > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset
> > > > > > > > it should
> > > > > > > > probably say as much?
> > > > > > >
> > > > > > > Yeah, agreed. Also, if the link is bad then we pretty much have
> > > > > > > to do a
> > > > > > > modeset to recover it, otherwise you'll be forever stuck with a
> > > > > > > bad
> > > > > > > screen.
> > > > > > >
> > > > > > > What we could try is to gate this of whether userspace touches
> > > > > > > the mode
> > > > > > > property on the corresponding CRTC. I.e. if that's touched (even
> > > > > > > if it's
> > > > > > > the same mode), and a link is bad in one of the connectors in the
> > > > > > > state
> > > > > > > then we do a full modeset to recover.
> > > > > > >
> > > > > > > Another option would be to make the link status writeable. Trying
> > > > > > > to
> > > > > > > change it from bad->good would force
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > >>> if connector link status property has changed. This will tell the
> > > > > >>> driver to do a complete modeset due to change in connector
> > > > > >>> property.
> > > > > >>>
> > > > > >>> Acked-by: Harry Wentland
> > > > > >>> Acked-by: Tony Cheng
> > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > >>> Cc: Jani Nikula
> > > > > >>> Cc: Daniel Vetter
> > > > > >>> Cc: Ville Syrjala
> > > > > >>> Signed-off-by: Manasi Navare
> > > > > >>> ---
> > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > >>> 1 file changed, 7 insertions(+)
> > > > > >>>
> > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> index 0b16587..2125fd1 100644
> > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > >>> connector_state);
> > > > > >>> if (ret)
> > > > > >>> return ret;
> > > > > >>> +
> > > > > >>> + if (connector->state->crtc) {
> > > > > >>> + crtc_state =
> > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > >>> +
> > > > > >>> connector->state->crtc);
> > > > > >>> + if (connector->link_status ==
> > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > >>> + crtc_state->connectors_changed = true;
> > > > > >>> + }
> > > > > >>> }
> > > > > >>>
> > > > > >>> /*
> > > > > >> This will cause ordinary atomic commits that happen to change
> > > > > >> connector flags to potentially fail with -EINVAL if ALLOW_MODESET
> > > > > >> is not set.
> > > > > >> For this reason I'm not sure this flag should be set automatically
> > > > > >> by the kernel. Could we add add a retrain link property instead,
> > > > > >> that
> > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > >> connectors_changed to be set on bad link status?
> > > > > > Or just check for allow_modeset before setting
> > > > > > connectors_changed=true here?
> > > > >
> > > > > I don't think modesets should be done automatically like that, even
> > > > > if ALLOW_MODESET is set a modeset may not be expected by userspace.
> > > >
> > > > Presumably userspace would want a picture on the screen using any means
> > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> > > > probably say as much?
> > >
> > > Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > screen.
> > >
> > > What we could try is to gate this of whether userspace touches the mode
> > > property on the corresponding CRTC. I.e. if that's touched (even if it's
> > > the same mode), and a link is bad in one of the connectors in the state
> > > then we do a full modeset to recover.
> > >
> > > Another option would be to make the link status writeable. Trying to
> > > change it from bad->good would force the modeset. That would be 100% clear
> > > to userspace, not special hacks needed with checking for allow_modeset,
> > > no magic property that auto-changes its value. And 100% backwards compat
> > > because existing userspace should never touch properties it doesn't
> > > understand (except when restoring a mode, and then it should allow a full
> > > modeset). And if someone does try a good->bad transition, we just silently
> > > keep it at good.
> > >
> > > Definitely need to document this properly in the property docs, no matter
> > > what we decide.
> >
> > Hmm. I think I kinda like this idea of userspace clear the state back
> > to good explicitly, if it happens with the same prop. So it's like
> > Maarten's retrain_link prop idea, but without having to add the second
> > prop to the mix.
> >
> > It would also save me from pointing out (for the nth time) that the
> > link status should really be cleared to good during the commit state
> > swap and not at some random point during the commit ;)
> >
>
> Okay, so change 1 is to make the userspace clear the state back to Good for
> the property..
> Then Change 2 is to set
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Mon, Nov 21, 2016 at 10:38:20AM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 09:44:49AM -0800, Manasi Navare wrote:
> > On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > > > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > > > >>> CRTC state connector_changed needs to be set to true
> > > > > > >>> if connector link status property has changed. This will tell
> > > > > > >>> the
> > > > > > >>> driver to do a complete modeset due to change in connector
> > > > > > >>> property.
> > > > > > >>>
> > > > > > >>> Acked-by: Harry Wentland
> > > > > > >>> Acked-by: Tony Cheng
> > > > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > > > >>> Cc: Jani Nikula
> > > > > > >>> Cc: Daniel Vetter
> > > > > > >>> Cc: Ville Syrjala
> > > > > > >>> Signed-off-by: Manasi Navare
> > > > > > >>> ---
> > > > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > > > >>> 1 file changed, 7 insertions(+)
> > > > > > >>>
> > > > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > >>> index 0b16587..2125fd1 100644
> > > > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > >>> @@ -519,6 +519,13 @@ static int
> > > > > > >>> handle_conflicting_encoders(struct drm_atomic_state *state,
> > > > > > >>>connector_state);
> > > > > > >>> if (ret)
> > > > > > >>> return ret;
> > > > > > >>> +
> > > > > > >>> + if (connector->state->crtc) {
> > > > > > >>> + crtc_state =
> > > > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > > > >>> +
> > > > > > >>> connector->state->crtc);
> > > > > > >>> + if (connector->link_status ==
> > > > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > > > >>> + crtc_state->connectors_changed
> > > > > > >>> = true;
> > > > > > >>> + }
> > > > > > >>> }
> > > > > > >>>
> > > > > > >>> /*
> > > > > > >> This will cause ordinary atomic commits that happen to change
> > > > > > >> connector flags to potentially fail with -EINVAL if
> > > > > > >> ALLOW_MODESET is not set.
> > > > > > >> For this reason I'm not sure this flag should be set
> > > > > > >> automatically by the kernel. Could we add add a retrain link
> > > > > > >> property instead, that
> > > > > > >> always return 0 when queried, but writing a 1 causing
> > > > > > >> connectors_changed to be set on bad link status?
> > > > > > > Or just check for allow_modeset before setting
> > > > > > > connectors_changed=true here?
> > > > > >
> > > > > > I don't think modesets should be done automatically like that, even
> > > > > > if ALLOW_MODESET is set a modeset may not be expected by userspace.
> > > > >
> > > > > Presumably userspace would want a picture on the screen using any
> > > > > means
> > > > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it
> > > > > should
> > > > > probably say as much?
> > > >
> > > > Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> > > > modeset to recover it, otherwise you'll be forever stuck with a bad
> > > > screen.
> > > >
> > > > What we could try is to gate this of whether userspace touches the mode
> > > > property on the corresponding CRTC. I.e. if that's touched (even if it's
> > > > the same mode), and a link is bad in one of the connectors in the state
> > > > then we do a full modeset to recover.
> > > >
> > > > Another option would be to make the link status writeable. Trying to
> > > > change it from bad->good would force the modeset. That would be 100%
> > > > clear
> > > > to userspace, not special hacks needed with checking for allow_modeset,
> > > > no magic property that auto-changes its value. And 100% backwards compat
> > > > because existing userspace should never touch properties it doesn't
> > > > understand (except when restoring a mode, and then it should allow a
> > > > full
> > > > modeset). And if someone does try a good->bad transition, we just
> > > > silently
> > > > keep it at good.
> > > >
> > > > Definitely need to document this properly in the property docs, no
> > > > matter
> > > > what we decide.
> > >
> > > Hmm. I think I kinda like this idea of userspace clear the state back
> > > to good explicitly, if it happens with the same prop. So it's like
> > > Maarten's retrain_link prop idea, but
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > >>> CRTC state connector_changed needs to be set to true
> > > >>> if connector link status property has changed. This will tell the
> > > >>> driver to do a complete modeset due to change in connector property.
> > > >>>
> > > >>> Acked-by: Harry Wentland
> > > >>> Acked-by: Tony Cheng
> > > >>> Cc: dri-devel at lists.freedesktop.org
> > > >>> Cc: Jani Nikula
> > > >>> Cc: Daniel Vetter
> > > >>> Cc: Ville Syrjala
> > > >>> Signed-off-by: Manasi Navare
> > > >>> ---
> > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > >>> 1 file changed, 7 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> index 0b16587..2125fd1 100644
> > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct
> > > >>> drm_atomic_state *state,
> > > >>> connector_state);
> > > >>> if (ret)
> > > >>> return ret;
> > > >>> +
> > > >>> + if (connector->state->crtc) {
> > > >>> + crtc_state =
> > > >>> drm_atomic_get_existing_crtc_state(state,
> > > >>> +
> > > >>> connector->state->crtc);
> > > >>> + if (connector->link_status ==
> > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > >>> + crtc_state->connectors_changed = true;
> > > >>> + }
> > > >>> }
> > > >>>
> > > >>> /*
> > > >> This will cause ordinary atomic commits that happen to change
> > > >> connector flags to potentially fail with -EINVAL if ALLOW_MODESET is
> > > >> not set.
> > > >> For this reason I'm not sure this flag should be set automatically by
> > > >> the kernel. Could we add add a retrain link property instead, that
> > > >> always return 0 when queried, but writing a 1 causing
> > > >> connectors_changed to be set on bad link status?
> > > > Or just check for allow_modeset before setting connectors_changed=true
> > > > here?
> > >
> > > I don't think modesets should be done automatically like that, even if
> > > ALLOW_MODESET is set a modeset may not be expected by userspace.
> >
> > Presumably userspace would want a picture on the screen using any means
> > if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> > probably say as much?
>
> Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> modeset to recover it, otherwise you'll be forever stuck with a bad
> screen.
>
> What we could try is to gate this of whether userspace touches the mode
> property on the corresponding CRTC. I.e. if that's touched (even if it's
> the same mode), and a link is bad in one of the connectors in the state
> then we do a full modeset to recover.
>
> Another option would be to make the link status writeable. Trying to
> change it from bad->good would force the modeset. That would be 100% clear
> to userspace, not special hacks needed with checking for allow_modeset,
> no magic property that auto-changes its value. And 100% backwards compat
> because existing userspace should never touch properties it doesn't
> understand (except when restoring a mode, and then it should allow a full
> modeset). And if someone does try a good->bad transition, we just silently
> keep it at good.
>
> Definitely need to document this properly in the property docs, no matter
> what we decide.
Hmm. I think I kinda like this idea of userspace clear the state back
to good explicitly, if it happens with the same prop. So it's like
Maarten's retrain_link prop idea, but without having to add the second
prop to the mix.
It would also save me from pointing out (for the nth time) that the
link status should really be cleared to good during the commit state
swap and not at some random point during the commit ;)
--
Ville Syrjälä
Intel OTC
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > >>> CRTC state connector_changed needs to be set to true
> > >>> if connector link status property has changed. This will tell the
> > >>> driver to do a complete modeset due to change in connector property.
> > >>>
> > >>> Acked-by: Harry Wentland
> > >>> Acked-by: Tony Cheng
> > >>> Cc: dri-devel at lists.freedesktop.org
> > >>> Cc: Jani Nikula
> > >>> Cc: Daniel Vetter
> > >>> Cc: Ville Syrjala
> > >>> Signed-off-by: Manasi Navare
> > >>> ---
> > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > >>> 1 file changed, 7 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > >>> index 0b16587..2125fd1 100644
> > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >>> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct
> > >>> drm_atomic_state *state,
> > >>>connector_state);
> > >>> if (ret)
> > >>> return ret;
> > >>> +
> > >>> + if (connector->state->crtc) {
> > >>> + crtc_state =
> > >>> drm_atomic_get_existing_crtc_state(state,
> > >>> +
> > >>> connector->state->crtc);
> > >>> + if (connector->link_status ==
> > >>> DRM_MODE_LINK_STATUS_BAD)
> > >>> + crtc_state->connectors_changed = true;
> > >>> + }
> > >>> }
> > >>>
> > >>> /*
> > >> This will cause ordinary atomic commits that happen to change connector
> > >> flags to potentially fail with -EINVAL if ALLOW_MODESET is not set.
> > >> For this reason I'm not sure this flag should be set automatically by
> > >> the kernel. Could we add add a retrain link property instead, that
> > >> always return 0 when queried, but writing a 1 causing connectors_changed
> > >> to be set on bad link status?
> > > Or just check for allow_modeset before setting connectors_changed=true
> > > here?
> >
> > I don't think modesets should be done automatically like that, even if
> > ALLOW_MODESET is set a modeset may not be expected by userspace.
>
> Presumably userspace would want a picture on the screen using any means
> if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> probably say as much?
Yeah, agreed. Also, if the link is bad then we pretty much have to do a
modeset to recover it, otherwise you'll be forever stuck with a bad
screen.
What we could try is to gate this of whether userspace touches the mode
property on the corresponding CRTC. I.e. if that's touched (even if it's
the same mode), and a link is bad in one of the connectors in the state
then we do a full modeset to recover.
Another option would be to make the link status writeable. Trying to
change it from bad->good would force the modeset. That would be 100% clear
to userspace, not special hacks needed with checking for allow_modeset,
no magic property that auto-changes its value. And 100% backwards compat
because existing userspace should never touch properties it doesn't
understand (except when restoring a mode, and then it should allow a full
modeset). And if someone does try a good->bad transition, we just silently
keep it at good.
Definitely need to document this properly in the property docs, no matter
what we decide.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > >>> CRTC state connector_changed needs to be set to true
> > > >>> if connector link status property has changed. This will tell the
> > > >>> driver to do a complete modeset due to change in connector property.
> > > >>>
> > > >>> Acked-by: Harry Wentland
> > > >>> Acked-by: Tony Cheng
> > > >>> Cc: dri-devel at lists.freedesktop.org
> > > >>> Cc: Jani Nikula
> > > >>> Cc: Daniel Vetter
> > > >>> Cc: Ville Syrjala
> > > >>> Signed-off-by: Manasi Navare
> > > >>> ---
> > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > >>> 1 file changed, 7 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> index 0b16587..2125fd1 100644
> > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > >>> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct
> > > >>> drm_atomic_state *state,
> > > >>> connector_state);
> > > >>> if (ret)
> > > >>> return ret;
> > > >>> +
> > > >>> + if (connector->state->crtc) {
> > > >>> + crtc_state =
> > > >>> drm_atomic_get_existing_crtc_state(state,
> > > >>> +
> > > >>> connector->state->crtc);
> > > >>> + if (connector->link_status ==
> > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > >>> + crtc_state->connectors_changed = true;
> > > >>> + }
> > > >>> }
> > > >>>
> > > >>> /*
> > > >> This will cause ordinary atomic commits that happen to change
> > > >> connector flags to potentially fail with -EINVAL if ALLOW_MODESET is
> > > >> not set.
> > > >> For this reason I'm not sure this flag should be set automatically by
> > > >> the kernel. Could we add add a retrain link property instead, that
> > > >> always return 0 when queried, but writing a 1 causing
> > > >> connectors_changed to be set on bad link status?
> > > > Or just check for allow_modeset before setting connectors_changed=true
> > > > here?
> > >
> > > I don't think modesets should be done automatically like that, even if
> > > ALLOW_MODESET is set a modeset may not be expected by userspace.
> >
> > Presumably userspace would want a picture on the screen using any means
> > if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> > probably say as much?
>
> Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> modeset to recover it, otherwise you'll be forever stuck with a bad
> screen.
>
> What we could try is to gate this of whether userspace touches the mode
> property on the corresponding CRTC. I.e. if that's touched (even if it's
> the same mode), and a link is bad in one of the connectors in the state
> then we do a full modeset to recover.
>
> Another option would be to make the link status writeable. Trying to
> change it from bad->good would force the modeset. That would be 100% clear
> to userspace, not special hacks needed with checking for allow_modeset,
> no magic property that auto-changes its value. And 100% backwards compat
> because existing userspace should never touch properties it doesn't
> understand (except when restoring a mode, and then it should allow a full
> modeset). And if someone does try a good->bad transition, we just silently
> keep it at good.
>
> Definitely need to document this properly in the property docs, no matter
> what we decide.
> -Daniel
> --
Daniel, but this isnt solving the problem of drm_atomic_helper_check_modeset()
actually gating this modeset requested by userspace. This function checks for
the
connector->state in each crtc and sees if connector_changed and only if it is
then it will
set needs_modeset flag and do a full modeset.
How should I solve this problem?
I am not familiar with the state magic and so I am asking for helo here since I
am completely
clueless as to how to propogate this link-status property change to this helper
function and
somehow indicate in the connector state that it changed so driver will do a
full modeset.
Even if I add a new connector->state variable like link_status_changed, where
can this be set?
Manasi
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/5] drm/i915: Update CRTC state if connector link status property changed
On Fri, Nov 18, 2016 at 06:21:21PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 18, 2016 at 04:35:25PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 18, 2016 at 05:28:54PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 18, 2016 at 03:18:06PM +0100, Maarten Lankhorst wrote:
> > > > Op 18-11-16 om 15:11 schreef Ville Syrjälä:
> > > > > On Fri, Nov 18, 2016 at 02:50:52PM +0100, Maarten Lankhorst wrote:
> > > > >> Op 18-11-16 om 08:13 schreef Manasi Navare:
> > > > >>> CRTC state connector_changed needs to be set to true
> > > > >>> if connector link status property has changed. This will tell the
> > > > >>> driver to do a complete modeset due to change in connector property.
> > > > >>>
> > > > >>> Acked-by: Harry Wentland
> > > > >>> Acked-by: Tony Cheng
> > > > >>> Cc: dri-devel at lists.freedesktop.org
> > > > >>> Cc: Jani Nikula
> > > > >>> Cc: Daniel Vetter
> > > > >>> Cc: Ville Syrjala
> > > > >>> Signed-off-by: Manasi Navare
> > > > >>> ---
> > > > >>> drivers/gpu/drm/drm_atomic_helper.c | 7 +++
> > > > >>> 1 file changed, 7 insertions(+)
> > > > >>>
> > > > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > >>> b/drivers/gpu/drm/drm_atomic_helper.c
> > > > >>> index 0b16587..2125fd1 100644
> > > > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > >>> @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct
> > > > >>> drm_atomic_state *state,
> > > > >>>connector_state);
> > > > >>> if (ret)
> > > > >>> return ret;
> > > > >>> +
> > > > >>> + if (connector->state->crtc) {
> > > > >>> + crtc_state =
> > > > >>> drm_atomic_get_existing_crtc_state(state,
> > > > >>> +
> > > > >>> connector->state->crtc);
> > > > >>> + if (connector->link_status ==
> > > > >>> DRM_MODE_LINK_STATUS_BAD)
> > > > >>> + crtc_state->connectors_changed = true;
> > > > >>> + }
> > > > >>> }
> > > > >>>
> > > > >>> /*
> > > > >> This will cause ordinary atomic commits that happen to change
> > > > >> connector flags to potentially fail with -EINVAL if ALLOW_MODESET is
> > > > >> not set.
> > > > >> For this reason I'm not sure this flag should be set automatically
> > > > >> by the kernel. Could we add add a retrain link property instead, that
> > > > >> always return 0 when queried, but writing a 1 causing
> > > > >> connectors_changed to be set on bad link status?
> > > > > Or just check for allow_modeset before setting
> > > > > connectors_changed=true here?
> > > >
> > > > I don't think modesets should be done automatically like that, even if
> > > > ALLOW_MODESET is set a modeset may not be expected by userspace.
> > >
> > > Presumably userspace would want a picture on the screen using any means
> > > if it said ALLOW_MODESET. So if it can't tolerate the modeset it should
> > > probably say as much?
> >
> > Yeah, agreed. Also, if the link is bad then we pretty much have to do a
> > modeset to recover it, otherwise you'll be forever stuck with a bad
> > screen.
> >
> > What we could try is to gate this of whether userspace touches the mode
> > property on the corresponding CRTC. I.e. if that's touched (even if it's
> > the same mode), and a link is bad in one of the connectors in the state
> > then we do a full modeset to recover.
> >
> > Another option would be to make the link status writeable. Trying to
> > change it from bad->good would force the modeset. That would be 100% clear
> > to userspace, not special hacks needed with checking for allow_modeset,
> > no magic property that auto-changes its value. And 100% backwards compat
> > because existing userspace should never touch properties it doesn't
> > understand (except when restoring a mode, and then it should allow a full
> > modeset). And if someone does try a good->bad transition, we just silently
> > keep it at good.
> >
> > Definitely need to document this properly in the property docs, no matter
> > what we decide.
>
> Hmm. I think I kinda like this idea of userspace clear the state back
> to good explicitly, if it happens with the same prop. So it's like
> Maarten's retrain_link prop idea, but without having to add the second
> prop to the mix.
>
> It would also save me from pointing out (for the nth time) that the
> link status should really be cleared to good during the commit state
> swap and not at some random point during the commit ;)
>
Okay, so change 1 is to make the userspace clear the state back to Good for the
property..
Then Change 2 is to set connector_changed flag in crtc_state to true if this
property changed
from BAD to GOOD. I am not quite how and where to change this to state
connector_changed to true.
Without this the full modeset will not happen and the whole design of
retrianing is lost.
Manasi
>
