Re: [PATCH 2/2] drm: exynos: compose and send avi and aui info frames

[김승우 <sw0312.kim@samsung.com>]

On 2012년 11월 21일 20:36, Rahul Sharma wrote:
> Hi Seung Woo,
> 
> Thanks for your inputs. Please find my response below.
> 
> On Wed, Nov 21, 2012 at 2:12 PM, 김승우 <sw0312.kim@samsung.com> wrote:
>> Hi Rahul,
>>
>> Control part seems good, and my comment is below.
>>
>> On 2012년 11월 10일 01:21, Rahul Sharma wrote:
>>> This patch adds code for composing AVI and AUI info frames
>>> and send them every VSYNC.
>>>
>>> This patch is important for hdmi certification.
>>>
>>> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
>>> Signed-off-by: Shirish S <s.shirish@samsung.com>
>>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_hdmi.c |   97 +++++++++++++++++++++++++++++++++-
>>>  drivers/gpu/drm/exynos/exynos_hdmi.h |   23 ++++++++
>>>  drivers/gpu/drm/exynos/regs-hdmi.h   |   17 +++++-
>>>  3 files changed, 133 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> index 2c115f8..bb8a045 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
>>
>> <snip>
>>
>>> @@ -1993,6 +2084,8 @@ static void hdmi_mode_set(void *ctx, void *mode)
>>>
>>>       DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>>>
>>> +     hdata->cur_video_id = drm_match_cea_mode(mode);
>>> +
>>
>> How do you think about using predefined cea video id in struct
>> hdmi_conf? drm_mode does not have cea video id, so drm_match_cea_mode()
>> compares only mode information. Considering this, IMHO, cea video id can
>> be embedded in struct hdmi_conf.
>>
> 
> I feel, It will leads to duplication of video id information. In
> edid_cea_modes, modes are
> strictly arranged in the order of respective cea video ID codes.
> "drm_add_edid_modes"
> also passes the cea codes (recieved after edid data parsing) as the index to
> edid_cea_modes to get mode details.

It might be a concern related with your first patch, anyway
edid_cea_modes has few pair of exact same modes because struct drm_mode
does not have picture ratio. For example, video id 2 and 3 have exact
same values for struct drm_mode. So cea video id can be used to get a
mode, but a drm_mode is not sufficient to get exact video id.
Considering that exynos hdmi does not support video ids with same mode,
I suggested video id in struct hdmi_conf.
At the point of exynos drm, I can ack this patch.

> 
> Secondly, mode to cea code translation is required by all platforms
> for AVI packet
> composition. By adding it to hdmi_conf, we are limiting its usage for exynos.

I agree with you at this point. I quickly checked i915 and radeon and I
found that they use fixed value for avi packet at sw level, but I don't
have information hw can properly build avi packet. If they also need
video id for building avi packet, video id translation can be used.

Best Regards,
- Seung-Woo Kim

> 
> regards,
> Rahul Sharma.
> 
>>>       conf_idx = hdmi_conf_index(hdata, mode);
>>>       if (conf_idx >= 0)
>>>               hdata->cur_conf = conf_idx;
>>
>> <snip>
>>
>> Thanks and Regards,
>> - Seung-Woo Kim
>>
>> --
>> Seung-Woo Kim
>> Samsung Software R&D Center
>> --
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel