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

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] and [2]:
[1] https://patchwork.freedesktop.org/series/118831/
[2] https://patchwork.freedesktop.org/series/129395/

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/dp: add an API to indicate if sink supports VSC SDP
  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/display/drm_dp_helper.c       |  21 +++
 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  |  35 +++-
 .../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           | 127 +++++++++++++-
 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               |   8 +-
 drivers/gpu/drm/msm/dp/dp_drm.h               |   3 +-
 drivers/gpu/drm/msm/dp/dp_panel.c             |  60 +++++++
 drivers/gpu/drm/msm/dp/dp_panel.h             |   2 +
 drivers/gpu/drm/msm/dp/dp_reg.h               |   5 +
 drivers/gpu/drm/msm/dp/dp_utils.c             | 151 ++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_utils.h             |  25 +++
 drivers/gpu/drm/msm/msm_drv.h                 |  22 ++-
 include/drm/display/drm_dp_helper.h           |   1 +
 25 files changed, 757 insertions(+), 242 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/dp/dp_utils.c
 create mode 100644 drivers/gpu/drm/msm/dp/dp_utils.h

On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
> YUV420 format is supported only in the VSC SDP packet and not through
> MSA. Hence add an API which indicates the sink support which can be used
> by the rest of the DP programming.
>
> Changes in v2:
>         - Move VSC SDP support check API from dp_panel.c to
>           drm_dp_helper.c
>
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 21 +++++++++++++++++++++
>  include/drm/display/drm_dp_helper.h     |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index d72b6f9a352c1..c6ee0f9ab5f8f 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2917,6 +2917,27 @@ void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
>  }
>  EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>
> +/**
> + * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
> + * @aux: DisplayPort AUX channel
> + * @dpcd: DisplayPort configuration data
> + *
> + * Returns true if vsc sdp is supported, else returns false
> + */
> +bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +       u8 rx_feature;
> +
> +       if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature) != 1) {
> +               drm_dbg_dp(aux->drm_dev, "failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
> +               return false;
> +       }
> +
> +       return (dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
> +               !!(rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);

Nit: we don't even need  the `!!` here. I'll probably drop it while applying.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> +}
> +EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
> +
>  /**
>   * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
>   * @dpcd: DisplayPort configuration data
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 863b2e7add29e..948381b2b0b1b 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -100,6 +100,7 @@ struct drm_dp_vsc_sdp {
>
>  void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
>                         const struct drm_dp_vsc_sdp *vsc);
> +bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
>
>  int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
>
> --
> 2.39.2
>

On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>
> Changes in v2:
>         - Minor formatting changes
>         - Move the modification of the dimesions for CDM setup to a new
>           patch
>
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>  .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 27 ++++++++++---------
>  2 files changed, 16 insertions(+), 15 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
>  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   |  59 ++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
>  drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
>  7 files changed, 260 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 5d84c089e520a..0f28a4897b7b7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -901,6 +901,111 @@ 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, u32 *buffer)
> +{
> +       struct dp_catalog_private *catalog;
> +
> +       if (!dp_catalog) {
> +               DRM_ERROR("invalid input\n");
> +               return;
> +       }
> +
> +       catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
> +
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, buffer[0]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, buffer[1]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_2, buffer[2]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_3, buffer[3]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_4, buffer[4]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_5, buffer[5]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_6, buffer[6]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_7, buffer[7]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_8, buffer[8]);
> +       dp_write_link(catalog, MMSS_DP_GENERIC0_9, buffer[9]);
> +}
> +
> +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, u32 *gen_buffer)
> +{
> +       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, gen_buffer);
> +
> +       /* 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..5b3a7ba40d55f 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"
> @@ -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, u32 *gen_buffer);
> +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..82897edf25e0f 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,60 @@ 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 dp_sdp_header sdp_header;
> +       struct drm_dp_vsc_sdp vsc_sdp_data;
> +       size_t buff_size;
> +       u32 gen_buffer[10];
> +       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;
> +       buff_size = sizeof(gen_buffer);
> +
> +       memset(&sdp_header, 0, sizeof(sdp_header));
> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
> +
> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
> +       sdp_header.HB0 = 0x00;
> +       sdp_header.HB1 = 0x07;
> +       sdp_header.HB2 = 0x05;
> +       sdp_header.HB3 = 0x13;

This should go to the packing function.

> +
> +       /* VSC SDP Payload for DB16 */

Comments are useless more or less. The code fills generic information
structure which might or might not be packed to the data buffer.

> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
> +
> +       /* VSC SDP Payload for DB17 */
> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
> +
> +       /* VSC SDP Payload for DB18 */
> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
> +
> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;

Consider extracting intel_dp_compute_vsc_colorimetry() and using it.

> +
> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header, gen_buffer, buff_size);
> +       if (rc) {
> +               DRM_ERROR("unable to pack vsc sdp\n");
> +               return rc;
> +       }
> +
> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
> +
> +       return rc;
> +}
> +
>  void dp_panel_dump_regs(struct dp_panel *dp_panel)
>  {
>         struct dp_catalog *catalog;
> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> @@ -4,6 +4,16 @@
>   */
>
>  #include <linux/types.h>
> +#include <drm/display/drm_dp_helper.h>
> +#include <drm/drm_print.h>
> +
> +#include "dp_utils.h"
> +
> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
> +
> +#define DP_SDP_HEADER_SIZE             8
> +#define DP_VSC_SDP_BUFFER_SIZE         40
>
>  u8 dp_utils_get_g0_value(u8 data)
>  {
> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>
>         return parity_byte;
>  }
> +
> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *buffer, size_t buff_size)
> +{
> +       u32 header, parity;
> +
> +       if (buff_size < DP_SDP_HEADER_SIZE)
> +               return -ENOSPC;
> +
> +       memset(buffer, 0, sizeof(buffer));
> +
> +       /* HEADER BYTE 0 */
> +       header = sdp_header->HB0;
> +       parity = dp_utils_calculate_parity(header);
> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity << PARITY_BYTE_0_BIT));
> +
> +       /* HEADER BYTE 1 */
> +       header = sdp_header->HB1;
> +       parity = dp_utils_calculate_parity(header);
> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity << PARITY_BYTE_1_BIT));
> +
> +       /* HEADER BYTE 2 */
> +       header = sdp_header->HB2;
> +       parity = dp_utils_calculate_parity(header);
> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity << PARITY_BYTE_2_BIT));
> +
> +       /* HEADER BYTE 3 */
> +       header = sdp_header->HB3;
> +       parity = dp_utils_calculate_parity(header);
> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity << PARITY_BYTE_3_BIT));
> +
> +       return 0;
> +}
> +
> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, struct dp_sdp_header *sdp_header,
> +                         u32 *buffer, size_t buff_size)
> +{

No. This function should pack data into struct dp_sdp and it should go
to drivers/video/hdmi.c

> +       u8 bpc;
> +       int ret = 0;
> +
> +       if (buff_size < DP_VSC_SDP_BUFFER_SIZE)
> +               return -ENOSPC;
> +
> +       ret = dp_utils_pack_sdp_header(sdp_header, buffer, buff_size);
> +       if (ret) {
> +               DRM_ERROR("unable to pack sdp header\n");
> +               return ret;
> +       }
> +
> +       switch (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 */
> +       buffer[6] = (vsc_sdp_data->colorimetry & 0xF) |
> +              ((vsc_sdp_data->pixelformat & 0xF) << 4) |
> +              (bpc << 8) |
> +              ((vsc_sdp_data->dynamic_range & 0x1) << 15) |
> +              ((vsc_sdp_data->content_type & 0x7) << 16);
> +
> +       print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", DUMP_PREFIX_NONE, 16, 4, buffer,
> +                      sizeof(buffer), false);
> +
> +       return ret;
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
> index c062e29d07898..7fd775de553c9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
> @@ -18,5 +18,8 @@
>  u8 dp_utils_get_g0_value(u8 data);
>  u8 dp_utils_get_g1_value(u8 data);
>  u8 dp_utils_calculate_parity(u32 data);
> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *buffer, size_t buff_size);
> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, struct dp_sdp_header *sdp_header,
> +                         u32 *buffer, size_t buff_size);
>
>  #endif /* _DP_UTILS_H_ */
> --
> 2.39.2
>

On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
> Adjust the encoder timing engine setup programming in the case of video
> mode for YUV420 over DP to accommodate CDM.
>
> Changes in v2:
>         - Move timing engine programming to this patch
>
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 3f102b2813ca8..fb46d907312a7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -235,8 +235,9 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>  {
>         struct drm_display_mode mode;
>         struct dpu_hw_intf_timing_params timing_params = { 0 };
> +       struct dpu_hw_cdm *hw_cdm;
>         const struct dpu_format *fmt = NULL;
> -       u32 fmt_fourcc = DRM_FORMAT_RGB888;
> +       u32 fmt_fourcc;
>         unsigned long lock_flags;
>         struct dpu_hw_intf_cfg intf_cfg = { 0 };
>
> @@ -255,17 +256,21 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>         DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n");
>         drm_mode_debug_printmodeline(&mode);
>
> -       if (phys_enc->split_role != ENC_ROLE_SOLO) {
> +       fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc);
> +
> +       if (phys_enc->split_role != ENC_ROLE_SOLO || fmt_fourcc == DRM_FORMAT_YUV420) {
>                 mode.hdisplay >>= 1;
>                 mode.htotal >>= 1;
>                 mode.hsync_start >>= 1;
>                 mode.hsync_end >>= 1;
> +               mode.hskew >>= 1;

hskew change seems to warrant a separate patch with Fixes for
25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

>
>                 DPU_DEBUG_VIDENC(phys_enc,
> -                       "split_role %d, halve horizontal %d %d %d %d\n",
> +                       "split_role %d, halve horizontal %d %d %d %d %d\n",
>                         phys_enc->split_role,
>                         mode.hdisplay, mode.htotal,
> -                       mode.hsync_start, mode.hsync_end);
> +                       mode.hsync_start, mode.hsync_end,
> +                       mode.hskew);
>         }
>
>         drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params);
> @@ -273,6 +278,9 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>         fmt = dpu_get_dpu_format(fmt_fourcc);
>         DPU_DEBUG_VIDENC(phys_enc, "fmt_fourcc 0x%X\n", fmt_fourcc);
>
> +       hw_cdm = phys_enc->hw_cdm;
> +       if (hw_cdm)
> +               intf_cfg.cdm = hw_cdm->idx;

No need for a separate local variable.

>         intf_cfg.intf = phys_enc->hw_intf->idx;
>         intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>         intf_cfg.stream_sel = 0; /* Don't care value for video mode */
> --
> 2.39.2
>

On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
> All the components of YUV420 over DP are added. Therefore, let's mark the
> connector property as true for DP connector when the DP type is not eDP
> and when there is a CDM block available.
>
> Changes in v2:
>         - Check for if dp_catalog has a CDM block available instead of
>           checking if VSC SDP is allowed when setting the dp connector's
>           ycbcr_420_allowed parameter
>
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
>  drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
>  drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
>  drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
>  drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
>  5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 723cc1d821431..beeaabe499abf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>  {
>         struct drm_encoder *encoder = NULL;
>         struct msm_display_info info;
> +       bool yuv_supported;
>         int rc;
>         int i;
>
> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>                         return PTR_ERR(encoder);
>                 }
>
> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
> +               yuv_supported = !!(dpu_kms->catalog->cdm);

Drop parentheses please.

> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
>                 if (rc) {
>                         DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>                         return rc;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ebcc76ef1d590..9b9f5f2921903 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>  }
>
>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> -                       struct drm_encoder *encoder)
> +                       struct drm_encoder *encoder, bool yuv_supported)
>  {
>         struct dp_display_private *dp_priv;
>         int ret;
> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>                 return ret;
>         }
>
> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
>         if (IS_ERR(dp_display->connector)) {
>                 ret = PTR_ERR(dp_display->connector);
>                 DRM_DEV_ERROR(dev->dev,
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 46e6889037e88..ab0d0d13b5e2c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
>  }
>
>  /* connector initialization */
> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
> +                                           bool yuv_supported)
>  {
>         struct drm_connector *connector = NULL;
>
> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
>         if (IS_ERR(connector))
>                 return connector;
>
> -       if (!dp_display->is_edp)
> +       if (!dp_display->is_edp) {
>                 drm_connector_attach_dp_subconnector_property(connector);
> +               if (yuv_supported)
> +                       connector->ycbcr_420_allowed = true;

Is there any reason to disallow YUV420 for eDP connectors?

> +       }
>
>         drm_connector_attach_encoder(connector, encoder);
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
> index b3d684db2383b..45e57ac25a4d9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.h
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.h
> @@ -19,7 +19,8 @@ struct msm_dp_bridge {
>
>  #define to_dp_bridge(x)     container_of((x), struct msm_dp_bridge, bridge)
>
> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder);
> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
> +                                           bool yuv_supported);
>  int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
>                         struct drm_encoder *encoder);
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index b876ebd48effe..37335777f5c09 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -385,7 +385,7 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_
>  int __init msm_dp_register(void);
>  void __exit msm_dp_unregister(void);
>  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> -                        struct drm_encoder *encoder);
> +                        struct drm_encoder *encoder, bool yuv_supported);
>  void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>  bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
>                                const struct drm_display_mode *mode);
> @@ -403,7 +403,8 @@ static inline void __exit msm_dp_unregister(void)
>  }
>  static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
>                                        struct drm_device *dev,
> -                                      struct drm_encoder *encoder)
> +                                      struct drm_encoder *encoder,
> +                                      bool yuv_supported)
>  {
>         return -EINVAL;
>  }
> --
> 2.39.2
>

On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
>>   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   |  59 ++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
>>   drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
>>   7 files changed, 260 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 5d84c089e520a..0f28a4897b7b7 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -901,6 +901,111 @@ 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, u32 *buffer)
>> +{
>> +       struct dp_catalog_private *catalog;
>> +
>> +       if (!dp_catalog) {
>> +               DRM_ERROR("invalid input\n");
>> +               return;
>> +       }
>> +
>> +       catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
>> +
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, buffer[0]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, buffer[1]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_2, buffer[2]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_3, buffer[3]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_4, buffer[4]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_5, buffer[5]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_6, buffer[6]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_7, buffer[7]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_8, buffer[8]);
>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_9, buffer[9]);
>> +}
>> +
>> +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, u32 *gen_buffer)
>> +{
>> +       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, gen_buffer);
>> +
>> +       /* 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..5b3a7ba40d55f 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"
>> @@ -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, u32 *gen_buffer);
>> +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..82897edf25e0f 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,60 @@ 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 dp_sdp_header sdp_header;
>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
>> +       size_t buff_size;
>> +       u32 gen_buffer[10];
>> +       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;
>> +       buff_size = sizeof(gen_buffer);
>> +
>> +       memset(&sdp_header, 0, sizeof(sdp_header));
>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
>> +
>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
>> +       sdp_header.HB0 = 0x00;
>> +       sdp_header.HB1 = 0x07;
>> +       sdp_header.HB2 = 0x05;
>> +       sdp_header.HB3 = 0x13;
> 
> This should go to the packing function.
> 

We can .... but ....

the header bytes can change based on the format. These values are 
specific to YUV420. Thats why this part was kept outside of the generic 
vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it 
but this was the reason.

>> +
>> +       /* VSC SDP Payload for DB16 */
> 
> Comments are useless more or less. The code fills generic information
> structure which might or might not be packed to the data buffer.
> 
>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
>> +
>> +       /* VSC SDP Payload for DB17 */
>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
>> +
>> +       /* VSC SDP Payload for DB18 */
>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
>> +
>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
> 
> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
> 

intel_dp_compute_vsc_colorimetry() uses colorspace property to pick 
YUV420, we do not. Right now, its a pure driver decision to use YUV420 
when the mode is supported only in that format.

Also, many params are to be used within this function cached inside 
intel_crtc_state . We will first need to make that API more generic to 
be re-usable by others.

I think overall, if we want to have a generic packing across vendors, it 
needs more work. I think one of us can take that up as a separate effort.

>> +
>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header, gen_buffer, buff_size);
>> +       if (rc) {
>> +               DRM_ERROR("unable to pack vsc sdp\n");
>> +               return rc;
>> +       }
>> +
>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
>> +
>> +       return rc;
>> +}
>> +
>>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>   {
>>          struct dp_catalog *catalog;
>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>> @@ -4,6 +4,16 @@
>>    */
>>
>>   #include <linux/types.h>
>> +#include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_print.h>
>> +
>> +#include "dp_utils.h"
>> +
>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
>> +
>> +#define DP_SDP_HEADER_SIZE             8
>> +#define DP_VSC_SDP_BUFFER_SIZE         40
>>
>>   u8 dp_utils_get_g0_value(u8 data)
>>   {
>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>>
>>          return parity_byte;
>>   }
>> +
>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *buffer, size_t buff_size)
>> +{
>> +       u32 header, parity;
>> +
>> +       if (buff_size < DP_SDP_HEADER_SIZE)
>> +               return -ENOSPC;
>> +
>> +       memset(buffer, 0, sizeof(buffer));
>> +
>> +       /* HEADER BYTE 0 */
>> +       header = sdp_header->HB0;
>> +       parity = dp_utils_calculate_parity(header);
>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity << PARITY_BYTE_0_BIT));
>> +
>> +       /* HEADER BYTE 1 */
>> +       header = sdp_header->HB1;
>> +       parity = dp_utils_calculate_parity(header);
>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity << PARITY_BYTE_1_BIT));
>> +
>> +       /* HEADER BYTE 2 */
>> +       header = sdp_header->HB2;
>> +       parity = dp_utils_calculate_parity(header);
>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity << PARITY_BYTE_2_BIT));
>> +
>> +       /* HEADER BYTE 3 */
>> +       header = sdp_header->HB3;
>> +       parity = dp_utils_calculate_parity(header);
>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity << PARITY_BYTE_3_BIT));
>> +
>> +       return 0;
>> +}
>> +
>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, struct dp_sdp_header *sdp_header,
>> +                         u32 *buffer, size_t buff_size)
>> +{
> 
> No. This function should pack data into struct dp_sdp and it should go
> to drivers/video/hdmi.c
> 

What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?

It seems like struct drm_dp_vsc_sdp is more appropriate for this.

Regarding hdmi.c, I think the packing needs to be more generic to move 
it there.

I am not seeing parity bytes packing with other vendors. So there will 
have to be some packing there and then some packing here.

Also, like you already noticed, to make the VSC SDP packing more generic 
to move to hdmi.c, it needs more work to make it more usable like 
supporting all pixel formats and all colorimetry values.

We do not have that much utility yet for that.

Thats why I wrote even in V1 that I will certainly do the migration to 
dp_utils for this one but cannot commit for hdmi.c in this series.

>> +       u8 bpc;
>> +       int ret = 0;
>> +
>> +       if (buff_size < DP_VSC_SDP_BUFFER_SIZE)
>> +               return -ENOSPC;
>> +
>> +       ret = dp_utils_pack_sdp_header(sdp_header, buffer, buff_size);
>> +       if (ret) {
>> +               DRM_ERROR("unable to pack sdp header\n");
>> +               return ret;
>> +       }
>> +
>> +       switch (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 */
>> +       buffer[6] = (vsc_sdp_data->colorimetry & 0xF) |
>> +              ((vsc_sdp_data->pixelformat & 0xF) << 4) |
>> +              (bpc << 8) |
>> +              ((vsc_sdp_data->dynamic_range & 0x1) << 15) |
>> +              ((vsc_sdp_data->content_type & 0x7) << 16);
>> +
>> +       print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", DUMP_PREFIX_NONE, 16, 4, buffer,
>> +                      sizeof(buffer), false);
>> +
>> +       return ret;
>> +}
>> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h b/drivers/gpu/drm/msm/dp/dp_utils.h
>> index c062e29d07898..7fd775de553c9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
>> @@ -18,5 +18,8 @@
>>   u8 dp_utils_get_g0_value(u8 data);
>>   u8 dp_utils_get_g1_value(u8 data);
>>   u8 dp_utils_calculate_parity(u32 data);
>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 *buffer, size_t buff_size);
>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, struct dp_sdp_header *sdp_header,
>> +                         u32 *buffer, size_t buff_size);
>>
>>   #endif /* _DP_UTILS_H_ */
>> --
>> 2.39.2
>>
> 
>

