On Mon, 08 Feb 2016, "Thulasimani, Sivakumar" <sivakumar.thulasimani@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
