[PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Manasi Navare]

This function provides a way for the driver to redo a
modeset on the current mode and retry the link training
at a lower link rate/lane count/bpp. This will get called
incase the link training fails during the current modeset.

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  1 +
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f936276..0c1614e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
 
 /**
+ * drm_atomic_helper_connector_modeset - Force a modeset on a connector
+ * @connector: DRM connector
+ *
+ * Provides a way to redo a modeset with the current mode so that it can
+ * drop the bpp, link rate/lane count and retry the link training.
+ *
+ * Returns:
+ * Returns 0 on success, negative errno numbers on failure.
+ */
+int
+drm_atomic_helper_connector_modeset(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	struct drm_connector_state *connector_state;
+	struct drm_crtc_state *crtc_state;
+	int ret = 0;
+
+	drm_modeset_acquire_init(&ctx, 0);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+	state->acquire_ctx = &ctx;
+retry:
+	ret = 0;
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state)) {
+		ret = PTR_ERR(connector_state);
+		goto fail;
+	}
+	if (!connector_state->crtc)
+		goto fail;
+
+	crtc_state = drm_atomic_get_existing_crtc_state(state,
+							connector_state->crtc);
+	crtc_state->connectors_changed = true;
+	ret = drm_atomic_commit(state);
+fail:
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	if (state)
+		drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_modeset);
+
+/**
  * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
  *                                  ->best_encoder callback
  * @connector: Connector control structure
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b0..8de24dc 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				uint32_t flags);
 int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
 				     int mode);
+int drm_atomic_helper_connector_modeset(struct drm_connector *connector);
 struct drm_encoder *
 drm_atomic_helper_best_encoder(struct drm_connector *connector);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Manasi Navare]

This work struct will be used to schedule a uevent on a separate
thread. This will be scheduled after a link train failure during modeset
to indicate a modeset retry request. It will get executed after the
current modeset is complete and all locks are released. This was
required to avoid deadlock.

Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 include/drm/drm_connector.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ac9d7d8..fcf6b97 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -682,6 +682,11 @@ struct drm_connector {
 	uint8_t num_h_tile, num_v_tile;
 	uint8_t tile_h_loc, tile_v_loc;
 	uint16_t tile_h_size, tile_v_size;
+
+	/* Work struct to schedule a uevent on link train failure for
+	 * DisplayPort.
+	 */
+	struct work_struct i915_modeset_retry_work;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> This function provides a way for the driver to redo a
> modeset on the current mode and retry the link training
> at a lower link rate/lane count/bpp. This will get called
> incase the link training fails during the current modeset.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  1 +
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f936276..0c1614e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>  
>  /**
> + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> + * @connector: DRM connector
> + *
> + * Provides a way to redo a modeset with the current mode so that it can
> + * drop the bpp, link rate/lane count and retry the link training.
> + *
> + * Returns:
> + * Returns 0 on success, negative errno numbers on failure.
> + */
> +int
> +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector_state *connector_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +	state->acquire_ctx = &ctx;
> +retry:
> +	ret = 0;
> +	connector_state = drm_atomic_get_connector_state(state, connector);
> +	if (IS_ERR(connector_state)) {
> +		ret = PTR_ERR(connector_state);
> +		goto fail;
> +	}
> +	if (!connector_state->crtc)
> +		goto fail;
> +
> +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> +							connector_state->crtc);
> +	crtc_state->connectors_changed = true;

This line here only works if the driver uses the helpers for checking and
commit, and when it doesn't overwrite connectors_changed. I think the
kerneldoc should mention this.

The other issue: You cannot call this from modeset code itself because of
recursion. I think this also must be mentioned.
-Daniel

> +	ret = drm_atomic_commit(state);
> +fail:
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	if (state)
> +		drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset);
> +
> +/**
>   * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
>   *                                  ->best_encoder callback
>   * @connector: Connector control structure
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b0..8de24dc 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				uint32_t flags);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
> +int drm_atomic_helper_connector_modeset(struct drm_connector *connector);
>  struct drm_encoder *
>  drm_atomic_helper_best_encoder(struct drm_connector *connector);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Daniel Vetter]

On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote:
> This work struct will be used to schedule a uevent on a separate
> thread. This will be scheduled after a link train failure during modeset
> to indicate a modeset retry request. It will get executed after the
> current modeset is complete and all locks are released. This was
> required to avoid deadlock.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  include/drm/drm_connector.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ac9d7d8..fcf6b97 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -682,6 +682,11 @@ struct drm_connector {
>  	uint8_t num_h_tile, num_v_tile;
>  	uint8_t tile_h_loc, tile_v_loc;
>  	uint16_t tile_h_size, tile_v_size;
> +
> +	/* Work struct to schedule a uevent on link train failure for
> +	 * DisplayPort.
> +	 */
> +	struct work_struct i915_modeset_retry_work;

You cannot put i915 stuff into core structs.
-Daniel

