mbox series

[v5,00/19] Add support for CDM over DP

Message ID 20240222194025.25329-1-quic_parellan@quicinc.com
Headers show
Series Add support for CDM over DP | expand

Message

Paloma Arellano Feb. 22, 2024, 7:39 p.m. UTC 
The Chroma Down Sampling (CDM) block is a hardware component in the DPU
pipeline that includes a CSC block capable of converting RGB input from
the DPU to YUV data.

This block can be used with either HDMI, DP, or writeback interfaces.
This series adds support for the CDM block to be used with DP in
YUV420 mode format.

This series allows selection of the YUV420 format for monitors which support
certain resolutions only in YUV420 thus unblocking the validation of many
other resolutions which were previously filtered out if the connector did
not support YUV420.

This was validated using a DP connected monitor requiring the use of
YUV420 format.

This series is dependent on [1], [2], and [3]:
[1] https://patchwork.freedesktop.org/series/118831/
[2] https://patchwork.freedesktop.org/series/129395/
[3] https://patchwork.freedesktop.org/series/129864/

Changes in v5:
	- Slightly modify use of drm_dp_vsc_sdp_pack()
	- Remove dp_catalog NULL checks
	- Modify dp_utils_pack_sdp_header() to cleanly pack the header
	  buffer
	- Remove the dp_utils_pack_vsc_sdp() function and only call
	  drm_dp_vsc_sdp_pack() in dp_panel_setup_vsc_sdp_yuv_420()
	- To clearly show the relationship between the header buffer and
	  vsc_sdp struct, move dp_utils_pack_sdp_header() inside of
	  dp_catalog_panel_send_vsc_sdp()

Changes in v4:
	- Use dp_utils_pack_sdp_header() to pack the SDP header and
	  parity bytes into a buffer
	- Use this buffer when writing the VSC SDP data in
	  dp_catalog_panel_send_vsc_sdp() and write to all the
	  MMSS_DP_GENERIC0 registers
	- Clear up that DP_MAINLINK_CTRL_FLUSH_MODE register requires
	  the use of bits [24:23]
	- Modify certain macros to explicitly set their values in the
	  bits of DP_MAINLINK_CTRL_FLUSH_MODE_MASK
	- Remove hw_cdm check in dpu_encoder_needs_periph_flush() and
	  dpu_encoder_phys_vid_enable()

Changes in v3:
	- Change ordering of the header byte macros in dp_utils.h
	- Create a new struct, msm_dp_sdp_with_parity
 	- Utilize drm_dp_vsc_sdp_pack() from a new added dependency of
	  series [3] to pack the VSC SDP data into the new
	  msm_dp_sdp_with_parity struct instead of packing only for
	  YUV420
	- Modify dp_catalog_panel_send_vsc_sdp() so that it sends the VSC SDP data
	  using the new msm_dp_sdp_with_parity struct
	- Clear up that the DP_MAINLINK_FLUSH_MODE_SDE_PERIPH_UPDATE macro is setting
	  multiple bits and not just one
	- Move the connector's ycbcr_420_allowed parameter so it is no longer
	  dependent on if the dp_display is not eDP

Changes in v2:
	- Minor formatting changes throughout
	- Move 'fixes' patch to the top
	- Move VSC SDP support check API from dp_panel.c to drm_dp_helper.c
	- Create a separate patch for modifying the dimensions for CDM setup to be
	  non-WB specific
	- Remove a patch that modified the INTF_CONFIG2 register in favor of having
	  this series be dependent on [2]
	- Separate configuration ctrl programming from clock related programming into
	  two patches
	- Add a VSC SDP check in dp_bridge_mode_valid()
	- Move parity calculation functions to new files dp_utils.c and dp_utils.h
	- Remove dp_catalog_hw_revision() changes and utilize the original version of
	  the function when checking the DP hardware version
	- Create separate packing and programming functions for the VSC SDP
	- Make the packing header bytes function generic so it can be used with
	  dp_audio.c
	- Create two separate enable/disable VSC SDP functions instead of having one
	  with the ability to do both
	- Move timing engine programming to a separate patch from original encoder
	  programming patch
	- Move update_pending_flush_periph() code to be in the same patch as the
	  encoder programming
	- Create new API's to check if the dpu encoder needs a peripheral flush
	- Allow YUV420 modes for the DP connector when there's a CDM block available
	  instead of checking if VSC SDP is supported

Kuogee Hsieh (1):
  drm/msm/dpu: add support of new peripheral flush mechanism

Paloma Arellano (18):
  drm/msm/dpu: allow certain formats for CDM for DP
  drm/msm/dpu: add division of drm_display_mode's hskew parameter
  drm/msm/dpu: pass mode dimensions instead of fb size in CDM setup
  drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP
  drm/msm/dpu: move dpu_encoder_helper_phys_setup_cdm to dpu_encoder
  drm/msm/dp: rename wide_bus_en to wide_bus_supported
  drm/msm/dp: store mode YUV420 information to be used by rest of DP
  drm/msm/dp: check if VSC SDP is supported in DP programming
  drm/msm/dpu: move widebus logic to its own API
  drm/msm/dp: program config ctrl for YUV420 over DP
  drm/msm/dp: change clock related programming for YUV420 over DP
  drm/msm/dp: move parity calculation to dp_utils
  drm/msm/dp: add VSC SDP support for YUV420 over DP
  drm/msm/dp: enable SDP and SDE periph flush update
  drm/msm/dpu: modify encoder programming for CDM over DP
  drm/msm/dpu: modify timing engine programming for YUV420 over DP
  drm/msm/dpu: reserve CDM blocks for DP if mode is YUV420
  drm/msm/dp: allow YUV420 mode for DP connector when CDM available

 drivers/gpu/drm/msm/Makefile                  |   3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 164 +++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |   4 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  26 ++-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  30 +++-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 100 +----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c    |   2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c    |  17 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h    |  10 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   4 +-
 drivers/gpu/drm/msm/dp/dp_audio.c             | 101 ++---------
 drivers/gpu/drm/msm/dp/dp_catalog.c           | 115 +++++++++++-
 drivers/gpu/drm/msm/dp/dp_catalog.h           |   9 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c              |  17 +-
 drivers/gpu/drm/msm/dp/dp_display.c           |  82 ++++++---
 drivers/gpu/drm/msm/dp/dp_drm.c               |   6 +-
 drivers/gpu/drm/msm/dp/dp_drm.h               |   3 +-
 drivers/gpu/drm/msm/dp/dp_panel.c             |  53 ++++++
 drivers/gpu/drm/msm/dp/dp_panel.h             |   2 +
 drivers/gpu/drm/msm/dp/dp_reg.h               |   9 +
 drivers/gpu/drm/msm/dp/dp_utils.c             |  98 +++++++++++
 drivers/gpu/drm/msm/dp/dp_utils.h             |  36 ++++
 drivers/gpu/drm/msm/msm_drv.h                 |  22 ++-
 23 files changed, 672 insertions(+), 241 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/dp/dp_utils.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_utils.h

Comments

Dmitry Baryshkov Feb. 22, 2024, 9:56 p.m. UTC | #1 
On Thu, 22 Feb 2024 at 23:28, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
>
> On 2/22/2024 1:18 PM, Dmitry Baryshkov wrote:
> > On Thu, 22 Feb 2024 at 21:40, Paloma Arellano <quic_parellan@quicinc.com> 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 v5:
> >>          - Slightly modify use of drm_dp_vsc_sdp_pack()
> >>          - Remove dp_catalog NULL checks
> >>          - Modify dp_utils_pack_sdp_header() to more clearly pack the
> >>            header buffer
> >>          - Move dp_utils_pack_sdp_header() inside of
> >>            dp_catalog_panel_send_vsc_sdp to clearly show the relationship
> >>            between the header buffer and the vsc_sdp struct
> >>          - Due to the last point, remove the dp_utils_pack_vsc_sdp()
> >>            function and only call drm_dp_vsc_sdp_pack() in
> >>            dp_panel_setup_vsc_sdp_yuv_420()
> >>
> >> Changes in v4:
> >>          - Remove struct msm_dp_sdp_with_parity
> >>          - Use dp_utils_pack_sdp_header() to pack the SDP header and
> >>            parity bytes into a buffer
> >>          - Use this buffer when writing the VSC SDP data in
> >>            dp_catalog_panel_send_vsc_sdp()
> >>          - Write to all of the MMSS_DP_GENERIC0 registers instead of just
> >>            the ones with non-zero values
> >>
> >> 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@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_catalog.c | 93 +++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/dp/dp_catalog.h |  6 ++
> >>   drivers/gpu/drm/msm/dp/dp_ctrl.c    |  4 ++
> >>   drivers/gpu/drm/msm/dp/dp_panel.c   | 52 ++++++++++++++++
> >>   drivers/gpu/drm/msm/dp/dp_reg.h     |  3 +
> >>   drivers/gpu/drm/msm/dp/dp_utils.c   | 25 ++++++++
> >>   drivers/gpu/drm/msm/dp/dp_utils.h   | 14 +++++
> >>   7 files changed, 197 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index 5d84c089e520a..bfc6f53ae167f 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -901,6 +901,99 @@ 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 dp_sdp *vsc_sdp)
> >> +{
> >> +       struct dp_catalog_private *catalog;
> >> +       u32 header[2];
> >> +       u32 val;
> >> +       int i;
> >> +
> >> +       catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
> >> +
> >> +       dp_utils_pack_sdp_header(&vsc_sdp->sdp_header, header);
> >> +
> >> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, header[0]);
> >> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, header[1]);
> >> +
> >> +       for (i = 0; i < sizeof(vsc_sdp->db); i += 4) {
> >> +               val = ((vsc_sdp->db[i]) | (vsc_sdp->db[i + 1] << 8) | (vsc_sdp->db[i + 2] << 16) |
> >> +                      (vsc_sdp->db[i + 3] << 24));
> >> +               dp_write_link(catalog, MMSS_DP_GENERIC0_2 + i, val);
> >> +       }
> >> +}
> >> +
> >> +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 dp_sdp *vsc_sdp)
> >> +{
> >> +       struct dp_catalog_private *catalog;
> >> +       u32 cfg, cfg2, misc;
> >> +
> >> +       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, vsc_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;
> >> +
> >> +       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..3605252effb59 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,8 @@ 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 dp_sdp *vsc_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 bffb7bac2c2c8..a42b29f9902c1 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> @@ -1947,6 +1947,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);
> >>
> >> @@ -2021,6 +2023,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..e425a5c86acdc 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,53 @@ 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 dp_sdp vsc_sdp;
> >> +       ssize_t len;
> >> +
> >> +       if (!dp_panel) {
> >> +               DRM_ERROR("invalid input\n");
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       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;
> >> +
> >> +       len = drm_dp_vsc_sdp_pack(&vsc_sdp_data, &vsc_sdp);
> >> +       if (len < 0) {
> >> +               DRM_ERROR("unable to pack vsc sdp\n");
> >> +               return len;
> >> +       }
> >> +
> >> +       dp_catalog_panel_enable_vsc_sdp(catalog, &vsc_sdp);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>   {
> >>          struct dp_catalog *catalog;
> >> @@ -344,6 +392,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 78785ed4b40c4..aa9f6c3e4ddeb 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 DP_MISC0_COLORIMERY_CFG_LEGACY_RGB     (0)
> >>   #define DP_MISC0_COLORIMERY_CFG_CEA_RGB                (0x04)
> >> @@ -204,9 +205,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..3c8db0e9adfa6 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> >> @@ -7,6 +7,8 @@
> >>
> >>   #include "dp_utils.h"
> >>
> >> +#define DP_SDP_HEADER_SIZE             8
> >> +
> >>   u8 dp_utils_get_g0_value(u8 data)
> >>   {
> >>          u8 c[4];
> >> @@ -71,3 +73,26 @@ u8 dp_utils_calculate_parity(u32 data)
> >>
> >>          return parity_byte;
> >>   }
> >> +
> >> +ssize_t dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *header_buff)
> >> +{
> >> +       size_t length;
> >> +
> >> +       length = sizeof(header_buff);
> >> +       if (length < DP_SDP_HEADER_SIZE)
> >> +               return -ENOSPC;
> >> +
> >> +       memset(header_buff, 0, sizeof(header_buff));
> > Just out of curiosity:
> > 1) what is the amount of memory getting zeroed by this call?
> > 2) what's the point in zeroing it if the next two lines will overwrite it?
> >
> > If nobody objects, I'll remove this line while applying.
> >
> > If that's fine,
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
>
> Sounds good to me. Just didn't notice the redundancy of this line.

Please note the first question too. Do you understand the issue?

>
> -Paloma
>
>
> >
> >> +
> >> +       header_buff[0] = FIELD_PREP(HEADER_0_MASK, sdp_header->HB0) |
> >> +               FIELD_PREP(PARITY_0_MASK, dp_utils_calculate_parity(sdp_header->HB0)) |
> >> +               FIELD_PREP(HEADER_1_MASK, sdp_header->HB1) |
> >> +               FIELD_PREP(PARITY_1_MASK, dp_utils_calculate_parity(sdp_header->HB1));
> >> +
> >> +       header_buff[1] = FIELD_PREP(HEADER_2_MASK, sdp_header->HB2) |
> >> +               FIELD_PREP(PARITY_2_MASK, dp_utils_calculate_parity(sdp_header->HB2)) |
> >> +               FIELD_PREP(HEADER_3_MASK, sdp_header->HB3) |
> >> +               FIELD_PREP(PARITY_3_MASK, dp_utils_calculate_parity(sdp_header->HB3));
> >> +
> >> +       return length;
> >> +}
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
> >> index 5a505cbf3432b..7c056d9798dc3 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
> >> @@ -6,6 +6,10 @@
> >>   #ifndef _DP_UTILS_H_
> >>   #define _DP_UTILS_H_
> >>
> >> +#include <linux/bitfield.h>
> >> +#include <linux/bits.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 +19,18 @@
> >>   #define HEADER_BYTE_3_BIT      16
> >>   #define PARITY_BYTE_3_BIT      24
> >>
> >> +#define HEADER_0_MASK      GENMASK(7, 0)
> >> +#define PARITY_0_MASK      GENMASK(15, 8)
> >> +#define HEADER_1_MASK      GENMASK(23, 16)
> >> +#define PARITY_1_MASK      GENMASK(31, 24)
> >> +#define HEADER_2_MASK      GENMASK(7, 0)
> >> +#define PARITY_2_MASK      GENMASK(15, 8)
> >> +#define HEADER_3_MASK      GENMASK(23, 16)
> >> +#define PARITY_3_MASK      GENMASK(31, 24)
> >> +
> >>   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, u32 *header_buff);
> >>
> >>   #endif /* _DP_UTILS_H_ */
> >> --
> >> 2.39.2
> >>
> >
Paloma Arellano Feb. 22, 2024, 11:11 p.m. UTC | #2 
On 2/22/2024 1:56 PM, Dmitry Baryshkov wrote:
> On Thu, 22 Feb 2024 at 23:28, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>
>> On 2/22/2024 1:18 PM, Dmitry Baryshkov wrote:
>>> On Thu, 22 Feb 2024 at 21:40, Paloma Arellano <quic_parellan@quicinc.com> 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 v5:
>>>>           - Slightly modify use of drm_dp_vsc_sdp_pack()
>>>>           - Remove dp_catalog NULL checks
>>>>           - Modify dp_utils_pack_sdp_header() to more clearly pack the
>>>>             header buffer
>>>>           - Move dp_utils_pack_sdp_header() inside of
>>>>             dp_catalog_panel_send_vsc_sdp to clearly show the relationship
>>>>             between the header buffer and the vsc_sdp struct
>>>>           - Due to the last point, remove the dp_utils_pack_vsc_sdp()
>>>>             function and only call drm_dp_vsc_sdp_pack() in
>>>>             dp_panel_setup_vsc_sdp_yuv_420()
>>>>
>>>> Changes in v4:
>>>>           - Remove struct msm_dp_sdp_with_parity
>>>>           - Use dp_utils_pack_sdp_header() to pack the SDP header and
>>>>             parity bytes into a buffer
>>>>           - Use this buffer when writing the VSC SDP data in
>>>>             dp_catalog_panel_send_vsc_sdp()
>>>>           - Write to all of the MMSS_DP_GENERIC0 registers instead of just
>>>>             the ones with non-zero values
>>>>
>>>> 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@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_catalog.c | 93 +++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/msm/dp/dp_catalog.h |  6 ++
>>>>    drivers/gpu/drm/msm/dp/dp_ctrl.c    |  4 ++
>>>>    drivers/gpu/drm/msm/dp/dp_panel.c   | 52 ++++++++++++++++
>>>>    drivers/gpu/drm/msm/dp/dp_reg.h     |  3 +
>>>>    drivers/gpu/drm/msm/dp/dp_utils.c   | 25 ++++++++
>>>>    drivers/gpu/drm/msm/dp/dp_utils.h   | 14 +++++
>>>>    7 files changed, 197 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> index 5d84c089e520a..bfc6f53ae167f 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> @@ -901,6 +901,99 @@ 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 dp_sdp *vsc_sdp)
>>>> +{
>>>> +       struct dp_catalog_private *catalog;
>>>> +       u32 header[2];
>>>> +       u32 val;
>>>> +       int i;
>>>> +
>>>> +       catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
>>>> +
>>>> +       dp_utils_pack_sdp_header(&vsc_sdp->sdp_header, header);
>>>> +
>>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, header[0]);
>>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, header[1]);
>>>> +
>>>> +       for (i = 0; i < sizeof(vsc_sdp->db); i += 4) {
>>>> +               val = ((vsc_sdp->db[i]) | (vsc_sdp->db[i + 1] << 8) | (vsc_sdp->db[i + 2] << 16) |
>>>> +                      (vsc_sdp->db[i + 3] << 24));
>>>> +               dp_write_link(catalog, MMSS_DP_GENERIC0_2 + i, val);
>>>> +       }
>>>> +}
>>>> +
>>>> +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 dp_sdp *vsc_sdp)
>>>> +{
>>>> +       struct dp_catalog_private *catalog;
>>>> +       u32 cfg, cfg2, misc;
>>>> +
>>>> +       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, vsc_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;
>>>> +
>>>> +       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..3605252effb59 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,8 @@ 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 dp_sdp *vsc_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 bffb7bac2c2c8..a42b29f9902c1 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>>>> @@ -1947,6 +1947,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);
>>>>
>>>> @@ -2021,6 +2023,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..e425a5c86acdc 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,53 @@ 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 dp_sdp vsc_sdp;
>>>> +       ssize_t len;
>>>> +
>>>> +       if (!dp_panel) {
>>>> +               DRM_ERROR("invalid input\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       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;
>>>> +
>>>> +       len = drm_dp_vsc_sdp_pack(&vsc_sdp_data, &vsc_sdp);
>>>> +       if (len < 0) {
>>>> +               DRM_ERROR("unable to pack vsc sdp\n");
>>>> +               return len;
>>>> +       }
>>>> +
>>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, &vsc_sdp);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>    {
>>>>           struct dp_catalog *catalog;
>>>> @@ -344,6 +392,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 78785ed4b40c4..aa9f6c3e4ddeb 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 DP_MISC0_COLORIMERY_CFG_LEGACY_RGB     (0)
>>>>    #define DP_MISC0_COLORIMERY_CFG_CEA_RGB                (0x04)
>>>> @@ -204,9 +205,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..3c8db0e9adfa6 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>>>> @@ -7,6 +7,8 @@
>>>>
>>>>    #include "dp_utils.h"
>>>>
>>>> +#define DP_SDP_HEADER_SIZE             8
>>>> +
>>>>    u8 dp_utils_get_g0_value(u8 data)
>>>>    {
>>>>           u8 c[4];
>>>> @@ -71,3 +73,26 @@ u8 dp_utils_calculate_parity(u32 data)
>>>>
>>>>           return parity_byte;
>>>>    }
>>>> +
>>>> +ssize_t dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *header_buff)
>>>> +{
>>>> +       size_t length;
>>>> +
>>>> +       length = sizeof(header_buff);
>>>> +       if (length < DP_SDP_HEADER_SIZE)
>>>> +               return -ENOSPC;
>>>> +
>>>> +       memset(header_buff, 0, sizeof(header_buff));
>>> Just out of curiosity:
>>> 1) what is the amount of memory getting zeroed by this call?
>>> 2) what's the point in zeroing it if the next two lines will overwrite it?
>>>
>>> If nobody objects, I'll remove this line while applying.
>>>
>>> If that's fine,
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Sounds good to me. Just didn't notice the redundancy of this line.
> Please note the first question too. Do you understand the issue?


I'm assuming as the caller that the size of header_buff will always be 8 
bytes. But I see the issue where the API doesn't actually know the size 
of the header_buff and could set the whole header_buff to zero, when we 
are only trying to modify the first two values of the array. I 
originally had this line there since the header_buff[10] was a size of 
40 bytes in a past implementation where I was trying to set all of the 
values in the buffer to 0 before setting a select few to VSC SDP 
specific values. This was my bad.


>
>> -Paloma
>>
>>
>>>> +
>>>> +       header_buff[0] = FIELD_PREP(HEADER_0_MASK, sdp_header->HB0) |
>>>> +               FIELD_PREP(PARITY_0_MASK, dp_utils_calculate_parity(sdp_header->HB0)) |
>>>> +               FIELD_PREP(HEADER_1_MASK, sdp_header->HB1) |
>>>> +               FIELD_PREP(PARITY_1_MASK, dp_utils_calculate_parity(sdp_header->HB1));
>>>> +
>>>> +       header_buff[1] = FIELD_PREP(HEADER_2_MASK, sdp_header->HB2) |
>>>> +               FIELD_PREP(PARITY_2_MASK, dp_utils_calculate_parity(sdp_header->HB2)) |
>>>> +               FIELD_PREP(HEADER_3_MASK, sdp_header->HB3) |
>>>> +               FIELD_PREP(PARITY_3_MASK, dp_utils_calculate_parity(sdp_header->HB3));
>>>> +
>>>> +       return length;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
>>>> index 5a505cbf3432b..7c056d9798dc3 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
>>>> @@ -6,6 +6,10 @@
>>>>    #ifndef _DP_UTILS_H_
>>>>    #define _DP_UTILS_H_
>>>>
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bits.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 +19,18 @@
>>>>    #define HEADER_BYTE_3_BIT      16
>>>>    #define PARITY_BYTE_3_BIT      24
>>>>
>>>> +#define HEADER_0_MASK      GENMASK(7, 0)
>>>> +#define PARITY_0_MASK      GENMASK(15, 8)
>>>> +#define HEADER_1_MASK      GENMASK(23, 16)
>>>> +#define PARITY_1_MASK      GENMASK(31, 24)
>>>> +#define HEADER_2_MASK      GENMASK(7, 0)
>>>> +#define PARITY_2_MASK      GENMASK(15, 8)
>>>> +#define HEADER_3_MASK      GENMASK(23, 16)
>>>> +#define PARITY_3_MASK      GENMASK(31, 24)
>>>> +
>>>>    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, u32 *header_buff);
>>>>
>>>>    #endif /* _DP_UTILS_H_ */
>>>> --
>>>> 2.39.2
>>>>
>
>
Dmitry Baryshkov March 5, 2024, 12:28 a.m. UTC | #3 
On Thu, 22 Feb 2024 11:39:45 -0800, Paloma Arellano wrote:
> The Chroma Down Sampling (CDM) block is a hardware component in the DPU
> pipeline that includes a CSC block capable of converting RGB input from
> the DPU to YUV data.
> 
> This block can be used with either HDMI, DP, or writeback interfaces.
> This series adds support for the CDM block to be used with DP in
> YUV420 mode format.
> 
> [...]

Applied, thanks!

[01/19] drm/msm/dpu: allow certain formats for CDM for DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/32b6ff95b912
[02/19] drm/msm/dpu: add division of drm_display_mode's hskew parameter
        https://gitlab.freedesktop.org/lumag/msm/-/commit/551ee0f21099
[03/19] drm/msm/dpu: pass mode dimensions instead of fb size in CDM setup
        https://gitlab.freedesktop.org/lumag/msm/-/commit/d6e547c091d8
[04/19] drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/7cde7ce5be3e
[05/19] drm/msm/dpu: move dpu_encoder_helper_phys_setup_cdm to dpu_encoder
        https://gitlab.freedesktop.org/lumag/msm/-/commit/0ab07bb96826
[06/19] drm/msm/dp: rename wide_bus_en to wide_bus_supported
        https://gitlab.freedesktop.org/lumag/msm/-/commit/3ed77f333bb0
[07/19] drm/msm/dp: store mode YUV420 information to be used by rest of DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/5b30d1cbd4fb
[08/19] drm/msm/dp: check if VSC SDP is supported in DP programming
        https://gitlab.freedesktop.org/lumag/msm/-/commit/ab2f8603e22e
[09/19] drm/msm/dpu: move widebus logic to its own API
        https://gitlab.freedesktop.org/lumag/msm/-/commit/1cfc64a6ea55