On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
> 
> 
> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
>> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
>>>   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   |  59 ++++++++++++++++
>>>   drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
>>>   drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
>>>   drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
>>>   7 files changed, 260 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> index 5d84c089e520a..0f28a4897b7b7 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> @@ -901,6 +901,111 @@ 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, u32 *buffer)
>>> +{
>>> +       struct dp_catalog_private *catalog;
>>> +
>>> +       if (!dp_catalog) {
>>> +               DRM_ERROR("invalid input\n");
>>> +               return;
>>> +       }
>>> +
>>> +       catalog = container_of(dp_catalog, struct dp_catalog_private, 
>>> dp_catalog);
>>> +
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, buffer[0]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, buffer[1]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_2, buffer[2]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_3, buffer[3]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_4, buffer[4]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_5, buffer[5]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_6, buffer[6]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_7, buffer[7]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_8, buffer[8]);
>>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_9, buffer[9]);
>>> +}
>>> +
>>> +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, 
>>> u32 *gen_buffer)
>>> +{
>>> +       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, gen_buffer);
>>> +
>>> +       /* 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..5b3a7ba40d55f 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"
>>> @@ -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, 
>>> u32 *gen_buffer);
>>> +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..82897edf25e0f 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,60 @@ 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 dp_sdp_header sdp_header;
>>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
>>> +       size_t buff_size;
>>> +       u32 gen_buffer[10];
>>> +       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;
>>> +       buff_size = sizeof(gen_buffer);
>>> +
>>> +       memset(&sdp_header, 0, sizeof(sdp_header));
>>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
>>> +
>>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
>>> +       sdp_header.HB0 = 0x00;
>>> +       sdp_header.HB1 = 0x07;
>>> +       sdp_header.HB2 = 0x05;
>>> +       sdp_header.HB3 = 0x13;
>>
>> This should go to the packing function.
>>
> 
> We can .... but ....
> 
> the header bytes can change based on the format. These values are 
> specific to YUV420. Thats why this part was kept outside of the generic 
> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it 
> but this was the reason.
> 
>>> +
>>> +       /* VSC SDP Payload for DB16 */
>>
>> Comments are useless more or less. The code fills generic information
>> structure which might or might not be packed to the data buffer.
>>
>>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
>>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
>>> +
>>> +       /* VSC SDP Payload for DB17 */
>>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
>>> +
>>> +       /* VSC SDP Payload for DB18 */
>>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
>>> +
>>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
>>
>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
>>
> 
> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick 
> YUV420, we do not. Right now, its a pure driver decision to use YUV420 
> when the mode is supported only in that format.
> 
> Also, many params are to be used within this function cached inside 
> intel_crtc_state . We will first need to make that API more generic to 
> be re-usable by others.
> 
> I think overall, if we want to have a generic packing across vendors, it 
> needs more work. I think one of us can take that up as a separate effort.
> 
>>> +
>>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header, 
>>> gen_buffer, buff_size);
>>> +       if (rc) {
>>> +               DRM_ERROR("unable to pack vsc sdp\n");
>>> +               return rc;
>>> +       }
>>> +
>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
>>> +
>>> +       return rc;
>>> +}
>>> +
>>>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>   {
>>>          struct dp_catalog *catalog;
>>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>>> @@ -4,6 +4,16 @@
>>>    */
>>>
>>>   #include <linux/types.h>
>>> +#include <drm/display/drm_dp_helper.h>
>>> +#include <drm/drm_print.h>
>>> +
>>> +#include "dp_utils.h"
>>> +
>>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
>>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
>>> +
>>> +#define DP_SDP_HEADER_SIZE             8
>>> +#define DP_VSC_SDP_BUFFER_SIZE         40
>>>
>>>   u8 dp_utils_get_g0_value(u8 data)
>>>   {
>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>>>
>>>          return parity_byte;
>>>   }
>>> +
>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 
>>> *buffer, size_t buff_size)
>>> +{
>>> +       u32 header, parity;
>>> +
>>> +       if (buff_size < DP_SDP_HEADER_SIZE)
>>> +               return -ENOSPC;
>>> +
>>> +       memset(buffer, 0, sizeof(buffer));
>>> +
>>> +       /* HEADER BYTE 0 */
>>> +       header = sdp_header->HB0;
>>> +       parity = dp_utils_calculate_parity(header);
>>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity << 
>>> PARITY_BYTE_0_BIT));
>>> +
>>> +       /* HEADER BYTE 1 */
>>> +       header = sdp_header->HB1;
>>> +       parity = dp_utils_calculate_parity(header);
>>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity << 
>>> PARITY_BYTE_1_BIT));
>>> +
>>> +       /* HEADER BYTE 2 */
>>> +       header = sdp_header->HB2;
>>> +       parity = dp_utils_calculate_parity(header);
>>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity << 
>>> PARITY_BYTE_2_BIT));
>>> +
>>> +       /* HEADER BYTE 3 */
>>> +       header = sdp_header->HB3;
>>> +       parity = dp_utils_calculate_parity(header);
>>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity << 
>>> PARITY_BYTE_3_BIT));
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, 
>>> struct dp_sdp_header *sdp_header,
>>> +                         u32 *buffer, size_t buff_size)
>>> +{
>>
>> No. This function should pack data into struct dp_sdp and it should go
>> to drivers/video/hdmi.c
>>
> 
> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
> 

I think you were referring to the fact that instead of a custom buffer, 
we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.

If yes, I agree with this part.

> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
> 
> Regarding hdmi.c, I think the packing needs to be more generic to move 
> it there.
> 
> I am not seeing parity bytes packing with other vendors. So there will 
> have to be some packing there and then some packing here.
> 
> Also, like you already noticed, to make the VSC SDP packing more generic 
> to move to hdmi.c, it needs more work to make it more usable like 
> supporting all pixel formats and all colorimetry values.
> 
> We do not have that much utility yet for that.
> 
> Thats why I wrote even in V1 that I will certainly do the migration to 
> dp_utils for this one but cannot commit for hdmi.c in this series.
> 
>>> +       u8 bpc;
>>> +       int ret = 0;
>>> +
>>> +       if (buff_size < DP_VSC_SDP_BUFFER_SIZE)
>>> +               return -ENOSPC;
>>> +
>>> +       ret = dp_utils_pack_sdp_header(sdp_header, buffer, buff_size);
>>> +       if (ret) {
>>> +               DRM_ERROR("unable to pack sdp header\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       switch (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 */
>>> +       buffer[6] = (vsc_sdp_data->colorimetry & 0xF) |
>>> +              ((vsc_sdp_data->pixelformat & 0xF) << 4) |
>>> +              (bpc << 8) |
>>> +              ((vsc_sdp_data->dynamic_range & 0x1) << 15) |
>>> +              ((vsc_sdp_data->content_type & 0x7) << 16);
>>> +
>>> +       print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ", 
>>> DUMP_PREFIX_NONE, 16, 4, buffer,
>>> +                      sizeof(buffer), false);
>>> +
>>> +       return ret;
>>> +}
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h 
>>> b/drivers/gpu/drm/msm/dp/dp_utils.h
>>> index c062e29d07898..7fd775de553c9 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
>>> @@ -18,5 +18,8 @@
>>>   u8 dp_utils_get_g0_value(u8 data);
>>>   u8 dp_utils_get_g1_value(u8 data);
>>>   u8 dp_utils_calculate_parity(u32 data);
>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32 
>>> *buffer, size_t buff_size);
>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data, 
>>> struct dp_sdp_header *sdp_header,
>>> +                         u32 *buffer, size_t buff_size);
>>>
>>>   #endif /* _DP_UTILS_H_ */
>>> -- 
>>> 2.39.2
>>>
>>
>>

On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>
>> All the components of YUV420 over DP are added. Therefore, let's mark the
>> connector property as true for DP connector when the DP type is not eDP
>> and when there is a CDM block available.
>>
>> Changes in v2:
>>          - Check for if dp_catalog has a CDM block available instead of
>>            checking if VSC SDP is allowed when setting the dp connector's
>>            ycbcr_420_allowed parameter
>>
>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
>>   drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
>>   drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
>>   drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
>>   drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
>>   5 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 723cc1d821431..beeaabe499abf 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>   {
>>          struct drm_encoder *encoder = NULL;
>>          struct msm_display_info info;
>> +       bool yuv_supported;
>>          int rc;
>>          int i;
>>
>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>                          return PTR_ERR(encoder);
>>                  }
>>
>> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>> +               yuv_supported = !!(dpu_kms->catalog->cdm);
> 
> Drop parentheses please.
> 
>> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
>>                  if (rc) {
>>                          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>                          return rc;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index ebcc76ef1d590..9b9f5f2921903 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>>   }
>>
>>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>> -                       struct drm_encoder *encoder)
>> +                       struct drm_encoder *encoder, bool yuv_supported)
>>   {
>>          struct dp_display_private *dp_priv;
>>          int ret;
>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>                  return ret;
>>          }
>>
>> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
>> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
>>          if (IS_ERR(dp_display->connector)) {
>>                  ret = PTR_ERR(dp_display->connector);
>>                  DRM_DEV_ERROR(dev->dev,
>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>> index 46e6889037e88..ab0d0d13b5e2c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
>>   }
>>
>>   /* connector initialization */
>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
>> +                                           bool yuv_supported)
>>   {
>>          struct drm_connector *connector = NULL;
>>
>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
>>          if (IS_ERR(connector))
>>                  return connector;
>>
>> -       if (!dp_display->is_edp)
>> +       if (!dp_display->is_edp) {
>>                  drm_connector_attach_dp_subconnector_property(connector);
>> +               if (yuv_supported)
>> +                       connector->ycbcr_420_allowed = true;
> 
> Is there any reason to disallow YUV420 for eDP connectors?
> 
>> +       }
>>

Major reason was certainly validation but thinking about it more 
closely, I need to check whether CDM over eDP is even possible.

Historically, CDM could output only to HDMI OR WB using the bit we 
program while setting up CDM and the same HDMI path is being used by DP 
as well. But I need to check whether CDM can even output to eDP with the 
same DP path. I dont have any documentation on this part yet.

On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote:
> > On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>
> >> All the components of YUV420 over DP are added. Therefore, let's mark the
> >> connector property as true for DP connector when the DP type is not eDP
> >> and when there is a CDM block available.
> >>
> >> Changes in v2:
> >>          - Check for if dp_catalog has a CDM block available instead of
> >>            checking if VSC SDP is allowed when setting the dp connector's
> >>            ycbcr_420_allowed parameter
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
> >>   drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
> >>   drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
> >>   drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
> >>   drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
> >>   5 files changed, 16 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> index 723cc1d821431..beeaabe499abf 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
> >>   {
> >>          struct drm_encoder *encoder = NULL;
> >>          struct msm_display_info info;
> >> +       bool yuv_supported;
> >>          int rc;
> >>          int i;
> >>
> >> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
> >>                          return PTR_ERR(encoder);
> >>                  }
> >>
> >> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
> >> +               yuv_supported = !!(dpu_kms->catalog->cdm);
> >
> > Drop parentheses please.
> >
> >> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
> >>                  if (rc) {
> >>                          DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> >>                          return rc;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index ebcc76ef1d590..9b9f5f2921903 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
> >>   }
> >>
> >>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> >> -                       struct drm_encoder *encoder)
> >> +                       struct drm_encoder *encoder, bool yuv_supported)
> >>   {
> >>          struct dp_display_private *dp_priv;
> >>          int ret;
> >> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> >>                  return ret;
> >>          }
> >>
> >> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
> >> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
> >>          if (IS_ERR(dp_display->connector)) {
> >>                  ret = PTR_ERR(dp_display->connector);
> >>                  DRM_DEV_ERROR(dev->dev,
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> >> index 46e6889037e88..ab0d0d13b5e2c 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> >> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
> >>   }
> >>
> >>   /* connector initialization */
> >> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
> >> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
> >> +                                           bool yuv_supported)
> >>   {
> >>          struct drm_connector *connector = NULL;
> >>
> >> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
> >>          if (IS_ERR(connector))
> >>                  return connector;
> >>
> >> -       if (!dp_display->is_edp)
> >> +       if (!dp_display->is_edp) {
> >>                  drm_connector_attach_dp_subconnector_property(connector);
> >> +               if (yuv_supported)
> >> +                       connector->ycbcr_420_allowed = true;
> >
> > Is there any reason to disallow YUV420 for eDP connectors?
> >
> >> +       }
> >>
>
> Major reason was certainly validation but thinking about it more
> closely, I need to check whether CDM over eDP is even possible.
>
> Historically, CDM could output only to HDMI OR WB using the bit we
> program while setting up CDM and the same HDMI path is being used by DP
> as well. But I need to check whether CDM can even output to eDP with the
> same DP path. I dont have any documentation on this part yet.

I had the feeling that the DP / eDP difference on the chips is mostly
on the PHY and software side. So I assumed that it should be possible
to output YUV data to the eDP port in the same way as it is done for
the DP port.

On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
> >
> >
> > On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
> >> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
> >>>   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   |  59 ++++++++++++++++
> >>>   drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
> >>>   drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
> >>>   drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
> >>>   7 files changed, 260 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> index 5d84c089e520a..0f28a4897b7b7 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> @@ -901,6 +901,111 @@ 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, u32 *buffer)
> >>> +{
> >>> +       struct dp_catalog_private *catalog;
> >>> +
> >>> +       if (!dp_catalog) {
> >>> +               DRM_ERROR("invalid input\n");
> >>> +               return;
> >>> +       }
> >>> +
> >>> +       catalog = container_of(dp_catalog, struct dp_catalog_private,
> >>> dp_catalog);
> >>> +
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_0, buffer[0]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_1, buffer[1]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_2, buffer[2]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_3, buffer[3]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_4, buffer[4]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_5, buffer[5]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_6, buffer[6]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_7, buffer[7]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_8, buffer[8]);
> >>> +       dp_write_link(catalog, MMSS_DP_GENERIC0_9, buffer[9]);
> >>> +}
> >>> +
> >>> +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,
> >>> u32 *gen_buffer)
> >>> +{
> >>> +       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, gen_buffer);
> >>> +
> >>> +       /* 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..5b3a7ba40d55f 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"
> >>> @@ -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,
> >>> u32 *gen_buffer);
> >>> +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..82897edf25e0f 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,60 @@ 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 dp_sdp_header sdp_header;
> >>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
> >>> +       size_t buff_size;
> >>> +       u32 gen_buffer[10];
> >>> +       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;
> >>> +       buff_size = sizeof(gen_buffer);
> >>> +
> >>> +       memset(&sdp_header, 0, sizeof(sdp_header));
> >>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
> >>> +
> >>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
> >>> +       sdp_header.HB0 = 0x00;
> >>> +       sdp_header.HB1 = 0x07;
> >>> +       sdp_header.HB2 = 0x05;
> >>> +       sdp_header.HB3 = 0x13;
> >>
> >> This should go to the packing function.
> >>
> >
> > We can .... but ....
> >
> > the header bytes can change based on the format. These values are
> > specific to YUV420. Thats why this part was kept outside of the generic
> > vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
> > but this was the reason.

These values can be set from the sdp_type, revision and length fields
of struct drm_dp_vsc_sdp.
The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.

> >
> >>> +
> >>> +       /* VSC SDP Payload for DB16 */
> >>
> >> Comments are useless more or less. The code fills generic information
> >> structure which might or might not be packed to the data buffer.
> >>
> >>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
> >>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
> >>> +
> >>> +       /* VSC SDP Payload for DB17 */
> >>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
> >>> +
> >>> +       /* VSC SDP Payload for DB18 */
> >>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
> >>> +
> >>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
> >>
> >> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
> >>
> >
> > intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
> > YUV420, we do not.

Intel function also uses output_format, but it's true, it is full of
Intel specifics.

> > Right now, its a pure driver decision to use YUV420
> > when the mode is supported only in that format.
> >
> > Also, many params are to be used within this function cached inside
> > intel_crtc_state . We will first need to make that API more generic to
> > be re-usable by others.
> >
> > I think overall, if we want to have a generic packing across vendors, it
> > needs more work. I think one of us can take that up as a separate effort.

Ack, I agree here. I did only a quick glance over
intel_dp_compute_vsc_colorimetry function() beforehand.

> >
> >>> +
> >>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
> >>> gen_buffer, buff_size);
> >>> +       if (rc) {
> >>> +               DRM_ERROR("unable to pack vsc sdp\n");
> >>> +               return rc;
> >>> +       }
> >>> +
> >>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
> >>> +
> >>> +       return rc;
> >>> +}
> >>> +
> >>>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>>   {
> >>>          struct dp_catalog *catalog;
> >>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> >>> @@ -4,6 +4,16 @@
> >>>    */
> >>>
> >>>   #include <linux/types.h>
> >>> +#include <drm/display/drm_dp_helper.h>
> >>> +#include <drm/drm_print.h>
> >>> +
> >>> +#include "dp_utils.h"
> >>> +
> >>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
> >>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
> >>> +
> >>> +#define DP_SDP_HEADER_SIZE             8
> >>> +#define DP_VSC_SDP_BUFFER_SIZE         40
> >>>
> >>>   u8 dp_utils_get_g0_value(u8 data)
> >>>   {
> >>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
> >>>
> >>>          return parity_byte;
> >>>   }
> >>> +
> >>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
> >>> *buffer, size_t buff_size)
> >>> +{
> >>> +       u32 header, parity;
> >>> +
> >>> +       if (buff_size < DP_SDP_HEADER_SIZE)
> >>> +               return -ENOSPC;
> >>> +
> >>> +       memset(buffer, 0, sizeof(buffer));
> >>> +
> >>> +       /* HEADER BYTE 0 */
> >>> +       header = sdp_header->HB0;
> >>> +       parity = dp_utils_calculate_parity(header);
> >>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity <<
> >>> PARITY_BYTE_0_BIT));
> >>> +
> >>> +       /* HEADER BYTE 1 */
> >>> +       header = sdp_header->HB1;
> >>> +       parity = dp_utils_calculate_parity(header);
> >>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity <<
> >>> PARITY_BYTE_1_BIT));
> >>> +
> >>> +       /* HEADER BYTE 2 */
> >>> +       header = sdp_header->HB2;
> >>> +       parity = dp_utils_calculate_parity(header);
> >>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity <<
> >>> PARITY_BYTE_2_BIT));
> >>> +
> >>> +       /* HEADER BYTE 3 */
> >>> +       header = sdp_header->HB3;
> >>> +       parity = dp_utils_calculate_parity(header);
> >>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity <<
> >>> PARITY_BYTE_3_BIT));
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
> >>> struct dp_sdp_header *sdp_header,
> >>> +                         u32 *buffer, size_t buff_size)
> >>> +{
> >>
> >> No. This function should pack data into struct dp_sdp and it should go
> >> to drivers/video/hdmi.c
> >>
> >
> > What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
> >
>
> I think you were referring to the fact that instead of a custom buffer,
> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
>
> If yes, I agree with this part.

Yes.

>
> > It seems like struct drm_dp_vsc_sdp is more appropriate for this.
> >
> > Regarding hdmi.c, I think the packing needs to be more generic to move
> > it there.
> >
> > I am not seeing parity bytes packing with other vendors. So there will
> > have to be some packing there and then some packing here.

Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
Intel and AMD seem to write header bytes without parity.

> > Also, like you already noticed, to make the VSC SDP packing more generic
> > to move to hdmi.c, it needs more work to make it more usable like
> > supporting all pixel formats and all colorimetry values.
> >
> > We do not have that much utility yet for that.

I think you are mixing the filling of drm_dp_vsc_sdp and packing of
that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
or less as is.
Once somebody needs to support 3D (AMD does), they can extend the function.

> > Thats why I wrote even in V1 that I will certainly do the migration to
> > dp_utils for this one but cannot commit for hdmi.c in this series.
> >
> >>> +       u8 bpc;
> >>> +       int ret = 0;
> >>> +
> >>> +       if (buff_size < DP_VSC_SDP_BUFFER_SIZE)
> >>> +               return -ENOSPC;
> >>> +
> >>> +       ret = dp_utils_pack_sdp_header(sdp_header, buffer, buff_size);
> >>> +       if (ret) {
> >>> +               DRM_ERROR("unable to pack sdp header\n");
> >>> +               return ret;
> >>> +       }
> >>> +
> >>> +       switch (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 */
> >>> +       buffer[6] = (vsc_sdp_data->colorimetry & 0xF) |
> >>> +              ((vsc_sdp_data->pixelformat & 0xF) << 4) |
> >>> +              (bpc << 8) |
> >>> +              ((vsc_sdp_data->dynamic_range & 0x1) << 15) |
> >>> +              ((vsc_sdp_data->content_type & 0x7) << 16);
> >>> +
> >>> +       print_hex_dump(KERN_DEBUG, "[drm-dp] VSC: ",
> >>> DUMP_PREFIX_NONE, 16, 4, buffer,
> >>> +                      sizeof(buffer), false);
> >>> +
> >>> +       return ret;
> >>> +}
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.h
> >>> b/drivers/gpu/drm/msm/dp/dp_utils.h
> >>> index c062e29d07898..7fd775de553c9 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_utils.h
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.h
> >>> @@ -18,5 +18,8 @@
> >>>   u8 dp_utils_get_g0_value(u8 data);
> >>>   u8 dp_utils_get_g1_value(u8 data);
> >>>   u8 dp_utils_calculate_parity(u32 data);
> >>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
> >>> *buffer, size_t buff_size);
> >>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
> >>> struct dp_sdp_header *sdp_header,
> >>> +                         u32 *buffer, size_t buff_size);
> >>>
> >>>   #endif /* _DP_UTILS_H_ */
> >>> --
> >>> 2.39.2
> >>>
> >>
> >>

On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
>>>> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
>>>>>    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   |  59 ++++++++++++++++
>>>>>    drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
>>>>>    drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
>>>>>    drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
>>>>>    7 files changed, 260 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> index 5d84c089e520a..0f28a4897b7b7 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>> @@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
>>>>> dp_catalog *dp_catalog)
>>>>>           return 0;
>>>>>    }
>>>>>

<Snip>

