[Intel-gfx] [Resend][PATCH 3/4] drm/i915: Construct all possible sdvo outputs for one sdvo port
Ma, Ling
ling.ma at intel.com
Tue Jun 30 05:24:35 CEST 2009
>-----Original Message-----
>From: Ian Romanick [mailto:idr at freedesktop.org]
>Sent: 2009年6月30日 0:35
>To: Ma, Ling
>Cc: intel-gfx at lists.freedesktop.org
>Subject: Re: [Intel-gfx] [Resend][PATCH 3/4] drm/i915: Construct all possible
>sdvo outputs for one sdvo port
>
>-----BEGIN PGP SIGNED MESSAGE-----
>Hash: SHA1
>
>ling.ma at intel.com wrote:
>> Some SDVO encoders will advertise multiple functions,
>> We will be aware of the real one until sdvo output detection
>> completes, sometime we have to change connector and encoder
>> type, it looks strange for users. So we construct all possible
>> sdvo outputs at initialization time.
>>
>> Signed-off-by: Ma Ling <ling.ma at intel.com>
>> ---
>> update against with latest version
>>
>> drivers/gpu/drm/i915/intel_sdvo.c | 99
>++++++++++++++++++++++++-------------
>> 1 files changed, 65 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
>b/drivers/gpu/drm/i915/intel_sdvo.c
>> index 0407889..6a2520e 100644
>> --- a/drivers/gpu/drm/i915/intel_sdvo.c
>> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
>> @@ -1449,19 +1449,20 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector
>*connector)
>>
>> static enum drm_connector_status intel_sdvo_detect(struct drm_connector
>*connector)
>> {
>> - u8 response[2];
>> + u16 response;
>> u8 status;
>> struct intel_output *intel_output = to_intel_output(connector);
>> + struct intel_sdvo_priv *sdvo_priv = intel_output->dev_priv;
>>
>> intel_sdvo_write_cmd(intel_output, SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL,
>0);
>> status = intel_sdvo_read_response(intel_output, &response, 2);
>>
>> - DRM_DEBUG("SDVO response %d %d\n", response[0], response[1]);
>> + DRM_DEBUG("SDVO response %d %d\n", response & 0xff, response >> 8);
>>
>> if (status != SDVO_CMD_STATUS_SUCCESS)
>> return connector_status_unknown;
>>
>> - if ((response[0] != 0) || (response[1] != 0)) {
>> + if (response & sdvo_priv->controlled_output) {
>> intel_sdvo_hdmi_sink_detect(connector);
>> return connector_status_connected;
>> } else
>> @@ -1861,7 +1862,10 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, int
>output_device)
>> return 0x72;
>> }
>>
>> -bool intel_sdvo_init(struct drm_device *dev, int output_device)
>> +
>> +#define MAX_SDVO_OUTPUTS_BITS 0xFFFF
>> +static int intel_sdvo_outputs_init(struct drm_device *dev, int
>output_device,
>> + int sdvo_output_id, int sdvo_output_flags)
>> {
>> struct drm_connector *connector;
>> struct intel_output *intel_output;
>> @@ -1874,7 +1878,7 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>>
>> intel_output = kcalloc(sizeof(struct intel_output)+sizeof(struct
>intel_sdvo_priv), 1, GFP_KERNEL);
>> if (!intel_output) {
>> - return false;
>> + return -1;
>> }
>>
>> sdvo_priv = (struct intel_sdvo_priv *)(intel_output + 1);
>> @@ -1884,10 +1888,13 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>> intel_output->type = INTEL_OUTPUT_SDVO;
>>
>> /* setup the DDC bus. */
>> - if (output_device == SDVOB)
>> - intel_output->i2c_bus = intel_i2c_create(dev, GPIOE, "SDVOCTRL_E for
>SDVOB");
>> - else
>> - intel_output->i2c_bus = intel_i2c_create(dev, GPIOE, "SDVOCTRL_E for
>SDVOC");
>> + if (output_device == SDVOB) {
>> + sprintf(ch, "%s_%d", "SDVOCTRL_E for SDVOB", sdvo_output_id);
>> + intel_output->i2c_bus = intel_i2c_create(dev, GPIOE, (const char
>*)ch);
>> + } else {
>> + sprintf(ch, "%s_%d", "SDVOCTRL_E for SDVOC", sdvo_output_id);
>> + intel_output->i2c_bus = intel_i2c_create(dev, GPIOE, (const char
>*)ch);
>> + }
>
>Either make these 'sprintf(ch, "SDVOCTRL_E for SDVOC %d",
>sdvo_output_id);' or
>
> static const char fmt[] = "SDVOCTRL_E for SDVO%c_%d";
> ...
> sprintf(ch, fmt, 'C', sdvo_output_id);
>
>>
Good idea, fixed.
>> if (!intel_output->i2c_bus)
>> goto err_inteloutput;
>> @@ -1908,10 +1915,13 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>> }
>>
>> /* setup the DDC bus. */
>> - if (output_device == SDVOB)
>> - intel_output->ddc_bus = intel_i2c_create(dev, GPIOE, "SDVOB DDC
>BUS");
>> - else
>> - intel_output->ddc_bus = intel_i2c_create(dev, GPIOE, "SDVOC DDC
>BUS");
>> + if (output_device == SDVOB) {
>> + sprintf(ch, "%s_%d", "SDVOB DDC BUS", sdvo_output_id);
>> + intel_output->ddc_bus = intel_i2c_create(dev, GPIOE, (const char
>*)ch);
>> + } else {
>> + sprintf(ch, "%s_%d", "SDVOC DDC BUS", sdvo_output_id);
>> + intel_output->ddc_bus = intel_i2c_create(dev, GPIOE, (const char
>*)ch);
>> + }
>
>Same comment as above.
>
>>
>> if (intel_output->ddc_bus == NULL)
>> goto err_i2c;
>> @@ -1923,7 +1933,10 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>> sdvo_priv->is_lvds = false;
>> intel_sdvo_get_capabilities(intel_output, &sdvo_priv->caps);
>>
>> - if (sdvo_priv->caps.output_flags &
>> + if (sdvo_output_flags == MAX_SDVO_OUTPUTS_BITS)
>> + sdvo_output_flags = sdvo_priv->caps.output_flags;
>> +
>> + if (sdvo_priv->caps.output_flags & sdvo_output_flags &
>> (SDVO_OUTPUT_TMDS0 | SDVO_OUTPUT_TMDS1)) {
>> if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_TMDS0)
>> sdvo_priv->controlled_output = SDVO_OUTPUT_TMDS0;
>> @@ -1943,43 +1956,41 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>> SDVO_COLORIMETRY_RGB256);
>> connector_type = DRM_MODE_CONNECTOR_HDMIA;
>> }
>> - }
>> - else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_SVID0)
>> - {
>> + } else if (sdvo_priv->caps.output_flags &
>> + sdvo_output_flags & SDVO_OUTPUT_SVID0) {
>> +
>
>Since sdvo_priv->caps.output_flags is always masked with
>sdvo_output_flags, it would be more clear to create a new variable with
>the masked value:
>
> const u16 maked_output_flags = sdvo_priv->caps.output_flags &
> sdvo_output_flags;
>
Good idea, fixed.
>Then use masked_output_flags in the tests.
>
>> sdvo_priv->controlled_output = SDVO_OUTPUT_SVID0;
>> encoder_type = DRM_MODE_ENCODER_TVDAC;
>> connector_type = DRM_MODE_CONNECTOR_SVIDEO;
>> sdvo_priv->is_tv = true;
>> intel_output->needs_tv_clock = true;
>> - }
>> - else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_RGB0)
>> - {
>> + } else if (sdvo_priv->caps.output_flags &
>> + sdvo_output_flags & SDVO_OUTPUT_RGB0) {
>> +
>> sdvo_priv->controlled_output = SDVO_OUTPUT_RGB0;
>> encoder_type = DRM_MODE_ENCODER_DAC;
>> connector_type = DRM_MODE_CONNECTOR_VGA;
>> - }
>> - else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_RGB1)
>> - {
>> + } else if (sdvo_priv->caps.output_flags &
>> + sdvo_output_flags & SDVO_OUTPUT_RGB1) {
>> +
>> sdvo_priv->controlled_output = SDVO_OUTPUT_RGB1;
>> encoder_type = DRM_MODE_ENCODER_DAC;
>> connector_type = DRM_MODE_CONNECTOR_VGA;
>> - }
>> - else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_LVDS0)
>> - {
>> + } else if (sdvo_priv->caps.output_flags &
>> + sdvo_output_flags & SDVO_OUTPUT_LVDS0) {
>> +
>> sdvo_priv->controlled_output = SDVO_OUTPUT_LVDS0;
>> encoder_type = DRM_MODE_ENCODER_LVDS;
>> connector_type = DRM_MODE_CONNECTOR_LVDS;
>> sdvo_priv->is_lvds = true;
>> - }
>> - else if (sdvo_priv->caps.output_flags & SDVO_OUTPUT_LVDS1)
>> - {
>> + } else if (sdvo_priv->caps.output_flags &
>> + sdvo_output_flags & SDVO_OUTPUT_LVDS1) {
>> +
>> sdvo_priv->controlled_output = SDVO_OUTPUT_LVDS1;
>> encoder_type = DRM_MODE_ENCODER_LVDS;
>> connector_type = DRM_MODE_CONNECTOR_LVDS;
>> sdvo_priv->is_lvds = true;
>> - }
>> - else
>> - {
>> + } else {
>> unsigned char bytes[2];
>>
>> sdvo_priv->controlled_output = 0;
>> @@ -2034,7 +2045,7 @@ bool intel_sdvo_init(struct drm_device *dev, int
>output_device)
>> sdvo_priv->caps.output_flags &
>> (SDVO_OUTPUT_TMDS1 | SDVO_OUTPUT_RGB1) ? 'Y' : 'N');
>>
>> - return true;
>> + return sdvo_output_flags & (~sdvo_priv->controlled_output);
>>
>> err_i2c:
>> if (intel_output->ddc_bus != NULL)
>> @@ -2044,5 +2055,25 @@ err_i2c:
>> err_inteloutput:
>> kfree(intel_output);
>>
>> - return false;
>> + return -1;
>> +}
>> +
>> +bool intel_sdvo_init(struct drm_device *dev, int output_device)
>> +{
>> + int sdvo_output_id;
>> + int sdvo_output_flags = MAX_SDVO_OUTPUTS_BITS;
>> + bool ret = false;
>> +
>> + for (sdvo_output_id = 1; sdvo_output_flags > 0; sdvo_output_id++) {
>> +
>> + sdvo_output_flags =
>> + intel_sdvo_outputs_init(dev, output_device,
>> + sdvo_output_id,
>> + sdvo_output_flags);
>> + if (sdvo_output_flags >= 0)
>> + ret = true;
>> + }
>
>What if intel_sdvo_outputs_init never returns success? It would be bad
>to have an infinite loop in the kernel. :)
Once return -1 from intel_sdvo_outputs_init, sdvo_output_flags will be less or equal to 0, program will exit.
Thanks
Ma Ling
>
>> +
>> + return ret;
>> }
>> +
>
>-----BEGIN PGP SIGNATURE-----
>Version: GnuPG v1.4.9 (GNU/Linux)
>Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
>iEYEARECAAYFAkpI7TIACgkQX1gOwKyEAw9qEQCghPziBp61R/wT99I8MdnwT2L6
>qXcAnA3dHroJOJdgtczxIiVHJmPBHkwa
>=dDrZ
>-----END PGP SIGNATURE-----
More information about the Intel-gfx
mailing list