| Message ID | 1354725944-1862-6-git-send-email-thierry.reding@avionic-design.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 05, 2012 at 05:45:44PM +0100, Thierry Reding wrote: > Use the generic HDMI infoframe helpers to get rid of the duplicate > implementation in the i915 driver. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 4b07401..192d791 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -29,6 +29,7 @@ > #include <linux/slab.h> > #include <linux/delay.h> > #include <linux/export.h> > +#include <linux/hdmi.h> > #include <drm/drmP.h> > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > > static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) > { > - struct dip_infoframe avi_if = { > - .type = DIP_TYPE_AVI, > - .ver = DIP_VERSION_AVI, > - .len = DIP_LEN_AVI, > - }; > - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; > + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE]; > + struct hdmi_avi_infoframe frame; > + ssize_t len; > > - intel_dip_infoframe_csum(&avi_if); > + len = hdmi_avi_infoframe_init(&frame); > + if (len < 0) > + return false; > > - /* sdvo spec says that the ecc is handled by the hw, and it looks like > - * we must not send the ecc field, either. */ > - memcpy(sdvo_data, &avi_if, 3); > - sdvo_data[3] = avi_if.checksum; This difference between how we need to send the infoframe to the hw between sdvo (without ecc field present) and native hdmi (ecc field present, but still computed by the hw) seems to have been lost. On a quick read from your infoframe packer it looks like the ecc field isn't part of the packed infoframe, so this patch would break native hdmi ports (in the intel_hdmi.c file). -Daniel
On Thu, Dec 06, 2012 at 03:16:31PM +0100, Daniel Vetter wrote: > On Wed, Dec 05, 2012 at 05:45:44PM +0100, Thierry Reding wrote: > > Use the generic HDMI infoframe helpers to get rid of the duplicate > > implementation in the i915 driver. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index 4b07401..192d791 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -29,6 +29,7 @@ > > #include <linux/slab.h> > > #include <linux/delay.h> > > #include <linux/export.h> > > +#include <linux/hdmi.h> > > #include <drm/drmP.h> > > #include <drm/drm_crtc.h> > > #include <drm/drm_edid.h> > > @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > > > > static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) > > { > > - struct dip_infoframe avi_if = { > > - .type = DIP_TYPE_AVI, > > - .ver = DIP_VERSION_AVI, > > - .len = DIP_LEN_AVI, > > - }; > > - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; > > + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE]; > > + struct hdmi_avi_infoframe frame; > > + ssize_t len; > > > > - intel_dip_infoframe_csum(&avi_if); > > + len = hdmi_avi_infoframe_init(&frame); > > + if (len < 0) > > + return false; > > > > - /* sdvo spec says that the ecc is handled by the hw, and it looks like > > - * we must not send the ecc field, either. */ > > - memcpy(sdvo_data, &avi_if, 3); > > - sdvo_data[3] = avi_if.checksum; > > This difference between how we need to send the infoframe to the hw > between sdvo (without ecc field present) and native hdmi (ecc field > present, but still computed by the hw) seems to have been lost. On a quick > read from your infoframe packer it looks like the ecc field isn't part of > the packed infoframe, so this patch would break native hdmi ports (in the > intel_hdmi.c file). Actually this ECC field seems to be something specific to Intel hardware. It doesn't appear in the HDMI specification nor in CEA-861-D. So it the binary infoframe representation doesn't contain it, therefore it should be handled by the respective drivers. So in fact the code should be working fine for HDMI and SDVO, since for the HDMI ports I explicitly left the fourth byte of the header empty, while for SDVO, the checksum is the one as computed by hdmi_avi_infoframe_pack(), which is at byte 3 and therefore matches the previous code. Thierry
On Thu, Dec 6, 2012 at 3:23 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: >> > - /* sdvo spec says that the ecc is handled by the hw, and it looks like >> > - * we must not send the ecc field, either. */ >> > - memcpy(sdvo_data, &avi_if, 3); >> > - sdvo_data[3] = avi_if.checksum; >> >> This difference between how we need to send the infoframe to the hw >> between sdvo (without ecc field present) and native hdmi (ecc field >> present, but still computed by the hw) seems to have been lost. On a quick >> read from your infoframe packer it looks like the ecc field isn't part of >> the packed infoframe, so this patch would break native hdmi ports (in the >> intel_hdmi.c file). > > Actually this ECC field seems to be something specific to Intel > hardware. It doesn't appear in the HDMI specification nor in CEA-861-D. > So it the binary infoframe representation doesn't contain it, therefore > it should be handled by the respective drivers. So in fact the code > should be working fine for HDMI and SDVO, since for the HDMI ports I > explicitly left the fourth byte of the header empty, while for SDVO, the > checksum is the one as computed by hdmi_avi_infoframe_pack(), which is > at byte 3 and therefore matches the previous code. Oh dear, I've failed to read the patch correctly and totally missed the new intel_dip_pack function and assorted logic changes. Sorry for the confusion. -Daniel
On Thu, Dec 06, 2012 at 04:57:27PM +0100, Daniel Vetter wrote: > On Thu, Dec 6, 2012 at 3:23 PM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > >> > - /* sdvo spec says that the ecc is handled by the hw, and it looks like > >> > - * we must not send the ecc field, either. */ > >> > - memcpy(sdvo_data, &avi_if, 3); > >> > - sdvo_data[3] = avi_if.checksum; > >> > >> This difference between how we need to send the infoframe to the hw > >> between sdvo (without ecc field present) and native hdmi (ecc field > >> present, but still computed by the hw) seems to have been lost. On a quick > >> read from your infoframe packer it looks like the ecc field isn't part of > >> the packed infoframe, so this patch would break native hdmi ports (in the > >> intel_hdmi.c file). > > > > Actually this ECC field seems to be something specific to Intel > > hardware. It doesn't appear in the HDMI specification nor in CEA-861-D. > > So it the binary infoframe representation doesn't contain it, therefore > > it should be handled by the respective drivers. So in fact the code > > should be working fine for HDMI and SDVO, since for the HDMI ports I > > explicitly left the fourth byte of the header empty, while for SDVO, the > > checksum is the one as computed by hdmi_avi_infoframe_pack(), which is > > at byte 3 and therefore matches the previous code. > > Oh dear, I've failed to read the patch correctly and totally missed > the new intel_dip_pack function and assorted logic changes. Sorry for > the confusion. At first I thought to split that out in a separate patch but then decided it wasn't worth the trouble. But there's always the possibility that I've broken things horribly here since the patch is rather messy, so it's good that you look closely and some testing will certainly be good to validate that things still work as before. Thierry
Hi 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>: > Use the generic HDMI infoframe helpers to get rid of the duplicate > implementation in the i915 driver. A few comments: - I've compiled your patches and now "intel_infoframes -d" tells my my infoframes are full of zeroes. So there's a bug somewhere... I have to say that the i915 infoframe registers are quite complicated and it took me a long time to fix a lot of its problems, so please don't break it and read below for a suggestion :) - Why do we need that kconfig stuff? Why not just put all the code inside drivers/gpu/drm? - I really like having "common code between drivers merged", but I really don't see how the i915 driver is benefiting from this change. We're just basically complicating intel_hdmi.c to use a new struct that doesn't fit our needs due to the lack of the ECC byte. My idea: Instead of calling hdmi_avi_infoframe_pack, why don't we just implement intel_hdmi_avi_infoframe_pack that converts a "struct hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that has the cool ECC value in the right place? This would probably drastically reduce your chances of introducing bugs in our code :) - Instead of "converting our own structs into structs that don't exactly fit our usage", what I would really like to see is generic _optional_ drm functions to help me fill the infoframe data, like the things I did in the past. Examples: (i) I want a function that reads the EDID and tells me whether the monitor is going to respect my "underscan" settings (AVI infoframe field S) or not. (ii) I want unified overscan/underscan properties between all the drivers: we need properties to allow requesting specific overscan/underscan properties and also properties to allow the driver to force underscan in case the monitor doesn't want to cooperate (as far as I remember, radeon has these properties, but we need to have the behavior explicitly documented so all the drivers can try to implement the exact same thing, and then we also need to ask user-space guys to add support for these things in their GUIs). (iii) I want a function that helps me properly setting/changing the correct pixel and picture aspect ratios. it seems the only difference between some modes (with different VICs) is the pixel/aspect ratio, how do we expose this to the user-space and let it select the one it wants? How does the driver know exactly which one is the selected one? (iv) I want code that enables user-space to use those modes with variable pixel repetition rates and select exactly the one it wants to use with the correct pixel repetition. (v) I'd like a quirk list to recognize monitors/devices that don't work correctly when they receive infoframes, or when the VIC is 0, etc. (vi) I want magical functions that help me setting all those color-related stuff I did not study yet. (vii) Ideally, I'd like to accomplish all the above by looking at "struct drm_display_mode" and maybe passing it to helper functions that I can _optionally_ use if I want. (viii) I don't want the infoframe code to be a "mid layer" that gets in the way, I want it to be a set of optional Lego bricks I can use if I need. (ix) There's certainly more things I'm missing here, but I believe you got the idea :) Of course, implementing all I described will take a few months :) Thanks, Paulo > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/i915/intel_drv.h | 62 +-------- > drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++----------------- > drivers/gpu/drm/i915/intel_sdvo.c | 22 ++-- > 4 files changed, 159 insertions(+), 195 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 94a4623..5225012 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -144,6 +144,8 @@ config DRM_I915 > select INPUT if ACPI > select ACPI_VIDEO if ACPI > select ACPI_BUTTON if ACPI > + select DRM_HDMI > + select HDMI > help > Choose this option if you have a system that has "Intel Graphics > Media Accelerator" or "HD Graphics" integrated graphics, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 522061c..70a7440 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -276,63 +276,6 @@ struct cxsr_latency { > #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base) > #define to_intel_plane(x) container_of(x, struct intel_plane, base) > > -#define DIP_HEADER_SIZE 5 > - > -#define DIP_TYPE_AVI 0x82 > -#define DIP_VERSION_AVI 0x2 > -#define DIP_LEN_AVI 13 > -#define DIP_AVI_PR_1 0 > -#define DIP_AVI_PR_2 1 > - > -#define DIP_TYPE_SPD 0x83 > -#define DIP_VERSION_SPD 0x1 > -#define DIP_LEN_SPD 25 > -#define DIP_SPD_UNKNOWN 0 > -#define DIP_SPD_DSTB 0x1 > -#define DIP_SPD_DVDP 0x2 > -#define DIP_SPD_DVHS 0x3 > -#define DIP_SPD_HDDVR 0x4 > -#define DIP_SPD_DVC 0x5 > -#define DIP_SPD_DSC 0x6 > -#define DIP_SPD_VCD 0x7 > -#define DIP_SPD_GAME 0x8 > -#define DIP_SPD_PC 0x9 > -#define DIP_SPD_BD 0xa > -#define DIP_SPD_SCD 0xb > - > -struct dip_infoframe { > - uint8_t type; /* HB0 */ > - uint8_t ver; /* HB1 */ > - uint8_t len; /* HB2 - body len, not including checksum */ > - uint8_t ecc; /* Header ECC */ > - uint8_t checksum; /* PB0 */ > - union { > - struct { > - /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */ > - uint8_t Y_A_B_S; > - /* PB2 - C 7:6, M 5:4, R 3:0 */ > - uint8_t C_M_R; > - /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */ > - uint8_t ITC_EC_Q_SC; > - /* PB4 - VIC 6:0 */ > - uint8_t VIC; > - /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */ > - uint8_t YQ_CN_PR; > - /* PB6 to PB13 */ > - uint16_t top_bar_end; > - uint16_t bottom_bar_start; > - uint16_t left_bar_end; > - uint16_t right_bar_start; > - } __attribute__ ((packed)) avi; > - struct { > - uint8_t vn[8]; > - uint8_t pd[16]; > - uint8_t sdi; > - } __attribute__ ((packed)) spd; > - uint8_t payload[27]; > - } __attribute__ ((packed)) body; > -} __attribute__((packed)); > - > struct intel_hdmi { > u32 sdvox_reg; > int ddc_bus; > @@ -340,8 +283,8 @@ struct intel_hdmi { > bool has_hdmi_sink; > bool has_audio; > enum hdmi_force_audio force_audio; > - void (*write_infoframe)(struct drm_encoder *encoder, > - struct dip_infoframe *frame); > + void (*write_infoframe)(struct drm_encoder *encoder, const void *frame, > + size_t len); > void (*set_infoframes)(struct drm_encoder *encoder, > struct drm_display_mode *adjusted_mode); > }; > @@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); > extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > -extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if); > extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, > bool is_sdvob); > extern void intel_dvo_init(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 5c279b4..a5492b1 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -29,9 +29,11 @@ > #include <linux/i2c.h> > #include <linux/slab.h> > #include <linux/delay.h> > +#include <linux/hdmi.h> > #include <drm/drmP.h> > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > +#include <drm/drm_hdmi.h> > #include "intel_drv.h" > #include <drm/i915_drm.h> > #include "i915_drv.h" > @@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector) > return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base); > } > > -void intel_dip_infoframe_csum(struct dip_infoframe *frame) > +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type) > { > - uint8_t *data = (uint8_t *)frame; > - uint8_t sum = 0; > - unsigned i; > - > - frame->checksum = 0; > - frame->ecc = 0; > - > - for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++) > - sum += data[i]; > - > - frame->checksum = 0x100 - sum; > -} > - > -static u32 g4x_infoframe_index(struct dip_infoframe *frame) > -{ > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_SELECT_AVI; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_SELECT_SPD; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > > -static u32 g4x_infoframe_enable(struct dip_infoframe *frame) > +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_ENABLE_AVI; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_ENABLE_SPD; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > > -static u32 hsw_infoframe_enable(struct dip_infoframe *frame) > +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return VIDEO_DIP_ENABLE_AVI_HSW; > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return VIDEO_DIP_ENABLE_SPD_HSW; > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > > -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe) > +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe) > { > - switch (frame->type) { > - case DIP_TYPE_AVI: > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AVI: > return HSW_TVIDEO_DIP_AVI_DATA(pipe); > - case DIP_TYPE_SPD: > + case HDMI_INFOFRAME_TYPE_SPD: > return HSW_TVIDEO_DIP_SPD_DATA(pipe); > default: > - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); > return 0; > } > } > > -static void g4x_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > +static u32 intel_dip_pack(const u8 *data, size_t size) > { > - uint32_t *data = (uint32_t *)frame; > + u32 value = 0; > + size_t i; > + > + for (i = size; i > 0; i--) > + value = (value << 8) | data[i - 1]; > + > + return value; > +} > + > +static void g4x_dip_write(struct drm_i915_private *dev_priv, > + unsigned long offset, const void *dip, size_t len) > +{ > + u32 data; > + size_t i; > + > + /* Write infoframe header and zero out ECC at byte 4 */ > + data = intel_dip_pack(dip, 3); > + I915_WRITE(offset, data); > + > + for (i = 3; i < len; i += 4) { > + size_t rem = min_t(size_t, len - i, 4); > + data = intel_dip_pack(dip + i, rem); > + I915_WRITE(offset, data); > + } > + > + /* Write every possible data byte to force correct ECC calculation. */ > + for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > + I915_WRITE(offset, 0); > +} > + > +static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame, > + size_t len) > +{ > + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > u32 val = I915_READ(VIDEO_DIP_CTL); > - unsigned i, len = DIP_HEADER_SIZE + frame->len; > > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > > val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |= g4x_infoframe_index(frame); > + val |= g4x_infoframe_index(type); > > - val &= ~g4x_infoframe_enable(frame); > + val &= ~g4x_infoframe_enable(type); > > I915_WRITE(VIDEO_DIP_CTL, val); > > mmiowb(); > - for (i = 0; i < len; i += 4) { > - I915_WRITE(VIDEO_DIP_DATA, *data); > - data++; > - } > - /* Write every possible data byte to force correct ECC calculation. */ > - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > - I915_WRITE(VIDEO_DIP_DATA, 0); > + g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len); > mmiowb(); > > - val |= g4x_infoframe_enable(frame); > + val |= g4x_infoframe_enable(type); > val &= ~VIDEO_DIP_FREQ_MASK; > val |= VIDEO_DIP_FREQ_VSYNC; > > @@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder, > POSTING_READ(VIDEO_DIP_CTL); > } > > -static void ibx_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > +static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame, > + size_t len) > { > - uint32_t *data = (uint32_t *)frame; > + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); > int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len = DIP_HEADER_SIZE + frame->len; > u32 val = I915_READ(reg); > > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > > val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |= g4x_infoframe_index(frame); > + val |= g4x_infoframe_index(type); > > - val &= ~g4x_infoframe_enable(frame); > + val &= ~g4x_infoframe_enable(type); > > I915_WRITE(reg, val); > > mmiowb(); > - for (i = 0; i < len; i += 4) { > - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); > - data++; > - } > - /* Write every possible data byte to force correct ECC calculation. */ > - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, > + len); > mmiowb(); > > - val |= g4x_infoframe_enable(frame); > + val |= g4x_infoframe_enable(type); > val &= ~VIDEO_DIP_FREQ_MASK; > val |= VIDEO_DIP_FREQ_VSYNC; > > @@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder, > POSTING_READ(reg); > } > > -static void cpt_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > +static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame, > + size_t len) > { > - uint32_t *data = (uint32_t *)frame; > + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); > int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len = DIP_HEADER_SIZE + frame->len; > u32 val = I915_READ(reg); > > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > > val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |= g4x_infoframe_index(frame); > + val |= g4x_infoframe_index(type); > > /* The DIP control register spec says that we need to update the AVI > * infoframe without clearing its enable bit */ > - if (frame->type != DIP_TYPE_AVI) > - val &= ~g4x_infoframe_enable(frame); > + if (type != HDMI_INFOFRAME_TYPE_AVI) > + val &= ~g4x_infoframe_enable(type); > > I915_WRITE(reg, val); > > mmiowb(); > - for (i = 0; i < len; i += 4) { > - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); > - data++; > - } > - /* Write every possible data byte to force correct ECC calculation. */ > - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len); > mmiowb(); > > - val |= g4x_infoframe_enable(frame); > + val |= g4x_infoframe_enable(type); > val &= ~VIDEO_DIP_FREQ_MASK; > val |= VIDEO_DIP_FREQ_VSYNC; > > @@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder, > POSTING_READ(reg); > } > > -static void vlv_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > +static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame, > + size_t len) > { > - uint32_t *data = (uint32_t *)frame; > + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); > int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe); > - unsigned i, len = DIP_HEADER_SIZE + frame->len; > u32 val = I915_READ(reg); > > WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); > > val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ > - val |= g4x_infoframe_index(frame); > + val |= g4x_infoframe_index(type); > > - val &= ~g4x_infoframe_enable(frame); > + val &= ~g4x_infoframe_enable(type); > > I915_WRITE(reg, val); > > mmiowb(); > - for (i = 0; i < len; i += 4) { > - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data); > - data++; > - } > - /* Write every possible data byte to force correct ECC calculation. */ > - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0); > + g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame, > + len); > mmiowb(); > > - val |= g4x_infoframe_enable(frame); > + val |= g4x_infoframe_enable(type); > val &= ~VIDEO_DIP_FREQ_MASK; > val |= VIDEO_DIP_FREQ_VSYNC; > > @@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder, > POSTING_READ(reg); > } > > -static void hsw_write_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > +static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame, > + size_t len) > { > - uint32_t *data = (uint32_t *)frame; > + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); > u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe); > - u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe); > - unsigned int i, len = DIP_HEADER_SIZE + frame->len; > + u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe); > u32 val = I915_READ(ctl_reg); > + unsigned long offset; > + u32 data; > + size_t i; > > if (data_reg == 0) > return; > > - val &= ~hsw_infoframe_enable(frame); > + val &= ~hsw_infoframe_enable(type); > I915_WRITE(ctl_reg, val); > > mmiowb(); > - for (i = 0; i < len; i += 4) { > - I915_WRITE(data_reg + i, *data); > - data++; > + > + /* Write infoframe header and zero out ECC at byte 4 */ > + data = intel_dip_pack(frame, 3); > + I915_WRITE(data_reg, data); > + > + for (i = 3, offset = 4; i < len; i += 4, offset += 4) { > + size_t rem = min_t(size_t, len - i, 4); > + data = intel_dip_pack(frame + i, rem); > + I915_WRITE(data_reg + offset, data); > } > + > /* Write every possible data byte to force correct ECC calculation. */ > - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) > - I915_WRITE(data_reg + i, 0); > + for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4) > + I915_WRITE(data_reg + offset, 0); > + > mmiowb(); > > - val |= hsw_infoframe_enable(frame); > + val |= hsw_infoframe_enable(type); > I915_WRITE(ctl_reg, val); > POSTING_READ(ctl_reg); > } > > +/* > static void intel_set_infoframe(struct drm_encoder *encoder, > - struct dip_infoframe *frame) > + const struct hdmi_infoframe *frame) > { > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); > + u8 buffer[32]; > + ssize_t len; > > - intel_dip_infoframe_csum(frame); > - intel_hdmi->write_infoframe(encoder, frame); > + len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer)); > + if (len < 0) > + return; > + > + intel_hdmi->write_infoframe(encoder, buffer, len); > } > +*/ > > static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, > struct drm_display_mode *adjusted_mode) > { > - struct dip_infoframe avi_if = { > - .type = DIP_TYPE_AVI, > - .ver = DIP_VERSION_AVI, > - .len = DIP_LEN_AVI, > - }; > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); > + u8 buffer[HDMI_AVI_INFOFRAME_SIZE]; > + struct hdmi_avi_infoframe frame; > + ssize_t len; > + > + len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode); > + if (len < 0) > + return; > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > - avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2; > + frame.pixel_repeat = 2; > + > + len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer)); > + if (len < 0) > + return; > > - intel_set_infoframe(encoder, &avi_if); > + intel_hdmi->write_infoframe(encoder, buffer, len); > } > > static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder) > { > - struct dip_infoframe spd_if; > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); > + u8 buffer[HDMI_SPD_INFOFRAME_SIZE]; > + struct hdmi_spd_infoframe frame; > + ssize_t len; > + > + len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx"); > + if (len < 0) > + return; > + > + frame.sdi = HDMI_SPD_SDI_PC; > > - memset(&spd_if, 0, sizeof(spd_if)); > - spd_if.type = DIP_TYPE_SPD; > - spd_if.ver = DIP_VERSION_SPD; > - spd_if.len = DIP_LEN_SPD; > - strcpy(spd_if.body.spd.vn, "Intel"); > - strcpy(spd_if.body.spd.pd, "Integrated gfx"); > - spd_if.body.spd.sdi = DIP_SPD_PC; > + len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer)); > + if (len < 0) > + return; > > - intel_set_infoframe(encoder, &spd_if); > + intel_hdmi->write_infoframe(encoder, buffer, len); > } > > static void g4x_set_infoframes(struct drm_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 4b07401..192d791 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -29,6 +29,7 @@ > #include <linux/slab.h> > #include <linux/delay.h> > #include <linux/export.h> > +#include <linux/hdmi.h> > #include <drm/drmP.h> > #include <drm/drm_crtc.h> > #include <drm/drm_edid.h> > @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, > > static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) > { > - struct dip_infoframe avi_if = { > - .type = DIP_TYPE_AVI, > - .ver = DIP_VERSION_AVI, > - .len = DIP_LEN_AVI, > - }; > - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; > + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE]; > + struct hdmi_avi_infoframe frame; > + ssize_t len; > > - intel_dip_infoframe_csum(&avi_if); > + len = hdmi_avi_infoframe_init(&frame); > + if (len < 0) > + return false; > > - /* sdvo spec says that the ecc is handled by the hw, and it looks like > - * we must not send the ecc field, either. */ > - memcpy(sdvo_data, &avi_if, 3); > - sdvo_data[3] = avi_if.checksum; > - memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi)); > + len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data)); > + if (len < 0) > + return false; > > return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, > SDVO_HBUF_TX_VSYNC, > -- > 1.8.0.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi 2012/12/6 Paulo Zanoni <przanoni@gmail.com>: > Hi > > 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>: >> Use the generic HDMI infoframe helpers to get rid of the duplicate >> implementation in the i915 driver. > > A few comments: > > - I've compiled your patches and now "intel_infoframes -d" tells my my > infoframes are full of zeroes. So there's a bug somewhere... I have to > say that the i915 infoframe registers are quite complicated and it > took me a long time to fix a lot of its problems, so please don't > break it and read below for a suggestion :) > > - Why do we need that kconfig stuff? Why not just put all the code > inside drivers/gpu/drm? > > - I really like having "common code between drivers merged", but I > really don't see how the i915 driver is benefiting from this change. > We're just basically complicating intel_hdmi.c to use a new struct > that doesn't fit our needs due to the lack of the ECC byte. My idea: > Instead of calling hdmi_avi_infoframe_pack, why don't we just > implement intel_hdmi_avi_infoframe_pack that converts a "struct > hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that > has the cool ECC value in the right place? This would probably > drastically reduce your chances of introducing bugs in our code :) Just to be a little bit more clear about this paragraph since Daniel asked me a few questions: For the changes inside intel_hdmi.c, I'd really like your patch to not touch any of the "write_infoframe" or "set_infoframes" callbacks. I think you can do everything by just patching intel_set_infoframe, intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one of those functions you could call something like "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct dip_infoframe)" and then proceed. This way you don 't need to touch the code that actually writes stuff to the hardware. Also, it seems intel_sdvo.c can perfectly use the generic struct from your patch (the one that doesn't contain the ECC value), so maybe we could even try to move "struct dip_infoframe" to inside intel_hdmi.c, making sure intel_sdvo.c can't even use it. > > - Instead of "converting our own structs into structs that don't > exactly fit our usage", what I would really like to see is generic > _optional_ drm functions to help me fill the infoframe data, like the > things I did in the past. Examples: > (i) I want a function that reads the EDID and tells me whether the > monitor is going to respect my "underscan" settings (AVI infoframe > field S) or not. > (ii) I want unified overscan/underscan properties between all the > drivers: we need properties to allow requesting specific > overscan/underscan properties and also properties to allow the driver > to force underscan in case the monitor doesn't want to cooperate (as > far as I remember, radeon has these properties, but we need to have > the behavior explicitly documented so all the drivers can try to > implement the exact same thing, and then we also need to ask > user-space guys to add support for these things in their GUIs). > (iii) I want a function that helps me properly setting/changing the > correct pixel and picture aspect ratios. it seems the only difference > between some modes (with different VICs) is the pixel/aspect ratio, > how do we expose this to the user-space and let it select the one it > wants? How does the driver know exactly which one is the selected one? > (iv) I want code that enables user-space to use those modes with > variable pixel repetition rates and select exactly the one it wants to > use with the correct pixel repetition. > (v) I'd like a quirk list to recognize monitors/devices that don't > work correctly when they receive infoframes, or when the VIC is 0, > etc. > (vi) I want magical functions that help me setting all those > color-related stuff I did not study yet. > (vii) Ideally, I'd like to accomplish all the above by looking at > "struct drm_display_mode" and maybe passing it to helper functions > that I can _optionally_ use if I want. > (viii) I don't want the infoframe code to be a "mid layer" that gets > in the way, I want it to be a set of optional Lego bricks I can use if > I need. > (ix) There's certainly more things I'm missing here, but I believe you > got the idea :) > > Of course, implementing all I described will take a few months :) And to make it clear: the list above doesn't contain any "requirements to get your code merged", they're just suggestions, since you're already touching this code and you need to take these problems into consideration while designing things. We can merge the patch first and then later worry about fixing the items on the list :) > > Thanks, > Paulo > >> >> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> >> --- >> drivers/gpu/drm/Kconfig | 2 + >> drivers/gpu/drm/i915/intel_drv.h | 62 +-------- >> drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++----------------- >> drivers/gpu/drm/i915/intel_sdvo.c | 22 ++-- >> 4 files changed, 159 insertions(+), 195 deletions(-) >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index 94a4623..5225012 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -144,6 +144,8 @@ config DRM_I915 >> select INPUT if ACPI >> select ACPI_VIDEO if ACPI >> select ACPI_BUTTON if ACPI >> + select DRM_HDMI >> + select HDMI >> help >> Choose this option if you have a system that has "Intel Graphics >> Media Accelerator" or "HD Graphics" integrated graphics, >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 522061c..70a7440 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -276,63 +276,6 @@ struct cxsr_latency { >> #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base) >> #define to_intel_plane(x) container_of(x, struct intel_plane, base) >> >> -#define DIP_HEADER_SIZE 5 >> - >> -#define DIP_TYPE_AVI 0x82 >> -#define DIP_VERSION_AVI 0x2 >> -#define DIP_LEN_AVI 13 >> -#define DIP_AVI_PR_1 0 >> -#define DIP_AVI_PR_2 1 >> - >> -#define DIP_TYPE_SPD 0x83 >> -#define DIP_VERSION_SPD 0x1 >> -#define DIP_LEN_SPD 25 >> -#define DIP_SPD_UNKNOWN 0 >> -#define DIP_SPD_DSTB 0x1 >> -#define DIP_SPD_DVDP 0x2 >> -#define DIP_SPD_DVHS 0x3 >> -#define DIP_SPD_HDDVR 0x4 >> -#define DIP_SPD_DVC 0x5 >> -#define DIP_SPD_DSC 0x6 >> -#define DIP_SPD_VCD 0x7 >> -#define DIP_SPD_GAME 0x8 >> -#define DIP_SPD_PC 0x9 >> -#define DIP_SPD_BD 0xa >> -#define DIP_SPD_SCD 0xb >> - >> -struct dip_infoframe { >> - uint8_t type; /* HB0 */ >> - uint8_t ver; /* HB1 */ >> - uint8_t len; /* HB2 - body len, not including checksum */ >> - uint8_t ecc; /* Header ECC */ >> - uint8_t checksum; /* PB0 */ >> - union { >> - struct { >> - /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */ >> - uint8_t Y_A_B_S; >> - /* PB2 - C 7:6, M 5:4, R 3:0 */ >> - uint8_t C_M_R; >> - /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */ >> - uint8_t ITC_EC_Q_SC; >> - /* PB4 - VIC 6:0 */ >> - uint8_t VIC; >> - /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */ >> - uint8_t YQ_CN_PR; >> - /* PB6 to PB13 */ >> - uint16_t top_bar_end; >> - uint16_t bottom_bar_start; >> - uint16_t left_bar_end; >> - uint16_t right_bar_start; >> - } __attribute__ ((packed)) avi; >> - struct { >> - uint8_t vn[8]; >> - uint8_t pd[16]; >> - uint8_t sdi; >> - } __attribute__ ((packed)) spd; >> - uint8_t payload[27]; >> - } __attribute__ ((packed)) body; >> -} __attribute__((packed)); >> - >> struct intel_hdmi { >> u32 sdvox_reg; >> int ddc_bus; >> @@ -340,8 +283,8 @@ struct intel_hdmi { >> bool has_hdmi_sink; >> bool has_audio; >> enum hdmi_force_audio force_audio; >> - void (*write_infoframe)(struct drm_encoder *encoder, >> - struct dip_infoframe *frame); >> + void (*write_infoframe)(struct drm_encoder *encoder, const void *frame, >> + size_t len); >> void (*set_infoframes)(struct drm_encoder *encoder, >> struct drm_display_mode *adjusted_mode); >> }; >> @@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); >> extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder, >> const struct drm_display_mode *mode, >> struct drm_display_mode *adjusted_mode); >> -extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if); >> extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, >> bool is_sdvob); >> extern void intel_dvo_init(struct drm_device *dev); >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index 5c279b4..a5492b1 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -29,9 +29,11 @@ >> #include <linux/i2c.h> >> #include <linux/slab.h> >> #include <linux/delay.h> >> +#include <linux/hdmi.h> >> #include <drm/drmP.h> >> #include <drm/drm_crtc.h> >> #include <drm/drm_edid.h> >> +#include <drm/drm_hdmi.h> >> #include "intel_drv.h" >> #include <drm/i915_drm.h> >> #include "i915_drv.h" >> @@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector) >> return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base); >> } >> >> -void intel_dip_infoframe_csum(struct dip_infoframe *frame) >> +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type) >> { >> - uint8_t *data = (uint8_t *)frame; >> - uint8_t sum = 0; >> - unsigned i; >> - >> - frame->checksum = 0; >> - frame->ecc = 0; >> - >> - for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++) >> - sum += data[i]; >> - >> - frame->checksum = 0x100 - sum; >> -} >> - >> -static u32 g4x_infoframe_index(struct dip_infoframe *frame) >> -{ >> - switch (frame->type) { >> - case DIP_TYPE_AVI: >> + switch (type) { >> + case HDMI_INFOFRAME_TYPE_AVI: >> return VIDEO_DIP_SELECT_AVI; >> - case DIP_TYPE_SPD: >> + case HDMI_INFOFRAME_TYPE_SPD: >> return VIDEO_DIP_SELECT_SPD; >> default: >> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); >> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); >> return 0; >> } >> } >> >> -static u32 g4x_infoframe_enable(struct dip_infoframe *frame) >> +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type) >> { >> - switch (frame->type) { >> - case DIP_TYPE_AVI: >> + switch (type) { >> + case HDMI_INFOFRAME_TYPE_AVI: >> return VIDEO_DIP_ENABLE_AVI; >> - case DIP_TYPE_SPD: >> + case HDMI_INFOFRAME_TYPE_SPD: >> return VIDEO_DIP_ENABLE_SPD; >> default: >> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); >> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); >> return 0; >> } >> } >> >> -static u32 hsw_infoframe_enable(struct dip_infoframe *frame) >> +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type) >> { >> - switch (frame->type) { >> - case DIP_TYPE_AVI: >> + switch (type) { >> + case HDMI_INFOFRAME_TYPE_AVI: >> return VIDEO_DIP_ENABLE_AVI_HSW; >> - case DIP_TYPE_SPD: >> + case HDMI_INFOFRAME_TYPE_SPD: >> return VIDEO_DIP_ENABLE_SPD_HSW; >> default: >> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); >> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); >> return 0; >> } >> } >> >> -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe) >> +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe) >> { >> - switch (frame->type) { >> - case DIP_TYPE_AVI: >> + switch (type) { >> + case HDMI_INFOFRAME_TYPE_AVI: >> return HSW_TVIDEO_DIP_AVI_DATA(pipe); >> - case DIP_TYPE_SPD: >> + case HDMI_INFOFRAME_TYPE_SPD: >> return HSW_TVIDEO_DIP_SPD_DATA(pipe); >> default: >> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); >> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); >> return 0; >> } >> } >> >> -static void g4x_write_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> +static u32 intel_dip_pack(const u8 *data, size_t size) >> { >> - uint32_t *data = (uint32_t *)frame; >> + u32 value = 0; >> + size_t i; >> + >> + for (i = size; i > 0; i--) >> + value = (value << 8) | data[i - 1]; >> + >> + return value; >> +} >> + >> +static void g4x_dip_write(struct drm_i915_private *dev_priv, >> + unsigned long offset, const void *dip, size_t len) >> +{ >> + u32 data; >> + size_t i; >> + >> + /* Write infoframe header and zero out ECC at byte 4 */ >> + data = intel_dip_pack(dip, 3); >> + I915_WRITE(offset, data); >> + >> + for (i = 3; i < len; i += 4) { >> + size_t rem = min_t(size_t, len - i, 4); >> + data = intel_dip_pack(dip + i, rem); >> + I915_WRITE(offset, data); >> + } >> + >> + /* Write every possible data byte to force correct ECC calculation. */ >> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> + I915_WRITE(offset, 0); >> +} >> + >> +static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame, >> + size_t len) >> +{ >> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> u32 val = I915_READ(VIDEO_DIP_CTL); >> - unsigned i, len = DIP_HEADER_SIZE + frame->len; >> >> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); >> >> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ >> - val |= g4x_infoframe_index(frame); >> + val |= g4x_infoframe_index(type); >> >> - val &= ~g4x_infoframe_enable(frame); >> + val &= ~g4x_infoframe_enable(type); >> >> I915_WRITE(VIDEO_DIP_CTL, val); >> >> mmiowb(); >> - for (i = 0; i < len; i += 4) { >> - I915_WRITE(VIDEO_DIP_DATA, *data); >> - data++; >> - } >> - /* Write every possible data byte to force correct ECC calculation. */ >> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> - I915_WRITE(VIDEO_DIP_DATA, 0); >> + g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len); >> mmiowb(); >> >> - val |= g4x_infoframe_enable(frame); >> + val |= g4x_infoframe_enable(type); >> val &= ~VIDEO_DIP_FREQ_MASK; >> val |= VIDEO_DIP_FREQ_VSYNC; >> >> @@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder, >> POSTING_READ(VIDEO_DIP_CTL); >> } >> >> -static void ibx_write_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> +static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame, >> + size_t len) >> { >> - uint32_t *data = (uint32_t *)frame; >> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); >> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); >> - unsigned i, len = DIP_HEADER_SIZE + frame->len; >> u32 val = I915_READ(reg); >> >> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); >> >> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ >> - val |= g4x_infoframe_index(frame); >> + val |= g4x_infoframe_index(type); >> >> - val &= ~g4x_infoframe_enable(frame); >> + val &= ~g4x_infoframe_enable(type); >> >> I915_WRITE(reg, val); >> >> mmiowb(); >> - for (i = 0; i < len; i += 4) { >> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); >> - data++; >> - } >> - /* Write every possible data byte to force correct ECC calculation. */ >> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); >> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, >> + len); >> mmiowb(); >> >> - val |= g4x_infoframe_enable(frame); >> + val |= g4x_infoframe_enable(type); >> val &= ~VIDEO_DIP_FREQ_MASK; >> val |= VIDEO_DIP_FREQ_VSYNC; >> >> @@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder, >> POSTING_READ(reg); >> } >> >> -static void cpt_write_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> +static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame, >> + size_t len) >> { >> - uint32_t *data = (uint32_t *)frame; >> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); >> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); >> - unsigned i, len = DIP_HEADER_SIZE + frame->len; >> u32 val = I915_READ(reg); >> >> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); >> >> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ >> - val |= g4x_infoframe_index(frame); >> + val |= g4x_infoframe_index(type); >> >> /* The DIP control register spec says that we need to update the AVI >> * infoframe without clearing its enable bit */ >> - if (frame->type != DIP_TYPE_AVI) >> - val &= ~g4x_infoframe_enable(frame); >> + if (type != HDMI_INFOFRAME_TYPE_AVI) >> + val &= ~g4x_infoframe_enable(type); >> >> I915_WRITE(reg, val); >> >> mmiowb(); >> - for (i = 0; i < len; i += 4) { >> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); >> - data++; >> - } >> - /* Write every possible data byte to force correct ECC calculation. */ >> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); >> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len); >> mmiowb(); >> >> - val |= g4x_infoframe_enable(frame); >> + val |= g4x_infoframe_enable(type); >> val &= ~VIDEO_DIP_FREQ_MASK; >> val |= VIDEO_DIP_FREQ_VSYNC; >> >> @@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder, >> POSTING_READ(reg); >> } >> >> -static void vlv_write_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> +static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame, >> + size_t len) >> { >> - uint32_t *data = (uint32_t *)frame; >> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); >> int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe); >> - unsigned i, len = DIP_HEADER_SIZE + frame->len; >> u32 val = I915_READ(reg); >> >> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); >> >> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ >> - val |= g4x_infoframe_index(frame); >> + val |= g4x_infoframe_index(type); >> >> - val &= ~g4x_infoframe_enable(frame); >> + val &= ~g4x_infoframe_enable(type); >> >> I915_WRITE(reg, val); >> >> mmiowb(); >> - for (i = 0; i < len; i += 4) { >> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data); >> - data++; >> - } >> - /* Write every possible data byte to force correct ECC calculation. */ >> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0); >> + g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame, >> + len); >> mmiowb(); >> >> - val |= g4x_infoframe_enable(frame); >> + val |= g4x_infoframe_enable(type); >> val &= ~VIDEO_DIP_FREQ_MASK; >> val |= VIDEO_DIP_FREQ_VSYNC; >> >> @@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder, >> POSTING_READ(reg); >> } >> >> -static void hsw_write_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> +static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame, >> + size_t len) >> { >> - uint32_t *data = (uint32_t *)frame; >> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; >> struct drm_device *dev = encoder->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); >> u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe); >> - u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe); >> - unsigned int i, len = DIP_HEADER_SIZE + frame->len; >> + u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe); >> u32 val = I915_READ(ctl_reg); >> + unsigned long offset; >> + u32 data; >> + size_t i; >> >> if (data_reg == 0) >> return; >> >> - val &= ~hsw_infoframe_enable(frame); >> + val &= ~hsw_infoframe_enable(type); >> I915_WRITE(ctl_reg, val); >> >> mmiowb(); >> - for (i = 0; i < len; i += 4) { >> - I915_WRITE(data_reg + i, *data); >> - data++; >> + >> + /* Write infoframe header and zero out ECC at byte 4 */ >> + data = intel_dip_pack(frame, 3); >> + I915_WRITE(data_reg, data); >> + >> + for (i = 3, offset = 4; i < len; i += 4, offset += 4) { >> + size_t rem = min_t(size_t, len - i, 4); >> + data = intel_dip_pack(frame + i, rem); >> + I915_WRITE(data_reg + offset, data); >> } >> + >> /* Write every possible data byte to force correct ECC calculation. */ >> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) >> - I915_WRITE(data_reg + i, 0); >> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4) >> + I915_WRITE(data_reg + offset, 0); >> + >> mmiowb(); >> >> - val |= hsw_infoframe_enable(frame); >> + val |= hsw_infoframe_enable(type); >> I915_WRITE(ctl_reg, val); >> POSTING_READ(ctl_reg); >> } >> >> +/* >> static void intel_set_infoframe(struct drm_encoder *encoder, >> - struct dip_infoframe *frame) >> + const struct hdmi_infoframe *frame) >> { >> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); >> + u8 buffer[32]; >> + ssize_t len; >> >> - intel_dip_infoframe_csum(frame); >> - intel_hdmi->write_infoframe(encoder, frame); >> + len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer)); >> + if (len < 0) >> + return; >> + >> + intel_hdmi->write_infoframe(encoder, buffer, len); >> } >> +*/ >> >> static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, >> struct drm_display_mode *adjusted_mode) >> { >> - struct dip_infoframe avi_if = { >> - .type = DIP_TYPE_AVI, >> - .ver = DIP_VERSION_AVI, >> - .len = DIP_LEN_AVI, >> - }; >> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); >> + u8 buffer[HDMI_AVI_INFOFRAME_SIZE]; >> + struct hdmi_avi_infoframe frame; >> + ssize_t len; >> + >> + len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode); >> + if (len < 0) >> + return; >> >> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) >> - avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2; >> + frame.pixel_repeat = 2; >> + >> + len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer)); >> + if (len < 0) >> + return; >> >> - intel_set_infoframe(encoder, &avi_if); >> + intel_hdmi->write_infoframe(encoder, buffer, len); >> } >> >> static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder) >> { >> - struct dip_infoframe spd_if; >> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); >> + u8 buffer[HDMI_SPD_INFOFRAME_SIZE]; >> + struct hdmi_spd_infoframe frame; >> + ssize_t len; >> + >> + len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx"); >> + if (len < 0) >> + return; >> + >> + frame.sdi = HDMI_SPD_SDI_PC; >> >> - memset(&spd_if, 0, sizeof(spd_if)); >> - spd_if.type = DIP_TYPE_SPD; >> - spd_if.ver = DIP_VERSION_SPD; >> - spd_if.len = DIP_LEN_SPD; >> - strcpy(spd_if.body.spd.vn, "Intel"); >> - strcpy(spd_if.body.spd.pd, "Integrated gfx"); >> - spd_if.body.spd.sdi = DIP_SPD_PC; >> + len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer)); >> + if (len < 0) >> + return; >> >> - intel_set_infoframe(encoder, &spd_if); >> + intel_hdmi->write_infoframe(encoder, buffer, len); >> } >> >> static void g4x_set_infoframes(struct drm_encoder *encoder, >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c >> index 4b07401..192d791 100644 >> --- a/drivers/gpu/drm/i915/intel_sdvo.c >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c >> @@ -29,6 +29,7 @@ >> #include <linux/slab.h> >> #include <linux/delay.h> >> #include <linux/export.h> >> +#include <linux/hdmi.h> >> #include <drm/drmP.h> >> #include <drm/drm_crtc.h> >> #include <drm/drm_edid.h> >> @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, >> >> static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) >> { >> - struct dip_infoframe avi_if = { >> - .type = DIP_TYPE_AVI, >> - .ver = DIP_VERSION_AVI, >> - .len = DIP_LEN_AVI, >> - }; >> - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; >> + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE]; >> + struct hdmi_avi_infoframe frame; >> + ssize_t len; >> >> - intel_dip_infoframe_csum(&avi_if); >> + len = hdmi_avi_infoframe_init(&frame); >> + if (len < 0) >> + return false; >> >> - /* sdvo spec says that the ecc is handled by the hw, and it looks like >> - * we must not send the ecc field, either. */ >> - memcpy(sdvo_data, &avi_if, 3); >> - sdvo_data[3] = avi_if.checksum; >> - memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi)); >> + len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data)); >> + if (len < 0) >> + return false; >> >> return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, >> SDVO_HBUF_TX_VSYNC, >> -- >> 1.8.0.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Paulo Zanoni
On Thu, Dec 06, 2012 at 02:11:00PM -0200, Paulo Zanoni wrote: > Hi > > 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>: > > Use the generic HDMI infoframe helpers to get rid of the duplicate > > implementation in the i915 driver. > > A few comments: > > - I've compiled your patches and now "intel_infoframes -d" tells my my > infoframes are full of zeroes. So there's a bug somewhere... I have to > say that the i915 infoframe registers are quite complicated and it > took me a long time to fix a lot of its problems, so please don't > break it and read below for a suggestion :) I don't know this intel_infoframes utility, where can I get it from? > - Why do we need that kconfig stuff? Why not just put all the code > inside drivers/gpu/drm? The reason for this is that it was suggested to make these helpers generic and not DRM specific in order to allow them to be shared with other subsystems such as V4L. Daniel already suggested to move the DRM helper into drm_edid.c to get rid of the additional Kconfig option. > - I really like having "common code between drivers merged", but I > really don't see how the i915 driver is benefiting from this change. > We're just basically complicating intel_hdmi.c to use a new struct > that doesn't fit our needs due to the lack of the ECC byte. My idea: > Instead of calling hdmi_avi_infoframe_pack, why don't we just > implement intel_hdmi_avi_infoframe_pack that converts a "struct > hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that > has the cool ECC value in the right place? This would probably > drastically reduce your chances of introducing bugs in our code :) That would completely defeat the purpose of this patch series. The goal was to unify all implementations and only keep the hardware specifics in the drivers. Clearly the ECC byte is specific to the i915 hardware and not part of the HDMI specification. I don't see how any of the i915 code is complicated by this change. The only complicated thing about it is handling the ECC byte properly. How about we try to fix things so that i915 works properly with this series instead of doing some partial conversion? > - Instead of "converting our own structs into structs that don't > exactly fit our usage", what I would really like to see is generic > _optional_ drm functions to help me fill the infoframe data, like the > things I did in the past. Examples: But that's precisely what the DRM helper function is supposed to do. It is very natural to do this with a generic structure as well. The point being that this is defined by the HDMI specification, so compliant hardware needs to implement this in some way or another. If Intel chips require an extra ECC byte to be transmitted as part of the infoframe header, then that's something the Intel driver should take care of. The rest of the code can be shared between all implementations. > (i) I want a function that reads the EDID and tells me whether the > monitor is going to respect my "underscan" settings (AVI infoframe > field S) or not. > (ii) I want unified overscan/underscan properties between all the > drivers: we need properties to allow requesting specific > overscan/underscan properties and also properties to allow the driver > to force underscan in case the monitor doesn't want to cooperate (as > far as I remember, radeon has these properties, but we need to have > the behavior explicitly documented so all the drivers can try to > implement the exact same thing, and then we also need to ask > user-space guys to add support for these things in their GUIs). > (iii) I want a function that helps me properly setting/changing the > correct pixel and picture aspect ratios. it seems the only difference > between some modes (with different VICs) is the pixel/aspect ratio, > how do we expose this to the user-space and let it select the one it > wants? How does the driver know exactly which one is the selected one? > (iv) I want code that enables user-space to use those modes with > variable pixel repetition rates and select exactly the one it wants to > use with the correct pixel repetition. > (v) I'd like a quirk list to recognize monitors/devices that don't > work correctly when they receive infoframes, or when the VIC is 0, > etc. > (vi) I want magical functions that help me setting all those > color-related stuff I did not study yet. Most of the above require interfaces that we don't have. > (vii) Ideally, I'd like to accomplish all the above by looking at > "struct drm_display_mode" and maybe passing it to helper functions > that I can _optionally_ use if I want. That's what the DRM helper is used for. I don't see how breaking this up into smaller pieces would be useful. This is data that is transmitted to HDMI equipment, so ideally the data would be the same independent of the HDMI source. Having each driver repeat the same sequence to fill in the same fields in different structures is exactly what this patchset tries to fix. > (viii) I don't want the infoframe code to be a "mid layer" that gets > in the way, I want it to be a set of optional Lego bricks I can use if > I need. That's precisely the reason why there's a split between filling the infoframe structures and packing them. Filling in the data can be done either using the DRM helper, or drivers can choose to fill it themselves even though I don't see the point as I already explained above. Packing on the other hand is a very generic operation and none of the drivers in the kernel need to pack the data in any different way. Some hardware may required the packed data to be written to the registers in some slighly different ways, but as I also already explained I think that kind of code belongs in the drivers. Thierry
On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote: > Hi > > 2012/12/6 Paulo Zanoni <przanoni@gmail.com>: > > Hi > > > > 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>: > >> Use the generic HDMI infoframe helpers to get rid of the duplicate > >> implementation in the i915 driver. > > > > A few comments: > > > > - I've compiled your patches and now "intel_infoframes -d" tells my my > > infoframes are full of zeroes. So there's a bug somewhere... I have to > > say that the i915 infoframe registers are quite complicated and it > > took me a long time to fix a lot of its problems, so please don't > > break it and read below for a suggestion :) > > > > - Why do we need that kconfig stuff? Why not just put all the code > > inside drivers/gpu/drm? > > > > - I really like having "common code between drivers merged", but I > > really don't see how the i915 driver is benefiting from this change. > > We're just basically complicating intel_hdmi.c to use a new struct > > that doesn't fit our needs due to the lack of the ECC byte. My idea: > > Instead of calling hdmi_avi_infoframe_pack, why don't we just > > implement intel_hdmi_avi_infoframe_pack that converts a "struct > > hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that > > has the cool ECC value in the right place? This would probably > > drastically reduce your chances of introducing bugs in our code :) > > Just to be a little bit more clear about this paragraph since Daniel > asked me a few questions: > > For the changes inside intel_hdmi.c, I'd really like your patch to not > touch any of the "write_infoframe" or "set_infoframes" callbacks. I > think you can do everything by just patching intel_set_infoframe, > intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one > of those functions you could call something like > "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct > dip_infoframe)" and then proceed. This way you don 't need to touch > the code that actually writes stuff to the hardware. I think I already explained in my previous mail how IMO this completely defeats the purpose of this patch series. Furthermore the functions that write the infoframes to the hardware themselves could use some refactoring of their own. There is a lot of duplication there. Thierry
On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote: >> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>: >> For the changes inside intel_hdmi.c, I'd really like your patch to not >> touch any of the "write_infoframe" or "set_infoframes" callbacks. I >> think you can do everything by just patching intel_set_infoframe, >> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one >> of those functions you could call something like >> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct >> dip_infoframe)" and then proceed. This way you don 't need to touch >> the code that actually writes stuff to the hardware. > > I think I already explained in my previous mail how IMO this completely > defeats the purpose of this patch series. Furthermore the functions that > write the infoframes to the hardware themselves could use some > refactoring of their own. There is a lot of duplication there. I agree with Paulo that we shouldn't touch the hw interfacing functions - we've had too many bugs and regressions in there, so burned child et al applies. Now I also disagreed with Paulo's idea to keep around the infoframe stuff completed for intel_hdmi.c - common frameworks for sink handling are useful, if only that if forces people to yell at each another and discovered that way how utterly broken real world hw is ;-) I think the idea Paulo proposed in the first reply of keeping our own infoframe packing code, but using the new data structures to construct it is worse than the current code: Other people will extend the helpers, but totally forget about the special intel-packing function. The idea I've discussed a bit with Paulo is to have our own wrapper function which intel_hdmi_set_spd_infoframe and intel_hdmi_set_avi_infoframe can call. The wrapper allocates a new temporary buffer in the common layout without the ECC byte, calls the common packing function with that temporary bufffer and then copies things over into the intel layout. That way we can use the common infoframe construction&packing stuff, without running the risk of blowing up the dip write functions right away (now that they seem to actually work). At least that was my takeaway from the discussion yesterday with Paulo, and now reading his second mail I see that he doesn't detail how the intel infoframe packing function could also work. So it reads like the exact same idea as in the first mail unfortunately. Or Paulo and I still have a big disagreement, in which case I need to discuss things a bit more with him about how we could tackle this. Because to reiterate my point from the beginning, I think common sink handling helpers are a Good Thing. Cheers, Daniel
On Fri, Dec 07, 2012 at 09:30:34AM +0100, Daniel Vetter wrote: > On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote: > >> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>: > >> For the changes inside intel_hdmi.c, I'd really like your patch to not > >> touch any of the "write_infoframe" or "set_infoframes" callbacks. I > >> think you can do everything by just patching intel_set_infoframe, > >> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one > >> of those functions you could call something like > >> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct > >> dip_infoframe)" and then proceed. This way you don 't need to touch > >> the code that actually writes stuff to the hardware. > > > > I think I already explained in my previous mail how IMO this completely > > defeats the purpose of this patch series. Furthermore the functions that > > write the infoframes to the hardware themselves could use some > > refactoring of their own. There is a lot of duplication there. > > I agree with Paulo that we shouldn't touch the hw interfacing > functions - we've had too many bugs and regressions in there, so > burned child et al applies. Now I also disagreed with Paulo's idea to > keep around the infoframe stuff completed for intel_hdmi.c - common > frameworks for sink handling are useful, if only that if forces people > to yell at each another and discovered that way how utterly broken > real world hw is ;-) > > I think the idea Paulo proposed in the first reply of keeping our own > infoframe packing code, but using the new data structures to construct > it is worse than the current code: Other people will extend the > helpers, but totally forget about the special intel-packing function. > The idea I've discussed a bit with Paulo is to have our own wrapper > function which intel_hdmi_set_spd_infoframe and > intel_hdmi_set_avi_infoframe can call. The wrapper allocates a new > temporary buffer in the common layout without the ECC byte, calls the > common packing function with that temporary bufffer and then copies > things over into the intel layout. That way we can use the common > infoframe construction&packing stuff, without running the risk of > blowing up the dip write functions right away (now that they seem to > actually work). So looking at the differences between the standard HDMI infoframe and the dip infoframe structures, this would mean copying 3 bytes of header, add a 0 byte, then copying the remaining bytes. I think all of this can be done much more easily when writing to the hardware. Maybe it would help to do something similar to what I did on Tegra to validate that the same infoframe ends up in the registers as for the old code. I used a #ifdef HDMI_GENERIC to easily switch between both code paths and added some debug output to the register writes so that I could compare both at the register level. If we do the same for Intel hardware we should be able to fix this properly. Thierry
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 94a4623..5225012 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -144,6 +144,8 @@ config DRM_I915 select INPUT if ACPI select ACPI_VIDEO if ACPI select ACPI_BUTTON if ACPI + select DRM_HDMI + select HDMI help Choose this option if you have a system that has "Intel Graphics Media Accelerator" or "HD Graphics" integrated graphics, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 522061c..70a7440 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -276,63 +276,6 @@ struct cxsr_latency { #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base) #define to_intel_plane(x) container_of(x, struct intel_plane, base) -#define DIP_HEADER_SIZE 5 - -#define DIP_TYPE_AVI 0x82 -#define DIP_VERSION_AVI 0x2 -#define DIP_LEN_AVI 13 -#define DIP_AVI_PR_1 0 -#define DIP_AVI_PR_2 1 - -#define DIP_TYPE_SPD 0x83 -#define DIP_VERSION_SPD 0x1 -#define DIP_LEN_SPD 25 -#define DIP_SPD_UNKNOWN 0 -#define DIP_SPD_DSTB 0x1 -#define DIP_SPD_DVDP 0x2 -#define DIP_SPD_DVHS 0x3 -#define DIP_SPD_HDDVR 0x4 -#define DIP_SPD_DVC 0x5 -#define DIP_SPD_DSC 0x6 -#define DIP_SPD_VCD 0x7 -#define DIP_SPD_GAME 0x8 -#define DIP_SPD_PC 0x9 -#define DIP_SPD_BD 0xa -#define DIP_SPD_SCD 0xb - -struct dip_infoframe { - uint8_t type; /* HB0 */ - uint8_t ver; /* HB1 */ - uint8_t len; /* HB2 - body len, not including checksum */ - uint8_t ecc; /* Header ECC */ - uint8_t checksum; /* PB0 */ - union { - struct { - /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */ - uint8_t Y_A_B_S; - /* PB2 - C 7:6, M 5:4, R 3:0 */ - uint8_t C_M_R; - /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */ - uint8_t ITC_EC_Q_SC; - /* PB4 - VIC 6:0 */ - uint8_t VIC; - /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */ - uint8_t YQ_CN_PR; - /* PB6 to PB13 */ - uint16_t top_bar_end; - uint16_t bottom_bar_start; - uint16_t left_bar_end; - uint16_t right_bar_start; - } __attribute__ ((packed)) avi; - struct { - uint8_t vn[8]; - uint8_t pd[16]; - uint8_t sdi; - } __attribute__ ((packed)) spd; - uint8_t payload[27]; - } __attribute__ ((packed)) body; -} __attribute__((packed)); - struct intel_hdmi { u32 sdvox_reg; int ddc_bus; @@ -340,8 +283,8 @@ struct intel_hdmi { bool has_hdmi_sink; bool has_audio; enum hdmi_force_audio force_audio; - void (*write_infoframe)(struct drm_encoder *encoder, - struct dip_infoframe *frame); + void (*write_infoframe)(struct drm_encoder *encoder, const void *frame, + size_t len); void (*set_infoframes)(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode); }; @@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder); extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); -extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if); extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob); extern void intel_dvo_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 5c279b4..a5492b1 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -29,9 +29,11 @@ #include <linux/i2c.h> #include <linux/slab.h> #include <linux/delay.h> +#include <linux/hdmi.h> #include <drm/drmP.h> #include <drm/drm_crtc.h> #include <drm/drm_edid.h> +#include <drm/drm_hdmi.h> #include "intel_drv.h" #include <drm/i915_drm.h> #include "i915_drv.h" @@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector) return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base); } -void intel_dip_infoframe_csum(struct dip_infoframe *frame) +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type) { - uint8_t *data = (uint8_t *)frame; - uint8_t sum = 0; - unsigned i; - - frame->checksum = 0; - frame->ecc = 0; - - for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++) - sum += data[i]; - - frame->checksum = 0x100 - sum; -} - -static u32 g4x_infoframe_index(struct dip_infoframe *frame) -{ - switch (frame->type) { - case DIP_TYPE_AVI: + switch (type) { + case HDMI_INFOFRAME_TYPE_AVI: return VIDEO_DIP_SELECT_AVI; - case DIP_TYPE_SPD: + case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_SELECT_SPD; default: - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; } } -static u32 g4x_infoframe_enable(struct dip_infoframe *frame) +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type) { - switch (frame->type) { - case DIP_TYPE_AVI: + switch (type) { + case HDMI_INFOFRAME_TYPE_AVI: return VIDEO_DIP_ENABLE_AVI; - case DIP_TYPE_SPD: + case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_ENABLE_SPD; default: - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; } } -static u32 hsw_infoframe_enable(struct dip_infoframe *frame) +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type) { - switch (frame->type) { - case DIP_TYPE_AVI: + switch (type) { + case HDMI_INFOFRAME_TYPE_AVI: return VIDEO_DIP_ENABLE_AVI_HSW; - case DIP_TYPE_SPD: + case HDMI_INFOFRAME_TYPE_SPD: return VIDEO_DIP_ENABLE_SPD_HSW; default: - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; } } -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe) +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe) { - switch (frame->type) { - case DIP_TYPE_AVI: + switch (type) { + case HDMI_INFOFRAME_TYPE_AVI: return HSW_TVIDEO_DIP_AVI_DATA(pipe); - case DIP_TYPE_SPD: + case HDMI_INFOFRAME_TYPE_SPD: return HSW_TVIDEO_DIP_SPD_DATA(pipe); default: - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type); return 0; } } -static void g4x_write_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) +static u32 intel_dip_pack(const u8 *data, size_t size) { - uint32_t *data = (uint32_t *)frame; + u32 value = 0; + size_t i; + + for (i = size; i > 0; i--) + value = (value << 8) | data[i - 1]; + + return value; +} + +static void g4x_dip_write(struct drm_i915_private *dev_priv, + unsigned long offset, const void *dip, size_t len) +{ + u32 data; + size_t i; + + /* Write infoframe header and zero out ECC at byte 4 */ + data = intel_dip_pack(dip, 3); + I915_WRITE(offset, data); + + for (i = 3; i < len; i += 4) { + size_t rem = min_t(size_t, len - i, 4); + data = intel_dip_pack(dip + i, rem); + I915_WRITE(offset, data); + } + + /* Write every possible data byte to force correct ECC calculation. */ + for (; i < VIDEO_DIP_DATA_SIZE; i += 4) + I915_WRITE(offset, 0); +} + +static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame, + size_t len) +{ + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; u32 val = I915_READ(VIDEO_DIP_CTL); - unsigned i, len = DIP_HEADER_SIZE + frame->len; WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ - val |= g4x_infoframe_index(frame); + val |= g4x_infoframe_index(type); - val &= ~g4x_infoframe_enable(frame); + val &= ~g4x_infoframe_enable(type); I915_WRITE(VIDEO_DIP_CTL, val); mmiowb(); - for (i = 0; i < len; i += 4) { - I915_WRITE(VIDEO_DIP_DATA, *data); - data++; - } - /* Write every possible data byte to force correct ECC calculation. */ - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) - I915_WRITE(VIDEO_DIP_DATA, 0); + g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len); mmiowb(); - val |= g4x_infoframe_enable(frame); + val |= g4x_infoframe_enable(type); val &= ~VIDEO_DIP_FREQ_MASK; val |= VIDEO_DIP_FREQ_VSYNC; @@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder, POSTING_READ(VIDEO_DIP_CTL); } -static void ibx_write_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) +static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame, + size_t len) { - uint32_t *data = (uint32_t *)frame; + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); - unsigned i, len = DIP_HEADER_SIZE + frame->len; u32 val = I915_READ(reg); WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ - val |= g4x_infoframe_index(frame); + val |= g4x_infoframe_index(type); - val &= ~g4x_infoframe_enable(frame); + val &= ~g4x_infoframe_enable(type); I915_WRITE(reg, val); mmiowb(); - for (i = 0; i < len; i += 4) { - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); - data++; - } - /* Write every possible data byte to force correct ECC calculation. */ - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, + len); mmiowb(); - val |= g4x_infoframe_enable(frame); + val |= g4x_infoframe_enable(type); val &= ~VIDEO_DIP_FREQ_MASK; val |= VIDEO_DIP_FREQ_VSYNC; @@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder, POSTING_READ(reg); } -static void cpt_write_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) +static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame, + size_t len) { - uint32_t *data = (uint32_t *)frame; + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); int reg = TVIDEO_DIP_CTL(intel_crtc->pipe); - unsigned i, len = DIP_HEADER_SIZE + frame->len; u32 val = I915_READ(reg); WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ - val |= g4x_infoframe_index(frame); + val |= g4x_infoframe_index(type); /* The DIP control register spec says that we need to update the AVI * infoframe without clearing its enable bit */ - if (frame->type != DIP_TYPE_AVI) - val &= ~g4x_infoframe_enable(frame); + if (type != HDMI_INFOFRAME_TYPE_AVI) + val &= ~g4x_infoframe_enable(type); I915_WRITE(reg, val); mmiowb(); - for (i = 0; i < len; i += 4) { - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data); - data++; - } - /* Write every possible data byte to force correct ECC calculation. */ - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0); + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len); mmiowb(); - val |= g4x_infoframe_enable(frame); + val |= g4x_infoframe_enable(type); val &= ~VIDEO_DIP_FREQ_MASK; val |= VIDEO_DIP_FREQ_VSYNC; @@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder, POSTING_READ(reg); } -static void vlv_write_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) +static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame, + size_t len) { - uint32_t *data = (uint32_t *)frame; + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe); - unsigned i, len = DIP_HEADER_SIZE + frame->len; u32 val = I915_READ(reg); WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n"); val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */ - val |= g4x_infoframe_index(frame); + val |= g4x_infoframe_index(type); - val &= ~g4x_infoframe_enable(frame); + val &= ~g4x_infoframe_enable(type); I915_WRITE(reg, val); mmiowb(); - for (i = 0; i < len; i += 4) { - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data); - data++; - } - /* Write every possible data byte to force correct ECC calculation. */ - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0); + g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame, + len); mmiowb(); - val |= g4x_infoframe_enable(frame); + val |= g4x_infoframe_enable(type); val &= ~VIDEO_DIP_FREQ_MASK; val |= VIDEO_DIP_FREQ_VSYNC; @@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder, POSTING_READ(reg); } -static void hsw_write_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) +static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame, + size_t len) { - uint32_t *data = (uint32_t *)frame; + enum hdmi_infoframe_type type = ((const u8 *)frame)[0]; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc); u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe); - u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe); - unsigned int i, len = DIP_HEADER_SIZE + frame->len; + u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe); u32 val = I915_READ(ctl_reg); + unsigned long offset; + u32 data; + size_t i; if (data_reg == 0) return; - val &= ~hsw_infoframe_enable(frame); + val &= ~hsw_infoframe_enable(type); I915_WRITE(ctl_reg, val); mmiowb(); - for (i = 0; i < len; i += 4) { - I915_WRITE(data_reg + i, *data); - data++; + + /* Write infoframe header and zero out ECC at byte 4 */ + data = intel_dip_pack(frame, 3); + I915_WRITE(data_reg, data); + + for (i = 3, offset = 4; i < len; i += 4, offset += 4) { + size_t rem = min_t(size_t, len - i, 4); + data = intel_dip_pack(frame + i, rem); + I915_WRITE(data_reg + offset, data); } + /* Write every possible data byte to force correct ECC calculation. */ - for (; i < VIDEO_DIP_DATA_SIZE; i += 4) - I915_WRITE(data_reg + i, 0); + for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4) + I915_WRITE(data_reg + offset, 0); + mmiowb(); - val |= hsw_infoframe_enable(frame); + val |= hsw_infoframe_enable(type); I915_WRITE(ctl_reg, val); POSTING_READ(ctl_reg); } +/* static void intel_set_infoframe(struct drm_encoder *encoder, - struct dip_infoframe *frame) + const struct hdmi_infoframe *frame) { struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); + u8 buffer[32]; + ssize_t len; - intel_dip_infoframe_csum(frame); - intel_hdmi->write_infoframe(encoder, frame); + len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer)); + if (len < 0) + return; + + intel_hdmi->write_infoframe(encoder, buffer, len); } +*/ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { - struct dip_infoframe avi_if = { - .type = DIP_TYPE_AVI, - .ver = DIP_VERSION_AVI, - .len = DIP_LEN_AVI, - }; + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); + u8 buffer[HDMI_AVI_INFOFRAME_SIZE]; + struct hdmi_avi_infoframe frame; + ssize_t len; + + len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode); + if (len < 0) + return; if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) - avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2; + frame.pixel_repeat = 2; + + len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer)); + if (len < 0) + return; - intel_set_infoframe(encoder, &avi_if); + intel_hdmi->write_infoframe(encoder, buffer, len); } static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder) { - struct dip_infoframe spd_if; + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder); + u8 buffer[HDMI_SPD_INFOFRAME_SIZE]; + struct hdmi_spd_infoframe frame; + ssize_t len; + + len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx"); + if (len < 0) + return; + + frame.sdi = HDMI_SPD_SDI_PC; - memset(&spd_if, 0, sizeof(spd_if)); - spd_if.type = DIP_TYPE_SPD; - spd_if.ver = DIP_VERSION_SPD; - spd_if.len = DIP_LEN_SPD; - strcpy(spd_if.body.spd.vn, "Intel"); - strcpy(spd_if.body.spd.pd, "Integrated gfx"); - spd_if.body.spd.sdi = DIP_SPD_PC; + len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer)); + if (len < 0) + return; - intel_set_infoframe(encoder, &spd_if); + intel_hdmi->write_infoframe(encoder, buffer, len); } static void g4x_set_infoframes(struct drm_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 4b07401..192d791 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -29,6 +29,7 @@ #include <linux/slab.h> #include <linux/delay.h> #include <linux/export.h> +#include <linux/hdmi.h> #include <drm/drmP.h> #include <drm/drm_crtc.h> #include <drm/drm_edid.h> @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo, static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo) { - struct dip_infoframe avi_if = { - .type = DIP_TYPE_AVI, - .ver = DIP_VERSION_AVI, - .len = DIP_LEN_AVI, - }; - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)]; + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE]; + struct hdmi_avi_infoframe frame; + ssize_t len; - intel_dip_infoframe_csum(&avi_if); + len = hdmi_avi_infoframe_init(&frame); + if (len < 0) + return false; - /* sdvo spec says that the ecc is handled by the hw, and it looks like - * we must not send the ecc field, either. */ - memcpy(sdvo_data, &avi_if, 3); - sdvo_data[3] = avi_if.checksum; - memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi)); + len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data)); + if (len < 0) + return false; return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF, SDVO_HBUF_TX_VSYNC,
Use the generic HDMI infoframe helpers to get rid of the duplicate implementation in the i915 driver. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/i915/intel_drv.h | 62 +-------- drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++----------------- drivers/gpu/drm/i915/intel_sdvo.c | 22 ++-- 4 files changed, 159 insertions(+), 195 deletions(-)