Re: [PATCH] drm/i915: nuke the intel_lvds_connector

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 10, 2018 at 12:09:16AM +0300, Jani Nikula wrote:
> For a while we carried lvds connector specific data in the lvds
> connector, but since commit 05c72e77ccda ("drm/i915: Nuke the LVDS lid
> notifier") we haven't needed it. Revert back to plain intel_connector.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 42 +++++++++++----------------------------
>  1 file changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 1fe970cf9909..510585ed94b2 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -42,10 +42,6 @@
>  #include <linux/acpi.h>
>  
>  /* Private structure for the integrated LVDS support */
> -struct intel_lvds_connector {
> -	struct intel_connector base;
> -};
> -
>  struct intel_lvds_pps {
>  	/* 100us units */
>  	int t1_t2;
> @@ -70,7 +66,7 @@ struct intel_lvds_encoder {
>  	struct intel_lvds_pps init_pps;
>  	u32 init_lvds_val;
>  
> -	struct intel_lvds_connector *attached_connector;
> +	struct intel_connector *attached_connector;
>  };
>  
>  static struct intel_lvds_encoder *to_lvds_encoder(struct drm_encoder *encoder)
> @@ -78,11 +74,6 @@ static struct intel_lvds_encoder *to_lvds_encoder(struct drm_encoder *encoder)
>  	return container_of(encoder, struct intel_lvds_encoder, base.base);
>  }
>  
> -static struct intel_lvds_connector *to_lvds_connector(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct intel_lvds_connector, base.base);
> -}
> -
>  bool intel_lvds_port_enabled(struct drm_i915_private *dev_priv,
>  			     i915_reg_t lvds_reg, enum pipe *pipe)
>  {
> @@ -396,7 +387,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  	struct intel_lvds_encoder *lvds_encoder =
>  		to_lvds_encoder(&intel_encoder->base);
>  	struct intel_connector *intel_connector =
> -		&lvds_encoder->attached_connector->base;
> +		lvds_encoder->attached_connector;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>  	unsigned int lvds_bpp;
> @@ -461,15 +452,15 @@ intel_lvds_detect(struct drm_connector *connector, bool force)
>   */
>  static int intel_lvds_get_modes(struct drm_connector *connector)
>  {
> -	struct intel_lvds_connector *lvds_connector = to_lvds_connector(connector);
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct drm_device *dev = connector->dev;
>  	struct drm_display_mode *mode;
>  
>  	/* use cached edid if we have one */
> -	if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
> -		return drm_add_edid_modes(connector, lvds_connector->base.edid);
> +	if (!IS_ERR_OR_NULL(intel_connector->edid))
> +		return drm_add_edid_modes(connector, intel_connector->edid);
>  
> -	mode = drm_mode_duplicate(dev, lvds_connector->base.panel.fixed_mode);
> +	mode = drm_mode_duplicate(dev, intel_connector->panel.fixed_mode);
>  	if (mode == NULL)
>  		return 0;
>  
> @@ -781,8 +772,7 @@ static bool compute_is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
>  		return i915_modparams.lvds_channel_mode == 2;
>  
>  	/* single channel LVDS is limited to 112 MHz */
> -	if (lvds_encoder->attached_connector->base.panel.fixed_mode->clock
> -	    > 112999)
> +	if (lvds_encoder->attached_connector->panel.fixed_mode->clock > 112999)
>  		return true;
>  
>  	if (dmi_check_system(intel_dual_link_lvds))
> @@ -837,7 +827,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_lvds_encoder *lvds_encoder;
>  	struct intel_encoder *intel_encoder;
> -	struct intel_lvds_connector *lvds_connector;
>  	struct intel_connector *intel_connector;
>  	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
> @@ -890,23 +879,16 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	if (!lvds_encoder)
>  		return;
>  
> -	lvds_connector = kzalloc(sizeof(*lvds_connector), GFP_KERNEL);
> -	if (!lvds_connector) {
> -		kfree(lvds_encoder);
> -		return;
> -	}
> -
> -	if (intel_connector_init(&lvds_connector->base) < 0) {
> -		kfree(lvds_connector);
> +	intel_connector = intel_connector_alloc();
> +	if (!intel_connector) {
>  		kfree(lvds_encoder);
>  		return;
>  	}
>  
> -	lvds_encoder->attached_connector = lvds_connector;
> +	lvds_encoder->attached_connector = intel_connector;
>  
>  	intel_encoder = &lvds_encoder->base;
>  	encoder = &intel_encoder->base;
> -	intel_connector = &lvds_connector->base;
>  	connector = &intel_connector->base;
>  	drm_connector_init(dev, &intel_connector->base, &intel_lvds_connector_funcs,
>  			   DRM_MODE_CONNECTOR_LVDS);
> @@ -987,7 +969,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	} else {
>  		edid = ERR_PTR(-ENOENT);
>  	}
> -	lvds_connector->base.edid = edid;
> +	intel_connector->edid = edid;
>  
>  	list_for_each_entry(scan, &connector->probed_modes, head) {
>  		if (scan->type & DRM_MODE_TYPE_PREFERRED) {
> @@ -1051,6 +1033,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  	drm_connector_cleanup(connector);
>  	drm_encoder_cleanup(encoder);
>  	kfree(lvds_encoder);
> -	kfree(lvds_connector);
> +	intel_connector_free(intel_connector);

I was going to say that this will do a double free on the state, but
drm_connector_cleanup() & co. memset(0) the object so this should be
fine.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	return;
>  }
> -- 
> 2.11.0

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