[Intel-gfx] [PATCH v2 1/2] drm: Create new structure for HDMI info

[Jose Abreu]

Hi Shashank,


On 21-12-2016 03:49, Sharma, Shashank wrote:
> Thanks for the review Jose.
>
> My comments, inline.
>
> Regards
> Shashank
> On 12/20/2016 7:49 PM, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> On 20-12-2016 13:47, Shashank Sharma wrote:
>>> This patch creates a new structure drm_hdmi_info (inspired from
>>> drm_display_info). Driver will parse HDMI sink's advance
>>> capabilities
>>> from HF-VSDB and populate this structure. This structure will
>>> be kept
>>> and used as a sub-class within drm_display_info.
>> You populate the structure but I think you should add a helper to
>> reset it when there is a new EDID or when the previous EDID is no
>> longer valid. The same applies to other fields in
>> drm_display_info structure. I've had problems before because of
>> incorrect values in this structure.
> I agree, will add a clean-up function too, and will attach it
> with hot-unplug.
>>> We are adding parsing of HF-VSDB In the next patch.
>>>
>>> Cc: Thierry Reding <treding at nvidia.com>
>>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>>> Cc: Jose Abreu <joabreu at synopsys.com>
>>> Suggested-by: Thierry Reding <thierry.reding at gmail.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>> ---
>>>   drivers/gpu/drm/drm_edid.c  |  6 ++--
>>>   include/drm/drm_connector.h | 79
>>> ++++++++++++++++++++++++++++++++++++++++++---
>>>   2 files changed, 77 insertions(+), 8 deletions(-)
>>>
>> [snip]
>>
>>>     /**
>>> + * struct drm_hdmi_info - runtime data specific to a
>>> connected hdmi sink
>>> + *
>>> + * Describes a given hdmi display (e.g. CRT or flat panel)
>>> and its capabilities.
>>> + * Mostly refects the advanced features added in HDMI 2.0
>>> specs and the deep
>>> + * color support. This is a sub-segment of struct
>>> drm_display_info and should be
>>> + * used within.
>>> + *
>>> + * For sinks which provide an EDID this can be filled out by
>>> calling
>>> + * drm_add_edid_modes().
>>> + */
>>> +
>>> +struct drm_hdmi_info {
>> [snip]
>>
>>> +
>>> +    /**
>>> +     * @edid_yuv420_dc_modes: bpc for deep color yuv420
>>> encoding.
>>> +     * various sinks can support 10/12/16 bit per channel deep
>>> +     * color encoding. edid_yuv420_dc_modes = 0 means sink
>>> doesn't
>>> +     * support deep color yuv420 encoding.
>>> +     */
>>> +    u8 edid_yuv420_dc_modes;
>>> +
>>> +
>>> +#define DRM_HFVSDB_SCDC_SUPPORT    (1<<7)
>>> +#define DRM_HFVSDB_SCDC_RR_CAP        (1<<6)
>>> +#define DRM_HFVSDB_SCRAMBLING        (1<<3)
>>> +#define DRM_HFVSDB_INDEPENDENT_VIEW    (1<<2)
>>> +#define DRM_HFVSDB_DUAL_VIEW        (1<<1)
>>> +#define DRM_HFVSDB_3D_OSD        (1<<0)
>>> +
>>> +    /**
>>> +     * @scdc_supported: Sink supports SCDC functionality.
>>> +     */
>>> +    bool scdc_supported;
>>> +
>>> +    /**
>>> +     * @scdc_rr_cap: Sink has SCDC read request capability.
>>> +     */
>>> +    bool scdc_rr_cap;
>>> +
>>> +    /**
>>> +     * @scrambling: Sync supports scrambling for <=340 Mcsc
>>> TMDS
>>> +     * char rates. Above 340 Mcsc rates, scrambling is
>>> always reqd.
>>> +     */
>>> +    bool scrambling;
>>> +
>>> +    /**
>>> +     * @independent_view_3d: Sink supports 3d independent
>>> view signaling
>>> +     * in HF-VSIF.
>>> +     */
>>> +    bool independent_view_3d;
>>> +
>>> +    /**
>>> +     * @dual_view_3d: Sink supports 3d dual view signaling
>>> in HF-VSIF.
>>> +     */
>>> +    bool dual_view_3d;
>>> +
>>> +    /**
>>> +     * @osd_disparity_3d: Sink supports 3d osd disparity
>>> indication
>>> +     * in HF-VSIF.
>>> +     */
>>> +    bool osd_disparity_3d;
>> Maybe you should only add these fields in the second patch.
> I thought it was a good idea to introduce the new fields for
> which we are adding this new structure all together. Else this
> patch would just contain movement of few parameters from main
> structure to new one, and would look unnecessary.  Do you think
> so ?

Yes, I think it is better. And besides, in this patch you also
have to change the drivers that use drm_display_info structure to
use the newly created drm_hdmi_info instead so, the diff will be
bigger. If you don't do so we'll have build errors.

Best regards,
Jose Miguel Abreu

>>
>> Best regards,
>> Jose Miguel Abreu
>