Re: [PATCH RESEND 1/5] drm/i915: move VBT based TV presence check to intel_bios.c

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 08 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com> wrote:
> On 2/5/2016 4:06 PM, Jani Nikula wrote:
>> Hide knowledge about VBT child devices in intel_bios.c.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |  1 +
>>   drivers/gpu/drm/i915/intel_bios.c | 38 ++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_tv.c   | 43 +--------------------------------------
>>   3 files changed, 40 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 77227a39f3d5..715f200cfbf5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3397,6 +3397,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
>>   /* intel_bios.c */
>>   int intel_bios_init(struct drm_i915_private *dev_priv);
>>   bool intel_bios_is_valid_vbt(const void *buf, size_t size);
>> +bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv);
>>   
>>   /* intel_opregion.c */
>>   #ifdef CONFIG_ACPI
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index bf62a19c8f69..2800ae50465a 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1431,3 +1431,41 @@ intel_bios_init(struct drm_i915_private *dev_priv)
>>   
>>   	return 0;
>>   }
>> +
>> +/**
>> + * intel_bios_is_tv_present - is integrated TV present in VBT
>> + * @dev_priv:	i915 device instance
>> + *
>> + * Return true if TV is present. If no child devices were parsed from VBT,
>> + * assume TV is present.
>> + */
>> +bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv)
>> +{
>> +	union child_device_config *p_child;
>> +	int i;
>> +
>> +	if (!dev_priv->vbt.child_dev_num)
>> +		return true;
>> +
> This is old code moved here, but should we return true
> when child_dev_num == 0 ? I understand we shouldn't make
> functional changes when moving code but just pointing out
> to check if this can be fixed in the further patches.

I have no idea, but I'm guessing it's there for historical reasons. No
reason to change it now as the code gets run only for some gen 3/4
hardware.

>> +	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> +		p_child = dev_priv->vbt.child_dev + i;
>> +		/*
>> +		 * If the device type is not TV, continue.
>> +		 */
>> +		switch (p_child->old.device_type) {
>> +		case DEVICE_TYPE_INT_TV:
>> +		case DEVICE_TYPE_TV:
>> +		case DEVICE_TYPE_TV_SVIDEO_COMPOSITE:
>> +			break;
>> +		default:
>> +			continue;
>> +		}
>> +		/* Only when the addin_offset is non-zero, it is regarded
>> +		 * as present.
>> +		 */
>> +		if (p_child->old.addin_offset)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
>> index 5034b0055169..8417fcad02d2 100644
>> --- a/drivers/gpu/drm/i915/intel_tv.c
>> +++ b/drivers/gpu/drm/i915/intel_tv.c
>> @@ -1530,47 +1530,6 @@ static const struct drm_encoder_funcs intel_tv_enc_funcs = {
>>   	.destroy = intel_encoder_destroy,
>>   };
>>   
>> -/*
>> - * Enumerate the child dev array parsed from VBT to check whether
>> - * the integrated TV is present.
>> - * If it is present, return 1.
>> - * If it is not present, return false.
>> - * If no child dev is parsed from VBT, it assumes that the TV is present.
>> - */
>> -static int tv_is_present_in_vbt(struct drm_device *dev)
>> -{
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	union child_device_config *p_child;
>> -	int i, ret;
>> -
>> -	if (!dev_priv->vbt.child_dev_num)
>> -		return 1;
>> -
>> -	ret = 0;
>> -	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> -		p_child = dev_priv->vbt.child_dev + i;
>> -		/*
>> -		 * If the device type is not TV, continue.
>> -		 */
>> -		switch (p_child->old.device_type) {
>> -		case DEVICE_TYPE_INT_TV:
>> -		case DEVICE_TYPE_TV:
>> -		case DEVICE_TYPE_TV_SVIDEO_COMPOSITE:
>> -			break;
>> -		default:
>> -			continue;
>> -		}
>> -		/* Only when the addin_offset is non-zero, it is regarded
>> -		 * as present.
>> -		 */
>> -		if (p_child->old.addin_offset) {
>> -			ret = 1;
>> -			break;
>> -		}
>> -	}
>> -	return ret;
>> -}
>> -
>>   void
>>   intel_tv_init(struct drm_device *dev)
>>   {
>> @@ -1586,7 +1545,7 @@ intel_tv_init(struct drm_device *dev)
>>   	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>>   		return;
>>   
>> -	if (!tv_is_present_in_vbt(dev)) {
>> +	if (!intel_bios_is_tv_present(dev_priv)) {
>>   		DRM_DEBUG_KMS("Integrated TV is not present.\n");
>>   		return;
>>   	}
> saw below line immediately after the tv_is_present_in_vbt() call.
> can you move this inside the new function as well since we
> consider it as well before enumerating tv encoder.

Makes sense.

BR,
Jani.

>
> 1593         /* Even if we have an encoder we may not have a connector */
> 1594         if (!dev_priv->vbt.int_tv_support)
> 1595                 return;
> 1596
>
> regards,
> Sivakumar
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx