| Message ID | 1352478066-1077-3-git-send-email-rahul.sharma@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > conf_idx = hdmi_conf_index(hdata, mode); > if (conf_idx >= 0) > hdata->cur_conf = conf_idx; <snip> Thanks and Regards, - Seung-Woo Kim
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. 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. 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
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 >
On Thu, Nov 22, 2012 at 12:06 PM, ??? <sw0312.kim@samsung.com> 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@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. 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@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>
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 @@ -82,6 +82,7 @@ struct hdmi_context { /* current hdmiphy conf index */ int cur_conf; + int cur_video_id; struct hdmi_resources res; void *parent_ctx; @@ -944,6 +945,11 @@ static const struct hdmi_conf hdmi_confs[] = { { 1920, 1080, 60, false, hdmiphy_conf148_5, &hdmi_conf_1080p60 }, }; +struct hdmi_infoframe { + enum HDMI_PACKET_TYPE type; + u8 ver; + u8 len; +}; static inline u32 hdmi_reg_read(struct hdmi_context *hdata, u32 reg_id) { @@ -1267,6 +1273,81 @@ static int hdmi_conf_index(struct hdmi_context *hdata, return hdmi_v14_conf_index(mode); } +static u8 hdmi_chksum(struct hdmi_context *hdata, + u32 start, u8 len, u32 hdr_sum) +{ + int i; + /* hdr_sum : header0 + header1 + header2 + * start : start address of packet byte1 + * len : packet bytes - 1 */ + for (i = 0; i < len; ++i) + hdr_sum += 0xff & hdmi_reg_read(hdata, start + i * 4); + + return (u8)(0x100 - (hdr_sum & 0xff)); +} + +void hdmi_reg_infoframe(struct hdmi_context *hdata, + struct hdmi_infoframe *infoframe) +{ + u32 hdr_sum; + u8 chksum; + u32 aspect_ratio; + u32 mod; + + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); + + mod = hdmi_reg_read(hdata, HDMI_MODE_SEL); + if (hdata->dvi_mode) { + hdmi_reg_writeb(hdata, HDMI_VSI_CON, + HDMI_VSI_CON_DO_NOT_TRANSMIT); + hdmi_reg_writeb(hdata, HDMI_AVI_CON, + HDMI_AVI_CON_DO_NOT_TRANSMIT); + hdmi_reg_writeb(hdata, HDMI_AUI_CON, HDMI_AUI_CON_NO_TRAN); + return; + } + + switch (infoframe->type) { + + case HDMI_PACKET_TYPE_AVI: + hdmi_reg_writeb(hdata, HDMI_AVI_CON, HDMI_AVI_CON_EVERY_VSYNC); + hdmi_reg_writeb(hdata, HDMI_AVI_HEADER0, infoframe->type); + hdmi_reg_writeb(hdata, HDMI_AVI_HEADER1, infoframe->ver); + hdmi_reg_writeb(hdata, HDMI_AVI_HEADER2, infoframe->len); + hdr_sum = infoframe->type + infoframe->ver + infoframe->len; + /* Output format zero hardcoded ,RGB YBCR selection */ + hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(1), 0 << 5 | + AVI_ACTIVE_FORMAT_VALID | + AVI_UNDERSCANNED_DISPLAY_VALID); + + aspect_ratio = AVI_PIC_ASPECT_RATIO_16_9; + + hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(2), aspect_ratio | + AVI_SAME_AS_PIC_ASPECT_RATIO); + hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(4), hdata->cur_video_id); + + chksum = hdmi_chksum(hdata, HDMI_AVI_BYTE(1), + infoframe->len, hdr_sum); + DRM_DEBUG_KMS("AVI checksum = 0x%x\n", chksum); + hdmi_reg_writeb(hdata, HDMI_AVI_CHECK_SUM, chksum); + break; + + case HDMI_PACKET_TYPE_AUI: + hdmi_reg_writeb(hdata, HDMI_AUI_CON, 0x02); + hdmi_reg_writeb(hdata, HDMI_AUI_HEADER0, infoframe->type); + hdmi_reg_writeb(hdata, HDMI_AUI_HEADER1, infoframe->ver); + hdmi_reg_writeb(hdata, HDMI_AUI_HEADER2, infoframe->len); + hdr_sum = infoframe->type + infoframe->ver + infoframe->len; + chksum = hdmi_chksum(hdata, HDMI_AUI_BYTE(1), + infoframe->len, hdr_sum); + DRM_DEBUG_KMS("AUI checksum = 0x%x\n", chksum); + hdmi_reg_writeb(hdata, HDMI_AUI_CHECK_SUM, chksum); + break; + + default: + break; + } +} + static bool hdmi_is_connected(void *ctx) { struct hdmi_context *hdata = ctx; @@ -1541,6 +1622,8 @@ static void hdmi_conf_reset(struct hdmi_context *hdata) static void hdmi_conf_init(struct hdmi_context *hdata) { + struct hdmi_infoframe infoframe; + /* disable HPD interrupts */ hdmi_reg_writemask(hdata, HDMI_INTC_CON, 0, HDMI_INTC_EN_GLOBAL | HDMI_INTC_EN_HPD_PLUG | HDMI_INTC_EN_HPD_UNPLUG); @@ -1575,9 +1658,17 @@ static void hdmi_conf_init(struct hdmi_context *hdata) hdmi_reg_writeb(hdata, HDMI_V13_AUI_CON, 0x02); hdmi_reg_writeb(hdata, HDMI_V13_ACR_CON, 0x04); } else { + infoframe.type = HDMI_PACKET_TYPE_AVI; + infoframe.ver = HDMI_AVI_VERSION; + infoframe.len = HDMI_AVI_LENGTH; + hdmi_reg_infoframe(hdata, &infoframe); + + infoframe.type = HDMI_PACKET_TYPE_AUI; + infoframe.ver = HDMI_AUI_VERSION; + infoframe.len = HDMI_AUI_LENGTH; + hdmi_reg_infoframe(hdata, &infoframe); + /* enable AVI packet every vsync, fixes purple line problem */ - hdmi_reg_writeb(hdata, HDMI_AVI_CON, 0x02); - hdmi_reg_writeb(hdata, HDMI_AVI_BYTE(1), 2 << 5); hdmi_reg_writemask(hdata, HDMI_CON_1, 2, 3 << 5); } } @@ -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); + conf_idx = hdmi_conf_index(hdata, mode); if (conf_idx >= 0) hdata->cur_conf = conf_idx; diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.h b/drivers/gpu/drm/exynos/exynos_hdmi.h index 1c3b6d8..fc4de49 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_hdmi.h @@ -28,6 +28,29 @@ #ifndef _EXYNOS_HDMI_H_ #define _EXYNOS_HDMI_H_ +/* AVI header and aspect ratio */ +#define HDMI_AVI_VERSION 0x02 +#define HDMI_AVI_LENGTH 0x0d +#define AVI_PIC_ASPECT_RATIO_16_9 (2 << 4) +#define AVI_SAME_AS_PIC_ASPECT_RATIO 8 + +/* AUI header info */ +#define HDMI_AUI_VERSION 0x01 +#define HDMI_AUI_LENGTH 0x0a + +/* HDMI infoframe to configure HDMI out packet header, AUI and AVI */ +enum HDMI_PACKET_TYPE { + /** refer to Table 5-8 Packet Type in HDMI specification v1.4a */ + /** InfoFrame packet type */ + HDMI_PACKET_TYPE_INFOFRAME = 0X80, + /** Vendor-Specific InfoFrame */ + HDMI_PACKET_TYPE_VSI = HDMI_PACKET_TYPE_INFOFRAME + 1, + /** Auxiliary Video information InfoFrame */ + HDMI_PACKET_TYPE_AVI = HDMI_PACKET_TYPE_INFOFRAME + 2, + /** Audio information InfoFrame */ + HDMI_PACKET_TYPE_AUI = HDMI_PACKET_TYPE_INFOFRAME + 4 +}; + void hdmi_attach_ddc_client(struct i2c_client *ddc); void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy); diff --git a/drivers/gpu/drm/exynos/regs-hdmi.h b/drivers/gpu/drm/exynos/regs-hdmi.h index 9cc7c5e..970cdb5 100644 --- a/drivers/gpu/drm/exynos/regs-hdmi.h +++ b/drivers/gpu/drm/exynos/regs-hdmi.h @@ -298,14 +298,14 @@ #define HDMI_AVI_HEADER1 HDMI_CORE_BASE(0x0714) #define HDMI_AVI_HEADER2 HDMI_CORE_BASE(0x0718) #define HDMI_AVI_CHECK_SUM HDMI_CORE_BASE(0x071C) -#define HDMI_AVI_BYTE(n) HDMI_CORE_BASE(0x0720 + 4 * (n)) +#define HDMI_AVI_BYTE(n) HDMI_CORE_BASE(0x0720 + 4 * (n-1)) #define HDMI_AUI_CON HDMI_CORE_BASE(0x0800) #define HDMI_AUI_HEADER0 HDMI_CORE_BASE(0x0810) #define HDMI_AUI_HEADER1 HDMI_CORE_BASE(0x0814) #define HDMI_AUI_HEADER2 HDMI_CORE_BASE(0x0818) #define HDMI_AUI_CHECK_SUM HDMI_CORE_BASE(0x081C) -#define HDMI_AUI_BYTE(n) HDMI_CORE_BASE(0x0820 + 4 * (n)) +#define HDMI_AUI_BYTE(n) HDMI_CORE_BASE(0x0820 + 4 * (n-1)) #define HDMI_MPG_CON HDMI_CORE_BASE(0x0900) #define HDMI_MPG_CHECK_SUM HDMI_CORE_BASE(0x091C) @@ -338,6 +338,19 @@ #define HDMI_AN_SEED_2 HDMI_CORE_BASE(0x0E60) #define HDMI_AN_SEED_3 HDMI_CORE_BASE(0x0E64) +/* AVI bit definition */ +#define HDMI_AVI_CON_DO_NOT_TRANSMIT (0 << 1) +#define HDMI_AVI_CON_EVERY_VSYNC (1 << 1) + +#define AVI_ACTIVE_FORMAT_VALID (1 << 4) +#define AVI_UNDERSCANNED_DISPLAY_VALID (1 << 1) + +/* AUI bit definition */ +#define HDMI_AUI_CON_NO_TRAN (0 << 0) + +/* VSI bit definition */ +#define HDMI_VSI_CON_DO_NOT_TRANSMIT (0 << 0) + /* HDCP related registers */ #define HDMI_HDCP_SHA1(n) HDMI_CORE_BASE(0x7000 + 4 * (n)) #define HDMI_HDCP_KSV_LIST(n) HDMI_CORE_BASE(0x7050 + 4 * (n))