>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > This function provides a way for the driver to redo a
> > modeset on the current mode and retry the link training
> > at a lower link rate/lane count/bpp. This will get called
> > incase the link training fails during the current modeset.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  1 +
> >  2 files changed, 59 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f936276..0c1614e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> >  
> >  /**
> > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > + * @connector: DRM connector
> > + *
> > + * Provides a way to redo a modeset with the current mode so that it can
> > + * drop the bpp, link rate/lane count and retry the link training.
> > + *
> > + * Returns:
> > + * Returns 0 on success, negative errno numbers on failure.
> > + */
> > +int
> > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto fail;
> > +	}
> > +	state->acquire_ctx = &ctx;
> > +retry:
> > +	ret = 0;
> > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > +	if (IS_ERR(connector_state)) {
> > +		ret = PTR_ERR(connector_state);
> > +		goto fail;
> > +	}
> > +	if (!connector_state->crtc)
> > +		goto fail;
> > +
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > +							connector_state->crtc);
> > +	crtc_state->connectors_changed = true;
> 
> This line here only works if the driver uses the helpers for checking and
> commit, and when it doesn't overwrite connectors_changed. I think the
> kerneldoc should mention this.
> 
> The other issue: You cannot call this from modeset code itself because of
> recursion. I think this also must be mentioned.

And if this indeed does a modeset then we have again the same issue as
with upfront link training when the pipe is supposed to be on: Userspace
can observe the kernel playing tricks because the vblank support will be
temporarily disabled.

Not sure how to best fix this, but might be good to grab the crtc->mutex
in the vblank code for stuff like this.
-Daniel
-- 
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

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Ville Syrjälä]

On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > This function provides a way for the driver to redo a
> > > modeset on the current mode and retry the link training
> > > at a lower link rate/lane count/bpp. This will get called
> > > incase the link training fails during the current modeset.
> > > 
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_atomic_helper.h     |  1 +
> > >  2 files changed, 59 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index f936276..0c1614e 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > >  
> > >  /**
> > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > + * @connector: DRM connector
> > > + *
> > > + * Provides a way to redo a modeset with the current mode so that it can
> > > + * drop the bpp, link rate/lane count and retry the link training.
> > > + *
> > > + * Returns:
> > > + * Returns 0 on success, negative errno numbers on failure.
> > > + */
> > > +int
> > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > +{
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_atomic_state *state;
> > > +	struct drm_connector_state *connector_state;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	int ret = 0;
> > > +
> > > +	drm_modeset_acquire_init(&ctx, 0);
> > > +	state = drm_atomic_state_alloc(dev);
> > > +	if (!state) {
> > > +		ret = -ENOMEM;
> > > +		goto fail;
> > > +	}
> > > +	state->acquire_ctx = &ctx;
> > > +retry:
> > > +	ret = 0;
> > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > +	if (IS_ERR(connector_state)) {
> > > +		ret = PTR_ERR(connector_state);
> > > +		goto fail;
> > > +	}
> > > +	if (!connector_state->crtc)
> > > +		goto fail;
> > > +
> > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > +							connector_state->crtc);
> > > +	crtc_state->connectors_changed = true;
> > 
> > This line here only works if the driver uses the helpers for checking and
> > commit, and when it doesn't overwrite connectors_changed. I think the
> > kerneldoc should mention this.
> > 
> > The other issue: You cannot call this from modeset code itself because of
> > recursion. I think this also must be mentioned.
> 
> And if this indeed does a modeset then we have again the same issue as
> with upfront link training when the pipe is supposed to be on: Userspace
> can observe the kernel playing tricks because the vblank support will be
> temporarily disabled.
> 
> Not sure how to best fix this, but might be good to grab the crtc->mutex
> in the vblank code for stuff like this.

We're going to have the same problem with async modeset as well.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote:
> On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > > This function provides a way for the driver to redo a
> > > > modeset on the current mode and retry the link training
> > > > at a lower link rate/lane count/bpp. This will get called
> > > > incase the link training fails during the current modeset.
> > > > 
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > >  2 files changed, 59 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index f936276..0c1614e 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > > >  
> > > >  /**
> > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > > + * @connector: DRM connector
> > > > + *
> > > > + * Provides a way to redo a modeset with the current mode so that it can
> > > > + * drop the bpp, link rate/lane count and retry the link training.
> > > > + *
> > > > + * Returns:
> > > > + * Returns 0 on success, negative errno numbers on failure.
> > > > + */
> > > > +int
> > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > > +{
> > > > +	struct drm_device *dev = connector->dev;
> > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > +	struct drm_atomic_state *state;
> > > > +	struct drm_connector_state *connector_state;
> > > > +	struct drm_crtc_state *crtc_state;
> > > > +	int ret = 0;
> > > > +
> > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > +	state = drm_atomic_state_alloc(dev);
> > > > +	if (!state) {
> > > > +		ret = -ENOMEM;
> > > > +		goto fail;
> > > > +	}
> > > > +	state->acquire_ctx = &ctx;
> > > > +retry:
> > > > +	ret = 0;
> > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > +	if (IS_ERR(connector_state)) {
> > > > +		ret = PTR_ERR(connector_state);
> > > > +		goto fail;
> > > > +	}
> > > > +	if (!connector_state->crtc)
> > > > +		goto fail;
> > > > +
> > > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > > +							connector_state->crtc);
> > > > +	crtc_state->connectors_changed = true;
> > > 
> > > This line here only works if the driver uses the helpers for checking and
> > > commit, and when it doesn't overwrite connectors_changed. I think the
> > > kerneldoc should mention this.
> > > 
> > > The other issue: You cannot call this from modeset code itself because of
> > > recursion. I think this also must be mentioned.
> > 
> > And if this indeed does a modeset then we have again the same issue as
> > with upfront link training when the pipe is supposed to be on: Userspace
> > can observe the kernel playing tricks because the vblank support will be
> > temporarily disabled.
> > 
> > Not sure how to best fix this, but might be good to grab the crtc->mutex
> > in the vblank code for stuff like this.
> 
> We're going to have the same problem with async modeset as well.

Async modeset is ok because userspace expects that the pipe will go
off/on, and the kernel will send out an event to signal when the pipe is
guaranteed to be on and can be started to be used. It's random modesets
afterwards I'm concerned about.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Manasi Navare]

On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote:
> On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote:
> > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > > > This function provides a way for the driver to redo a
> > > > > modeset on the current mode and retry the link training
> > > > > at a lower link rate/lane count/bpp. This will get called
> > > > > incase the link training fails during the current modeset.
> > > > > 
> > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > > >  2 files changed, 59 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index f936276..0c1614e 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > > > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > > > >  
> > > > >  /**
> > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > > > + * @connector: DRM connector
> > > > > + *
> > > > > + * Provides a way to redo a modeset with the current mode so that it can
> > > > > + * drop the bpp, link rate/lane count and retry the link training.
> > > > > + *
> > > > > + * Returns:
> > > > > + * Returns 0 on success, negative errno numbers on failure.
> > > > > + */
> > > > > +int
> > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > > > +{
> > > > > +	struct drm_device *dev = connector->dev;
> > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > +	struct drm_atomic_state *state;
> > > > > +	struct drm_connector_state *connector_state;
> > > > > +	struct drm_crtc_state *crtc_state;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > > +	state = drm_atomic_state_alloc(dev);
> > > > > +	if (!state) {
> > > > > +		ret = -ENOMEM;
> > > > > +		goto fail;
> > > > > +	}
> > > > > +	state->acquire_ctx = &ctx;
> > > > > +retry:
> > > > > +	ret = 0;
> > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > +	if (IS_ERR(connector_state)) {
> > > > > +		ret = PTR_ERR(connector_state);
> > > > > +		goto fail;
> > > > > +	}
> > > > > +	if (!connector_state->crtc)
> > > > > +		goto fail;
> > > > > +
> > > > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > > > +							connector_state->crtc);
> > > > > +	crtc_state->connectors_changed = true;
> > > > 
> > > > This line here only works if the driver uses the helpers for checking and
> > > > commit, and when it doesn't overwrite connectors_changed. I think the
> > > > kerneldoc should mention this.
> > > > 
> > > > The other issue: You cannot call this from modeset code itself because of
> > > > recursion. I think this also must be mentioned.

I will add this to the kernel doc.
In our case, we call this in a work func that will get scheduled after
the current modeset is completed.

> > > 
> > > And if this indeed does a modeset then we have again the same issue as
> > > with upfront link training when the pipe is supposed to be on: Userspace
> > > can observe the kernel playing tricks because the vblank support will be
> > > temporarily disabled.
> > > 
> > > Not sure how to best fix this, but might be good to grab the crtc->mutex
> > > in the vblank code for stuff like this.
> > 
> > We're going to have the same problem with async modeset as well.
> 
> Async modeset is ok because userspace expects that the pipe will go
> off/on, and the kernel will send out an event to signal when the pipe is
> guaranteed to be on and can be started to be used. It's random modesets
> afterwards I'm concerned about.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

These wont be random modesets, this will occur only in case of link training
failure in which case if the mode has not changed/pruned, it will attempt
modeset at lower link rate.

Manasi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Sun, Oct 23, 2016 at 11:12:31PM -0700, Manasi Navare wrote:
> On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote:
> > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote:
> > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > > > > This function provides a way for the driver to redo a
> > > > > > modeset on the current mode and retry the link training
> > > > > > at a lower link rate/lane count/bpp. This will get called
> > > > > > incase the link training fails during the current modeset.
> > > > > > 
> > > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > > > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > > > >  2 files changed, 59 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > index f936276..0c1614e 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > > > > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > > > > >  
> > > > > >  /**
> > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > > > > + * @connector: DRM connector
> > > > > > + *
> > > > > > + * Provides a way to redo a modeset with the current mode so that it can
> > > > > > + * drop the bpp, link rate/lane count and retry the link training.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * Returns 0 on success, negative errno numbers on failure.
> > > > > > + */
> > > > > > +int
> > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > > > > +{
> > > > > > +	struct drm_device *dev = connector->dev;
> > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > +	struct drm_atomic_state *state;
> > > > > > +	struct drm_connector_state *connector_state;
> > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > > > +	state = drm_atomic_state_alloc(dev);
> > > > > > +	if (!state) {
> > > > > > +		ret = -ENOMEM;
> > > > > > +		goto fail;
> > > > > > +	}
> > > > > > +	state->acquire_ctx = &ctx;
> > > > > > +retry:
> > > > > > +	ret = 0;
> > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > +		ret = PTR_ERR(connector_state);
> > > > > > +		goto fail;
> > > > > > +	}
> > > > > > +	if (!connector_state->crtc)
> > > > > > +		goto fail;
> > > > > > +
> > > > > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > > > > +							connector_state->crtc);
> > > > > > +	crtc_state->connectors_changed = true;
> > > > > 
> > > > > This line here only works if the driver uses the helpers for checking and
> > > > > commit, and when it doesn't overwrite connectors_changed. I think the
> > > > > kerneldoc should mention this.
> > > > > 
> > > > > The other issue: You cannot call this from modeset code itself because of
> > > > > recursion. I think this also must be mentioned.
> 
> I will add this to the kernel doc.
> In our case, we call this in a work func that will get scheduled after
> the current modeset is completed.
> 
> > > > 
> > > > And if this indeed does a modeset then we have again the same issue as
> > > > with upfront link training when the pipe is supposed to be on: Userspace
> > > > can observe the kernel playing tricks because the vblank support will be
> > > > temporarily disabled.
> > > > 
> > > > Not sure how to best fix this, but might be good to grab the crtc->mutex
> > > > in the vblank code for stuff like this.
> > > 
> > > We're going to have the same problem with async modeset as well.
> > 
> > Async modeset is ok because userspace expects that the pipe will go
> > off/on, and the kernel will send out an event to signal when the pipe is
> > guaranteed to be on and can be started to be used. It's random modesets
> > afterwards I'm concerned about.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> These wont be random modesets, this will occur only in case of link training
> failure in which case if the mode has not changed/pruned, it will attempt
> modeset at lower link rate.

"Cat ate the cable" is very much random from userspace's pov. It's not
random from the kernel's pov, because the kernel is aware of what's going
on - it just tried to (re)train the link. But userspace has no idea at
all. Note that link training failures can not just happen right after a
modeset, but also:
- anytime the sink feels like the link is bad and asks us to retrain (yay,
  same issue there with having to stop the pipe).
- system suspend/resume might also result in link train fail (the screen
  might not even be there any more), but the link should still keep
  running.

I guess we just need to do some additional work on top to make sure the
vblank ioctl can't see invalid state. Which would then again make upfront
link trainig possible ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Manasi Navare]

On Mon, Oct 24, 2016 at 08:33:10AM +0200, Daniel Vetter wrote:
> On Sun, Oct 23, 2016 at 11:12:31PM -0700, Manasi Navare wrote:
> > On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote:
> > > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote:
> > > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote:
> > > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote:
> > > > > > > This function provides a way for the driver to redo a
> > > > > > > modeset on the current mode and retry the link training
> > > > > > > at a lower link rate/lane count/bpp. This will get called
> > > > > > > incase the link training fails during the current modeset.
> > > > > > > 
> > > > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> > > > > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > > > > >  2 files changed, 59 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > index f936276..0c1614e 100644
> > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> > > > > > >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> > > > > > >  
> > > > > > >  /**
> > > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > > > > > > + * @connector: DRM connector
> > > > > > > + *
> > > > > > > + * Provides a way to redo a modeset with the current mode so that it can
> > > > > > > + * drop the bpp, link rate/lane count and retry the link training.
> > > > > > > + *
> > > > > > > + * Returns:
> > > > > > > + * Returns 0 on success, negative errno numbers on failure.
> > > > > > > + */
> > > > > > > +int
> > > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > > > > > > +{
> > > > > > > +	struct drm_device *dev = connector->dev;
> > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > +	struct drm_atomic_state *state;
> > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > > > +	int ret = 0;
> > > > > > > +
> > > > > > > +	drm_modeset_acquire_init(&ctx, 0);
> > > > > > > +	state = drm_atomic_state_alloc(dev);
> > > > > > > +	if (!state) {
> > > > > > > +		ret = -ENOMEM;
> > > > > > > +		goto fail;
> > > > > > > +	}
> > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > +retry:
> > > > > > > +	ret = 0;
> > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > +		ret = PTR_ERR(connector_state);
> > > > > > > +		goto fail;
> > > > > > > +	}
> > > > > > > +	if (!connector_state->crtc)
> > > > > > > +		goto fail;
> > > > > > > +
> > > > > > > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > > > > > +							connector_state->crtc);
> > > > > > > +	crtc_state->connectors_changed = true;
> > > > > > 
> > > > > > This line here only works if the driver uses the helpers for checking and
> > > > > > commit, and when it doesn't overwrite connectors_changed. I think the
> > > > > > kerneldoc should mention this.
> > > > > > 
> > > > > > The other issue: You cannot call this from modeset code itself because of
> > > > > > recursion. I think this also must be mentioned.
> > 
> > I will add this to the kernel doc.
> > In our case, we call this in a work func that will get scheduled after
> > the current modeset is completed.
> > 
> > > > > 
> > > > > And if this indeed does a modeset then we have again the same issue as
> > > > > with upfront link training when the pipe is supposed to be on: Userspace
> > > > > can observe the kernel playing tricks because the vblank support will be
> > > > > temporarily disabled.
> > > > > 
> > > > > Not sure how to best fix this, but might be good to grab the crtc->mutex
> > > > > in the vblank code for stuff like this.
> > > > 
> > > > We're going to have the same problem with async modeset as well.
> > > 
> > > Async modeset is ok because userspace expects that the pipe will go
> > > off/on, and the kernel will send out an event to signal when the pipe is
> > > guaranteed to be on and can be started to be used. It's random modesets
> > > afterwards I'm concerned about.
> > > -Daniel
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > These wont be random modesets, this will occur only in case of link training
> > failure in which case if the mode has not changed/pruned, it will attempt
> > modeset at lower link rate.
> 
> "Cat ate the cable" is very much random from userspace's pov. It's not
> random from the kernel's pov, because the kernel is aware of what's going
> on - it just tried to (re)train the link. But userspace has no idea at
> all. Note that link training failures can not just happen right after a
> modeset, but also:
> - anytime the sink feels like the link is bad and asks us to retrain (yay,
>   same issue there with having to stop the pipe).
> - system suspend/resume might also result in link train fail (the screen
>   might not even be there any more), but the link should still keep
>   running.
> 
> I guess we just need to do some additional work on top to make sure the
> vblank ioctl can't see invalid state. Which would then again make upfront
> link trainig possible ...
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Could you share some more details on what work would need to be done
for vblank? Then I can investigate it a little bit more tomor during the day.

Manasi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
<manasi.d.navare@intel.com> wrote:
>> I guess we just need to do some additional work on top to make sure the
>> vblank ioctl can't see invalid state. Which would then again make upfront
>> link trainig possible ...
>
> Could you share some more details on what work would need to be done
> for vblank? Then I can investigate it a little bit more tomor during the day.

So the trouble is that drm_irq.c is still from the old pre-kms days.
Which means it deals in int pipe instead of struct drm_crtc *, among
other things. But we can't simply switch over because some old drivers
(pre-kms ones) still depend upon that old code. Hence we need:

1. Duplicate most of the code in drm_irq.c into a new
drm_crtc_vblank.c, which is only used for kms drivers. And in the
ioctl code have a DRIVER_MODESET check and either call the new kms
version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c).

2. Convert the int pipe into a drm_crtc pointer in the new kms code.
For prettiness we could (try) to do that as much as possible.

3. Grab the drm_crtc->mutex in the ioctl code to block these races.

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Sean Paul]

On Mon, Oct 24, 2016 at 3:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
> <manasi.d.navare@intel.com> wrote:
>>> I guess we just need to do some additional work on top to make sure the
>>> vblank ioctl can't see invalid state. Which would then again make upfront
>>> link trainig possible ...
>>
>> Could you share some more details on what work would need to be done
>> for vblank? Then I can investigate it a little bit more tomor during the day.
>
> So the trouble is that drm_irq.c is still from the old pre-kms days.
> Which means it deals in int pipe instead of struct drm_crtc *, among
> other things. But we can't simply switch over because some old drivers
> (pre-kms ones) still depend upon that old code. Hence we need:
>
> 1. Duplicate most of the code in drm_irq.c into a new
> drm_crtc_vblank.c, which is only used for kms drivers.

Why duplicate all of this code? That seems a bit wasteful.

Can't we just add the inverse of drm_crtc_pipe() (coincidentally, I
also suggested we introduce this function in "drm: zte: add initial
vou drm driver"), and use those to map between pipe and crtc where
appropriate? The crtc locks could be acquired/dropped based on
DRIVER_MODESET in drm_irq.c

Unrelated to the vblank discussion: This functionality is pretty
similar to what happens on suspend/resume. Any chance we can share?

Sean

> And in the
> ioctl code have a DRIVER_MODESET check and either call the new kms
> version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c).
>
> 2. Convert the int pipe into a drm_crtc pointer in the new kms code.
> For prettiness we could (try) to do that as much as possible.
>
> 3. Grab the drm_crtc->mutex in the ioctl code to block these races.
>
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Manasi Navare]

On Mon, Oct 24, 2016 at 09:12:35AM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
> <manasi.d.navare@intel.com> wrote:
> >> I guess we just need to do some additional work on top to make sure the
> >> vblank ioctl can't see invalid state. Which would then again make upfront
> >> link trainig possible ...

Thanks for the explanation. So the atomic_commit code in the driver
calls drm_crtc_send_vblank_event(crtc, crtc->state->event);, is this
what needs to be filtered to be seen by Vblank IOCTL?


> >
> > Could you share some more details on what work would need to be done
> > for vblank? Then I can investigate it a little bit more tomor during the day.
> 
> So the trouble is that drm_irq.c is still from the old pre-kms days.
> Which means it deals in int pipe instead of struct drm_crtc *, among
> other things. But we can't simply switch over because some old drivers
> (pre-kms ones) still depend upon that old code. Hence we need:
> 
> 1. Duplicate most of the code in drm_irq.c into a new
> drm_crtc_vblank.c, which is only used for kms drivers. And in the
> ioctl code have a DRIVER_MODESET check and either call the new kms
> version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c).

What functions need to be duplicated?

Which IOCTL are we talking about here?  Do you mean in the atomic_commit_tail
where we check for driver modeset, is where we should then call the new kms version
of drm_crtc_send_vblank_event()?
> 
> 2. Convert the int pipe into a drm_crtc pointer in the new kms code.
> For prettiness we could (try) to do that as much as possible.
> 
> 3. Grab the drm_crtc->mutex in the ioctl code to block these races.

Again which IOCTL needs to grab the drm_crtc->mutex?

What is the end goal of thsi task, what race conditions are we trying to avoid
in case of these modesets during link training failures?

Manasi

> 
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Manasi Navare]

On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote:
> On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote:
> > This work struct will be used to schedule a uevent on a separate
> > thread. This will be scheduled after a link train failure during modeset
> > to indicate a modeset retry request. It will get executed after the
> > current modeset is complete and all locks are released. This was
> > required to avoid deadlock.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > 
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  include/drm/drm_connector.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index ac9d7d8..fcf6b97 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -682,6 +682,11 @@ struct drm_connector {
> >  	uint8_t num_h_tile, num_v_tile;
> >  	uint8_t tile_h_loc, tile_v_loc;
> >  	uint16_t tile_h_size, tile_v_size;
> > +
> > +	/* Work struct to schedule a uevent on link train failure for
> > +	 * DisplayPort.
> > +	 */
> > +	struct work_struct i915_modeset_retry_work;
> 
> You cannot put i915 stuff into core structs.
> -Daniel
> 
> >  };

I will rename it to use modeset_retry_work instead.

Manasi
> >  
> >  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Pandiyan, Dhinakaran]

On Mon, 2016-10-24 at 23:28 -0700, Manasi Navare wrote:
> On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote:
> > > This work struct will be used to schedule a uevent on a separate
> > > thread. This will be scheduled after a link train failure during modeset
> > > to indicate a modeset retry request. It will get executed after the
> > > current modeset is complete and all locks are released. This was
> > > required to avoid deadlock.
> > > 
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > 
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  include/drm/drm_connector.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index ac9d7d8..fcf6b97 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -682,6 +682,11 @@ struct drm_connector {
> > >  	uint8_t num_h_tile, num_v_tile;
> > >  	uint8_t tile_h_loc, tile_v_loc;
> > >  	uint16_t tile_h_size, tile_v_size;
> > > +
> > > +	/* Work struct to schedule a uevent on link train failure for
> > > +	 * DisplayPort.
> > > +	 */
> > > +	struct work_struct i915_modeset_retry_work;
> > 
> > You cannot put i915 stuff into core structs.
> > -Daniel
> > 
> > >  };
> 
> I will rename it to use modeset_retry_work instead.
> 
> Manasi


Why not move it struct intel_connector? The work function is defined in
intel_dp.c afaict

-DK
> > >  
> > >  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Mon, Oct 24, 2016 at 02:38:17PM -0400, Sean Paul wrote:
> On Mon, Oct 24, 2016 at 3:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
> > <manasi.d.navare@intel.com> wrote:
> >>> I guess we just need to do some additional work on top to make sure the
> >>> vblank ioctl can't see invalid state. Which would then again make upfront
> >>> link trainig possible ...
> >>
> >> Could you share some more details on what work would need to be done
> >> for vblank? Then I can investigate it a little bit more tomor during the day.
> >
> > So the trouble is that drm_irq.c is still from the old pre-kms days.
> > Which means it deals in int pipe instead of struct drm_crtc *, among
> > other things. But we can't simply switch over because some old drivers
> > (pre-kms ones) still depend upon that old code. Hence we need:
> >
> > 1. Duplicate most of the code in drm_irq.c into a new
> > drm_crtc_vblank.c, which is only used for kms drivers.
> 
> Why duplicate all of this code? That seems a bit wasteful.
> 
> Can't we just add the inverse of drm_crtc_pipe() (coincidentally, I
> also suggested we introduce this function in "drm: zte: add initial
> vou drm driver"), and use those to map between pipe and crtc where
> appropriate? The crtc locks could be acquired/dropped based on
> DRIVER_MODESET in drm_irq.c

Only if you have a DRIVER_MODESET. For DRIVER_LEGACY this code all still
needs to work, without there ever being a drm_crtc. If you try to squeeze
kms and non-kms paths into one function that will be seriously ugly, and a
complete maintenance pain. Copypasting is imo the sound approach here.
That way we can let the old non-kms code rot unchanged until it's really
dead.

> Unrelated to the vblank discussion: This functionality is pretty
> similar to what happens on suspend/resume. Any chance we can share?

Hm, what do you mean?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Daniel Vetter]

On Mon, Oct 24, 2016 at 03:08:48PM -0700, Manasi Navare wrote:
> On Mon, Oct 24, 2016 at 09:12:35AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
> > <manasi.d.navare@intel.com> wrote:
> > >> I guess we just need to do some additional work on top to make sure the
> > >> vblank ioctl can't see invalid state. Which would then again make upfront
> > >> link trainig possible ...
> 
> Thanks for the explanation. So the atomic_commit code in the driver
> calls drm_crtc_send_vblank_event(crtc, crtc->state->event);, is this
> what needs to be filtered to be seen by Vblank IOCTL?
> 
> 
> > >
> > > Could you share some more details on what work would need to be done
> > > for vblank? Then I can investigate it a little bit more tomor during the day.
> > 
> > So the trouble is that drm_irq.c is still from the old pre-kms days.
> > Which means it deals in int pipe instead of struct drm_crtc *, among
> > other things. But we can't simply switch over because some old drivers
> > (pre-kms ones) still depend upon that old code. Hence we need:
> > 
> > 1. Duplicate most of the code in drm_irq.c into a new
> > drm_crtc_vblank.c, which is only used for kms drivers. And in the
> > ioctl code have a DRIVER_MODESET check and either call the new kms
> > version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c).
> 
> What functions need to be duplicated?
 
Figuring out the exact list is way this is painful. Since we don't need
everything, but just enough to make the new drm_crtc_vblank.c work. And
then as a second step we probably should move all the functions starting
with drm_crtc_vblank_ which are exported to drivers over to that file too.

> Which IOCTL are we talking about here?  Do you mean in the atomic_commit_tail
> where we check for driver modeset, is where we should then call the new kms version
> of drm_crtc_send_vblank_event()?
> > 
> > 2. Convert the int pipe into a drm_crtc pointer in the new kms code.
> > For prettiness we could (try) to do that as much as possible.
> > 
> > 3. Grab the drm_crtc->mutex in the ioctl code to block these races.
> 
> Again which IOCTL needs to grab the drm_crtc->mutex?

The vblank ioctl in drm_irq.c, implemented by drm_wait_vblank.

> What is the end goal of thsi task, what race conditions are we trying to avoid
> in case of these modesets during link training failures?

Modeset code calls drm_crtc_vblank_on/off when doing a full modeset. That
changes the vblank status, which can be observed through the vblank ioctl
by userspace: In between the calls to _off/_on it will return -EINVAL,
instead of the last vblank timestamp (for a query) or doing the vblank
wait (otherwise), which is what it should do.
-Daniel
-- 
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

