[Intel-gfx] [PATCH] drm: add support for additional stereo 3D modes

[Thomas Wood]

On 11 October 2013 12:12, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Thu, Oct 10, 2013 at 02:19:15PM +0100, Thomas Wood wrote:
>> Parse the 3D_Structure_ALL and 3D_MASK fields of the HDMI Vendor
>> Specific Data Block to expose more stereo 3D modes.
>>
>> Signed-off-by: Thomas Wood <thomas.wood at intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 93 ++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 85 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 9e81609..b3949f9 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2652,6 +2652,50 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
>>       return 1;
>>  }
>>
>> +static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>> +                            const u8 *video_db, u8 video_len, u8 video_index)
>> +{
>> +     struct drm_device *dev = connector->dev;
>> +     struct drm_display_mode *newmode;
>> +     int modes = 0;
>> +     u8 cea_mode;
>> +
>> +     if (video_db == NULL || video_index > video_len)
>> +             return 0;
>> +
>> +     /* CEA modes are numbered 1..127 */
>> +     cea_mode = (video_db[video_index] & 127) - 1;
>> +     if (cea_mode >= ARRAY_SIZE(edid_cea_modes))
>> +             return 0;
>> +
>> +     if (structure & 1) {
>
> Could use (1 << 0) for consistency.

I've updated this for v2.

>
> I'm also wondering if some displays might include some of the mandatory
> modes in 3D_Structure_ALL, and if so should we filter out the
> duplicates?

As Damien mentioned, duplicates are filtered out later on.

>
>> +     if ((multi_present == 1 || multi_present == 2) &&
>> +         len >= (9 + offset)) {
>
> If multi_present==2 and len is too small for the mask, I think we should
> skip adding the modes since the block is clearly incorrect/corrupted.
>
> So maybe just something like this:
>  if ((multi_present == 1 && len < (9 + offset)) ||
>      (multi_present == 2 && len < (11 + offset)))
>      goto out;

I've added this check to v2 and also made sure multi_present is either 1 or 2
before continuing.

>
> I would also add a similar check for HDMI_3D_LEN since that is
> supposed to include the space required by 3D_Structure_ALL and
> 3D_MASK. Or you could just check HDMI_3D_LEN against 'len' and bail
> out if that doesn't fit. And then you could just check
> 3D_Structure_ALL and 3D_MASK against HDMI_3D_LEN.
>

I've added a check to ensure the value of HDMI_3D_LEN is large enough to
include 3D_Structure_ALL and 3D_MASK, if they are present.