On Wed, 14 Feb 2024 at 22:15, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 2/14/2024 11:39 AM, Dmitry Baryshkov wrote: > > On Wed, 14 Feb 2024 at 20:04, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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. > >> > >> Changes in v3: > >> - Create a new struct, msm_dp_sdp_with_parity, which holds the > >> packing information for VSC SDP > >> - Use drm_dp_vsc_sdp_pack() to pack the data into the new > >> msm_dp_sdp_with_parity struct instead of specifically packing > >> for YUV420 format > >> - Modify dp_catalog_panel_send_vsc_sdp() to send the VSC SDP > >> data using the new msm_dp_sdp_with_parity struct > >> > >> Changes in v2: > >> - Rename GENERIC0_SDPSIZE macro to GENERIC0_SDPSIZE_VALID > >> - Remove dp_sdp from the dp_catalog struct since this data is > >> being allocated at the point used > >> - Create a new function in dp_utils to pack the VSC SDP data > >> into a buffer > >> - Create a new function that packs the SDP header bytes into a > >> buffer. This function is made generic so that it can be > >> utilized by dp_audio > >> header bytes into a buffer > >> - Create a new function in dp_utils that takes the packed buffer > >> and writes to the DP_GENERIC0_* registers > >> - Split the dp_catalog_panel_config_vsc_sdp() function into two > >> to disable/enable sending VSC SDP packets > >> - Check the DP HW version using the original useage of > >> dp_catalog_hw_revision() and correct the version checking > >> logic > >> - Rename dp_panel_setup_vsc_sdp() to > >> dp_panel_setup_vsc_sdp_yuv_420() to explicitly state that > >> currently VSC SDP is only being set up to support YUV420 modes > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/msm/dp/dp_catalog.c | 113 ++++++++++++++++++++++++++++ > >> drivers/gpu/drm/msm/dp/dp_catalog.h | 7 ++ > >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 + > >> drivers/gpu/drm/msm/dp/dp_panel.c | 55 ++++++++++++++ > >> drivers/gpu/drm/msm/dp/dp_reg.h | 3 + > >> drivers/gpu/drm/msm/dp/dp_utils.c | 48 ++++++++++++ > >> drivers/gpu/drm/msm/dp/dp_utils.h | 18 +++++ > >> 7 files changed, 248 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > >> index 5d84c089e520a..61d5317efe683 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > >> @@ -901,6 +901,119 @@ int dp_catalog_panel_timing_cfg(struct dp_catalog *dp_catalog) > >> return 0; > >> } > >> > >> +static void dp_catalog_panel_send_vsc_sdp(struct dp_catalog *dp_catalog, > >> + struct msm_dp_sdp_with_parity *msm_dp_sdp) > >> +{ > >> + struct dp_catalog_private *catalog; > >> + u32 val; > >> + > >> + if (!dp_catalog) { > >> + DRM_ERROR("invalid input\n"); > >> + return; > >> + } > >> + > >> + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); > >> + > >> + val = ((msm_dp_sdp->vsc_sdp.sdp_header.HB0) << HEADER_BYTE_0_BIT | > >> + (msm_dp_sdp->pb.PB0 << PARITY_BYTE_0_BIT) | > >> + (msm_dp_sdp->vsc_sdp.sdp_header.HB1) << HEADER_BYTE_1_BIT | > >> + (msm_dp_sdp->pb.PB1 << PARITY_BYTE_1_BIT)); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_0, val); > >> + > >> + val = ((msm_dp_sdp->vsc_sdp.sdp_header.HB2) << HEADER_BYTE_2_BIT | > >> + (msm_dp_sdp->pb.PB2 << PARITY_BYTE_2_BIT) | > >> + (msm_dp_sdp->vsc_sdp.sdp_header.HB3) << HEADER_BYTE_3_BIT | > >> + (msm_dp_sdp->pb.PB3 << PARITY_BYTE_3_BIT)); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_1, val); > > > > I still think that this is not the way to do it. Could you please > > extract the function that takes struct dp_sdp_header, calculates > > padding and writes resulting data to the hardware? This way we can > > reuse it later for all the dp_audio stuff. > > > > hmmm ... dp_utils_pack_sdp_header() does that you are asking for right? > > OR are you asking for another function like: > > 1) rename dp_utils_pack_sdp_header() to dp_utils_calc_sdp_parity() > 2) dp_utils_pack_sdp() takes two u32 to pack the header and parity > together and we move the << HEADER_BYTE_xx | part to it > > dp_catalog_panel_send_vsc_sdp() just uses these two u32 to write the > headers. I'm really looking for the following function: void dp_catalog_panel_send_vsc_sdp(struct dp_catalog *dp_catalog, struct dp_sdp *dp_sdp) { dp_write_vsc_header(dp_catalog, MMSS_DP_GENERIC0_0, &dp_sdp->sdp_header); dp_write_vsc_packet(dp_catalog, MMSS_DP_GENERIC0_2, dp_sdp); } Then dp_audio functions will be able to fill struct dp_sdp_header and call dp_write_vsc_header (or whatever other name for that function) directly. > > > >> + > >> + val = ((msm_dp_sdp->vsc_sdp.db[16]) | (msm_dp_sdp->vsc_sdp.db[17] << 8) | > >> + (msm_dp_sdp->vsc_sdp.db[18] << 16)); > >> + dp_write_link(catalog, MMSS_DP_GENERIC0_6, val); > > > > Shouldn't we write full dp_sdp data, including all zeroes? Here you > > assume that there is no other data in dp_sdp and also that nobody > > wrote anything senseless to those registers. > > > > As per documentation, it says db[0] to db[15] are reserved so I thought > its better not to touch/use them and start writing for 16 onwards. > > 1592 * VSC SDP Payload for Pixel Encoding/Colorimetry Format > 1593 * db[0] - db[15]: Reserved > 1594 * db[16]: Pixel Encoding and Colorimetry Formats > 1595 * db[17]: Dynamic Range and Component Bit Depth > 1596 * db[18]: Content Type > 1597 * db[19] - db[31]: Reserved > 1598 */ If I understand correctly, it also supports 3D Stereo and other bits. Also other revisions of VSC packets have other field requirements. So, I don't think it is incorrect to write just bytes 16-18. > > >> +} > >> + > >> +static void dp_catalog_panel_update_sdp(struct dp_catalog *dp_catalog) > >> +{ > >> + struct dp_catalog_private *catalog; > >> + u32 hw_revision; > >> + > >> + catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); > >> + > >> + hw_revision = dp_catalog_hw_revision(dp_catalog); > >> + if (hw_revision < DP_HW_VERSION_1_2 && hw_revision >= DP_HW_VERSION_1_0) { > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x01); > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x00); > >> + } > >> +} > >> + > >> +void dp_catalog_panel_enable_vsc_sdp(struct dp_catalog *dp_catalog, > >> + struct msm_dp_sdp_with_parity *msm_dp_sdp) > >> +{ > >> + struct dp_catalog_private *catalog; > >> + u32 cfg, cfg2, misc; > >> + > >> + if (!dp_catalog) { > >> + DRM_ERROR("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); > >> + > >> + cfg |= GEN0_SDP_EN; > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > >> + > >> + cfg2 |= GENERIC0_SDPSIZE_VALID; > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG2, cfg2); > >> + > >> + dp_catalog_panel_send_vsc_sdp(dp_catalog, msm_dp_sdp); > >> + > >> + /* indicates presence of VSC (BIT(6) of MISC1) */ > >> + misc |= DP_MISC1_VSC_SDP; > >> + > >> + drm_dbg_dp(catalog->drm_dev, "vsc sdp enable=1\n"); > >> + > >> + pr_debug("misc settings = 0x%x\n", misc); > >> + dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); > >> + > >> + dp_catalog_panel_update_sdp(dp_catalog); > >> +} > >> + > >> +void dp_catalog_panel_disable_vsc_sdp(struct dp_catalog *dp_catalog) > >> +{ > >> + struct dp_catalog_private *catalog; > >> + u32 cfg, cfg2, misc; > >> + > >> + if (!dp_catalog) { > >> + DRM_ERROR("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); > >> + > >> + cfg &= ~GEN0_SDP_EN; > >> + dp_write_link(catalog, MMSS_DP_SDP_CFG, cfg); > >> + > >> + cfg2 &= ~GENERIC0_SDPSIZE_VALID; > >> + 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=0\n"); > >> + > >> + pr_debug("misc settings = 0x%x\n", misc); > >> + dp_write_link(catalog, REG_DP_MISC1_MISC0, misc); > >> + > >> + dp_catalog_panel_update_sdp(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_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h > >> index 6cb5e2a243de2..4bf08c27a9bf3 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h > >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > >> @@ -9,6 +9,7 @@ > >> #include <drm/drm_modes.h> > >> > >> #include "dp_parser.h" > >> +#include "dp_utils.h" > >> #include "disp/msm_disp_snapshot.h" > >> > >> /* interrupts */ > >> @@ -30,6 +31,9 @@ > >> > >> #define DP_AUX_CFG_MAX_VALUE_CNT 3 > >> > >> +#define DP_HW_VERSION_1_0 0x10000000 > >> +#define DP_HW_VERSION_1_2 0x10020000 > >> + > >> /* PHY AUX config registers */ > >> enum dp_phy_aux_config_type { > >> PHY_AUX_CFG0, > >> @@ -124,6 +128,9 @@ 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_enable_vsc_sdp(struct dp_catalog *dp_catalog, > >> + struct msm_dp_sdp_with_parity *msm_dp_sdp); > >> +void dp_catalog_panel_disable_vsc_sdp(struct dp_catalog *dp_catalog); > >> 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..beef86b1aaf81 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_disable_vsc_sdp(ctrl->catalog); > >> + > >> /* 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_disable_vsc_sdp(ctrl->catalog); > >> + > >> 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 db1942794f1a4..18420a7ba4ab3 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > >> @@ -4,6 +4,7 @@ > >> */ > >> > >> #include "dp_panel.h" > >> +#include "dp_utils.h" > >> > >> #include <drm/drm_connector.h> > >> #include <drm/drm_edid.h> > >> @@ -281,6 +282,56 @@ void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable) > >> dp_catalog_panel_tpg_enable(catalog, &panel->dp_panel.dp_mode.drm_mode); > >> } > >> > >> +static int dp_panel_setup_vsc_sdp_yuv_420(struct dp_panel *dp_panel) > >> +{ > >> + struct dp_catalog *catalog; > >> + struct dp_panel_private *panel; > >> + struct dp_display_mode *dp_mode; > >> + struct drm_dp_vsc_sdp vsc_sdp_data; > >> + struct msm_dp_sdp_with_parity msm_dp_sdp; > >> + ssize_t len; > >> + int rc = 0; > >> + > >> + if (!dp_panel) { > >> + DRM_ERROR("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(&vsc_sdp_data, 0, sizeof(vsc_sdp_data)); > >> + > >> + /* VSC SDP header as per table 2-118 of DP 1.4 specification */ > >> + vsc_sdp_data.sdp_type = DP_SDP_VSC; > >> + vsc_sdp_data.revision = 0x05; > >> + vsc_sdp_data.length = 0x13; > >> + > >> + /* VSC SDP Payload for DB16 */ > >> + vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420; > >> + vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT; > >> + > >> + /* VSC SDP Payload for DB17 */ > >> + vsc_sdp_data.bpc = dp_mode->bpp / 3; > >> + vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA; > >> + > >> + /* VSC SDP Payload for DB18 */ > >> + vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS; > >> + > >> + // rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header, gen_buffer, buff_size); > >> + len = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &msm_dp_sdp); > >> + if (len < 0) { > >> + DRM_ERROR("unable to pack vsc sdp\n"); > >> + return len; > >> + } > >> + > >> + dp_catalog_panel_enable_vsc_sdp(catalog, &msm_dp_sdp); > >> + > >> + return rc; > >> +} > >> + > >> void dp_panel_dump_regs(struct dp_panel *dp_panel) > >> { > >> struct dp_catalog *catalog; > >> @@ -344,6 +395,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_yuv_420(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..2983756c125cd 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_VALID (0x00010000) > >> > >> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240) > >> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244) > >> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c > >> index 3a44fe738c004..81601f3c414fc 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_utils.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c > >> @@ -4,6 +4,7 @@ > >> */ > >> > >> #include <linux/types.h> > >> +#include <drm/drm_print.h> > >> > >> #include "dp_utils.h" > >> > >> @@ -71,3 +72,50 @@ u8 dp_utils_calculate_parity(u32 data) > >> > >> return parity_byte; > >> } > >> + > >> +ssize_t dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, struct msm_dp_vsc_sdp_parity *pb, > >> + size_t size) > >> +{ > >> + u8 header; > >> + > >> + size_t length = sizeof(struct msm_dp_vsc_sdp_parity); > >> + > >> + if (size < length) > >> + return -ENOSPC; > >> + > >> + memset(pb, 0, size); > >> + > >> + header = sdp_header->HB0; > >> + pb->PB0 = dp_utils_calculate_parity(header); > >> + > >> + header = sdp_header->HB1; > >> + pb->PB1 = dp_utils_calculate_parity(header); > >> + > >> + header = sdp_header->HB2; > >> + pb->PB2 = dp_utils_calculate_parity(header); > >> + > >> + header = sdp_header->HB3; > >> + pb->PB3 = dp_utils_calculate_parity(header); > >> + > >> + return length; > >> +} > >> + > >> +ssize_t dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc, struct msm_dp_sdp_with_parity *msm_dp_sdp) > >> +{ > >> + ssize_t len; > >> + > >> + len = drm_dp_vsc_sdp_pack(vsc, &msm_dp_sdp->vsc_sdp, sizeof(msm_dp_sdp->vsc_sdp)); > >> + if (len < 0) { > >> + DRM_ERROR("unable to pack vsc sdp\n"); > >> + return len; > >> + } > >> + > >> + len = dp_utils_pack_sdp_header(&msm_dp_sdp->vsc_sdp.sdp_header, &msm_dp_sdp->pb, > >> + sizeof(msm_dp_sdp->pb)); > >> + if (len < 0) { > >> + DRM_ERROR("unable to pack sdp header\n"); > >> + return len; > >> + } > >> + > >> + return len; > >> +} > >> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h > >> index 5a505cbf3432b..6946bc51cae97 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_utils.h > >> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h > >> @@ -6,6 +6,8 @@ > >> #ifndef _DP_UTILS_H_ > >> #define _DP_UTILS_H_ > >> > >> +#include <drm/display/drm_dp_helper.h> > >> + > >> #define HEADER_BYTE_0_BIT 0 > >> #define PARITY_BYTE_0_BIT 8 > >> #define HEADER_BYTE_1_BIT 16 > >> @@ -15,8 +17,24 @@ > >> #define HEADER_BYTE_3_BIT 16 > >> #define PARITY_BYTE_3_BIT 24 > >> > >> +struct msm_dp_vsc_sdp_parity { > >> + u8 PB0; > >> + u8 PB1; > >> + u8 PB2; > >> + u8 PB3; > >> +} __packed; > >> + > >> +struct msm_dp_sdp_with_parity { > >> + struct dp_sdp vsc_sdp; > >> + struct msm_dp_vsc_sdp_parity pb; > >> +}; > >> + > >> u8 dp_utils_get_g0_value(u8 data); > >> u8 dp_utils_get_g1_value(u8 data); > >> u8 dp_utils_calculate_parity(u32 data); > >> +ssize_t dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, struct msm_dp_vsc_sdp_parity *pb, > >> + size_t size); > >> +ssize_t dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc, > >> + struct msm_dp_sdp_with_parity *msm_dp_sdp); > >> > >> #endif /* _DP_UTILS_H_ */ > >> -- > >> 2.39.2 > >> > > > > -- With best wishes Dmitry