Re: [Intel-gfx] [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry

[Daniel Vetter]

On Tue, Oct 25, 2016 at 06:30:29AM +0000, Pandiyan, Dhinakaran wrote:
> On Mon, 2016-10-24 at 23:28 -0700, Manasi Navare wrote:
> > On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote:
> > > > This work struct will be used to schedule a uevent on a separate
> > > > thread. This will be scheduled after a link train failure during modeset
> > > > to indicate a modeset retry request. It will get executed after the
> > > > current modeset is complete and all locks are released. This was
> > > > required to avoid deadlock.
> > > > 
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > 
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  include/drm/drm_connector.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index ac9d7d8..fcf6b97 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -682,6 +682,11 @@ struct drm_connector {
> > > >  	uint8_t num_h_tile, num_v_tile;
> > > >  	uint8_t tile_h_loc, tile_v_loc;
> > > >  	uint16_t tile_h_size, tile_v_size;
> > > > +
> > > > +	/* Work struct to schedule a uevent on link train failure for
> > > > +	 * DisplayPort.
> > > > +	 */
> > > > +	struct work_struct i915_modeset_retry_work;
> > > 
> > > You cannot put i915 stuff into core structs.
> > > -Daniel
> > > 
> > > >  };
> > 
> > I will rename it to use modeset_retry_work instead.
> > 
> > Manasi
> 
> 
> Why not move it struct intel_connector? The work function is defined in
> intel_dp.c afaict

Yeah, the place is wrong, not just the name.
-Daniel
> 
> -DK
> > > >  
> > > >  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > > > -- 
> > > > 1.9.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
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

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Jani Nikula]

On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> This function provides a way for the driver to redo a
> modeset on the current mode and retry the link training
> at a lower link rate/lane count/bpp. This will get called
> incase the link training fails during the current modeset.

Based on discussions on #intel-gfx, I would dodge all the problems here
by having the userspace do the modeset.

If we add a connector property to indicate the link status (and this
does not have to be DP or link training specific, really), we can set
that to "bad", and fire off the hotplug uevent. Then userspace has the
information to force a modeset on that connector even if the mode has
not changed. (Credits to Ville for the idea.)

If we find a solution later on that allows us to handle all of this in
kernel, we can do so, and remove the property or always report
"good". Drivers can choose different approaches, depending on the
capabilities of the hardware, for instance.

Deferring this to the userspace does not regress anything now, because
currently we just completely fail and end up with a black screen. The
property would allow an enlightened userspace to fix that. And userspace
can't rely on the property being there, as it's currently not there.


BR,
Jani.