[10/19] drm/msm/dp: program config ctrl for YUV420 over DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/683d374582e3
[11/19] drm/msm/dp: change clock related programming for YUV420 over DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/6db6e5606576
[12/19] drm/msm/dp: move parity calculation to dp_utils
        https://gitlab.freedesktop.org/lumag/msm/-/commit/09b27a482a18
[13/19] drm/msm/dp: add VSC SDP support for YUV420 over DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/55fb8ffc1802
[14/19] drm/msm/dpu: add support of new peripheral flush mechanism
        https://gitlab.freedesktop.org/lumag/msm/-/commit/64f7b81f0358
[15/19] drm/msm/dp: enable SDP and SDE periph flush update
        https://gitlab.freedesktop.org/lumag/msm/-/commit/21497a463347
[16/19] drm/msm/dpu: modify encoder programming for CDM over DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/b40c8377d792
[17/19] drm/msm/dpu: modify timing engine programming for YUV420 over DP
        https://gitlab.freedesktop.org/lumag/msm/-/commit/57d6ca4c1917
[18/19] drm/msm/dpu: reserve CDM blocks for DP if mode is YUV420
        https://gitlab.freedesktop.org/lumag/msm/-/commit/984809d85007
[19/19] drm/msm/dp: allow YUV420 mode for DP connector when CDM available
        https://gitlab.freedesktop.org/lumag/msm/-/commit/ee2fcc0f514b

Best regards,