On Thu, Nov 22, 2012 at 12:06 PM, 김승우 <sw0312.kim@xxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>> Signed-off-by: Shirish S <s.shirish@xxxxxxxxxxx>
>>>> Signed-off-by: Rahul Sharma <rahul.sharma@xxxxxxxxxxx>
>>>> ---
>>>> 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.
You are right. This ambiguity is still present about the video code when
drm framework sets the mode to hdmi. hdmi_check_timing also doesn't care
about picture aspect ratio. I am not sure how to get exact vic from the mode.
I have submitted another patch that where vic is provided the hdmi_conf.
I preferred 16:9 aspect ratio. Kindly review that.
regards,
Rahul Sharma
>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