>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  1 +
>  2 files changed, 59 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f936276..0c1614e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>  
>  /**
> + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> + * @connector: DRM connector
> + *
> + * Provides a way to redo a modeset with the current mode so that it can
> + * drop the bpp, link rate/lane count and retry the link training.
> + *
> + * Returns:
> + * Returns 0 on success, negative errno numbers on failure.
> + */
> +int
> +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector_state *connector_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +	state->acquire_ctx = &ctx;
> +retry:
> +	ret = 0;
> +	connector_state = drm_atomic_get_connector_state(state, connector);
> +	if (IS_ERR(connector_state)) {
> +		ret = PTR_ERR(connector_state);
> +		goto fail;
> +	}
> +	if (!connector_state->crtc)
> +		goto fail;
> +
> +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> +							connector_state->crtc);
> +	crtc_state->connectors_changed = true;
> +	ret = drm_atomic_commit(state);
> +fail:
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	if (state)
> +		drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset);
> +
> +/**
>   * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
>   *                                  ->best_encoder callback
>   * @connector: Connector control structure
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b0..8de24dc 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				uint32_t flags);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
> +int drm_atomic_helper_connector_modeset(struct drm_connector *connector);
>  struct drm_encoder *
>  drm_atomic_helper_best_encoder(struct drm_connector *connector);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Manasi Navare]

On Tue, Oct 25, 2016 at 03:09:39PM +0300, Jani Nikula wrote:
> On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > This function provides a way for the driver to redo a
> > modeset on the current mode and retry the link training
> > at a lower link rate/lane count/bpp. This will get called
> > incase the link training fails during the current modeset.
> 
> Based on discussions on #intel-gfx, I would dodge all the problems here
> by having the userspace do the modeset.
> 
> If we add a connector property to indicate the link status (and this
> does not have to be DP or link training specific, really), we can set
> that to "bad", and fire off the hotplug uevent. Then userspace has the
> information to force a modeset on that connector even if the mode has
> not changed. (Credits to Ville for the idea.)
> 
> If we find a solution later on that allows us to handle all of this in
> kernel, we can do so, and remove the property or always report
> "good". Drivers can choose different approaches, depending on the
> capabilities of the hardware, for instance.
> 
> Deferring this to the userspace does not regress anything now, because
> currently we just completely fail and end up with a black screen. The
> property would allow an enlightened userspace to fix that. And userspace
> can't rely on the property being there, as it's currently not there.
> 
> 
> BR,
> Jani.
> 
>

Thanks for your feedback Jani. I will try implementing this, but isn't this going
to require changes in all the userspace drivers (modesetting driver, ChromeOS) to
make use of this new property to trigger another modeset?
How easy would it be to have all the userspace drivers adopt this change?

Regards
Manasi
 
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  1 +
> >  2 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f936276..0c1614e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> >  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
> >  
> >  /**
> > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
> > + * @connector: DRM connector
> > + *
> > + * Provides a way to redo a modeset with the current mode so that it can
> > + * drop the bpp, link rate/lane count and retry the link training.
> > + *
> > + * Returns:
> > + * Returns 0 on success, negative errno numbers on failure.
> > + */
> > +int
> > +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_atomic_state *state;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret = 0;
> > +
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto fail;
> > +	}
> > +	state->acquire_ctx = &ctx;
> > +retry:
> > +	ret = 0;
> > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > +	if (IS_ERR(connector_state)) {
> > +		ret = PTR_ERR(connector_state);
> > +		goto fail;
> > +	}
> > +	if (!connector_state->crtc)
> > +		goto fail;
> > +
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > +							connector_state->crtc);
> > +	crtc_state->connectors_changed = true;
> > +	ret = drm_atomic_commit(state);
> > +fail:
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	if (state)
> > +		drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset);
> > +
> > +/**
> >   * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
> >   *                                  ->best_encoder callback
> >   * @connector: Connector control structure
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 7ff92b0..8de24dc 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> >  				uint32_t flags);
> >  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> >  				     int mode);
> > +int drm_atomic_helper_connector_modeset(struct drm_connector *connector);
> >  struct drm_encoder *
> >  drm_atomic_helper_best_encoder(struct drm_connector *connector);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

[Rodrigo Vivi]

On Tue, Oct 25, 2016 at 5:09 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> This function provides a way for the driver to redo a
>> modeset on the current mode and retry the link training
>> at a lower link rate/lane count/bpp. This will get called
>> incase the link training fails during the current modeset.
>
> Based on discussions on #intel-gfx, I would dodge all the problems here
> by having the userspace do the modeset.

This is not like going back to UMS times?

> If we add a connector property to indicate the link status (and this
> does not have to be DP or link training specific, really), we can set
> that to "bad", and fire off the hotplug uevent. Then userspace has the
> information to force a modeset on that connector even if the mode has
> not changed. (Credits to Ville for the idea.)

This would require changes in all user space drivers out there right?
Ok, maybe faster than fix vblank layer for good...

>
> If we find a solution later on that allows us to handle all of this in
> kernel, we can do so, and remove the property or always report
> "good". Drivers can choose different approaches, depending on the
> capabilities of the hardware, for instance.

So, is this temporary then? While we update the vblank layer?

> Deferring this to the userspace does not regress anything now, because
> currently we just completely fail and end up with a black screen. The
> property would allow an enlightened userspace to fix that. And userspace
> can't rely on the property being there, as it's currently not there.

Also this discussion remind me about the PSR where we refused to give control
to userspace or create any property and kmd should control all cases.
Should we revisit that and change userspace to control PSR?

Thanks,
Rodrigo.

>
>
> BR,
> Jani.
>
>
>>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_atomic_helper.h     |  1 +
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index f936276..0c1614e 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>>  EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>>
>>  /**
>> + * drm_atomic_helper_connector_modeset - Force a modeset on a connector
>> + * @connector: DRM connector
>> + *
>> + * Provides a way to redo a modeset with the current mode so that it can
>> + * drop the bpp, link rate/lane count and retry the link training.
>> + *
>> + * Returns:
>> + * Returns 0 on success, negative errno numbers on failure.
>> + */
>> +int
>> +drm_atomic_helper_connector_modeset(struct drm_connector *connector)
>> +{
>> +     struct drm_device *dev = connector->dev;
>> +     struct drm_modeset_acquire_ctx ctx;
>> +     struct drm_atomic_state *state;
>> +     struct drm_connector_state *connector_state;
>> +     struct drm_crtc_state *crtc_state;
>> +     int ret = 0;
>> +
>> +     drm_modeset_acquire_init(&ctx, 0);
>> +     state = drm_atomic_state_alloc(dev);
>> +     if (!state) {
>> +             ret = -ENOMEM;
>> +             goto fail;
>> +     }
>> +     state->acquire_ctx = &ctx;
>> +retry:
>> +     ret = 0;
>> +     connector_state = drm_atomic_get_connector_state(state, connector);
>> +     if (IS_ERR(connector_state)) {
>> +             ret = PTR_ERR(connector_state);
>> +             goto fail;
>> +     }
>> +     if (!connector_state->crtc)
>> +             goto fail;
>> +
>> +     crtc_state = drm_atomic_get_existing_crtc_state(state,
>> +                                                     connector_state->crtc);
>> +     crtc_state->connectors_changed = true;
>> +     ret = drm_atomic_commit(state);
>> +fail:
>> +     if (ret == -EDEADLK) {
>> +             drm_atomic_state_clear(state);
>> +             drm_modeset_backoff(&ctx);
>> +             goto retry;
>> +     }
>> +
>> +     if (state)
>> +             drm_atomic_state_put(state);
>> +
>> +     drm_modeset_drop_locks(&ctx);
>> +     drm_modeset_acquire_fini(&ctx);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset);
>> +
>> +/**
>>   * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs
>>   *                                  ->best_encoder callback
>>   * @connector: Connector control structure
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 7ff92b0..8de24dc 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>>                               uint32_t flags);
>>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>>                                    int mode);
>> +int drm_atomic_helper_connector_modeset(struct drm_connector *connector);
>>  struct drm_encoder *
>>  drm_atomic_helper_best_encoder(struct drm_connector *connector);
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx