| Message ID | 20240125193834.7065-12-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for CDM over DP | expand |
On 25/01/2024 21:38, Paloma Arellano wrote: > Add support to pack and send the VSC SDP packet for DP. This therefore > allows the transmision of format information to the sinks which is > needed for YUV420 support over DP. > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + > drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + > drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ > drivers/gpu/drm/msm/dp/dp_reg.h | 3 + > 5 files changed, 205 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index c025786170ba5..7e4c68be23e56 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -29,6 +29,9 @@ > > #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) > > +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) > +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) > + > #define DP_INTERRUPT_STATUS1 \ > (DP_INTR_AUX_XFER_DONE| \ > DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ > @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog) > return 0; > } > > +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog *dp_catalog) > +{ > + struct dp_catalog_private *catalog; > + u32 header, parity, data; > + u8 bpc, off = 0; > + u8 buf[SZ_128]; > + > + if (!dp_catalog) { > + pr_err("invalid input\n"); > + return; > + } > + > + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); > + > + /* HEADER BYTE 1 */ > + header = dp_catalog->sdp.sdp_header.HB1; > + parity = dp_catalog_calculate_parity(header); > + data = ((header << HEADER_BYTE_1_BIT) | (parity << PARITY_BYTE_1_BIT)); > + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + /* HEADER BYTE 2 */ > + header = dp_catalog->sdp.sdp_header.HB2; > + parity = dp_catalog_calculate_parity(header); > + data = ((header << HEADER_BYTE_2_BIT) | (parity << PARITY_BYTE_2_BIT)); > + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > + > + /* HEADER BYTE 3 */ > + header = dp_catalog->sdp.sdp_header.HB3; > + parity = dp_catalog_calculate_parity(header); > + data = ((header << HEADER_BYTE_3_BIT) | (parity << PARITY_BYTE_3_BIT)); > + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); > + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); This seems to be common with the dp_audio code. Please extract this header writing too. > + > + data = 0; > + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); Generally this is not how these functions are expected to be written. Please take a look at drivers/video/hdmi.c. It should be split into: - generic function that packs the C structure into a flat byte buffer, - driver-specific function that formats and writes the buffer to the hardware. > + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + switch (dp_catalog->vsc_sdp_data.bpc) { > + case 10: > + bpc = DP_GENERIC0_6_YUV_10_BPC; > + break; > + case 8: > + default: > + bpc = DP_GENERIC0_6_YUV_8_BPC; > + break; > + } > + > + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ > + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | > + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | > + (bpc << 8) | > + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | > + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); > + > + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + data = 0; > + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + dp_write_link(catalog, MMSS_DP_GENERIC0_9, data); > + memcpy(buf + off, &data, sizeof(data)); > + off += sizeof(data); > + > + print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", DUMP_PREFIX_NONE, 16, 4, buf, off, false); > +} > + > +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, bool en) > +{ > + struct dp_catalog_private *catalog; > + u32 cfg, cfg2, misc; > + u16 major = 0, minor = 0; > + > + if (!dp_catalog) { > + pr_err("invalid input\n"); > + return; > + } > + > + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); > + > + cfg = dp_read_link(catalog, MMSS_DP_SDP_CFG); > + cfg2 = dp_read_link(catalog, MMSS_DP_SDP_CFG2); > + misc = dp_read_link(catalog, REG_DP_MISC1_MISC0); > + > + if (en) { > + cfg |= GEN0_SDP_EN; > + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > + > + cfg2 |= GENERIC0_SDPSIZE; When I see a something_SIZE macro, I'd naturally expect it to be an actual size of some data. Please consider renaming to e.g. GENERIC0_SDPSIZE_VALID. > + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); > + > + dp_catalog_panel_setup_vsc_sdp(dp_catalog); > + > + /* indicates presence of VSC (BIT(6) of MISC1) */ > + misc |= DP_MISC1_VSC_SDP; > + > + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); > + } else { > + cfg &= ~GEN0_SDP_EN; > + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > + > + cfg2 &= ~GENERIC0_SDPSIZE; > + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); > + > + /* switch back to MSA */ > + misc &= ~DP_MISC1_VSC_SDP; > + > + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); > + } > + > + pr_debug("misc settings = 0x%x\n", misc); > + dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); > + > + dp_catalog_hw_revision(dp_catalog, &major, &minor); > + if (major == 1 && minor < 2) { > + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); > + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); > + } > +} > + > void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, > struct drm_display_mode *drm_mode) > { > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h > index 94c377ef90c35..6b757249c0698 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.h > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > @@ -7,6 +7,7 @@ > #define _DP_CATALOG_H_ > > #include <drm/drm_modes.h> > +#include <drm/display/drm_dp_helper.h> > > #include "dp_parser.h" > #include "disp/msm_disp_snapshot.h" > @@ -76,6 +77,8 @@ struct dp_catalog { > u32 dp_active; > enum dp_catalog_audio_sdp_type sdp_type; > enum dp_catalog_audio_header_type sdp_header; > + struct dp_sdp sdp; I assume that the sdp field contains only transient data, which is not used after it gets written to the hardware. Please remove it from the struct allocate on a stack or via kzalloc. > + struct drm_dp_vsc_sdp vsc_sdp_data; > u32 audio_data; > bool wide_bus_en; > }; > @@ -196,6 +199,7 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog); > > /* DP Panel APIs */ > int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog); > +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, bool en); > void dp_catalog_dump_regs(struct dp_catalog *dp_catalog); > void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, > struct drm_display_mode *drm_mode); > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 209cf2a35642f..ddd92a63d5a67 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1952,6 +1952,8 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl) > dp_io = &ctrl->parser->io; > phy = dp_io->phy; > > + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); > + > /* set dongle to D3 (power off) mode */ > dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true); > > @@ -2026,6 +2028,8 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl) > dp_io = &ctrl->parser->io; > phy = dp_io->phy; > > + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); > + > dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); > > dp_catalog_ctrl_reset(ctrl->catalog); > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c > index af7820b6d35ec..d6af9898b00d8 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -307,6 +307,49 @@ bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel) > return panel->major >= 1 && panel->minor >= 3 && panel->vsc_supported; > } > > +static int dp_panel_setup_vsc_sdp(struct dp_panel *dp_panel) > +{ > + struct dp_catalog *catalog; > + struct dp_panel_private *panel; > + struct dp_display_mode *dp_mode; > + int rc = 0; > + > + if (!dp_panel) { > + pr_err("invalid input\n"); > + rc = -EINVAL; > + return rc; > + } > + > + panel = container_of(dp_panel, struct dp_panel_private, dp_panel); > + catalog = panel->catalog; > + dp_mode = &dp_panel->dp_mode; > + > + memset(&catalog->sdp, 0, sizeof(catalog->sdp)); > + memset(&catalog->vsc_sdp_data, 0, sizeof(catalog->vsc_sdp_data)); > + > + /* VSC SDP header as per table 2-118 of DP 1.4 specification */ > + catalog->sdp.sdp_header.HB0 = 0x00; > + catalog->sdp.sdp_header.HB1 = 0x07; > + catalog->sdp.sdp_header.HB2 = 0x05; > + catalog->sdp.sdp_header.HB3 = 0x13; > + > + /* VSC SDP Payload for DB16 */ > + catalog->vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420; > + catalog->vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT; > + > + /* VSC SDP Payload for DB17 */ > + catalog->vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA; > + > + /* VSC SDP Payload for DB18 */ > + catalog->vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS; > + > + catalog->vsc_sdp_data.bpc = dp_mode->bpp / 3; > + > + dp_catalog_panel_config_vsc_sdp(catalog, true); > + > + return rc; > +} > + > void dp_panel_dump_regs(struct dp_panel *dp_panel) > { > struct dp_catalog *catalog; > @@ -370,6 +413,10 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel) > catalog->dp_active = data; > > dp_catalog_panel_timing_cfg(catalog); > + > + if (dp_panel->dp_mode.out_fmt_is_yuv_420) > + dp_panel_setup_vsc_sdp(dp_panel); > + > panel->panel_on = true; > > return 0; > diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h > index ea85a691e72b5..756ddf85b1e81 100644 > --- a/drivers/gpu/drm/msm/dp/dp_reg.h > +++ b/drivers/gpu/drm/msm/dp/dp_reg.h > @@ -142,6 +142,7 @@ > #define DP_MISC0_SYNCHRONOUS_CLK (0x00000001) > #define DP_MISC0_COLORIMETRY_CFG_SHIFT (0x00000001) > #define DP_MISC0_TEST_BITS_DEPTH_SHIFT (0x00000005) > +#define DP_MISC1_VSC_SDP (0x00004000) > > #define REG_DP_VALID_BOUNDARY (0x00000030) > #define REG_DP_VALID_BOUNDARY_2 (0x00000034) > @@ -201,9 +202,11 @@ > #define MMSS_DP_AUDIO_CTRL_RESET (0x00000214) > > #define MMSS_DP_SDP_CFG (0x00000228) > +#define GEN0_SDP_EN (0x00020000) > #define MMSS_DP_SDP_CFG2 (0x0000022C) > #define MMSS_DP_AUDIO_TIMESTAMP_0 (0x00000230) > #define MMSS_DP_AUDIO_TIMESTAMP_1 (0x00000234) > +#define GENERIC0_SDPSIZE (0x00010000) > > #define MMSS_DP_AUDIO_STREAM_0 (0x00000240) > #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: > On 25/01/2024 21:38, Paloma Arellano wrote: >> Add support to pack and send the VSC SDP packet for DP. This therefore >> allows the transmision of format information to the sinks which is >> needed for YUV420 support over DP. >> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + >> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ >> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + >> 5 files changed, 205 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >> b/drivers/gpu/drm/msm/dp/dp_catalog.c >> index c025786170ba5..7e4c68be23e56 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >> @@ -29,6 +29,9 @@ >> #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) >> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) >> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) >> + >> #define DP_INTERRUPT_STATUS1 \ >> (DP_INTR_AUX_XFER_DONE| \ >> DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ >> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct >> dp_catalog *dp_catalog) >> return 0; >> } >> +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog >> *dp_catalog) >> +{ >> + struct dp_catalog_private *catalog; >> + u32 header, parity, data; >> + u8 bpc, off = 0; >> + u8 buf[SZ_128]; >> + >> + if (!dp_catalog) { >> + pr_err("invalid input\n"); >> + return; >> + } >> + >> + catalog = container_of(dp_catalog, struct dp_catalog_private, >> dp_catalog); >> + >> + /* HEADER BYTE 1 */ >> + header = dp_catalog->sdp.sdp_header.HB1; >> + parity = dp_catalog_calculate_parity(header); >> + data = ((header << HEADER_BYTE_1_BIT) | (parity << >> PARITY_BYTE_1_BIT)); >> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + /* HEADER BYTE 2 */ >> + header = dp_catalog->sdp.sdp_header.HB2; >> + parity = dp_catalog_calculate_parity(header); >> + data = ((header << HEADER_BYTE_2_BIT) | (parity << >> PARITY_BYTE_2_BIT)); >> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >> + >> + /* HEADER BYTE 3 */ >> + header = dp_catalog->sdp.sdp_header.HB3; >> + parity = dp_catalog_calculate_parity(header); >> + data = ((header << HEADER_BYTE_3_BIT) | (parity << >> PARITY_BYTE_3_BIT)); >> + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); >> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); > > This seems to be common with the dp_audio code. Please extract this > header writing too. These are two different sdp's. audio and vsc, are different with different registers being written to and different amount of registers being set. Can you please clarify since in audio we only need 3 registers to write to, and in vsc we need 10. > >> + >> + data = 0; >> + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); > > Generally this is not how these functions are expected to be written. > Please take a look at drivers/video/hdmi.c. It should be split into: > - generic function that packs the C structure into a flat byte buffer, > - driver-specific function that formats and writes the buffer to the > hardware. > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + switch (dp_catalog->vsc_sdp_data.bpc) { >> + case 10: >> + bpc = DP_GENERIC0_6_YUV_10_BPC; >> + break; >> + case 8: >> + default: >> + bpc = DP_GENERIC0_6_YUV_8_BPC; >> + break; >> + } >> + >> + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ >> + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | >> + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | >> + (bpc << 8) | >> + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | >> + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); >> + >> + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + data = 0; >> + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + dp_write_link(catalog, MMSS_DP_GENERIC0_9, data); >> + memcpy(buf + off, &data, sizeof(data)); >> + off += sizeof(data); >> + >> + print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", DUMP_PREFIX_NONE, >> 16, 4, buf, off, false); >> +} >> + >> +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, >> bool en) >> +{ >> + struct dp_catalog_private *catalog; >> + u32 cfg, cfg2, misc; >> + u16 major = 0, minor = 0; >> + >> + if (!dp_catalog) { >> + pr_err("invalid input\n"); >> + return; >> + } >> + >> + catalog = container_of(dp_catalog, struct dp_catalog_private, >> dp_catalog); >> + >> + cfg = dp_read_link(catalog, MMSS_DP_SDP_CFG); >> + cfg2 = dp_read_link(catalog, MMSS_DP_SDP_CFG2); >> + misc = dp_read_link(catalog, REG_DP_MISC1_MISC0); >> + >> + if (en) { >> + cfg |= GEN0_SDP_EN; >> + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); >> + >> + cfg2 |= GENERIC0_SDPSIZE; > > When I see a something_SIZE macro, I'd naturally expect it to be an > actual size of some data. Please consider renaming to e.g. > GENERIC0_SDPSIZE_VALID. Ack > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); >> + >> + dp_catalog_panel_setup_vsc_sdp(dp_catalog); >> + >> + /* indicates presence of VSC (BIT(6) of MISC1) */ >> + misc |= DP_MISC1_VSC_SDP; >> + >> + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); >> + } else { >> + cfg &= ~GEN0_SDP_EN; >> + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); >> + >> + cfg2 &= ~GENERIC0_SDPSIZE; >> + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); >> + >> + /* switch back to MSA */ >> + misc &= ~DP_MISC1_VSC_SDP; >> + >> + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); >> + } >> + >> + pr_debug("misc settings = 0x%x\n", misc); >> + dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); >> + >> + dp_catalog_hw_revision(dp_catalog, &major, &minor); >> + if (major == 1 && minor < 2) { >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); >> + } >> +} >> + >> void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, >> struct drm_display_mode *drm_mode) >> { >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h >> b/drivers/gpu/drm/msm/dp/dp_catalog.h >> index 94c377ef90c35..6b757249c0698 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h >> @@ -7,6 +7,7 @@ >> #define _DP_CATALOG_H_ >> #include <drm/drm_modes.h> >> +#include <drm/display/drm_dp_helper.h> >> #include "dp_parser.h" >> #include "disp/msm_disp_snapshot.h" >> @@ -76,6 +77,8 @@ struct dp_catalog { >> u32 dp_active; >> enum dp_catalog_audio_sdp_type sdp_type; >> enum dp_catalog_audio_header_type sdp_header; >> + struct dp_sdp sdp; > > I assume that the sdp field contains only transient data, which is not > used after it gets written to the hardware. Please remove it from the > struct allocate on a stack or via kzalloc. Ack > >> + struct drm_dp_vsc_sdp vsc_sdp_data; >> u32 audio_data; >> bool wide_bus_en; >> }; >> @@ -196,6 +199,7 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct >> dp_catalog *dp_catalog); >> /* DP Panel APIs */ >> int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog); >> +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, >> bool en); >> void dp_catalog_dump_regs(struct dp_catalog *dp_catalog); >> void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, >> struct drm_display_mode *drm_mode); >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 209cf2a35642f..ddd92a63d5a67 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1952,6 +1952,8 @@ int dp_ctrl_off_link_stream(struct dp_ctrl >> *dp_ctrl) >> dp_io = &ctrl->parser->io; >> phy = dp_io->phy; >> + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); >> + >> /* set dongle to D3 (power off) mode */ >> dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true); >> @@ -2026,6 +2028,8 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl) >> dp_io = &ctrl->parser->io; >> phy = dp_io->phy; >> + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); >> + >> dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); >> dp_catalog_ctrl_reset(ctrl->catalog); >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c >> b/drivers/gpu/drm/msm/dp/dp_panel.c >> index af7820b6d35ec..d6af9898b00d8 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c >> @@ -307,6 +307,49 @@ bool dp_panel_vsc_sdp_supported(struct dp_panel >> *dp_panel) >> return panel->major >= 1 && panel->minor >= 3 && >> panel->vsc_supported; >> } >> +static int dp_panel_setup_vsc_sdp(struct dp_panel *dp_panel) >> +{ >> + struct dp_catalog *catalog; >> + struct dp_panel_private *panel; >> + struct dp_display_mode *dp_mode; >> + int rc = 0; >> + >> + if (!dp_panel) { >> + pr_err("invalid input\n"); >> + rc = -EINVAL; >> + return rc; >> + } >> + >> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel); >> + catalog = panel->catalog; >> + dp_mode = &dp_panel->dp_mode; >> + >> + memset(&catalog->sdp, 0, sizeof(catalog->sdp)); >> + memset(&catalog->vsc_sdp_data, 0, sizeof(catalog->vsc_sdp_data)); >> + >> + /* VSC SDP header as per table 2-118 of DP 1.4 specification */ >> + catalog->sdp.sdp_header.HB0 = 0x00; >> + catalog->sdp.sdp_header.HB1 = 0x07; >> + catalog->sdp.sdp_header.HB2 = 0x05; >> + catalog->sdp.sdp_header.HB3 = 0x13; >> + >> + /* VSC SDP Payload for DB16 */ >> + catalog->vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420; >> + catalog->vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT; >> + >> + /* VSC SDP Payload for DB17 */ >> + catalog->vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA; >> + >> + /* VSC SDP Payload for DB18 */ >> + catalog->vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS; >> + >> + catalog->vsc_sdp_data.bpc = dp_mode->bpp / 3; >> + >> + dp_catalog_panel_config_vsc_sdp(catalog, true); >> + >> + return rc; >> +} >> + >> void dp_panel_dump_regs(struct dp_panel *dp_panel) >> { >> struct dp_catalog *catalog; >> @@ -370,6 +413,10 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel) >> catalog->dp_active = data; >> dp_catalog_panel_timing_cfg(catalog); >> + >> + if (dp_panel->dp_mode.out_fmt_is_yuv_420) >> + dp_panel_setup_vsc_sdp(dp_panel); >> + >> panel->panel_on = true; >> return 0; >> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h >> b/drivers/gpu/drm/msm/dp/dp_reg.h >> index ea85a691e72b5..756ddf85b1e81 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_reg.h >> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h >> @@ -142,6 +142,7 @@ >> #define DP_MISC0_SYNCHRONOUS_CLK (0x00000001) >> #define DP_MISC0_COLORIMETRY_CFG_SHIFT (0x00000001) >> #define DP_MISC0_TEST_BITS_DEPTH_SHIFT (0x00000005) >> +#define DP_MISC1_VSC_SDP (0x00004000) >> #define REG_DP_VALID_BOUNDARY (0x00000030) >> #define REG_DP_VALID_BOUNDARY_2 (0x00000034) >> @@ -201,9 +202,11 @@ >> #define MMSS_DP_AUDIO_CTRL_RESET (0x00000214) >> #define MMSS_DP_SDP_CFG (0x00000228) >> +#define GEN0_SDP_EN (0x00020000) >> #define MMSS_DP_SDP_CFG2 (0x0000022C) >> #define MMSS_DP_AUDIO_TIMESTAMP_0 (0x00000230) >> #define MMSS_DP_AUDIO_TIMESTAMP_1 (0x00000234) >> +#define GENERIC0_SDPSIZE (0x00010000) >> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240) >> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244) >
On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > > On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: > > On 25/01/2024 21:38, Paloma Arellano wrote: > >> Add support to pack and send the VSC SDP packet for DP. This therefore > >> allows the transmision of format information to the sinks which is > >> needed for YUV420 support over DP. > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ > >> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + > >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + > >> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ > >> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + > >> 5 files changed, 205 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > >> b/drivers/gpu/drm/msm/dp/dp_catalog.c > >> index c025786170ba5..7e4c68be23e56 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > >> @@ -29,6 +29,9 @@ > >> #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) > >> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) > >> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) > >> + > >> #define DP_INTERRUPT_STATUS1 \ > >> (DP_INTR_AUX_XFER_DONE| \ > >> DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ > >> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct > >> dp_catalog *dp_catalog) > >> return 0; > >> } > >> +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog > >> *dp_catalog) > >> +{ > >> + struct dp_catalog_private *catalog; > >> + u32 header, parity, data; > >> + u8 bpc, off = 0; > >> + u8 buf[SZ_128]; > >> + > >> + if (!dp_catalog) { > >> + pr_err("invalid input\n"); > >> + return; > >> + } > >> + > >> + catalog = container_of(dp_catalog, struct dp_catalog_private, > >> dp_catalog); > >> + > >> + /* HEADER BYTE 1 */ > >> + header = dp_catalog->sdp.sdp_header.HB1; > >> + parity = dp_catalog_calculate_parity(header); > >> + data = ((header << HEADER_BYTE_1_BIT) | (parity << > >> PARITY_BYTE_1_BIT)); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + /* HEADER BYTE 2 */ > >> + header = dp_catalog->sdp.sdp_header.HB2; > >> + parity = dp_catalog_calculate_parity(header); > >> + data = ((header << HEADER_BYTE_2_BIT) | (parity << > >> PARITY_BYTE_2_BIT)); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > >> + > >> + /* HEADER BYTE 3 */ > >> + header = dp_catalog->sdp.sdp_header.HB3; > >> + parity = dp_catalog_calculate_parity(header); > >> + data = ((header << HEADER_BYTE_3_BIT) | (parity << > >> PARITY_BYTE_3_BIT)); > >> + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > > > > This seems to be common with the dp_audio code. Please extract this > > header writing too. > These are two different sdp's. audio and vsc, are different with > different registers being written to and different amount of registers > being set. Can you please clarify since in audio we only need 3 > registers to write to, and in vsc we need 10. Bitmagic with the header is the same. Then the rest of the data is written one dword per register, if I'm not mistaken. > > > >> + > >> + data = 0; > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > > > > Generally this is not how these functions are expected to be written. > > Please take a look at drivers/video/hdmi.c. It should be split into: > > - generic function that packs the C structure into a flat byte buffer, > > - driver-specific function that formats and writes the buffer to the > > hardware. > > > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + switch (dp_catalog->vsc_sdp_data.bpc) { > >> + case 10: > >> + bpc = DP_GENERIC0_6_YUV_10_BPC; > >> + break; > >> + case 8: > >> + default: > >> + bpc = DP_GENERIC0_6_YUV_8_BPC; > >> + break; > >> + } > >> + > >> + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ > >> + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | > >> + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | > >> + (bpc << 8) | > >> + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | > >> + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); > >> + > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + data = 0; > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data); > >> + > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); > >> + memcpy(buf + off, &data, sizeof(data)); > >> + off += sizeof(data);
On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote: > On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> >> On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: >>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>> Add support to pack and send the VSC SDP packet for DP. This therefore >>>> allows the transmision of format information to the sinks which is >>>> needed for YUV420 support over DP. >>>> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + >>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + >>>> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ >>>> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + >>>> 5 files changed, 205 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c >>>> index c025786170ba5..7e4c68be23e56 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >>>> @@ -29,6 +29,9 @@ >>>> #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) >>>> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) >>>> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) >>>> + >>>> #define DP_INTERRUPT_STATUS1 \ >>>> (DP_INTR_AUX_XFER_DONE| \ >>>> DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ >>>> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct >>>> dp_catalog *dp_catalog) >>>> return 0; >>>> } >>>> +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog >>>> *dp_catalog) >>>> +{ >>>> + struct dp_catalog_private *catalog; >>>> + u32 header, parity, data; >>>> + u8 bpc, off = 0; >>>> + u8 buf[SZ_128]; >>>> + >>>> + if (!dp_catalog) { >>>> + pr_err("invalid input\n"); >>>> + return; >>>> + } >>>> + >>>> + catalog = container_of(dp_catalog, struct dp_catalog_private, >>>> dp_catalog); >>>> + >>>> + /* HEADER BYTE 1 */ >>>> + header = dp_catalog->sdp.sdp_header.HB1; >>>> + parity = dp_catalog_calculate_parity(header); >>>> + data = ((header << HEADER_BYTE_1_BIT) | (parity << >>>> PARITY_BYTE_1_BIT)); >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + /* HEADER BYTE 2 */ >>>> + header = dp_catalog->sdp.sdp_header.HB2; >>>> + parity = dp_catalog_calculate_parity(header); >>>> + data = ((header << HEADER_BYTE_2_BIT) | (parity << >>>> PARITY_BYTE_2_BIT)); >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >>>> + >>>> + /* HEADER BYTE 3 */ >>>> + header = dp_catalog->sdp.sdp_header.HB3; >>>> + parity = dp_catalog_calculate_parity(header); >>>> + data = ((header << HEADER_BYTE_3_BIT) | (parity << >>>> PARITY_BYTE_3_BIT)); >>>> + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>> >>> This seems to be common with the dp_audio code. Please extract this >>> header writing too. >> These are two different sdp's. audio and vsc, are different with >> different registers being written to and different amount of registers >> being set. Can you please clarify since in audio we only need 3 >> registers to write to, and in vsc we need 10. > > Bitmagic with the header is the same. Then the rest of the data is > written one dword per register, if I'm not mistaken. > We can generalize the MMSS_DP_GENERIC0 register writing by breaking it up to two things: 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only() 2) dp_catalog_write_generic_pkt() which will just write the packed buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register But audio seems a bit different. We use DP_AUDIO_STREAM_0/1. More importantly, it uses this sdp_map and writes each header one by one with dp_catalog_audio_set_header(). Not sure if that entirely fits with this pack and then write model. It can be simplified. But I dont think this effort is needed for this series. So I would prefer to generalize audio SDP programming separately. >>> >>>> + >>>> + data = 0; >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>> >>> Generally this is not how these functions are expected to be written. >>> Please take a look at drivers/video/hdmi.c. It should be split into: >>> - generic function that packs the C structure into a flat byte buffer, >>> - driver-specific function that formats and writes the buffer to the >>> hardware. >>> >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + switch (dp_catalog->vsc_sdp_data.bpc) { >>>> + case 10: >>>> + bpc = DP_GENERIC0_6_YUV_10_BPC; >>>> + break; >>>> + case 8: >>>> + default: >>>> + bpc = DP_GENERIC0_6_YUV_8_BPC; >>>> + break; >>>> + } >>>> + >>>> + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ >>>> + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | >>>> + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | >>>> + (bpc << 8) | >>>> + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | >>>> + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); >>>> + >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + data = 0; >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); >>>> + >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); >>>> + memcpy(buf + off, &data, sizeof(data)); >>>> + off += sizeof(data); > >
On Thu, 1 Feb 2024 at 03:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote: > > On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >> > >> > >> On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: > >>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>> Add support to pack and send the VSC SDP packet for DP. This therefore > >>>> allows the transmision of format information to the sinks which is > >>>> needed for YUV420 support over DP. > >>>> > >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>> --- > >>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ > >>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + > >>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + > >>>> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ > >>>> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + > >>>> 5 files changed, 205 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c > >>>> index c025786170ba5..7e4c68be23e56 100644 > >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > >>>> @@ -29,6 +29,9 @@ > >>>> #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) > >>>> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) > >>>> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) > >>>> + > >>>> #define DP_INTERRUPT_STATUS1 \ > >>>> (DP_INTR_AUX_XFER_DONE| \ > >>>> DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ > >>>> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct > >>>> dp_catalog *dp_catalog) > >>>> return 0; > >>>> } > >>>> +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog > >>>> *dp_catalog) > >>>> +{ > >>>> + struct dp_catalog_private *catalog; > >>>> + u32 header, parity, data; > >>>> + u8 bpc, off = 0; > >>>> + u8 buf[SZ_128]; > >>>> + > >>>> + if (!dp_catalog) { > >>>> + pr_err("invalid input\n"); > >>>> + return; > >>>> + } > >>>> + > >>>> + catalog = container_of(dp_catalog, struct dp_catalog_private, > >>>> dp_catalog); > >>>> + > >>>> + /* HEADER BYTE 1 */ > >>>> + header = dp_catalog->sdp.sdp_header.HB1; > >>>> + parity = dp_catalog_calculate_parity(header); > >>>> + data = ((header << HEADER_BYTE_1_BIT) | (parity << > >>>> PARITY_BYTE_1_BIT)); > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + /* HEADER BYTE 2 */ > >>>> + header = dp_catalog->sdp.sdp_header.HB2; > >>>> + parity = dp_catalog_calculate_parity(header); > >>>> + data = ((header << HEADER_BYTE_2_BIT) | (parity << > >>>> PARITY_BYTE_2_BIT)); > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > >>>> + > >>>> + /* HEADER BYTE 3 */ > >>>> + header = dp_catalog->sdp.sdp_header.HB3; > >>>> + parity = dp_catalog_calculate_parity(header); > >>>> + data = ((header << HEADER_BYTE_3_BIT) | (parity << > >>>> PARITY_BYTE_3_BIT)); > >>>> + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>> > >>> This seems to be common with the dp_audio code. Please extract this > >>> header writing too. > >> These are two different sdp's. audio and vsc, are different with > >> different registers being written to and different amount of registers > >> being set. Can you please clarify since in audio we only need 3 > >> registers to write to, and in vsc we need 10. > > > > Bitmagic with the header is the same. Then the rest of the data is > > written one dword per register, if I'm not mistaken. > > > > We can generalize the MMSS_DP_GENERIC0 register writing by breaking it > up to two things: > > 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only() Note, there is already a hdmi_audio_infoframe_pack_for_dp() function. I think this patchset can add hdmi_colorimetry_infoframe_pack_for_dp() [you can choose any other similar name that suits from your POV]. Also please extract the function that inits the dp_sdp_header. It can be reused as is for both existing hdmi_audio_infoframe_pack_for_dp(), your new function and the dp_audio code. > > 2) dp_catalog_write_generic_pkt() which will just write the packed > buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register > > But audio seems a bit different. We use DP_AUDIO_STREAM_0/1. > More importantly, it uses this sdp_map and writes each header one by one > with dp_catalog_audio_set_header(). > > Not sure if that entirely fits with this pack and then write model. > > It can be simplified. But I dont think this effort is needed for this > series. > > So I would prefer to generalize audio SDP programming separately. I'd definitely ask to add a utility function that merges 4 header bytes with the parity data. We already have 5 instances of that code in dp_audio.c, which is already too much by the number of 4. Adding the 6th copy is NAKed. BTW, I see both in this path and in dp_audio that the driver reads a register, ORs it with the value for the next header byte and writes it back to the hardware. Shouldn't the driver clear the corresponding data bits first? I see the clears in the techpack, but not in the upstream code. If my assumption is correct, we should end up with the utility function that packs dp_sdp_header into u32[2], which can then be used by both YUV and dp_audio code to just write two corresponding registers. BTW2: where is the rest of the audio infoframe? I see that the old fbdev driver was at least clearing the first 4 bytes of the frame. > > >>> > >>>> + > >>>> + data = 0; > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>> > >>> Generally this is not how these functions are expected to be written. > >>> Please take a look at drivers/video/hdmi.c. It should be split into: > >>> - generic function that packs the C structure into a flat byte buffer, > >>> - driver-specific function that formats and writes the buffer to the > >>> hardware. > >>> > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + switch (dp_catalog->vsc_sdp_data.bpc) { > >>>> + case 10: > >>>> + bpc = DP_GENERIC0_6_YUV_10_BPC; > >>>> + break; > >>>> + case 8: > >>>> + default: > >>>> + bpc = DP_GENERIC0_6_YUV_8_BPC; > >>>> + break; > >>>> + } > >>>> + > >>>> + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ > >>>> + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | > >>>> + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | > >>>> + (bpc << 8) | > >>>> + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | > >>>> + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); > >>>> + > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + data = 0; > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > >>>> + > >>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); > >>>> + memcpy(buf + off, &data, sizeof(data)); > >>>> + off += sizeof(data); > > > >
On 1/31/2024 8:36 PM, Dmitry Baryshkov wrote: > On Thu, 1 Feb 2024 at 03:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/27/2024 9:39 PM, Dmitry Baryshkov wrote: >>> On Sun, 28 Jan 2024 at 07:34, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>> >>>> >>>> On 1/25/2024 1:48 PM, Dmitry Baryshkov wrote: >>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>> Add support to pack and send the VSC SDP packet for DP. This therefore >>>>>> allows the transmision of format information to the sinks which is >>>>>> needed for YUV420 support over DP. >>>>>> >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ >>>>>> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + >>>>>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + >>>>>> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ >>>>>> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + >>>>>> 5 files changed, 205 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c >>>>>> index c025786170ba5..7e4c68be23e56 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >>>>>> @@ -29,6 +29,9 @@ >>>>>> #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) >>>>>> +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) >>>>>> +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) >>>>>> + >>>>>> #define DP_INTERRUPT_STATUS1 \ >>>>>> (DP_INTR_AUX_XFER_DONE| \ >>>>>> DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ >>>>>> @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct >>>>>> dp_catalog *dp_catalog) >>>>>> return 0; >>>>>> } >>>>>> +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog >>>>>> *dp_catalog) >>>>>> +{ >>>>>> + struct dp_catalog_private *catalog; >>>>>> + u32 header, parity, data; >>>>>> + u8 bpc, off = 0; >>>>>> + u8 buf[SZ_128]; >>>>>> + >>>>>> + if (!dp_catalog) { >>>>>> + pr_err("invalid input\n"); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + catalog = container_of(dp_catalog, struct dp_catalog_private, >>>>>> dp_catalog); >>>>>> + >>>>>> + /* HEADER BYTE 1 */ >>>>>> + header = dp_catalog->sdp.sdp_header.HB1; >>>>>> + parity = dp_catalog_calculate_parity(header); >>>>>> + data = ((header << HEADER_BYTE_1_BIT) | (parity << >>>>>> PARITY_BYTE_1_BIT)); >>>>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); >>>>>> + memcpy(buf + off, &data, sizeof(data)); >>>>>> + off += sizeof(data); >>>>>> + >>>>>> + /* HEADER BYTE 2 */ >>>>>> + header = dp_catalog->sdp.sdp_header.HB2; >>>>>> + parity = dp_catalog_calculate_parity(header); >>>>>> + data = ((header << HEADER_BYTE_2_BIT) | (parity << >>>>>> PARITY_BYTE_2_BIT)); >>>>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >>>>>> + >>>>>> + /* HEADER BYTE 3 */ >>>>>> + header = dp_catalog->sdp.sdp_header.HB3; >>>>>> + parity = dp_catalog_calculate_parity(header); >>>>>> + data = ((header << HEADER_BYTE_3_BIT) | (parity << >>>>>> PARITY_BYTE_3_BIT)); >>>>>> + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); >>>>>> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); >>>>>> + memcpy(buf + off, &data, sizeof(data)); >>>>>> + off += sizeof(data); >>>>> >>>>> This seems to be common with the dp_audio code. Please extract this >>>>> header writing too. >>>> These are two different sdp's. audio and vsc, are different with >>>> different registers being written to and different amount of registers >>>> being set. Can you please clarify since in audio we only need 3 >>>> registers to write to, and in vsc we need 10. >>> >>> Bitmagic with the header is the same. Then the rest of the data is >>> written one dword per register, if I'm not mistaken. >>> >> >> We can generalize the MMSS_DP_GENERIC0 register writing by breaking it >> up to two things: >> >> 1) Add a function vsc_sdp_pack() similar to hdmi_avi_infoframe_pack_only() > > Note, there is already a hdmi_audio_infoframe_pack_for_dp() function. > I think this patchset can add hdmi_colorimetry_infoframe_pack_for_dp() > [you can choose any other similar name that suits from your POV]. > > Also please extract the function that inits the dp_sdp_header. It can > be reused as is for both existing hdmi_audio_infoframe_pack_for_dp(), > your new function and the dp_audio code. > Not sure if extracting the header will work as all other functions in hdmi.c pack the header too so its a half and half implementation. I am going to start with keeping this pack function in msm/dp/dp_utils.c to start with. If it gets to a form which is generic enough to keep in a helper which can be common we can consider it once posted in v2. >> >> 2) dp_catalog_write_generic_pkt() which will just write the packed >> buffer byte-by-byte to these MMSS_DP_GENERIC0_xxx register >> >> But audio seems a bit different. We use DP_AUDIO_STREAM_0/1. >> More importantly, it uses this sdp_map and writes each header one by one >> with dp_catalog_audio_set_header(). >> >> Not sure if that entirely fits with this pack and then write model. >> >> It can be simplified. But I dont think this effort is needed for this >> series. >> >> So I would prefer to generalize audio SDP programming separately. > > I'd definitely ask to add a utility function that merges 4 header > bytes with the parity data. We already have 5 instances of that code > in dp_audio.c, which is already too much by the number of 4. Adding > the 6th copy is NAKed. > I acknowledge the overlap but coupling them with this feature doesn't make sense as we have to individually test out audio after that change even if its another cleanup. In case my previous response was not clear. I will certainly add a sdp packing function and utils which pack the 4 header/parity bytes so that they can be used by other sub-modules of DP but will still prefer to push them as a separate series for audio as I want to re-test audio with that. As discussed, I will work on that and post it in parallel. > BTW, I see both in this path and in dp_audio that the driver reads a > register, ORs it with the value for the next header byte and writes it > back to the hardware. Shouldn't the driver clear the corresponding > data bits first? I see the clears in the techpack, but not in the > upstream code. If my assumption is correct, we should end up with the > utility function that packs dp_sdp_header into u32[2], which can then > be used by both YUV and dp_audio code to just write two corresponding > registers. > Correct. Thats my goal too to have a common utility function which can be used by audio code as well but in this series i will only introduce the function. The audio usage will be another one. I will also incorporate the clearing part if applicable once I check it closely. > BTW2: where is the rest of the audio infoframe? I see that the old > fbdev driver was at least clearing the first 4 bytes of the frame. > hmm .... in DP we use audio infoframe SDP to send the audio information. IIRC, we only fill the header/parity but the payload is fully controlled by LPASS (audio side).
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index c025786170ba5..7e4c68be23e56 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -29,6 +29,9 @@ #define DP_INTF_CONFIG_DATABUS_WIDEN BIT(4) +#define DP_GENERIC0_6_YUV_8_BPC BIT(0) +#define DP_GENERIC0_6_YUV_10_BPC BIT(1) + #define DP_INTERRUPT_STATUS1 \ (DP_INTR_AUX_XFER_DONE| \ DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ @@ -907,6 +910,150 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog) return 0; } +static void dp_catalog_panel_setup_vsc_sdp(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog; + u32 header, parity, data; + u8 bpc, off = 0; + u8 buf[SZ_128]; + + if (!dp_catalog) { + pr_err("invalid input\n"); + return; + } + + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); + + /* HEADER BYTE 1 */ + header = dp_catalog->sdp.sdp_header.HB1; + parity = dp_catalog_calculate_parity(header); + data = ((header << HEADER_BYTE_1_BIT) | (parity << PARITY_BYTE_1_BIT)); + dp_write_link(catalog, MMSS_DP_GENERIC0_0, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + /* HEADER BYTE 2 */ + header = dp_catalog->sdp.sdp_header.HB2; + parity = dp_catalog_calculate_parity(header); + data = ((header << HEADER_BYTE_2_BIT) | (parity << PARITY_BYTE_2_BIT)); + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); + + /* HEADER BYTE 3 */ + header = dp_catalog->sdp.sdp_header.HB3; + parity = dp_catalog_calculate_parity(header); + data = ((header << HEADER_BYTE_3_BIT) | (parity << PARITY_BYTE_3_BIT)); + data |= dp_read_link(catalog, MMSS_DP_GENERIC0_1); + dp_write_link(catalog, MMSS_DP_GENERIC0_1, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + data = 0; + dp_write_link(catalog, MMSS_DP_GENERIC0_2, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + dp_write_link(catalog, MMSS_DP_GENERIC0_3, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + dp_write_link(catalog, MMSS_DP_GENERIC0_4, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + dp_write_link(catalog, MMSS_DP_GENERIC0_5, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + switch (dp_catalog->vsc_sdp_data.bpc) { + case 10: + bpc = DP_GENERIC0_6_YUV_10_BPC; + break; + case 8: + default: + bpc = DP_GENERIC0_6_YUV_8_BPC; + break; + } + + /* VSC SDP payload as per table 2-117 of DP 1.4 specification */ + data = (dp_catalog->vsc_sdp_data.colorimetry & 0xF) | + ((dp_catalog->vsc_sdp_data.pixelformat & 0xF) << 4) | + (bpc << 8) | + ((dp_catalog->vsc_sdp_data.dynamic_range & 0x1) << 15) | + ((dp_catalog->vsc_sdp_data.content_type & 0x7) << 16); + + dp_write_link(catalog, MMSS_DP_GENERIC0_6, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + data = 0; + dp_write_link(catalog, MMSS_DP_GENERIC0_7, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + dp_write_link(catalog, MMSS_DP_GENERIC0_8, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + dp_write_link(catalog, MMSS_DP_GENERIC0_9, data); + memcpy(buf + off, &data, sizeof(data)); + off += sizeof(data); + + print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", DUMP_PREFIX_NONE, 16, 4, buf, off, false); +} + +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, bool en) +{ + struct dp_catalog_private *catalog; + u32 cfg, cfg2, misc; + u16 major = 0, minor = 0; + + if (!dp_catalog) { + pr_err("invalid input\n"); + return; + } + + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); + + cfg = dp_read_link(catalog, MMSS_DP_SDP_CFG); + cfg2 = dp_read_link(catalog, MMSS_DP_SDP_CFG2); + misc = dp_read_link(catalog, REG_DP_MISC1_MISC0); + + if (en) { + cfg |= GEN0_SDP_EN; + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); + + cfg2 |= GENERIC0_SDPSIZE; + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); + + dp_catalog_panel_setup_vsc_sdp(dp_catalog); + + /* indicates presence of VSC (BIT(6) of MISC1) */ + misc |= DP_MISC1_VSC_SDP; + + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); + } else { + cfg &= ~GEN0_SDP_EN; + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); + + cfg2 &= ~GENERIC0_SDPSIZE; + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); + + /* switch back to MSA */ + misc &= ~DP_MISC1_VSC_SDP; + + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=%d\n", en); + } + + pr_debug("misc settings = 0x%x\n", misc); + dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); + + dp_catalog_hw_revision(dp_catalog, &major, &minor); + if (major == 1 && minor < 2) { + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); + } +} + void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, struct drm_display_mode *drm_mode) { diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 94c377ef90c35..6b757249c0698 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -7,6 +7,7 @@ #define _DP_CATALOG_H_ #include <drm/drm_modes.h> +#include <drm/display/drm_dp_helper.h> #include "dp_parser.h" #include "disp/msm_disp_snapshot.h" @@ -76,6 +77,8 @@ struct dp_catalog { u32 dp_active; enum dp_catalog_audio_sdp_type sdp_type; enum dp_catalog_audio_header_type sdp_header; + struct dp_sdp sdp; + struct drm_dp_vsc_sdp vsc_sdp_data; u32 audio_data; bool wide_bus_en; }; @@ -196,6 +199,7 @@ u32 dp_catalog_ctrl_read_phy_pattern(struct dp_catalog *dp_catalog); /* DP Panel APIs */ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog); +void dp_catalog_panel_config_vsc_sdp(struct dp_catalog *dp_catalog, bool en); void dp_catalog_dump_regs(struct dp_catalog *dp_catalog); void dp_catalog_panel_tpg_enable(struct dp_catalog *dp_catalog, struct drm_display_mode *drm_mode); diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c index 209cf2a35642f..ddd92a63d5a67 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1952,6 +1952,8 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl) dp_io = &ctrl->parser->io; phy = dp_io->phy; + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); + /* set dongle to D3 (power off) mode */ dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true); @@ -2026,6 +2028,8 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl) dp_io = &ctrl->parser->io; phy = dp_io->phy; + dp_catalog_panel_config_vsc_sdp(ctrl->catalog, false); + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); dp_catalog_ctrl_reset(ctrl->catalog); diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index af7820b6d35ec..d6af9898b00d8 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -307,6 +307,49 @@ bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel) return panel->major >= 1 && panel->minor >= 3 && panel->vsc_supported; } +static int dp_panel_setup_vsc_sdp(struct dp_panel *dp_panel) +{ + struct dp_catalog *catalog; + struct dp_panel_private *panel; + struct dp_display_mode *dp_mode; + int rc = 0; + + if (!dp_panel) { + pr_err("invalid input\n"); + rc = -EINVAL; + return rc; + } + + panel = container_of(dp_panel, struct dp_panel_private, dp_panel); + catalog = panel->catalog; + dp_mode = &dp_panel->dp_mode; + + memset(&catalog->sdp, 0, sizeof(catalog->sdp)); + memset(&catalog->vsc_sdp_data, 0, sizeof(catalog->vsc_sdp_data)); + + /* VSC SDP header as per table 2-118 of DP 1.4 specification */ + catalog->sdp.sdp_header.HB0 = 0x00; + catalog->sdp.sdp_header.HB1 = 0x07; + catalog->sdp.sdp_header.HB2 = 0x05; + catalog->sdp.sdp_header.HB3 = 0x13; + + /* VSC SDP Payload for DB16 */ + catalog->vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420; + catalog->vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT; + + /* VSC SDP Payload for DB17 */ + catalog->vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA; + + /* VSC SDP Payload for DB18 */ + catalog->vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS; + + catalog->vsc_sdp_data.bpc = dp_mode->bpp / 3; + + dp_catalog_panel_config_vsc_sdp(catalog, true); + + return rc; +} + void dp_panel_dump_regs(struct dp_panel *dp_panel) { struct dp_catalog *catalog; @@ -370,6 +413,10 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel) catalog->dp_active = data; dp_catalog_panel_timing_cfg(catalog); + + if (dp_panel->dp_mode.out_fmt_is_yuv_420) + dp_panel_setup_vsc_sdp(dp_panel); + panel->panel_on = true; return 0; diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h index ea85a691e72b5..756ddf85b1e81 100644 --- a/drivers/gpu/drm/msm/dp/dp_reg.h +++ b/drivers/gpu/drm/msm/dp/dp_reg.h @@ -142,6 +142,7 @@ #define DP_MISC0_SYNCHRONOUS_CLK (0x00000001) #define DP_MISC0_COLORIMETRY_CFG_SHIFT (0x00000001) #define DP_MISC0_TEST_BITS_DEPTH_SHIFT (0x00000005) +#define DP_MISC1_VSC_SDP (0x00004000) #define REG_DP_VALID_BOUNDARY (0x00000030) #define REG_DP_VALID_BOUNDARY_2 (0x00000034) @@ -201,9 +202,11 @@ #define MMSS_DP_AUDIO_CTRL_RESET (0x00000214) #define MMSS_DP_SDP_CFG (0x00000228) +#define GEN0_SDP_EN (0x00020000) #define MMSS_DP_SDP_CFG2 (0x0000022C) #define MMSS_DP_AUDIO_TIMESTAMP_0 (0x00000230) #define MMSS_DP_AUDIO_TIMESTAMP_1 (0x00000234) +#define GENERIC0_SDPSIZE (0x00010000) #define MMSS_DP_AUDIO_STREAM_0 (0x00000240) #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
Add support to pack and send the VSC SDP packet for DP. This therefore allows the transmision of format information to the sinks which is needed for YUV420 support over DP. Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_catalog.c | 147 ++++++++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++ drivers/gpu/drm/msm/dp/dp_reg.h | 3 + 5 files changed, 205 insertions(+)