[PATCH 14/17] drm/msm/dpu: modify encoder programming for CDM over DP

Dmitry Baryshkov dmitry.baryshkov at linaro.org
Sun Jan 28 05:55:42 UTC 2024


On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan at quicinc.com> wrote:
>
>
> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote:
> > On 25/01/2024 21:38, Paloma Arellano wrote:
> >> Adjust the encoder format programming in the case of video mode for DP
> >> to accommodate CDM related changes.
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 16 +++++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  8 +++++
> >>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 35 ++++++++++++++++---
> >>   drivers/gpu/drm/msm/dp/dp_display.c           | 12 +++++++
> >>   drivers/gpu/drm/msm/msm_drv.h                 |  9 ++++-
> >>   5 files changed, 75 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index b0896814c1562..99ec53446ad21 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> >>       15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> >>   };
> >>   +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
> >> const struct drm_display_mode *mode)
> >> +{
> >> +    const struct dpu_encoder_virt *dpu_enc;
> >> +    const struct msm_display_info *disp_info;
> >> +    struct msm_drm_private *priv;
> >> +
> >> +    dpu_enc = to_dpu_encoder_virt(drm_enc);
> >> +    disp_info = &dpu_enc->disp_info;
> >> +    priv = drm_enc->dev->dev_private;
> >> +
> >> +    if (disp_info->intf_type == INTF_DP &&
> >> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]],
> >> mode))
> >
> > This should not require interacting with DP. If we got here, we must
> > be sure that 4:2:0 is supported and can be configured.
> Ack. Will drop this function and only check for if the mode is YUV420.
> >
> >> +        return DRM_FORMAT_YUV420;
> >> +
> >> +    return DRM_FORMAT_RGB888;
> >> +}
> >>     bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> >> *drm_enc)
> >>   {
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >> index 7b4afa71f1f96..62255d0aa4487 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct
> >> drm_encoder *drm_enc);
> >>    */
> >>   bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> >> *drm_enc);
> >>   +/**
> >> + * dpu_encoder_get_drm_fmt - return DRM fourcc format
> >> + * @drm_enc:    Pointer to previously created drm encoder structure
> >> + * @mode:    Corresponding drm_display_mode for dpu encoder
> >> + */
> >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
> >> +                const struct drm_display_mode *mode);
> >> +
> >>   /**
> >>    * dpu_encoder_get_crc_values_cnt - get number of physical encoders
> >> contained
> >>    *    in virtual encoder that can collect CRC values
> >> 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 e284bf448bdda..a1dde0ff35dc8 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
> >> @@ -234,6 +234,7 @@ 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;
> >>       unsigned long lock_flags;
> >> @@ -254,17 +255,26 @@ 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) {
> >> +    hw_cdm = phys_enc->hw_cdm;
> >> +    if (hw_cdm) {
> >> +        intf_cfg.cdm = hw_cdm->idx;
> >> +        fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode);
> >> +    }
> >> +
> >> +    if (phys_enc->split_role != ENC_ROLE_SOLO ||
> >> +        dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) ==
> >> DRM_FORMAT_YUV420) {
> >>           mode.hdisplay >>= 1;
> >>           mode.htotal >>= 1;
> >>           mode.hsync_start >>= 1;
> >>           mode.hsync_end >>= 1;
> >> +        mode.hskew >>= 1;
> >
> > Separate patch.
> 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);
> >> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq(
> >>   static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys
> >> *phys_enc)
> >>   {
> >>       struct dpu_hw_ctl *ctl;
> >> +    struct dpu_hw_cdm *hw_cdm;
> >> +    const struct dpu_format *fmt = NULL;
> >> +    u32 fmt_fourcc = DRM_FORMAT_RGB888;
> >>         ctl = phys_enc->hw_ctl;
> >> +    hw_cdm = phys_enc->hw_cdm;
> >> +    if (hw_cdm)
> >> +        fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent,
> >> &phys_enc->cached_mode);
> >> +    fmt = dpu_get_dpu_format(fmt_fourcc);
> >>         DPU_DEBUG_VIDENC(phys_enc, "\n");
> >>   @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct
> >> dpu_encoder_phys *phys_enc)
> >>         dpu_encoder_helper_split_config(phys_enc,
> >> phys_enc->hw_intf->idx);
> >>   +    dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt,
> >> CDM_CDWN_OUTPUT_HDMI);
> >
> > If there is no CDM, why do we need to call this?
> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if
> there is a hw_cdm. If there is not, then it immediately exits the function.
> >
> >> +
> >>       dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
> >>         /*
> >> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct
> >> dpu_encoder_phys *phys_enc)
> >>       if (ctl->ops.update_pending_flush_merge_3d &&
> >> phys_enc->hw_pp->merge_3d)
> >>           ctl->ops.update_pending_flush_merge_3d(ctl,
> >> phys_enc->hw_pp->merge_3d->idx);
> >>   -    if (ctl->ops.update_pending_flush_periph &&
> >> phys_enc->hw_intf->cap->type == INTF_DP)
> >> +    if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm)
> >> +        ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx);
> >> +
> >> +    /*
> >> +     * Peripheral flush must be updated whenever flushing SDP
> >> packets is needed.
> >> +     * SDP packets are required for any YUV format (YUV420, YUV422,
> >> YUV444).
> >> +     */
> >> +    if (ctl->ops.update_pending_flush_periph &&
> >> phys_enc->hw_intf->cap->type == INTF_DP &&
> >> +        phys_enc->hw_cdm)
> >>           ctl->ops.update_pending_flush_periph(ctl,
> >> phys_enc->hw_intf->idx);
> >
> > Should there be a flush if we are switching from YUV 420 to RGB mode?
> We only need to flush for the sdp packet, but for msa we do not need to
> flush.

What about having SDP with RGB as colorimetry? In other words, if
there is a decision point, this one looks incorrect.

> >
> > Also, I'd say, we should move update_pending_flush_periph invocation
> > to this patch.
> Ack
> >
> >>     skip_flush:
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 6d764f5b08727..4329435518351 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void)
> >>       platform_driver_unregister(&dp_display_driver);
> >>   }
> >>   +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
> >> +                   const struct drm_display_mode *mode)
> >> +{
> >> +    struct dp_display_private *dp;
> >> +    const struct drm_display_info *info;
> >> +
> >> +    dp = container_of(dp_display, struct dp_display_private,
> >> dp_display);
> >> +    info = &dp_display->connector->display_info;
> >> +
> >> +    return dp_panel_vsc_sdp_supported(dp->panel) &&
> >> drm_mode_is_420_only(info, mode);
> >
> > YUV 420 modes should be filtered out in mode_valid if VSC SDP is not
> > supported.
> Ack. Will change
> >
> >> +}
> >> +
> >>   bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
> >>   {
> >>       struct dp_display_private *dp;
> >> diff --git a/drivers/gpu/drm/msm/msm_drv.h
> >> b/drivers/gpu/drm/msm/msm_drv.h
> >> index 16a7cbc0b7dd8..b9581bd934e9e 100644
> >> --- a/drivers/gpu/drm/msm/msm_drv.h
> >> +++ b/drivers/gpu/drm/msm/msm_drv.h
> >> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void);
> >>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct
> >> drm_device *dev,
> >>                struct drm_encoder *encoder);
> >>   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);
> >>   bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
> >>     #else
> >> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct
> >> msm_disp_state *disp_state, struct msm
> >>   {
> >>   }
> >>   +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp
> >> *dp_display,
> >> +                         const struct drm_display_mode *mode)
> >> +{
> >> +    return false;
> >> +}
> >> +
> >>   static inline bool msm_dp_wide_bus_available(const struct msm_dp
> >> *dp_display)
> >>   {
> >>       return false;
> >



--
With best wishes
Dmitry


More information about the dri-devel mailing list