Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 07:24:20PM -0400, Lyude Paul wrote:
> On Mon, 2018-03-12 at 23:01 +0200, Ville Syrjälä wrote:
> > On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> > > While having the modeset_retry_work in intel_connector makes sense with
> > > SST, this paradigm doesn't make a whole ton of sense when it comes to
> > > MST since we have to deal with multiple connectors. In most cases, it's
> > > more useful to just use the intel_dp struct since it indicates whether
> > > or not we're dealing with an MST device, along with being able to easily
> > > trace the intel_dp struct back to it's respective connector (if there is
> > > any). So, move the modeset_retry_work function out of the
> > > intel_connector struct and into intel_dp.
> > > 
> > > Signed-off-by: Lyude Paul 
> > > Reviewed-by: Manasi Navare 
> > > Cc: Manasi Navare 
> > > Cc: Ville Syrjälä 
> > > 
> > > V2:
> > >  - Remove accidental duplicate modeset_retry_work in intel_connector
> > > V3:
> > >  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> > > Signed-off-by: Lyude Paul 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c  | 25 
> > > +++-
> > > -
> > >  drivers/gpu/drm/i915/intel_dp.c   | 10 --
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
> > >  4 files changed, 31 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index f424fff477f6..3b7fa430a84a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct 
> > > drm_device
> > > *dev)
> > >  {
> > >   struct intel_connector *connector;
> > >   struct drm_connector_list_iter conn_iter;
> > > + struct work_struct *work;
> > > + int conn_type;
> > >  
> > >   /* Kill all the work that may have been queued by hpd. */
> > >   drm_connector_list_iter_begin(dev, _iter);
> > >   for_each_intel_connector_iter(connector, _iter) {
> > > - if (connector->modeset_retry_work.func)
> > > - cancel_work_sync(>modeset_retry_work);
> > >   if (connector->hdcp_shim) {
> > >   cancel_delayed_work_sync(
> > > >hdcp_check_work);
> > >   cancel_work_sync(>hdcp_prop_work);
> > >   }
> > > +
> > > + conn_type = connector->base.connector_type;
> > > + if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> > > + conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> > > + continue;
> > > +
> > > + if (connector->mst_port) {
> > > + work = >mst_port->modeset_retry_work;
> > > + } else {
> > > + struct intel_encoder *intel_encoder =
> > > + connector->encoder;
> > > + struct intel_dp *intel_dp;
> > > +
> > > + if (!intel_encoder)
> > > + continue;
> > > +
> > > + intel_dp = enc_to_intel_dp(_encoder->base);
> > > + work = _dp->modeset_retry_work;
> > > + }
> > > +
> > > + cancel_work_sync(work);
> > 
> > Why are we even walking the connectors for this? Can't we just
> > walk the encoders instead?
> We could walk the encoders for this, but seeing as we're already walking the
> connectors for the HDCP prop doesn't it make more sense to just stick our code
> there? or is there a simpler solution for this I'm missing

I think walking the encoders when you want encoders is a lot cleaner.
Keeps the snr much higher.

> > 
> > >   }
> > >   drm_connector_list_iter_end(_iter);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 4dd1b2287dd6..5abf0c95725a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct 
> > > intel_dp
> > > *intel_dp,
> > >  
> > >  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > >  {
> > > - struct intel_connector *intel_connector;
> > > - struct drm_connector *connector;
> > > + struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> > > +  modeset_retry_work);
> > > + struct drm_connector *connector = _dp->attached_connector-
> > > >base;
> > >  
> > > - intel_connector = container_of(work, typeof(*intel_connector),
> > > -modeset_retry_work);
> > > - connector = _connector->base;
> > >   DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > > connector->name);
> > >  
> > > @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port

Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 07:24:20PM -0400, Lyude Paul wrote:
> On Mon, 2018-03-12 at 23:01 +0200, Ville Syrjälä wrote:
> > On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> > > While having the modeset_retry_work in intel_connector makes sense with
> > > SST, this paradigm doesn't make a whole ton of sense when it comes to
> > > MST since we have to deal with multiple connectors. In most cases, it's
> > > more useful to just use the intel_dp struct since it indicates whether
> > > or not we're dealing with an MST device, along with being able to easily
> > > trace the intel_dp struct back to it's respective connector (if there is
> > > any). So, move the modeset_retry_work function out of the
> > > intel_connector struct and into intel_dp.
> > > 
> > > Signed-off-by: Lyude Paul 
> > > Reviewed-by: Manasi Navare 
> > > Cc: Manasi Navare 
> > > Cc: Ville Syrjälä 
> > > 
> > > V2:
> > >  - Remove accidental duplicate modeset_retry_work in intel_connector
> > > V3:
> > >  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> > > Signed-off-by: Lyude Paul 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c  | 25 
> > > +++-
> > > -
> > >  drivers/gpu/drm/i915/intel_dp.c   | 10 --
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
> > >  4 files changed, 31 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index f424fff477f6..3b7fa430a84a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct 
> > > drm_device
> > > *dev)
> > >  {
> > >   struct intel_connector *connector;
> > >   struct drm_connector_list_iter conn_iter;
> > > + struct work_struct *work;
> > > + int conn_type;
> > >  
> > >   /* Kill all the work that may have been queued by hpd. */
> > >   drm_connector_list_iter_begin(dev, _iter);
> > >   for_each_intel_connector_iter(connector, _iter) {
> > > - if (connector->modeset_retry_work.func)
> > > - cancel_work_sync(>modeset_retry_work);
> > >   if (connector->hdcp_shim) {
> > >   cancel_delayed_work_sync(
> > > >hdcp_check_work);
> > >   cancel_work_sync(>hdcp_prop_work);
> > >   }
> > > +
> > > + conn_type = connector->base.connector_type;
> > > + if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> > > + conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> > > + continue;
> > > +
> > > + if (connector->mst_port) {
> > > + work = >mst_port->modeset_retry_work;
> > > + } else {
> > > + struct intel_encoder *intel_encoder =
> > > + connector->encoder;
> > > + struct intel_dp *intel_dp;
> > > +
> > > + if (!intel_encoder)
> > > + continue;
> > > +
> > > + intel_dp = enc_to_intel_dp(_encoder->base);
> > > + work = _dp->modeset_retry_work;
> > > + }
> > > +
> > > + cancel_work_sync(work);
> > 
> > Why are we even walking the connectors for this? Can't we just
> > walk the encoders instead?
> We could walk the encoders for this, but seeing as we're already walking the
> connectors for the HDCP prop doesn't it make more sense to just stick our code
> there? or is there a simpler solution for this I'm missing

I think walking the encoders when you want encoders is a lot cleaner.
Keeps the snr much higher.

> > 
> > >   }
> > >   drm_connector_list_iter_end(_iter);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 4dd1b2287dd6..5abf0c95725a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct 
> > > intel_dp
> > > *intel_dp,
> > >  
> > >  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> > >  {
> > > - struct intel_connector *intel_connector;
> > > - struct drm_connector *connector;
> > > + struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> > > +  modeset_retry_work);
> > > + struct drm_connector *connector = _dp->attached_connector-
> > > >base;
> > >  
> > > - intel_connector = container_of(work, typeof(*intel_connector),
> > > -modeset_retry_work);
> > > - connector = _connector->base;
> > >   DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> > > connector->name);
> > >  
> > > @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port
> > > *intel_dig_port,
> > >   int type;
> > >  
> > >   /* Initialize the work for modeset in case of link train 

Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Lyude Paul]

On Mon, 2018-03-12 at 23:01 +0200, Ville Syrjälä wrote:
> On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> > While having the modeset_retry_work in intel_connector makes sense with
> > SST, this paradigm doesn't make a whole ton of sense when it comes to
> > MST since we have to deal with multiple connectors. In most cases, it's
> > more useful to just use the intel_dp struct since it indicates whether
> > or not we're dealing with an MST device, along with being able to easily
> > trace the intel_dp struct back to it's respective connector (if there is
> > any). So, move the modeset_retry_work function out of the
> > intel_connector struct and into intel_dp.
> > 
> > Signed-off-by: Lyude Paul 
> > Reviewed-by: Manasi Navare 
> > Cc: Manasi Navare 
> > Cc: Ville Syrjälä 
> > 
> > V2:
> >  - Remove accidental duplicate modeset_retry_work in intel_connector
> > V3:
> >  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> > Signed-off-by: Lyude Paul 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 25 +++-
> > -
> >  drivers/gpu/drm/i915/intel_dp.c   | 10 --
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
> >  4 files changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index f424fff477f6..3b7fa430a84a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct drm_device
> > *dev)
> >  {
> > struct intel_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > +   struct work_struct *work;
> > +   int conn_type;
> >  
> > /* Kill all the work that may have been queued by hpd. */
> > drm_connector_list_iter_begin(dev, _iter);
> > for_each_intel_connector_iter(connector, _iter) {
> > -   if (connector->modeset_retry_work.func)
> > -   cancel_work_sync(>modeset_retry_work);
> > if (connector->hdcp_shim) {
> > cancel_delayed_work_sync(
> > >hdcp_check_work);
> > cancel_work_sync(>hdcp_prop_work);
> > }
> > +
> > +   conn_type = connector->base.connector_type;
> > +   if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> > +   conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> > +   continue;
> > +
> > +   if (connector->mst_port) {
> > +   work = >mst_port->modeset_retry_work;
> > +   } else {
> > +   struct intel_encoder *intel_encoder =
> > +   connector->encoder;
> > +   struct intel_dp *intel_dp;
> > +
> > +   if (!intel_encoder)
> > +   continue;
> > +
> > +   intel_dp = enc_to_intel_dp(_encoder->base);
> > +   work = _dp->modeset_retry_work;
> > +   }
> > +
> > +   cancel_work_sync(work);
> 
> Why are we even walking the connectors for this? Can't we just
> walk the encoders instead?
We could walk the encoders for this, but seeing as we're already walking the
connectors for the HDCP prop doesn't it make more sense to just stick our code
there? or is there a simpler solution for this I'm missing
> 
> > }
> > drm_connector_list_iter_end(_iter);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 4dd1b2287dd6..5abf0c95725a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct intel_dp
> > *intel_dp,
> >  
> >  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> >  {
> > -   struct intel_connector *intel_connector;
> > -   struct drm_connector *connector;
> > +   struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> > +modeset_retry_work);
> > +   struct drm_connector *connector = _dp->attached_connector-
> > >base;
> >  
> > -   intel_connector = container_of(work, typeof(*intel_connector),
> > -  modeset_retry_work);
> > -   connector = _connector->base;
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> >   connector->name);
> >  
> > @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port
> > *intel_dig_port,
> > int type;
> >  
> > /* Initialize the work for modeset in case of link train failure */
> > -   INIT_WORK(_connector->modeset_retry_work,
> > +   INIT_WORK(_dp->modeset_retry_work,
> >   intel_dp_modeset_retry_work_fn);
> >  
> > if 

Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Lyude Paul]

On Mon, 2018-03-12 at 23:01 +0200, Ville Syrjälä wrote:
> On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> > While having the modeset_retry_work in intel_connector makes sense with
> > SST, this paradigm doesn't make a whole ton of sense when it comes to
> > MST since we have to deal with multiple connectors. In most cases, it's
> > more useful to just use the intel_dp struct since it indicates whether
> > or not we're dealing with an MST device, along with being able to easily
> > trace the intel_dp struct back to it's respective connector (if there is
> > any). So, move the modeset_retry_work function out of the
> > intel_connector struct and into intel_dp.
> > 
> > Signed-off-by: Lyude Paul 
> > Reviewed-by: Manasi Navare 
> > Cc: Manasi Navare 
> > Cc: Ville Syrjälä 
> > 
> > V2:
> >  - Remove accidental duplicate modeset_retry_work in intel_connector
> > V3:
> >  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> > Signed-off-by: Lyude Paul 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 25 +++-
> > -
> >  drivers/gpu/drm/i915/intel_dp.c   | 10 --
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
> >  4 files changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index f424fff477f6..3b7fa430a84a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct drm_device
> > *dev)
> >  {
> > struct intel_connector *connector;
> > struct drm_connector_list_iter conn_iter;
> > +   struct work_struct *work;
> > +   int conn_type;
> >  
> > /* Kill all the work that may have been queued by hpd. */
> > drm_connector_list_iter_begin(dev, _iter);
> > for_each_intel_connector_iter(connector, _iter) {
> > -   if (connector->modeset_retry_work.func)
> > -   cancel_work_sync(>modeset_retry_work);
> > if (connector->hdcp_shim) {
> > cancel_delayed_work_sync(
> > >hdcp_check_work);
> > cancel_work_sync(>hdcp_prop_work);
> > }
> > +
> > +   conn_type = connector->base.connector_type;
> > +   if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> > +   conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> > +   continue;
> > +
> > +   if (connector->mst_port) {
> > +   work = >mst_port->modeset_retry_work;
> > +   } else {
> > +   struct intel_encoder *intel_encoder =
> > +   connector->encoder;
> > +   struct intel_dp *intel_dp;
> > +
> > +   if (!intel_encoder)
> > +   continue;
> > +
> > +   intel_dp = enc_to_intel_dp(_encoder->base);
> > +   work = _dp->modeset_retry_work;
> > +   }
> > +
> > +   cancel_work_sync(work);
> 
> Why are we even walking the connectors for this? Can't we just
> walk the encoders instead?
We could walk the encoders for this, but seeing as we're already walking the
connectors for the HDCP prop doesn't it make more sense to just stick our code
there? or is there a simpler solution for this I'm missing
> 
> > }
> > drm_connector_list_iter_end(_iter);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 4dd1b2287dd6..5abf0c95725a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct intel_dp
> > *intel_dp,
> >  
> >  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
> >  {
> > -   struct intel_connector *intel_connector;
> > -   struct drm_connector *connector;
> > +   struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> > +modeset_retry_work);
> > +   struct drm_connector *connector = _dp->attached_connector-
> > >base;
> >  
> > -   intel_connector = container_of(work, typeof(*intel_connector),
> > -  modeset_retry_work);
> > -   connector = _connector->base;
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> >   connector->name);
> >  
> > @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port
> > *intel_dig_port,
> > int type;
> >  
> > /* Initialize the work for modeset in case of link train failure */
> > -   INIT_WORK(_connector->modeset_retry_work,
> > +   INIT_WORK(_dp->modeset_retry_work,
> >   intel_dp_modeset_retry_work_fn);
> >  
> > if (WARN(intel_dig_port->max_lanes < 1,
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > 

Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Ville Syrjälä]

On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> While having the modeset_retry_work in intel_connector makes sense with
> SST, this paradigm doesn't make a whole ton of sense when it comes to
> MST since we have to deal with multiple connectors. In most cases, it's
> more useful to just use the intel_dp struct since it indicates whether
> or not we're dealing with an MST device, along with being able to easily
> trace the intel_dp struct back to it's respective connector (if there is
> any). So, move the modeset_retry_work function out of the
> intel_connector struct and into intel_dp.
> 
> Signed-off-by: Lyude Paul 
> Reviewed-by: Manasi Navare 
> Cc: Manasi Navare 
> Cc: Ville Syrjälä 
> 
> V2:
>  - Remove accidental duplicate modeset_retry_work in intel_connector
> V3:
>  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 25 +++--
>  drivers/gpu/drm/i915/intel_dp.c   | 10 --
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
>  4 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index f424fff477f6..3b7fa430a84a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct drm_device 
> *dev)
>  {
>   struct intel_connector *connector;
>   struct drm_connector_list_iter conn_iter;
> + struct work_struct *work;
> + int conn_type;
>  
>   /* Kill all the work that may have been queued by hpd. */
>   drm_connector_list_iter_begin(dev, _iter);
>   for_each_intel_connector_iter(connector, _iter) {
> - if (connector->modeset_retry_work.func)
> - cancel_work_sync(>modeset_retry_work);
>   if (connector->hdcp_shim) {
>   cancel_delayed_work_sync(>hdcp_check_work);
>   cancel_work_sync(>hdcp_prop_work);
>   }
> +
> + conn_type = connector->base.connector_type;
> + if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> + conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->mst_port) {
> + work = >mst_port->modeset_retry_work;
> + } else {
> + struct intel_encoder *intel_encoder =
> + connector->encoder;
> + struct intel_dp *intel_dp;
> +
> + if (!intel_encoder)
> + continue;
> +
> + intel_dp = enc_to_intel_dp(_encoder->base);
> + work = _dp->modeset_retry_work;
> + }
> +
> + cancel_work_sync(work);

Why are we even walking the connectors for this? Can't we just
walk the encoders instead?

>   }
>   drm_connector_list_iter_end(_iter);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4dd1b2287dd6..5abf0c95725a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct intel_dp 
> *intel_dp,
>  
>  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
>  {
> - struct intel_connector *intel_connector;
> - struct drm_connector *connector;
> + struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> +  modeset_retry_work);
> + struct drm_connector *connector = _dp->attached_connector->base;
>  
> - intel_connector = container_of(work, typeof(*intel_connector),
> -modeset_retry_work);
> - connector = _connector->base;
>   DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> connector->name);
>  
> @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port 
> *intel_dig_port,
>   int type;
>  
>   /* Initialize the work for modeset in case of link train failure */
> - INIT_WORK(_connector->modeset_retry_work,
> + INIT_WORK(_dp->modeset_retry_work,
> intel_dp_modeset_retry_work_fn);
>  
>   if (WARN(intel_dig_port->max_lanes < 1,
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index f59b59bb0a21..2cfa58ce1f95 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -340,7 +340,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>
> 

Re: [PATCH v3 1/5] drm/i915: Move DP modeset retry work into intel_dp

[Ville Syrjälä]

On Fri, Mar 09, 2018 at 04:32:27PM -0500, Lyude Paul wrote:
> While having the modeset_retry_work in intel_connector makes sense with
> SST, this paradigm doesn't make a whole ton of sense when it comes to
> MST since we have to deal with multiple connectors. In most cases, it's
> more useful to just use the intel_dp struct since it indicates whether
> or not we're dealing with an MST device, along with being able to easily
> trace the intel_dp struct back to it's respective connector (if there is
> any). So, move the modeset_retry_work function out of the
> intel_connector struct and into intel_dp.
> 
> Signed-off-by: Lyude Paul 
> Reviewed-by: Manasi Navare 
> Cc: Manasi Navare 
> Cc: Ville Syrjälä 
> 
> V2:
>  - Remove accidental duplicate modeset_retry_work in intel_connector
> V3:
>  - Also check against eDP in intel_hpd_poll_fini() - mdnavare
> Signed-off-by: Lyude Paul 
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 25 +++--
>  drivers/gpu/drm/i915/intel_dp.c   | 10 --
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h  |  6 +++---
>  4 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index f424fff477f6..3b7fa430a84a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15394,16 +15394,37 @@ static void intel_hpd_poll_fini(struct drm_device 
> *dev)
>  {
>   struct intel_connector *connector;
>   struct drm_connector_list_iter conn_iter;
> + struct work_struct *work;
> + int conn_type;
>  
>   /* Kill all the work that may have been queued by hpd. */
>   drm_connector_list_iter_begin(dev, _iter);
>   for_each_intel_connector_iter(connector, _iter) {
> - if (connector->modeset_retry_work.func)
> - cancel_work_sync(>modeset_retry_work);
>   if (connector->hdcp_shim) {
>   cancel_delayed_work_sync(>hdcp_check_work);
>   cancel_work_sync(>hdcp_prop_work);
>   }
> +
> + conn_type = connector->base.connector_type;
> + if (conn_type != DRM_MODE_CONNECTOR_eDP &&
> + conn_type != DRM_MODE_CONNECTOR_DisplayPort)
> + continue;
> +
> + if (connector->mst_port) {
> + work = >mst_port->modeset_retry_work;
> + } else {
> + struct intel_encoder *intel_encoder =
> + connector->encoder;
> + struct intel_dp *intel_dp;
> +
> + if (!intel_encoder)
> + continue;
> +
> + intel_dp = enc_to_intel_dp(_encoder->base);
> + work = _dp->modeset_retry_work;
> + }
> +
> + cancel_work_sync(work);

Why are we even walking the connectors for this? Can't we just
walk the encoders instead?

>   }
>   drm_connector_list_iter_end(_iter);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4dd1b2287dd6..5abf0c95725a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6261,12 +6261,10 @@ static bool intel_edp_init_connector(struct intel_dp 
> *intel_dp,
>  
>  static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
>  {
> - struct intel_connector *intel_connector;
> - struct drm_connector *connector;
> + struct intel_dp *intel_dp = container_of(work, typeof(*intel_dp),
> +  modeset_retry_work);
> + struct drm_connector *connector = _dp->attached_connector->base;
>  
> - intel_connector = container_of(work, typeof(*intel_connector),
> -modeset_retry_work);
> - connector = _connector->base;
>   DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
> connector->name);
>  
> @@ -6295,7 +6293,7 @@ intel_dp_init_connector(struct intel_digital_port 
> *intel_dig_port,
>   int type;
>  
>   /* Initialize the work for modeset in case of link train failure */
> - INIT_WORK(_connector->modeset_retry_work,
> + INIT_WORK(_dp->modeset_retry_work,
> intel_dp_modeset_retry_work_fn);
>  
>   if (WARN(intel_dig_port->max_lanes < 1,
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index f59b59bb0a21..2cfa58ce1f95 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -340,7 +340,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>
> intel_dp->link_rate,
>
> intel_dp->lane_count))
>