>>>>> +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 dp_sdp_header sdp_header;
>>>>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
>>>>> +       size_t buff_size;
>>>>> +       u32 gen_buffer[10];
>>>>> +       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;
>>>>> +       buff_size = sizeof(gen_buffer);
>>>>> +
>>>>> +       memset(&sdp_header, 0, sizeof(sdp_header));
>>>>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
>>>>> +
>>>>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
>>>>> +       sdp_header.HB0 = 0x00;
>>>>> +       sdp_header.HB1 = 0x07;
>>>>> +       sdp_header.HB2 = 0x05;
>>>>> +       sdp_header.HB3 = 0x13;
>>>>
>>>> This should go to the packing function.
>>>>
>>>
>>> We can .... but ....
>>>
>>> the header bytes can change based on the format. These values are
>>> specific to YUV420. Thats why this part was kept outside of the generic
>>> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
>>> but this was the reason.
> 
> These values can be set from the sdp_type, revision and length fields
> of struct drm_dp_vsc_sdp.
> The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
> 
>>>
>>>>> +
>>>>> +       /* VSC SDP Payload for DB16 */
>>>>
>>>> Comments are useless more or less. The code fills generic information
>>>> structure which might or might not be packed to the data buffer.
>>>>
>>>>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
>>>>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
>>>>> +
>>>>> +       /* VSC SDP Payload for DB17 */
>>>>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
>>>>> +
>>>>> +       /* VSC SDP Payload for DB18 */
>>>>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
>>>>> +
>>>>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
>>>>
>>>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
>>>>
>>>
>>> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
>>> YUV420, we do not.
> 
> Intel function also uses output_format, but it's true, it is full of
> Intel specifics.
> 
>>> Right now, its a pure driver decision to use YUV420
>>> when the mode is supported only in that format.
>>>
>>> Also, many params are to be used within this function cached inside
>>> intel_crtc_state . We will first need to make that API more generic to
>>> be re-usable by others.
>>>
>>> I think overall, if we want to have a generic packing across vendors, it
>>> needs more work. I think one of us can take that up as a separate effort.
> 
> Ack, I agree here. I did only a quick glance over
> intel_dp_compute_vsc_colorimetry function() beforehand.
> 
>>>
>>>>> +
>>>>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
>>>>> gen_buffer, buff_size);
>>>>> +       if (rc) {
>>>>> +               DRM_ERROR("unable to pack vsc sdp\n");
>>>>> +               return rc;
>>>>> +       }
>>>>> +
>>>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
>>>>> +
>>>>> +       return rc;
>>>>> +}
>>>>> +
>>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>>    {
>>>>>           struct dp_catalog *catalog;
>>>>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>> @@ -4,6 +4,16 @@
>>>>>     */
>>>>>
>>>>>    #include <linux/types.h>
>>>>> +#include <drm/display/drm_dp_helper.h>
>>>>> +#include <drm/drm_print.h>
>>>>> +
>>>>> +#include "dp_utils.h"
>>>>> +
>>>>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
>>>>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
>>>>> +
>>>>> +#define DP_SDP_HEADER_SIZE             8
>>>>> +#define DP_VSC_SDP_BUFFER_SIZE         40
>>>>>
>>>>>    u8 dp_utils_get_g0_value(u8 data)
>>>>>    {
>>>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>>>>>
>>>>>           return parity_byte;
>>>>>    }
>>>>> +
>>>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
>>>>> *buffer, size_t buff_size)
>>>>> +{
>>>>> +       u32 header, parity;
>>>>> +
>>>>> +       if (buff_size < DP_SDP_HEADER_SIZE)
>>>>> +               return -ENOSPC;
>>>>> +
>>>>> +       memset(buffer, 0, sizeof(buffer));
>>>>> +
>>>>> +       /* HEADER BYTE 0 */
>>>>> +       header = sdp_header->HB0;
>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity <<
>>>>> PARITY_BYTE_0_BIT));
>>>>> +
>>>>> +       /* HEADER BYTE 1 */
>>>>> +       header = sdp_header->HB1;
>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity <<
>>>>> PARITY_BYTE_1_BIT));
>>>>> +
>>>>> +       /* HEADER BYTE 2 */
>>>>> +       header = sdp_header->HB2;
>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity <<
>>>>> PARITY_BYTE_2_BIT));
>>>>> +
>>>>> +       /* HEADER BYTE 3 */
>>>>> +       header = sdp_header->HB3;
>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity <<
>>>>> PARITY_BYTE_3_BIT));
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
>>>>> struct dp_sdp_header *sdp_header,
>>>>> +                         u32 *buffer, size_t buff_size)
>>>>> +{
>>>>
>>>> No. This function should pack data into struct dp_sdp and it should go
>>>> to drivers/video/hdmi.c
>>>>
>>>
>>> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
>>>
>>
>> I think you were referring to the fact that instead of a custom buffer,
>> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
>>
>> If yes, I agree with this part.
> 
> Yes.
> 
>>
>>> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
>>>
>>> Regarding hdmi.c, I think the packing needs to be more generic to move
>>> it there.
>>>
>>> I am not seeing parity bytes packing with other vendors. So there will
>>> have to be some packing there and then some packing here.
> 
> Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
> Intel and AMD seem to write header bytes without parity.
> 
>>> Also, like you already noticed, to make the VSC SDP packing more generic
>>> to move to hdmi.c, it needs more work to make it more usable like
>>> supporting all pixel formats and all colorimetry values.
>>>
>>> We do not have that much utility yet for that.
> 
> I think you are mixing the filling of drm_dp_vsc_sdp and packing of
> that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
> or less as is.
> Once somebody needs to support 3D (AMD does), they can extend the function.
> 

Yes once I corrected my understanding of using the function to just pack 
struct dp_sdp instead of the buffer, I understood the intention.

We can try it. I have written up a RFC to move the 
intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate 
now for it and not hdmi.c as we already have some vsc sdp utils in 
drm_dp_helper.

But before I post, we will do some cleanup and see if things look 
reasonable in this patch too because we will still need to write a 
common utility to pack the parity bytes for the sdp header which we need 
to utilize for audio and vsc sdp in our DP case.

I am thinking of a

struct msm_dp_sdp_pb {
	u8 PB0;
	u8 PB1;
  	u8 PB2;
  	u8 PB3;
} __packed;

struct msm_dp_sdp_with_parity {
	struct dp_sdp vsc_sdp;
	struct msm_dp_sdp_pb pb;
};

intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to 
do some additional parity byte packing after that. That part will remain 
common for audio and vsc for msm.

On Sun, 11 Feb 2024 at 06:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
> > On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
> >>>
> >>>
> >>> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
> >>>> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
> >>>>>    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   |  59 ++++++++++++++++
> >>>>>    drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
> >>>>>    drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
> >>>>>    drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
> >>>>>    7 files changed, 260 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>> index 5d84c089e520a..0f28a4897b7b7 100644
> >>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>> @@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
> >>>>> dp_catalog *dp_catalog)
> >>>>>           return 0;
> >>>>>    }
> >>>>>
>
> <Snip>
>
> >>>>> +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 dp_sdp_header sdp_header;
> >>>>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
> >>>>> +       size_t buff_size;
> >>>>> +       u32 gen_buffer[10];
> >>>>> +       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;
> >>>>> +       buff_size = sizeof(gen_buffer);
> >>>>> +
> >>>>> +       memset(&sdp_header, 0, sizeof(sdp_header));
> >>>>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
> >>>>> +
> >>>>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
> >>>>> +       sdp_header.HB0 = 0x00;
> >>>>> +       sdp_header.HB1 = 0x07;
> >>>>> +       sdp_header.HB2 = 0x05;
> >>>>> +       sdp_header.HB3 = 0x13;
> >>>>
> >>>> This should go to the packing function.
> >>>>
> >>>
> >>> We can .... but ....
> >>>
> >>> the header bytes can change based on the format. These values are
> >>> specific to YUV420. Thats why this part was kept outside of the generic
> >>> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
> >>> but this was the reason.
> >
> > These values can be set from the sdp_type, revision and length fields
> > of struct drm_dp_vsc_sdp.
> > The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
> >
> >>>
> >>>>> +
> >>>>> +       /* VSC SDP Payload for DB16 */
> >>>>
> >>>> Comments are useless more or less. The code fills generic information
> >>>> structure which might or might not be packed to the data buffer.
> >>>>
> >>>>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
> >>>>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
> >>>>> +
> >>>>> +       /* VSC SDP Payload for DB17 */
> >>>>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
> >>>>> +
> >>>>> +       /* VSC SDP Payload for DB18 */
> >>>>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
> >>>>> +
> >>>>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
> >>>>
> >>>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
> >>>>
> >>>
> >>> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
> >>> YUV420, we do not.
> >
> > Intel function also uses output_format, but it's true, it is full of
> > Intel specifics.
> >
> >>> Right now, its a pure driver decision to use YUV420
> >>> when the mode is supported only in that format.
> >>>
> >>> Also, many params are to be used within this function cached inside
> >>> intel_crtc_state . We will first need to make that API more generic to
> >>> be re-usable by others.
> >>>
> >>> I think overall, if we want to have a generic packing across vendors, it
> >>> needs more work. I think one of us can take that up as a separate effort.
> >
> > Ack, I agree here. I did only a quick glance over
> > intel_dp_compute_vsc_colorimetry function() beforehand.
> >
> >>>
> >>>>> +
> >>>>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
> >>>>> gen_buffer, buff_size);
> >>>>> +       if (rc) {
> >>>>> +               DRM_ERROR("unable to pack vsc sdp\n");
> >>>>> +               return rc;
> >>>>> +       }
> >>>>> +
> >>>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
> >>>>> +
> >>>>> +       return rc;
> >>>>> +}
> >>>>> +
> >>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>>>>    {
> >>>>>           struct dp_catalog *catalog;
> >>>>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
> >>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> >>>>> @@ -4,6 +4,16 @@
> >>>>>     */
> >>>>>
> >>>>>    #include <linux/types.h>
> >>>>> +#include <drm/display/drm_dp_helper.h>
> >>>>> +#include <drm/drm_print.h>
> >>>>> +
> >>>>> +#include "dp_utils.h"
> >>>>> +
> >>>>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
> >>>>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
> >>>>> +
> >>>>> +#define DP_SDP_HEADER_SIZE             8
> >>>>> +#define DP_VSC_SDP_BUFFER_SIZE         40
> >>>>>
> >>>>>    u8 dp_utils_get_g0_value(u8 data)
> >>>>>    {
> >>>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
> >>>>>
> >>>>>           return parity_byte;
> >>>>>    }
> >>>>> +
> >>>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
> >>>>> *buffer, size_t buff_size)
> >>>>> +{
> >>>>> +       u32 header, parity;
> >>>>> +
> >>>>> +       if (buff_size < DP_SDP_HEADER_SIZE)
> >>>>> +               return -ENOSPC;
> >>>>> +
> >>>>> +       memset(buffer, 0, sizeof(buffer));
> >>>>> +
> >>>>> +       /* HEADER BYTE 0 */
> >>>>> +       header = sdp_header->HB0;
> >>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity <<
> >>>>> PARITY_BYTE_0_BIT));
> >>>>> +
> >>>>> +       /* HEADER BYTE 1 */
> >>>>> +       header = sdp_header->HB1;
> >>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity <<
> >>>>> PARITY_BYTE_1_BIT));
> >>>>> +
> >>>>> +       /* HEADER BYTE 2 */
> >>>>> +       header = sdp_header->HB2;
> >>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity <<
> >>>>> PARITY_BYTE_2_BIT));
> >>>>> +
> >>>>> +       /* HEADER BYTE 3 */
> >>>>> +       header = sdp_header->HB3;
> >>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity <<
> >>>>> PARITY_BYTE_3_BIT));
> >>>>> +
> >>>>> +       return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
> >>>>> struct dp_sdp_header *sdp_header,
> >>>>> +                         u32 *buffer, size_t buff_size)
> >>>>> +{
> >>>>
> >>>> No. This function should pack data into struct dp_sdp and it should go
> >>>> to drivers/video/hdmi.c
> >>>>
> >>>
> >>> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
> >>>
> >>
> >> I think you were referring to the fact that instead of a custom buffer,
> >> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
> >>
> >> If yes, I agree with this part.
> >
> > Yes.
> >
> >>
> >>> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
> >>>
> >>> Regarding hdmi.c, I think the packing needs to be more generic to move
> >>> it there.
> >>>
> >>> I am not seeing parity bytes packing with other vendors. So there will
> >>> have to be some packing there and then some packing here.
> >
> > Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
> > Intel and AMD seem to write header bytes without parity.
> >
> >>> Also, like you already noticed, to make the VSC SDP packing more generic
> >>> to move to hdmi.c, it needs more work to make it more usable like
> >>> supporting all pixel formats and all colorimetry values.
> >>>
> >>> We do not have that much utility yet for that.
> >
> > I think you are mixing the filling of drm_dp_vsc_sdp and packing of
> > that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
> > or less as is.
> > Once somebody needs to support 3D (AMD does), they can extend the function.
> >
>
> Yes once I corrected my understanding of using the function to just pack
> struct dp_sdp instead of the buffer, I understood the intention.
>
> We can try it. I have written up a RFC to move the
> intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate
> now for it and not hdmi.c as we already have some vsc sdp utils in
> drm_dp_helper.

Yes, we have. However all structure manipulation functions, even for
DP, are usually a part of the hdmi.c. If I understand correctly, we
can still touch this file from drm-misc (or from other drm trees).

> But before I post, we will do some cleanup and see if things look
> reasonable in this patch too because we will still need to write a
> common utility to pack the parity bytes for the sdp header which we need
> to utilize for audio and vsc sdp in our DP case.
>
> I am thinking of a
>
> struct msm_dp_sdp_pb {
>         u8 PB0;
>         u8 PB1;
>         u8 PB2;
>         u8 PB3;
> } __packed;
>
> struct msm_dp_sdp_with_parity {
>         struct dp_sdp vsc_sdp;
>         struct msm_dp_sdp_pb pb;
> };
>
> intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to
> do some additional parity byte packing after that. That part will remain
> common for audio and vsc for msm.

I think you can do it in a much easier way:

struct dp_sdp sdp
u32 header[2];

hdmi_dp_vsc_sdp_pack(vsc_sdp, &sdp);
msm_dp_pack_header(&sdp, header);
dp_write_register(catalog, MMSS_DP_FOO_HEADER0, header[0]);
dp_write_register(catalog, MMSS_DP_FOO_HEADER1, header[1]);
// write sdp.db

On 2/10/2024 10:57 PM, Dmitry Baryshkov wrote:
> On Sun, 11 Feb 2024 at 06:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
>>> On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
>>>>>> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
>>>>>>>     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   |  59 ++++++++++++++++
>>>>>>>     drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
>>>>>>>     drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
>>>>>>>     drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
>>>>>>>     7 files changed, 260 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>>>> index 5d84c089e520a..0f28a4897b7b7 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>>>>> @@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
>>>>>>> dp_catalog *dp_catalog)
>>>>>>>            return 0;
>>>>>>>     }
>>>>>>>
>>
>> <Snip>
>>
>>>>>>> +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 dp_sdp_header sdp_header;
>>>>>>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
>>>>>>> +       size_t buff_size;
>>>>>>> +       u32 gen_buffer[10];
>>>>>>> +       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;
>>>>>>> +       buff_size = sizeof(gen_buffer);
>>>>>>> +
>>>>>>> +       memset(&sdp_header, 0, sizeof(sdp_header));
>>>>>>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
>>>>>>> +
>>>>>>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
>>>>>>> +       sdp_header.HB0 = 0x00;
>>>>>>> +       sdp_header.HB1 = 0x07;
>>>>>>> +       sdp_header.HB2 = 0x05;
>>>>>>> +       sdp_header.HB3 = 0x13;
>>>>>>
>>>>>> This should go to the packing function.
>>>>>>
>>>>>
>>>>> We can .... but ....
>>>>>
>>>>> the header bytes can change based on the format. These values are
>>>>> specific to YUV420. Thats why this part was kept outside of the generic
>>>>> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
>>>>> but this was the reason.
>>>
>>> These values can be set from the sdp_type, revision and length fields
>>> of struct drm_dp_vsc_sdp.
>>> The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
>>>
>>>>>
>>>>>>> +
>>>>>>> +       /* VSC SDP Payload for DB16 */
>>>>>>
>>>>>> Comments are useless more or less. The code fills generic information
>>>>>> structure which might or might not be packed to the data buffer.
>>>>>>
>>>>>>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
>>>>>>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
>>>>>>> +
>>>>>>> +       /* VSC SDP Payload for DB17 */
>>>>>>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
>>>>>>> +
>>>>>>> +       /* VSC SDP Payload for DB18 */
>>>>>>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
>>>>>>> +
>>>>>>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
>>>>>>
>>>>>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
>>>>>>
>>>>>
>>>>> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
>>>>> YUV420, we do not.
>>>
>>> Intel function also uses output_format, but it's true, it is full of
>>> Intel specifics.
>>>
>>>>> Right now, its a pure driver decision to use YUV420
>>>>> when the mode is supported only in that format.
>>>>>
>>>>> Also, many params are to be used within this function cached inside
>>>>> intel_crtc_state . We will first need to make that API more generic to
>>>>> be re-usable by others.
>>>>>
>>>>> I think overall, if we want to have a generic packing across vendors, it
>>>>> needs more work. I think one of us can take that up as a separate effort.
>>>
>>> Ack, I agree here. I did only a quick glance over
>>> intel_dp_compute_vsc_colorimetry function() beforehand.
>>>
>>>>>
>>>>>>> +
>>>>>>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
>>>>>>> gen_buffer, buff_size);
>>>>>>> +       if (rc) {
>>>>>>> +               DRM_ERROR("unable to pack vsc sdp\n");
>>>>>>> +               return rc;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
>>>>>>> +
>>>>>>> +       return rc;
>>>>>>> +}
>>>>>>> +
>>>>>>>     void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>>>>     {
>>>>>>>            struct dp_catalog *catalog;
>>>>>>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
>>>>>>> @@ -4,6 +4,16 @@
>>>>>>>      */
>>>>>>>
>>>>>>>     #include <linux/types.h>
>>>>>>> +#include <drm/display/drm_dp_helper.h>
>>>>>>> +#include <drm/drm_print.h>
>>>>>>> +
>>>>>>> +#include "dp_utils.h"
>>>>>>> +
>>>>>>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
>>>>>>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
>>>>>>> +
>>>>>>> +#define DP_SDP_HEADER_SIZE             8
>>>>>>> +#define DP_VSC_SDP_BUFFER_SIZE         40
>>>>>>>
>>>>>>>     u8 dp_utils_get_g0_value(u8 data)
>>>>>>>     {
>>>>>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
>>>>>>>
>>>>>>>            return parity_byte;
>>>>>>>     }
>>>>>>> +
>>>>>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
>>>>>>> *buffer, size_t buff_size)
>>>>>>> +{
>>>>>>> +       u32 header, parity;
>>>>>>> +
>>>>>>> +       if (buff_size < DP_SDP_HEADER_SIZE)
>>>>>>> +               return -ENOSPC;
>>>>>>> +
>>>>>>> +       memset(buffer, 0, sizeof(buffer));
>>>>>>> +
>>>>>>> +       /* HEADER BYTE 0 */
>>>>>>> +       header = sdp_header->HB0;
>>>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>>>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity <<
>>>>>>> PARITY_BYTE_0_BIT));
>>>>>>> +
>>>>>>> +       /* HEADER BYTE 1 */
>>>>>>> +       header = sdp_header->HB1;
>>>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>>>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity <<
>>>>>>> PARITY_BYTE_1_BIT));
>>>>>>> +
>>>>>>> +       /* HEADER BYTE 2 */
>>>>>>> +       header = sdp_header->HB2;
>>>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>>>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity <<
>>>>>>> PARITY_BYTE_2_BIT));
>>>>>>> +
>>>>>>> +       /* HEADER BYTE 3 */
>>>>>>> +       header = sdp_header->HB3;
>>>>>>> +       parity = dp_utils_calculate_parity(header);
>>>>>>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity <<
>>>>>>> PARITY_BYTE_3_BIT));
>>>>>>> +
>>>>>>> +       return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
>>>>>>> struct dp_sdp_header *sdp_header,
>>>>>>> +                         u32 *buffer, size_t buff_size)
>>>>>>> +{
>>>>>>
>>>>>> No. This function should pack data into struct dp_sdp and it should go
>>>>>> to drivers/video/hdmi.c
>>>>>>
>>>>>
>>>>> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
>>>>>
>>>>
>>>> I think you were referring to the fact that instead of a custom buffer,
>>>> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
>>>>
>>>> If yes, I agree with this part.
>>>
>>> Yes.
>>>
>>>>
>>>>> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
>>>>>
>>>>> Regarding hdmi.c, I think the packing needs to be more generic to move
>>>>> it there.
>>>>>
>>>>> I am not seeing parity bytes packing with other vendors. So there will
>>>>> have to be some packing there and then some packing here.
>>>
>>> Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
>>> Intel and AMD seem to write header bytes without parity.
>>>
>>>>> Also, like you already noticed, to make the VSC SDP packing more generic
>>>>> to move to hdmi.c, it needs more work to make it more usable like
>>>>> supporting all pixel formats and all colorimetry values.
>>>>>
>>>>> We do not have that much utility yet for that.
>>>
>>> I think you are mixing the filling of drm_dp_vsc_sdp and packing of
>>> that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
>>> or less as is.
>>> Once somebody needs to support 3D (AMD does), they can extend the function.
>>>
>>
>> Yes once I corrected my understanding of using the function to just pack
>> struct dp_sdp instead of the buffer, I understood the intention.
>>
>> We can try it. I have written up a RFC to move the
>> intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate
>> now for it and not hdmi.c as we already have some vsc sdp utils in
>> drm_dp_helper.
> 
> Yes, we have. However all structure manipulation functions, even for
> DP, are usually a part of the hdmi.c. If I understand correctly, we
> can still touch this file from drm-misc (or from other drm trees).
> 

hmm ... I think the only reason we have 
hdmi_audio_infoframe_pack_for_dp() is because of re-using 
hdmi_audio_infoframe to pack into a struct dp_sdp.

We will not be doing that here. It will be from a drm_dp_vsc_sdp to 
dp_sdp. Thats why I thought drm_dp_helper is more appropriate.

>> But before I post, we will do some cleanup and see if things look
>> reasonable in this patch too because we will still need to write a
>> common utility to pack the parity bytes for the sdp header which we need
>> to utilize for audio and vsc sdp in our DP case.
>>
>> I am thinking of a
>>
>> struct msm_dp_sdp_pb {
>>          u8 PB0;
>>          u8 PB1;
>>          u8 PB2;
>>          u8 PB3;
>> } __packed;
>>
>> struct msm_dp_sdp_with_parity {
>>          struct dp_sdp vsc_sdp;
>>          struct msm_dp_sdp_pb pb;
>> };
>>
>> intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to
>> do some additional parity byte packing after that. That part will remain
>> common for audio and vsc for msm.
> 
> I think you can do it in a much easier way:
> 
> struct dp_sdp sdp
> u32 header[2];
> 
> hdmi_dp_vsc_sdp_pack(vsc_sdp, &sdp);
> msm_dp_pack_header(&sdp, header);
> dp_write_register(catalog, MMSS_DP_FOO_HEADER0, header[0]);
> dp_write_register(catalog, MMSS_DP_FOO_HEADER1, header[1]);
> // write sdp.db
> 

Sure. Just a difference of packing it together Vs a local header. We 
will see which one looks better and post it. I felt that my way makes it 
clear that msm_dp needs extra parity specific to msm.

On Sun, 11 Feb 2024 at 19:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/10/2024 10:57 PM, Dmitry Baryshkov wrote:
> > On Sun, 11 Feb 2024 at 06:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
> >>>>>
> >>>>>
> >>>>> On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
> >>>>>> On Sat, 10 Feb 2024 at 03:52, 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 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 | 105 ++++++++++++++++++++++++++++
> >>>>>>>     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   |  59 ++++++++++++++++
> >>>>>>>     drivers/gpu/drm/msm/dp/dp_reg.h     |   3 +
> >>>>>>>     drivers/gpu/drm/msm/dp/dp_utils.c   |  80 +++++++++++++++++++++
> >>>>>>>     drivers/gpu/drm/msm/dp/dp_utils.h   |   3 +
> >>>>>>>     7 files changed, 260 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>>>> index 5d84c089e520a..0f28a4897b7b7 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>>>>> @@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
> >>>>>>> dp_catalog *dp_catalog)
> >>>>>>>            return 0;
> >>>>>>>     }
> >>>>>>>
> >>
> >> <Snip>
> >>
> >>>>>>> +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 dp_sdp_header sdp_header;
> >>>>>>> +       struct drm_dp_vsc_sdp vsc_sdp_data;
> >>>>>>> +       size_t buff_size;
> >>>>>>> +       u32 gen_buffer[10];
> >>>>>>> +       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;
> >>>>>>> +       buff_size = sizeof(gen_buffer);
> >>>>>>> +
> >>>>>>> +       memset(&sdp_header, 0, sizeof(sdp_header));
> >>>>>>> +       memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
> >>>>>>> +
> >>>>>>> +       /* VSC SDP header as per table 2-118 of DP 1.4 specification */
> >>>>>>> +       sdp_header.HB0 = 0x00;
> >>>>>>> +       sdp_header.HB1 = 0x07;
> >>>>>>> +       sdp_header.HB2 = 0x05;
> >>>>>>> +       sdp_header.HB3 = 0x13;
> >>>>>>
> >>>>>> This should go to the packing function.
> >>>>>>
> >>>>>
> >>>>> We can .... but ....
> >>>>>
> >>>>> the header bytes can change based on the format. These values are
> >>>>> specific to YUV420. Thats why this part was kept outside of the generic
> >>>>> vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
> >>>>> but this was the reason.
> >>>
> >>> These values can be set from the sdp_type, revision and length fields
> >>> of struct drm_dp_vsc_sdp.
> >>> The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
> >>>
> >>>>>
> >>>>>>> +
> >>>>>>> +       /* VSC SDP Payload for DB16 */
> >>>>>>
> >>>>>> Comments are useless more or less. The code fills generic information
> >>>>>> structure which might or might not be packed to the data buffer.
> >>>>>>
> >>>>>>> +       vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
> >>>>>>> +       vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
> >>>>>>> +
> >>>>>>> +       /* VSC SDP Payload for DB17 */
> >>>>>>> +       vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
> >>>>>>> +
> >>>>>>> +       /* VSC SDP Payload for DB18 */
> >>>>>>> +       vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
> >>>>>>> +
> >>>>>>> +       vsc_sdp_data.bpc = dp_mode->bpp / 3;
> >>>>>>
> >>>>>> Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
> >>>>>>
> >>>>>
> >>>>> intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
> >>>>> YUV420, we do not.
> >>>
> >>> Intel function also uses output_format, but it's true, it is full of
> >>> Intel specifics.
> >>>
> >>>>> Right now, its a pure driver decision to use YUV420
> >>>>> when the mode is supported only in that format.
> >>>>>
> >>>>> Also, many params are to be used within this function cached inside
> >>>>> intel_crtc_state . We will first need to make that API more generic to
> >>>>> be re-usable by others.
> >>>>>
> >>>>> I think overall, if we want to have a generic packing across vendors, it
> >>>>> needs more work. I think one of us can take that up as a separate effort.
> >>>
> >>> Ack, I agree here. I did only a quick glance over
> >>> intel_dp_compute_vsc_colorimetry function() beforehand.
> >>>
> >>>>>
> >>>>>>> +
> >>>>>>> +       rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
> >>>>>>> gen_buffer, buff_size);
> >>>>>>> +       if (rc) {
> >>>>>>> +               DRM_ERROR("unable to pack vsc sdp\n");
> >>>>>>> +               return rc;
> >>>>>>> +       }
> >>>>>>> +
> >>>>>>> +       dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
> >>>>>>> +
> >>>>>>> +       return rc;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>     void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>>>>>>     {
> >>>>>>>            struct dp_catalog *catalog;
> >>>>>>> @@ -344,6 +399,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 176d839906cec..05e0133eb50f3 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
> >>>>>>> @@ -4,6 +4,16 @@
> >>>>>>>      */
> >>>>>>>
> >>>>>>>     #include <linux/types.h>
> >>>>>>> +#include <drm/display/drm_dp_helper.h>
> >>>>>>> +#include <drm/drm_print.h>
> >>>>>>> +
> >>>>>>> +#include "dp_utils.h"
> >>>>>>> +
> >>>>>>> +#define DP_GENERIC0_6_YUV_8_BPC                BIT(0)
> >>>>>>> +#define DP_GENERIC0_6_YUV_10_BPC       BIT(1)
> >>>>>>> +
> >>>>>>> +#define DP_SDP_HEADER_SIZE             8
> >>>>>>> +#define DP_VSC_SDP_BUFFER_SIZE         40
> >>>>>>>
> >>>>>>>     u8 dp_utils_get_g0_value(u8 data)
> >>>>>>>     {
> >>>>>>> @@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
> >>>>>>>
> >>>>>>>            return parity_byte;
> >>>>>>>     }
> >>>>>>> +
> >>>>>>> +int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
> >>>>>>> *buffer, size_t buff_size)
> >>>>>>> +{
> >>>>>>> +       u32 header, parity;
> >>>>>>> +
> >>>>>>> +       if (buff_size < DP_SDP_HEADER_SIZE)
> >>>>>>> +               return -ENOSPC;
> >>>>>>> +
> >>>>>>> +       memset(buffer, 0, sizeof(buffer));
> >>>>>>> +
> >>>>>>> +       /* HEADER BYTE 0 */
> >>>>>>> +       header = sdp_header->HB0;
> >>>>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>>>> +       buffer[0]   = ((header << HEADER_BYTE_0_BIT) | (parity <<
> >>>>>>> PARITY_BYTE_0_BIT));
> >>>>>>> +
> >>>>>>> +       /* HEADER BYTE 1 */
> >>>>>>> +       header = sdp_header->HB1;
> >>>>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>>>> +       buffer[0]  |= ((header << HEADER_BYTE_1_BIT) | (parity <<
> >>>>>>> PARITY_BYTE_1_BIT));
> >>>>>>> +
> >>>>>>> +       /* HEADER BYTE 2 */
> >>>>>>> +       header = sdp_header->HB2;
> >>>>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>>>> +       buffer[1]   = ((header << HEADER_BYTE_2_BIT) | (parity <<
> >>>>>>> PARITY_BYTE_2_BIT));
> >>>>>>> +
> >>>>>>> +       /* HEADER BYTE 3 */
> >>>>>>> +       header = sdp_header->HB3;
> >>>>>>> +       parity = dp_utils_calculate_parity(header);
> >>>>>>> +       buffer[1]  |= ((header << HEADER_BYTE_3_BIT) | (parity <<
> >>>>>>> PARITY_BYTE_3_BIT));
> >>>>>>> +
> >>>>>>> +       return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
> >>>>>>> struct dp_sdp_header *sdp_header,
> >>>>>>> +                         u32 *buffer, size_t buff_size)
> >>>>>>> +{
> >>>>>>
> >>>>>> No. This function should pack data into struct dp_sdp and it should go
> >>>>>> to drivers/video/hdmi.c
> >>>>>>
> >>>>>
> >>>>> What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
> >>>>>
> >>>>
> >>>> I think you were referring to the fact that instead of a custom buffer,
> >>>> we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
> >>>>
> >>>> If yes, I agree with this part.
> >>>
> >>> Yes.
> >>>
> >>>>
> >>>>> It seems like struct drm_dp_vsc_sdp is more appropriate for this.
> >>>>>
> >>>>> Regarding hdmi.c, I think the packing needs to be more generic to move
> >>>>> it there.
> >>>>>
> >>>>> I am not seeing parity bytes packing with other vendors. So there will
> >>>>> have to be some packing there and then some packing here.
> >>>
> >>> Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
> >>> Intel and AMD seem to write header bytes without parity.
> >>>
> >>>>> Also, like you already noticed, to make the VSC SDP packing more generic
> >>>>> to move to hdmi.c, it needs more work to make it more usable like
> >>>>> supporting all pixel formats and all colorimetry values.
> >>>>>
> >>>>> We do not have that much utility yet for that.
> >>>
> >>> I think you are mixing the filling of drm_dp_vsc_sdp and packing of
> >>> that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
> >>> or less as is.
> >>> Once somebody needs to support 3D (AMD does), they can extend the function.
> >>>
> >>
> >> Yes once I corrected my understanding of using the function to just pack
> >> struct dp_sdp instead of the buffer, I understood the intention.
> >>
> >> We can try it. I have written up a RFC to move the
> >> intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate
> >> now for it and not hdmi.c as we already have some vsc sdp utils in
> >> drm_dp_helper.
> >
> > Yes, we have. However all structure manipulation functions, even for
> > DP, are usually a part of the hdmi.c. If I understand correctly, we
> > can still touch this file from drm-misc (or from other drm trees).
> >
>
> hmm ... I think the only reason we have
> hdmi_audio_infoframe_pack_for_dp() is because of re-using
> hdmi_audio_infoframe to pack into a struct dp_sdp.
>
> We will not be doing that here. It will be from a drm_dp_vsc_sdp to
> dp_sdp. Thats why I thought drm_dp_helper is more appropriate.

Could you please raise this question at #dri-devel? Let's get Jani's
and other maintainers' opinion.

>
> >> But before I post, we will do some cleanup and see if things look
> >> reasonable in this patch too because we will still need to write a
> >> common utility to pack the parity bytes for the sdp header which we need
> >> to utilize for audio and vsc sdp in our DP case.
> >>
> >> I am thinking of a
> >>
> >> struct msm_dp_sdp_pb {
> >>          u8 PB0;
> >>          u8 PB1;
> >>          u8 PB2;
> >>          u8 PB3;
> >> } __packed;
> >>
> >> struct msm_dp_sdp_with_parity {
> >>          struct dp_sdp vsc_sdp;
> >>          struct msm_dp_sdp_pb pb;
> >> };
> >>
> >> intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to
> >> do some additional parity byte packing after that. That part will remain
> >> common for audio and vsc for msm.
> >
> > I think you can do it in a much easier way:
> >
> > struct dp_sdp sdp
> > u32 header[2];
> >
> > hdmi_dp_vsc_sdp_pack(vsc_sdp, &sdp);
> > msm_dp_pack_header(&sdp, header);
> > dp_write_register(catalog, MMSS_DP_FOO_HEADER0, header[0]);
> > dp_write_register(catalog, MMSS_DP_FOO_HEADER1, header[1]);
> > // write sdp.db
> >
>
> Sure. Just a difference of packing it together Vs a local header. We
> will see which one looks better and post it. I felt that my way makes it
> clear that msm_dp needs extra parity specific to msm.

I just don't like extra wrapping structures. But really, let's see
your patches first.

On 2/10/2024 2:50 AM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>> Adjust the encoder timing engine setup programming in the case of video
>> mode for YUV420 over DP to accommodate CDM.
>>
>> Changes in v2:
>>          - Move timing engine programming to this patch
>>
>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>> ---
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index 3f102b2813ca8..fb46d907312a7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -235,8 +235,9 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>   {
>>          struct drm_display_mode mode;
>>          struct dpu_hw_intf_timing_params timing_params = { 0 };
>> +       struct dpu_hw_cdm *hw_cdm;
>>          const struct dpu_format *fmt = NULL;
>> -       u32 fmt_fourcc = DRM_FORMAT_RGB888;
>> +       u32 fmt_fourcc;
>>          unsigned long lock_flags;
>>          struct dpu_hw_intf_cfg intf_cfg = { 0 };
>>
>> @@ -255,17 +256,21 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>          DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n");
>>          drm_mode_debug_printmodeline(&mode);
>>
>> -       if (phys_enc->split_role != ENC_ROLE_SOLO) {
>> +       fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc);
>> +
>> +       if (phys_enc->split_role != ENC_ROLE_SOLO || fmt_fourcc == DRM_FORMAT_YUV420) {
>>                  mode.hdisplay >>= 1;
>>                  mode.htotal >>= 1;
>>                  mode.hsync_start >>= 1;
>>                  mode.hsync_end >>= 1;
>> +               mode.hskew >>= 1;
> hskew change seems to warrant a separate patch with Fixes for
> 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")


Ack


>
>>                  DPU_DEBUG_VIDENC(phys_enc,
>> -                       "split_role %d, halve horizontal %d %d %d %d\n",
>> +                       "split_role %d, halve horizontal %d %d %d %d %d\n",
>>                          phys_enc->split_role,
>>                          mode.hdisplay, mode.htotal,
>> -                       mode.hsync_start, mode.hsync_end);
>> +                       mode.hsync_start, mode.hsync_end,
>> +                       mode.hskew);
>>          }
>>
>>          drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params);
>> @@ -273,6 +278,9 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>          fmt = dpu_get_dpu_format(fmt_fourcc);
>>          DPU_DEBUG_VIDENC(phys_enc, "fmt_fourcc 0x%X\n", fmt_fourcc);
>>
>> +       hw_cdm = phys_enc->hw_cdm;
>> +       if (hw_cdm)
>> +               intf_cfg.cdm = hw_cdm->idx;
> No need for a separate local variable.


