Re: [PATCH v2 1/2] drm: Create new structure for HDMI info

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards

Shashank


On 12/21/2016 3:02 PM, Jose Abreu wrote:
> 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@nvidia.com>
>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Jose Abreu <joabreu@synopsys.com>
>>>> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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
Thanks, and yes, I will extend this patch to update other drivers using 
this structure.
Shashank
>>> Best regards,
>>> Jose Miguel Abreu

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel