On Thu, Mar 08, 2018 at 06:41:22PM -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.
> 
> V2:
>  - Remove accidental duplicate modeset_retry_work in intel_connector
> 
> Signed-off-by: Lyude Paul <ly...@redhat.com>
> Cc: Manasi Navare <manasi.d.nav...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c          | 23 +++++++++++++++++++++--
>  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, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index f424fff477f6..85d5af4e4f5b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15394,16 +15394,35 @@ 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;
>  
>       /* Kill all the work that may have been queued by hpd. */
>       drm_connector_list_iter_begin(dev, &conn_iter);
>       for_each_intel_connector_iter(connector, &conn_iter) {
> -             if (connector->modeset_retry_work.func)
> -                     cancel_work_sync(&connector->modeset_retry_work);
>               if (connector->hdcp_shim) {
>                       cancel_delayed_work_sync(&connector->hdcp_check_work);
>                       cancel_work_sync(&connector->hdcp_prop_work);
>               }
> +
> +             if (connector->base.connector_type !=
> +                 DRM_MODE_CONNECTOR_DisplayPort)

I know we dont fallback on eDP now but since link training is a common code 
between
eDP and DP, this modeset retry can also be called from eDP. So we should check 
against
DRM_MODE_CONNECTOR_eDP type as well.

Other than that, I agree that it is better for this work function to be part of 
intel_dp struct
as opposed to intel_connector. 

So after this change,

Reviewed-by: Manasi Navare <manasi.d.nav...@intel.com>

Manasi

> +                     continue;
> +
> +             if (connector->mst_port) {
> +                     work = &connector->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(&intel_encoder->base);
> +                     work = &intel_dp->modeset_retry_work;
> +             }
> +
> +             cancel_work_sync(work);
>       }
>       drm_connector_list_iter_end(&conn_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 = &intel_dp->attached_connector->base;
>  
> -     intel_connector = container_of(work, typeof(*intel_connector),
> -                                    modeset_retry_work);
> -     connector = &intel_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(&intel_connector->modeset_retry_work,
> +     INIT_WORK(&intel_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))
>                       /* Schedule a Hotplug Uevent to userspace to start 
> modeset */
> -                     schedule_work(&intel_connector->modeset_retry_work);
> +                     schedule_work(&intel_dp->modeset_retry_work);
>       } else {
>               DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate 
> = %d, lane count = %d",
>                         intel_connector->base.base.id,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 83e5ca889d9c..3f19dc80997f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -406,9 +406,6 @@ struct intel_connector {
>  
>       struct intel_dp *mst_port;
>  
> -     /* Work struct to schedule a uevent on link train failure */
> -     struct work_struct modeset_retry_work;
> -
>       const struct intel_hdcp_shim *hdcp_shim;
>       struct mutex hdcp_mutex;
>       uint64_t hdcp_value; /* protected by hdcp_mutex */
> @@ -1135,6 +1132,9 @@ struct intel_dp {
>  
>       /* Displayport compliance testing */
>       struct intel_dp_compliance compliance;
> +
> +     /* Work struct to schedule a uevent on link train failure */
> +     struct work_struct modeset_retry_work;
>  };
>  
>  struct intel_lspcon {
> -- 
> 2.14.3
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to