Ack


>
>>          intf_cfg.intf = phys_enc->hw_intf->idx;
>>          intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>>          intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>> --
>> 2.39.2
>>
>

On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote:
>>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>>>
>>>> All the components of YUV420 over DP are added. Therefore, let's mark the
>>>> connector property as true for DP connector when the DP type is not eDP
>>>> and when there is a CDM block available.
>>>>
>>>> Changes in v2:
>>>>           - Check for if dp_catalog has a CDM block available instead of
>>>>             checking if VSC SDP is allowed when setting the dp connector's
>>>>             ycbcr_420_allowed parameter
>>>>
>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
>>>>    drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
>>>>    drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
>>>>    drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
>>>>    drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
>>>>    5 files changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 723cc1d821431..beeaabe499abf 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>    {
>>>>           struct drm_encoder *encoder = NULL;
>>>>           struct msm_display_info info;
>>>> +       bool yuv_supported;
>>>>           int rc;
>>>>           int i;
>>>>
>>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>                           return PTR_ERR(encoder);
>>>>                   }
>>>>
>>>> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>>>> +               yuv_supported = !!(dpu_kms->catalog->cdm);
>>>
>>> Drop parentheses please.
>>>
>>>> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
>>>>                   if (rc) {
>>>>                           DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>>                           return rc;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index ebcc76ef1d590..9b9f5f2921903 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>>>>    }
>>>>
>>>>    int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>> -                       struct drm_encoder *encoder)
>>>> +                       struct drm_encoder *encoder, bool yuv_supported)
>>>>    {
>>>>           struct dp_display_private *dp_priv;
>>>>           int ret;
>>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>                   return ret;
>>>>           }
>>>>
>>>> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
>>>> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
>>>>           if (IS_ERR(dp_display->connector)) {
>>>>                   ret = PTR_ERR(dp_display->connector);
>>>>                   DRM_DEV_ERROR(dev->dev,
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>> index 46e6889037e88..ab0d0d13b5e2c 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>    }
>>>>
>>>>    /* connector initialization */
>>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
>>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
>>>> +                                           bool yuv_supported)
>>>>    {
>>>>           struct drm_connector *connector = NULL;
>>>>
>>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
>>>>           if (IS_ERR(connector))
>>>>                   return connector;
>>>>
>>>> -       if (!dp_display->is_edp)
>>>> +       if (!dp_display->is_edp) {
>>>>                   drm_connector_attach_dp_subconnector_property(connector);
>>>> +               if (yuv_supported)
>>>> +                       connector->ycbcr_420_allowed = true;
>>>
>>> Is there any reason to disallow YUV420 for eDP connectors?
>>>
>>>> +       }
>>>>
>>
>> Major reason was certainly validation but thinking about it more
>> closely, I need to check whether CDM over eDP is even possible.
>>
>> Historically, CDM could output only to HDMI OR WB using the bit we
>> program while setting up CDM and the same HDMI path is being used by DP
>> as well. But I need to check whether CDM can even output to eDP with the
>> same DP path. I dont have any documentation on this part yet.
> 
> I had the feeling that the DP / eDP difference on the chips is mostly
> on the PHY and software side. So I assumed that it should be possible
> to output YUV data to the eDP port in the same way as it is done for
> the DP port.
> 

This is true. I was not worried about DP / eDP controller but mostly 
whether eDP spec really allows YUV. From what I can read, it does.

So this check shall remain only because CDM has not been validated with eDP.

Do you need a TODO here and if we ever get a eDP panel which supports 
that, we can validate and relax this.

On Mon, 12 Feb 2024 at 23:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote:
> > On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote:
> >>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>>>
> >>>> All the components of YUV420 over DP are added. Therefore, let's mark the
> >>>> connector property as true for DP connector when the DP type is not eDP
> >>>> and when there is a CDM block available.
> >>>>
> >>>> Changes in v2:
> >>>>           - Check for if dp_catalog has a CDM block available instead of
> >>>>             checking if VSC SDP is allowed when setting the dp connector's
> >>>>             ycbcr_420_allowed parameter
> >>>>
> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
> >>>>    drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
> >>>>    drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
> >>>>    drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
> >>>>    drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
> >>>>    5 files changed, 16 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> index 723cc1d821431..beeaabe499abf 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
> >>>>    {
> >>>>           struct drm_encoder *encoder = NULL;
> >>>>           struct msm_display_info info;
> >>>> +       bool yuv_supported;
> >>>>           int rc;
> >>>>           int i;
> >>>>
> >>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
> >>>>                           return PTR_ERR(encoder);
> >>>>                   }
> >>>>
> >>>> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
> >>>> +               yuv_supported = !!(dpu_kms->catalog->cdm);
> >>>
> >>> Drop parentheses please.
> >>>
> >>>> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
> >>>>                   if (rc) {
> >>>>                           DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> >>>>                           return rc;
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index ebcc76ef1d590..9b9f5f2921903 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
> >>>>    }
> >>>>
> >>>>    int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> >>>> -                       struct drm_encoder *encoder)
> >>>> +                       struct drm_encoder *encoder, bool yuv_supported)
> >>>>    {
> >>>>           struct dp_display_private *dp_priv;
> >>>>           int ret;
> >>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> >>>>                   return ret;
> >>>>           }
> >>>>
> >>>> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
> >>>> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
> >>>>           if (IS_ERR(dp_display->connector)) {
> >>>>                   ret = PTR_ERR(dp_display->connector);
> >>>>                   DRM_DEV_ERROR(dev->dev,
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>> index 46e6889037e88..ab0d0d13b5e2c 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> >>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
> >>>>    }
> >>>>
> >>>>    /* connector initialization */
> >>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
> >>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
> >>>> +                                           bool yuv_supported)
> >>>>    {
> >>>>           struct drm_connector *connector = NULL;
> >>>>
> >>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
> >>>>           if (IS_ERR(connector))
> >>>>                   return connector;
> >>>>
> >>>> -       if (!dp_display->is_edp)
> >>>> +       if (!dp_display->is_edp) {
> >>>>                   drm_connector_attach_dp_subconnector_property(connector);
> >>>> +               if (yuv_supported)
> >>>> +                       connector->ycbcr_420_allowed = true;
> >>>
> >>> Is there any reason to disallow YUV420 for eDP connectors?
> >>>
> >>>> +       }
> >>>>
> >>
> >> Major reason was certainly validation but thinking about it more
> >> closely, I need to check whether CDM over eDP is even possible.
> >>
> >> Historically, CDM could output only to HDMI OR WB using the bit we
> >> program while setting up CDM and the same HDMI path is being used by DP
> >> as well. But I need to check whether CDM can even output to eDP with the
> >> same DP path. I dont have any documentation on this part yet.
> >
> > I had the feeling that the DP / eDP difference on the chips is mostly
> > on the PHY and software side. So I assumed that it should be possible
> > to output YUV data to the eDP port in the same way as it is done for
> > the DP port.
> >
>
> This is true. I was not worried about DP / eDP controller but mostly
> whether eDP spec really allows YUV. From what I can read, it does.
>
> So this check shall remain only because CDM has not been validated with eDP.
>
> Do you need a TODO here and if we ever get a eDP panel which supports
> that, we can validate and relax this.

Just move it out of the eDP check.

On 2/12/2024 1:20 PM, Dmitry Baryshkov wrote:
> On Mon, 12 Feb 2024 at 23:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote:
>>> On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote:
>>>>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>>>>>
>>>>>> All the components of YUV420 over DP are added. Therefore, let's mark the
>>>>>> connector property as true for DP connector when the DP type is not eDP
>>>>>> and when there is a CDM block available.
>>>>>>
>>>>>> Changes in v2:
>>>>>>            - Check for if dp_catalog has a CDM block available instead of
>>>>>>              checking if VSC SDP is allowed when setting the dp connector's
>>>>>>              ycbcr_420_allowed parameter
>>>>>>
>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++-
>>>>>>     drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
>>>>>>     drivers/gpu/drm/msm/dp/dp_drm.c         | 8 ++++++--
>>>>>>     drivers/gpu/drm/msm/dp/dp_drm.h         | 3 ++-
>>>>>>     drivers/gpu/drm/msm/msm_drv.h           | 5 +++--
>>>>>>     5 files changed, 16 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> index 723cc1d821431..beeaabe499abf 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>>>     {
>>>>>>            struct drm_encoder *encoder = NULL;
>>>>>>            struct msm_display_info info;
>>>>>> +       bool yuv_supported;
>>>>>>            int rc;
>>>>>>            int i;
>>>>>>
>>>>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>>>                            return PTR_ERR(encoder);
>>>>>>                    }
>>>>>>
>>>>>> -               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>>>>>> +               yuv_supported = !!(dpu_kms->catalog->cdm);
>>>>>
>>>>> Drop parentheses please.
>>>>>
>>>>>> +               rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported);
>>>>>>                    if (rc) {
>>>>>>                            DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>>>>                            return rc;
>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> index ebcc76ef1d590..9b9f5f2921903 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>>>>>>     }
>>>>>>
>>>>>>     int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>>> -                       struct drm_encoder *encoder)
>>>>>> +                       struct drm_encoder *encoder, bool yuv_supported)
>>>>>>     {
>>>>>>            struct dp_display_private *dp_priv;
>>>>>>            int ret;
>>>>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>>>                    return ret;
>>>>>>            }
>>>>>>
>>>>>> -       dp_display->connector = dp_drm_connector_init(dp_display, encoder);
>>>>>> +       dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported);
>>>>>>            if (IS_ERR(dp_display->connector)) {
>>>>>>                    ret = PTR_ERR(dp_display->connector);
>>>>>>                    DRM_DEV_ERROR(dev->dev,
>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>> index 46e6889037e88..ab0d0d13b5e2c 100644
>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
>>>>>>     }
>>>>>>
>>>>>>     /* connector initialization */
>>>>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder)
>>>>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder,
>>>>>> +                                           bool yuv_supported)
>>>>>>     {
>>>>>>            struct drm_connector *connector = NULL;
>>>>>>
>>>>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
>>>>>>            if (IS_ERR(connector))
>>>>>>                    return connector;
>>>>>>
>>>>>> -       if (!dp_display->is_edp)
>>>>>> +       if (!dp_display->is_edp) {
>>>>>>                    drm_connector_attach_dp_subconnector_property(connector);
>>>>>> +               if (yuv_supported)
>>>>>> +                       connector->ycbcr_420_allowed = true;
>>>>>
>>>>> Is there any reason to disallow YUV420 for eDP connectors?
>>>>>
>>>>>> +       }
>>>>>>
>>>>
>>>> Major reason was certainly validation but thinking about it more
>>>> closely, I need to check whether CDM over eDP is even possible.
>>>>
>>>> Historically, CDM could output only to HDMI OR WB using the bit we
>>>> program while setting up CDM and the same HDMI path is being used by DP
>>>> as well. But I need to check whether CDM can even output to eDP with the
>>>> same DP path. I dont have any documentation on this part yet.
>>>
>>> I had the feeling that the DP / eDP difference on the chips is mostly
>>> on the PHY and software side. So I assumed that it should be possible
>>> to output YUV data to the eDP port in the same way as it is done for
>>> the DP port.
>>>
>>
>> This is true. I was not worried about DP / eDP controller but mostly
>> whether eDP spec really allows YUV. From what I can read, it does.
>>
>> So this check shall remain only because CDM has not been validated with eDP.
>>
>> Do you need a TODO here and if we ever get a eDP panel which supports
>> that, we can validate and relax this.
> 
> Just move it out of the eDP check.
> 

I would have said a no to this because it opens a trap door for untested 
path which I usually hesitate but in this case, I am also curious to 
know if there is going to be a eDP panel to test this out for us because 
I have not seen any yet. So lets go ahead with the removal of !is_edp.