[Intel-gfx] [PATCH] drm/i915: Determine type before initialising connector

[Ma Ling]

Hi Jonas

It is good patch, by this patch we can resolve duplicate name
by moving drm_connector_init after connector type is confirmed.
However if we can not get real output when SDVO connector advertise
multiple functions we have to meet the same issue, i.e. SDVO connector
advertise two functions - DVI and VGA, actually only VGA is real one,
in sdvo init function card0-VGA-1 is allocated for CRT, and
card0-DVI-D-1 is allocated for DVI, but until sdvo detection complete
we can be aware of VGA, not DVI, and we have to change sdvo connector type as VGA.
"the name card0-VGA-1 would be allocted to both a CRT and an SDVO connector"
occurs again. So I hope to export two functions for drm -get/put connector type id
to try resolving it, what's your opinion?

On Tue, 2009-05-19 at 15:05 +0800, Jonas Bonn wrote:
> drm_connector_init sets both the connector type and the connector type_id
> on the newly initialised connector.  As the connector type_id is coupled to
> the connector type, the connector type cannot simply be modified on an
> initialised connector.
> 
> This patch changes the order of operations on intel_sdvo_init so that the
> type is determined before the connector is intialised.
> 
> This fixes a bug whereby the name card0-VGA-1 would be allocted to both a
> CRT and an SDVO connector since the SDVO connector would be initialised
> with type 'unknown' and hence have its type_id assigned from the wrong pool.
> 
> Signed-off-by: Jonas Bonn <jonas at southpole.se>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   29 ++++++++++-------------------
>  1 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 9913651..605b402 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1676,17 +1676,9 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  		return false;
>  	}
>  
> -	connector = &intel_output->base;
> -
> -	drm_connector_init(dev, connector, &intel_sdvo_connector_funcs,
> -			   DRM_MODE_CONNECTOR_Unknown);
> -	drm_connector_helper_add(connector, &intel_sdvo_connector_helper_funcs);
>  	sdvo_priv = (struct intel_sdvo_priv *)(intel_output + 1);
>  	intel_output->type = INTEL_OUTPUT_SDVO;
>  
> -	connector->interlace_allowed = 0;
> -	connector->doublescan_allowed = 0;
> -
>  	/* setup the DDC bus. */
>  	if (output_device == SDVOB)
>  		i2cbus = intel_i2c_create(dev, GPIOE, "SDVOCTRL_E for SDVOB");
> @@ -1694,7 +1686,7 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  		i2cbus = intel_i2c_create(dev, GPIOE, "SDVOCTRL_E for SDVOC");
>  
>  	if (!i2cbus)
> -		goto err_connector;
> +		goto err_inteloutput;
>  
>  	sdvo_priv->i2c_bus = i2cbus;
>  
> @@ -1710,7 +1702,6 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  	intel_output->i2c_bus = i2cbus;
>  	intel_output->dev_priv = sdvo_priv;
>  
> -
>  	/* Read the regs to test if we can talk to the device */
>  	for (i = 0; i < 0x40; i++) {
>  		if (!intel_sdvo_read_byte(intel_output, i, &ch[i])) {
> @@ -1729,7 +1720,6 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  		else
>  			sdvo_priv->controlled_output = SDVO_OUTPUT_TMDS1;
>  
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_TMDS;
>  		connector_type = DRM_MODE_CONNECTOR_DVID;
>  
> @@ -1747,7 +1737,6 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  	else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_SVID0)
>  	{
>  		sdvo_priv->controlled_output = SDVO_OUTPUT_SVID0;
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_TVDAC;
>  		connector_type = DRM_MODE_CONNECTOR_SVIDEO;
>  		sdvo_priv->is_tv = true;
> @@ -1756,28 +1745,24 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  	else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_RGB0)
>  	{
>  		sdvo_priv->controlled_output = SDVO_OUTPUT_RGB0;
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_DAC;
>  		connector_type = DRM_MODE_CONNECTOR_VGA;
>  	}
>  	else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_RGB1)
>  	{
>  		sdvo_priv->controlled_output = SDVO_OUTPUT_RGB1;
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_DAC;
>  		connector_type = DRM_MODE_CONNECTOR_VGA;
>  	}
>  	else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_LVDS0)
>  	{
>  		sdvo_priv->controlled_output = SDVO_OUTPUT_LVDS0;
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_LVDS;
>  		connector_type = DRM_MODE_CONNECTOR_LVDS;
>  	}
>  	else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_LVDS1)
>  	{
>  		sdvo_priv->controlled_output = SDVO_OUTPUT_LVDS1;
> -		connector->display_info.subpixel_order = SubPixelHorizontalRGB;
>  		encoder_type = DRM_MODE_ENCODER_LVDS;
>  		connector_type = DRM_MODE_CONNECTOR_LVDS;
>  	}
> @@ -1795,9 +1780,16 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  		goto err_i2c;
>  	}
>  
> +	connector = &intel_output->base;
> +	drm_connector_init(dev, connector, &intel_sdvo_connector_funcs,
> +			   connector_type);
> +	drm_connector_helper_add(connector, &intel_sdvo_connector_helper_funcs);
> +	connector->interlace_allowed = 0;
> +	connector->doublescan_allowed = 0;
> +	connector->display_info.subpixel_order = SubPixelHorizontalRGB;
> +
>  	drm_encoder_init(dev, &intel_output->enc, &intel_sdvo_enc_funcs, encoder_type);
>  	drm_encoder_helper_add(&intel_output->enc, &intel_sdvo_helper_funcs);
> -	connector->connector_type = connector_type;
>  
>  	drm_mode_connector_attach_encoder(&intel_output->base, &intel_output->enc);
>  	drm_sysfs_connector_add(connector);
> @@ -1835,8 +1827,7 @@ bool intel_sdvo_init(struct drm_device *dev, int output_device)
>  
>  err_i2c:
>  	intel_i2c_destroy(intel_output->i2c_bus);
> -err_connector:
> -	drm_connector_cleanup(connector);
> +err_inteloutput:
>  	kfree(intel_output);
>  
>  	return false;