Re: [PATCH v3 16/19] drm/msm/dpu: modify encoder programming for CDM over DP

[Paloma Arellano <quic_parellan@xxxxxxxxxxx>]

On 2/15/2024 7:47 AM, Abhinav Kumar wrote:
> On 2/15/2024 12:45 AM, Dmitry Baryshkov wrote:
> > On Wed, 14 Feb 2024 at 20:04, Paloma Arellano 
> > <quic_parellan@xxxxxxxxxxx> wrote:
> > > Adjust the encoder format programming in the case of video mode for DP
> > > to accommodate CDM related changes.
> > >
> > > Changes in v2:
> > >          - Move timing engine programming to a separate patch from this
> > >            one
> > >          - Move update_pending_flush_periph() invocation completely to
> > >            this patch
> > >          - Change the logic of dpu_encoder_get_drm_fmt() so that it 
> > > only
> > >            calls drm_mode_is_420_only() instead of doing additional
> > >            unnecessary checks
> > >          - Create new functions msm_dp_needs_periph_flush() and it's
> > >            supporting function dpu_encoder_needs_periph_flush() to 
> > > check
> > >            if the mode is YUV420 and VSC SDP is enabled before doing a
> > >            peripheral flush
> > >
> > > Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 35 
> > > +++++++++++++++++++
> > >   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  | 13 +++++++
> > >   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 19 ++++++++++
> > >   drivers/gpu/drm/msm/dp/dp_display.c           | 18 ++++++++++
> > >   drivers/gpu/drm/msm/msm_drv.h                 | 17 ++++++++-
> > >   5 files changed, 101 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 7e7796561009a..6280c6be6dca9 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -222,6 +222,41 @@ 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(struct dpu_encoder_phys *phys_enc)
> > > +{
> > > +       struct drm_encoder *drm_enc;
> > > +       struct dpu_encoder_virt *dpu_enc;
> > > +       struct drm_display_info *info;
> > > +       struct drm_display_mode *mode;
> > > +
> > > +       drm_enc = phys_enc->parent;
> > > +       dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > +       info = &dpu_enc->connector->display_info;
> > > +       mode = &phys_enc->cached_mode;
> > > +
> > > +       if (drm_mode_is_420_only(info, mode))
> > > +               return DRM_FORMAT_YUV420;
> > > +
> > > +       return DRM_FORMAT_RGB888;
> > > +}
> > > +
> > > +bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys *phys_enc)
> > > +{
> > > +       struct drm_encoder *drm_enc;
> > > +       struct dpu_encoder_virt *dpu_enc;
> > > +       struct msm_display_info *disp_info;
> > > +       struct msm_drm_private *priv;
> > > +       struct drm_display_mode *mode;
> > > +
> > > +       drm_enc = phys_enc->parent;
> > > +       dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > +       disp_info = &dpu_enc->disp_info;
> > > +       priv = drm_enc->dev->dev_private;
> > > +       mode = &phys_enc->cached_mode;
> > > +
> > > +       return phys_enc->hw_intf->cap->type == INTF_DP && 
> > > phys_enc->hw_cdm &&
> > > + msm_dp_needs_periph_flush(priv->dp[disp_info->h_tile_instance[0]], 
> > > mode);
> > > +}
> > >
> > >   bool dpu_encoder_is_widebus_enabled(const struct drm_encoder 
> > > *drm_enc)
> > >   {
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > index f43d57d9c74e1..211a3d90eb690 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > @@ -341,6 +341,19 @@ static inline enum dpu_3d_blend_mode 
> > > dpu_encoder_helper_get_3d_blend_mode(
> > >    */
> > >   unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys 
> > > *phys_enc);
> > >
> > > +/**
> > > + * dpu_encoder_get_drm_fmt - return DRM fourcc format
> > > + * @phys_enc: Pointer to physical encoder structure
> > > + */
> > > +u32 dpu_encoder_get_drm_fmt(struct dpu_encoder_phys *phys_enc);
> > > +
> > > +/**
> > > + * dpu_encoder_needs_periph_flush - return true if physical encoder 
> > > requires
> > > + *     peripheral flush
> > > + * @phys_enc: Pointer to physical encoder structure
> > > + */
> > > +bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys 
> > > *phys_enc);
> > > +
> > >   /**
> > >    * dpu_encoder_helper_split_config - split display configuration 
> > > helper function
> > >    *     This helper function may be used by physical encoders to 
> > > configure
> > > 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 f02411b062c4c..e29bc4bd39208 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
> > > @@ -415,8 +415,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)
> >
> > I thought that Abhinav proposed to drop the if(hw_cdm) condition here.
> > LGTM otherwise.
>
> Yes I did.
>
> This needs to be fixed in v4.


Ack, I must have forgotten to drop it, but I'll do it in the v4

> > > +               fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc);
> > > +       fmt = dpu_get_dpu_format(fmt_fourcc);
> > >
> > >          DPU_DEBUG_VIDENC(phys_enc, "\n");
> > >
> > > @@ -425,6 +432,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);
> > > +
> > >          dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
> > >
> > >          /*
> > > @@ -440,6 +449,16 @@ 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_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 && 
> > > dpu_encoder_needs_periph_flush(phys_enc))
> > > +               ctl->ops.update_pending_flush_periph(ctl, 
> > > phys_enc->hw_intf->idx);
> > > +
> > >   skip_flush:
> > >          DPU_DEBUG_VIDENC(phys_enc,
> > >                  "update pending flush ctl %d intf %d\n",
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 4b04388719363..ebcc76ef1d590 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1397,6 +1397,24 @@ 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 && 
> > > drm_mode_is_420_only(info, mode);
> > > +}
> > > +
> > > +bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
> > > +                              const struct drm_display_mode *mode)
> > > +{
> > > +       return msm_dp_is_yuv_420_enabled(dp_display, mode);
> > > +}
> > > +
> > >   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..b876ebd48effe 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.h
> > > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > > @@ -387,7 +387,10 @@ 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_needs_periph_flush(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 +412,18 @@ 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_needs_periph_flush(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;
> > > --
> > > 2.39.2