[Intel-gfx] [PATCH] drm/i915: set the AVI VIC of the HDMI mode

[Paulo Zanoni]

Hi

2012/11/22 Thierry Reding <thierry.reding at avionic-design.de>:
> On Wed, Nov 21, 2012 at 01:39:48PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> We currently set "0" as the VIC value of the AVI InfoFrames. According
>> to the specs this should be fine and work for every mode, so to my
>> point of view we can't consider the current behavior as a bug. The
>> problem is that  we recently received a bug report (Kernel bug #50371)
>> from a user that has an AV receiver that gives a black screen for any
>> mode with VIC set to 0.
>>
>> So in order to make at least some modes work for him, this patch sets
>> the correct VIC number when sending AVI InfoFrames. We add a generic
>> drm function to calculate the VIC number and then call it from the
>> i915 driver.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=50371
>> Cc: Thierry Reding <thierry.reding at avionic-design.de>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c        |   19 +++++++++++++++++++
>>  drivers/gpu/drm/drm_modes.c       |    3 ++-
>>  drivers/gpu/drm/i915/intel_hdmi.c |    2 ++
>>  include/drm/drm_crtc.h            |    4 +++-
>>  4 files changed, 26 insertions(+), 2 deletions(-)
>>
>> Patch applies on top of Daniel's drm-intel-next-queued. I'm not sure who exactly
>> is going to merge this (Dave or Daniel).
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index fadcd44..c57fc46 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2062,3 +2062,22 @@ int drm_add_modes_noedid(struct drm_connector *connector,
>>       return num_modes;
>>  }
>>  EXPORT_SYMBOL(drm_add_modes_noedid);
>> +
>> +/**
>> + * drm_mode_vic - return the CEA-861 VIC of a given mode
>
> The name in the comment here doesn't match the actual function name.

Will fix.

>
>> + * @mode: mode
>> + *
>> + * RETURNS:
>> + * The VIC number, 0 in case it's not a CEA-861 mode.
>> + */
>> +uint8_t drm_mode_cea_vic(const struct drm_display_mode *mode)
>
> I understand the reason for returning an uint8_t here, but ...
>
>> +{
>> +     uint8_t i;
>> +
>> +     for (i = 0; i < drm_num_cea_modes; i++)
>
> ... maybe unsigned int would be better for the iteration variable here.
> Looking at drm_edid_modes.h, drm_num_cea_modes is actually signed, which
> isn't necessary to store an array size, so maybe that should be changed
> as well.

I used uint8_t because "i" is the thing we return. I don't think
there's a perfect solution to this problem and I don't really think it
matters too much.

>
>> +             if (drm_mode_equal(mode, &edid_cea_modes[i]))
>> +                     return i + 1;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mode_cea_vic);
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 59450f3..9ef6750 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -768,7 +768,8 @@ EXPORT_SYMBOL(drm_mode_duplicate);
>>   * RETURNS:
>>   * True if the modes are equal, false otherwise.
>>   */
>> -bool drm_mode_equal(struct drm_display_mode *mode1, struct drm_display_mode *mode2)
>> +bool drm_mode_equal(const struct drm_display_mode *mode1,
>> +                 const struct drm_display_mode *mode2)
>
> I think this change warrants a separate commit.

I just noticed Dave's tree already has these things as const. So V2
will be based on Dave's drm-next tree.

>
> Thierry



-- 
Paulo Zanoni