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@xxxxxxxxxx>
>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>>> Cc: Jose Abreu <joabreu@xxxxxxxxxxxx>
>>> Suggested-by: Thierry Reding <thierry.reding@xxxxxxxxx>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>>> ---
>>> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
