| Message ID | 20240210015223.24670-17-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for CDM over DP | expand |
On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> > --- > 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 && Do we really need to check for phys_enc->hw_cdm here? > + 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 f562beb6f7971..3f102b2813ca8 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 > @@ -413,8 +413,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); Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > + fmt = dpu_get_dpu_format(fmt_fourcc); Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe we can move both calls into the helper? I mean, fmt_fourcc is not used at all if the CDM is not used. > > DPU_DEBUG_VIDENC(phys_enc, "\n"); > > @@ -423,6 +430,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); > > /* > @@ -438,6 +447,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 >
On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: > On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> >> --- >> 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 && > > Do we really need to check for phys_enc->hw_cdm here? > hmmm I dont think so. If CDM was not there, then after the last patch, YUV420 removes will not be present at all. The only other case I could think of was, if for some reason CDM was used by some other interface such as WB, then hw_cdm will not be assigned. But, I think even for that msm_dp_needs_periph_flush() will take care of it because we use the cached_mode which is assigned only in mode_set(). >> + 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 f562beb6f7971..3f102b2813ca8 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 >> @@ -413,8 +413,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); > > Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > I think we dont need to check for if (hw_cdm) at all. dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns the fourcc code based on the mode. It doesnt need to know if there is cdm or not. We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for WB, we do not change the fourcc based on the mode. Its specific to video mode. >> + fmt = dpu_get_dpu_format(fmt_fourcc); > > Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe > we can move both calls into the helper? I mean, fmt_fourcc is not used > at all if the CDM is not used. > fourcc is always used to get the fmt to setup the timing engine params. Its just that it was always hard-coded to RGB. With CDM, it can change based on the mode. Thats why this utility was introduced to return the fourcc from the mode information. >> >> DPU_DEBUG_VIDENC(phys_enc, "\n"); >> >> @@ -423,6 +430,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); >> >> /* >> @@ -438,6 +447,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 >> > >
On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: > > On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> > >> --- > >> 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 && > > > > Do we really need to check for phys_enc->hw_cdm here? > > > > hmmm I dont think so. If CDM was not there, then after the last patch, > YUV420 removes will not be present at all. > > The only other case I could think of was, if for some reason CDM was > used by some other interface such as WB, then hw_cdm will not be assigned. > > But, I think even for that msm_dp_needs_periph_flush() will take care of > it because we use the cached_mode which is assigned only in mode_set(). > > >> + 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 f562beb6f7971..3f102b2813ca8 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 > >> @@ -413,8 +413,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); > > > > Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > > > > I think we dont need to check for if (hw_cdm) at all. > dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns > the fourcc code based on the mode. It doesnt need to know if there is > cdm or not. > > We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for > WB, we do not change the fourcc based on the mode. Its specific to video > mode. Wait... In the case of the WB we use the fourcc + modifier from the framebuffer. Ok, this looks fine then. > > >> + fmt = dpu_get_dpu_format(fmt_fourcc); > > > > Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe > > we can move both calls into the helper? I mean, fmt_fourcc is not used > > at all if the CDM is not used. > > > > fourcc is always used to get the fmt to setup the timing engine params. > Its just that it was always hard-coded to RGB. With CDM, it can change > based on the mode. Thats why this utility was introduced to return the > fourcc from the mode information. Yes, I perfectly understand that. I just disliked the idea of calling dpu_get_dpu_format() if the result gets unused. What about passing fourcc + modifier to dpu_encoder_helper_phys_setup_cdm() instead? > > >> > >> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >> > >> @@ -423,6 +430,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); > >> > >> /* > >> @@ -438,6 +447,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 > >> > > > >
On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: > On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: >>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> >>>> --- >>>> 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 && >>> >>> Do we really need to check for phys_enc->hw_cdm here? >>> >> >> hmmm I dont think so. If CDM was not there, then after the last patch, >> YUV420 removes will not be present at all. >> >> The only other case I could think of was, if for some reason CDM was >> used by some other interface such as WB, then hw_cdm will not be assigned. >> >> But, I think even for that msm_dp_needs_periph_flush() will take care of >> it because we use the cached_mode which is assigned only in mode_set(). >> >>>> + 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 f562beb6f7971..3f102b2813ca8 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 >>>> @@ -413,8 +413,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); >>> >>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). >>> >> >> I think we dont need to check for if (hw_cdm) at all. >> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns >> the fourcc code based on the mode. It doesnt need to know if there is >> cdm or not. >> >> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for >> WB, we do not change the fourcc based on the mode. Its specific to video >> mode. > > Wait... In the case of the WB we use the fourcc + modifier from the > framebuffer. Ok, this looks fine then. > > >> >>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>> >>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe >>> we can move both calls into the helper? I mean, fmt_fourcc is not used >>> at all if the CDM is not used. >>> >> >> fourcc is always used to get the fmt to setup the timing engine params. >> Its just that it was always hard-coded to RGB. With CDM, it can change >> based on the mode. Thats why this utility was introduced to return the >> fourcc from the mode information. > > Yes, I perfectly understand that. I just disliked the idea of calling > dpu_get_dpu_format() if the result gets unused. > What about passing fourcc + modifier to > dpu_encoder_helper_phys_setup_cdm() instead? > Why would it be unused? Its returning YUV420 if the mode is YUV420 and RGB otherwise. We need fourcc + modifier for video mode timing engine setup. I didnt quite follow your suggestion of passing these to dpu_encoder_helper_phys_setup_cdm(). >> >>>> >>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>> >>>> @@ -423,6 +430,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); >>>> >>>> /* >>>> @@ -438,6 +447,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 >>>> >>> >>> > > >
On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: > > On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: > >>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> > >>>> --- > >>>> 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 && > >>> > >>> Do we really need to check for phys_enc->hw_cdm here? > >>> > >> > >> hmmm I dont think so. If CDM was not there, then after the last patch, > >> YUV420 removes will not be present at all. > >> > >> The only other case I could think of was, if for some reason CDM was > >> used by some other interface such as WB, then hw_cdm will not be assigned. > >> > >> But, I think even for that msm_dp_needs_periph_flush() will take care of > >> it because we use the cached_mode which is assigned only in mode_set(). > >> > >>>> + 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 f562beb6f7971..3f102b2813ca8 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 > >>>> @@ -413,8 +413,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); > >>> > >>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > >>> > >> > >> I think we dont need to check for if (hw_cdm) at all. > >> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns > >> the fourcc code based on the mode. It doesnt need to know if there is > >> cdm or not. > >> > >> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for > >> WB, we do not change the fourcc based on the mode. Its specific to video > >> mode. > > > > Wait... In the case of the WB we use the fourcc + modifier from the > > framebuffer. Ok, this looks fine then. > > > > > >> > >>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>> > >>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe > >>> we can move both calls into the helper? I mean, fmt_fourcc is not used > >>> at all if the CDM is not used. > >>> > >> > >> fourcc is always used to get the fmt to setup the timing engine params. > >> Its just that it was always hard-coded to RGB. With CDM, it can change > >> based on the mode. Thats why this utility was introduced to return the > >> fourcc from the mode information. > > > > Yes, I perfectly understand that. I just disliked the idea of calling > > dpu_get_dpu_format() if the result gets unused. > > What about passing fourcc + modifier to > > dpu_encoder_helper_phys_setup_cdm() instead? > > > > Why would it be unused? Its returning YUV420 if the mode is YUV420 and > RGB otherwise. I thought about it being unused if there is no CDM in play. > > We need fourcc + modifier for video mode timing engine setup. I didnt > quite follow your suggestion of passing these to > dpu_encoder_helper_phys_setup_cdm(). Ah, this is modified in the next patch... Maybe passing fmt_fourcc from this function to setup_timing_engine will solve my troubles. > > > >> > >>>> > >>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>> > >>>> @@ -423,6 +430,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); > >>>> > >>>> /* > >>>> @@ -438,6 +447,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 > >>>> > >>> > >>> > > > > > >
On 2/13/2024 11:31 AM, Dmitry Baryshkov wrote: > On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: >>> On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: >>>>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> >>>>>> --- >>>>>> 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 && >>>>> >>>>> Do we really need to check for phys_enc->hw_cdm here? >>>>> >>>> >>>> hmmm I dont think so. If CDM was not there, then after the last patch, >>>> YUV420 removes will not be present at all. >>>> >>>> The only other case I could think of was, if for some reason CDM was >>>> used by some other interface such as WB, then hw_cdm will not be assigned. >>>> >>>> But, I think even for that msm_dp_needs_periph_flush() will take care of >>>> it because we use the cached_mode which is assigned only in mode_set(). >>>> >>>>>> + 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 f562beb6f7971..3f102b2813ca8 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 >>>>>> @@ -413,8 +413,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); >>>>> >>>>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). >>>>> >>>> >>>> I think we dont need to check for if (hw_cdm) at all. >>>> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns >>>> the fourcc code based on the mode. It doesnt need to know if there is >>>> cdm or not. >>>> >>>> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for >>>> WB, we do not change the fourcc based on the mode. Its specific to video >>>> mode. >>> >>> Wait... In the case of the WB we use the fourcc + modifier from the >>> framebuffer. Ok, this looks fine then. >>> >>> >>>> >>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>> >>>>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe >>>>> we can move both calls into the helper? I mean, fmt_fourcc is not used >>>>> at all if the CDM is not used. >>>>> >>>> >>>> fourcc is always used to get the fmt to setup the timing engine params. >>>> Its just that it was always hard-coded to RGB. With CDM, it can change >>>> based on the mode. Thats why this utility was introduced to return the >>>> fourcc from the mode information. >>> >>> Yes, I perfectly understand that. I just disliked the idea of calling >>> dpu_get_dpu_format() if the result gets unused. >>> What about passing fourcc + modifier to >>> dpu_encoder_helper_phys_setup_cdm() instead? >>> >> >> Why would it be unused? Its returning YUV420 if the mode is YUV420 and >> RGB otherwise. > > I thought about it being unused if there is no CDM in play. > >> >> We need fourcc + modifier for video mode timing engine setup. I didnt >> quite follow your suggestion of passing these to >> dpu_encoder_helper_phys_setup_cdm(). > > Ah, this is modified in the next patch... > > Maybe passing fmt_fourcc from this function to setup_timing_engine > will solve my troubles. > Sorry but isnt that what the next patch is doing? dpu_encoder_phys_vid_setup_timing_engine() gets the fourcc from dpu_encoder_get_drm_fmt() now. There should be no link between setup_cdm() and setup_timing_engine(). What we have right now makes looks fine. >> >> >>>> >>>>>> >>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>> >>>>>> @@ -423,6 +430,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); >>>>>> >>>>>> /* >>>>>> @@ -438,6 +447,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 >>>>>> >>>>> >>>>> >>> >>> >>> > > >
On Tue, 13 Feb 2024 at 23:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/13/2024 11:31 AM, Dmitry Baryshkov wrote: > > On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: > >>> On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: > >>>>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> > >>>>>> --- > >>>>>> 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 && > >>>>> > >>>>> Do we really need to check for phys_enc->hw_cdm here? > >>>>> > >>>> > >>>> hmmm I dont think so. If CDM was not there, then after the last patch, > >>>> YUV420 removes will not be present at all. > >>>> > >>>> The only other case I could think of was, if for some reason CDM was > >>>> used by some other interface such as WB, then hw_cdm will not be assigned. > >>>> > >>>> But, I think even for that msm_dp_needs_periph_flush() will take care of > >>>> it because we use the cached_mode which is assigned only in mode_set(). > >>>> > >>>>>> + 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 f562beb6f7971..3f102b2813ca8 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 > >>>>>> @@ -413,8 +413,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); > >>>>> > >>>>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > >>>>> > >>>> > >>>> I think we dont need to check for if (hw_cdm) at all. > >>>> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns > >>>> the fourcc code based on the mode. It doesnt need to know if there is > >>>> cdm or not. > >>>> > >>>> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for > >>>> WB, we do not change the fourcc based on the mode. Its specific to video > >>>> mode. > >>> > >>> Wait... In the case of the WB we use the fourcc + modifier from the > >>> framebuffer. Ok, this looks fine then. > >>> > >>> > >>>> > >>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>>> > >>>>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe > >>>>> we can move both calls into the helper? I mean, fmt_fourcc is not used > >>>>> at all if the CDM is not used. > >>>>> > >>>> > >>>> fourcc is always used to get the fmt to setup the timing engine params. > >>>> Its just that it was always hard-coded to RGB. With CDM, it can change > >>>> based on the mode. Thats why this utility was introduced to return the > >>>> fourcc from the mode information. > >>> > >>> Yes, I perfectly understand that. I just disliked the idea of calling > >>> dpu_get_dpu_format() if the result gets unused. > >>> What about passing fourcc + modifier to > >>> dpu_encoder_helper_phys_setup_cdm() instead? > >>> > >> > >> Why would it be unused? Its returning YUV420 if the mode is YUV420 and > >> RGB otherwise. > > > > I thought about it being unused if there is no CDM in play. > > > >> > >> We need fourcc + modifier for video mode timing engine setup. I didnt > >> quite follow your suggestion of passing these to > >> dpu_encoder_helper_phys_setup_cdm(). > > > > Ah, this is modified in the next patch... > > > > Maybe passing fmt_fourcc from this function to setup_timing_engine > > will solve my troubles. > > > > Sorry but isnt that what the next patch is doing? I thought about passing it from dpu_encoder_phys_vid_enable(). Unless I miss something. > > dpu_encoder_phys_vid_setup_timing_engine() gets the fourcc from > dpu_encoder_get_drm_fmt() now. > > There should be no link between setup_cdm() and setup_timing_engine(). > > What we have right now makes looks fine. > > >> > >> > >>>> > >>>>>> > >>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>>>> > >>>>>> @@ -423,6 +430,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); > >>>>>> > >>>>>> /* > >>>>>> @@ -438,6 +447,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 > >>>>>> > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
On 2/13/2024 1:16 PM, Dmitry Baryshkov wrote: > On Tue, 13 Feb 2024 at 23:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 2/13/2024 11:31 AM, Dmitry Baryshkov wrote: >>> On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: >>>>> On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: >>>>>>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> >>>>>>>> --- >>>>>>>> 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 && >>>>>>> >>>>>>> Do we really need to check for phys_enc->hw_cdm here? >>>>>>> >>>>>> >>>>>> hmmm I dont think so. If CDM was not there, then after the last patch, >>>>>> YUV420 removes will not be present at all. >>>>>> >>>>>> The only other case I could think of was, if for some reason CDM was >>>>>> used by some other interface such as WB, then hw_cdm will not be assigned. >>>>>> >>>>>> But, I think even for that msm_dp_needs_periph_flush() will take care of >>>>>> it because we use the cached_mode which is assigned only in mode_set(). >>>>>> >>>>>>>> + 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 f562beb6f7971..3f102b2813ca8 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 >>>>>>>> @@ -413,8 +413,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); >>>>>>> >>>>>>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). >>>>>>> >>>>>> >>>>>> I think we dont need to check for if (hw_cdm) at all. >>>>>> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns >>>>>> the fourcc code based on the mode. It doesnt need to know if there is >>>>>> cdm or not. >>>>>> >>>>>> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for >>>>>> WB, we do not change the fourcc based on the mode. Its specific to video >>>>>> mode. >>>>> >>>>> Wait... In the case of the WB we use the fourcc + modifier from the >>>>> framebuffer. Ok, this looks fine then. >>>>> >>>>> >>>>>> >>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>>>> >>>>>>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe >>>>>>> we can move both calls into the helper? I mean, fmt_fourcc is not used >>>>>>> at all if the CDM is not used. >>>>>>> >>>>>> >>>>>> fourcc is always used to get the fmt to setup the timing engine params. >>>>>> Its just that it was always hard-coded to RGB. With CDM, it can change >>>>>> based on the mode. Thats why this utility was introduced to return the >>>>>> fourcc from the mode information. >>>>> >>>>> Yes, I perfectly understand that. I just disliked the idea of calling >>>>> dpu_get_dpu_format() if the result gets unused. >>>>> What about passing fourcc + modifier to >>>>> dpu_encoder_helper_phys_setup_cdm() instead? >>>>> >>>> >>>> Why would it be unused? Its returning YUV420 if the mode is YUV420 and >>>> RGB otherwise. >>> >>> I thought about it being unused if there is no CDM in play. >>> >>>> >>>> We need fourcc + modifier for video mode timing engine setup. I didnt >>>> quite follow your suggestion of passing these to >>>> dpu_encoder_helper_phys_setup_cdm(). >>> >>> Ah, this is modified in the next patch... >>> >>> Maybe passing fmt_fourcc from this function to setup_timing_engine >>> will solve my troubles. >>> >> >> Sorry but isnt that what the next patch is doing? > > I thought about passing it from dpu_encoder_phys_vid_enable(). Unless > I miss something. > So you want to pass fourcc_fmt to dpu_encoder_phys_vid_enable()? But then the dpu_encoder_phys_ops->enable() syntax will need to be changed to accept the fourcc but that is not needed for WB. So I dont think its optimal. I think you can post your suggested idea as a cleanup on top of the feature and we can review it that time. I think this one is fine as it is. >> >> dpu_encoder_phys_vid_setup_timing_engine() gets the fourcc from >> dpu_encoder_get_drm_fmt() now. >> >> There should be no link between setup_cdm() and setup_timing_engine(). >> >> What we have right now makes looks fine. >> >>>> >>>> >>>>>> >>>>>>>> >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>>>> >>>>>>>> @@ -423,6 +430,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); >>>>>>>> >>>>>>>> /* >>>>>>>> @@ -438,6 +447,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 >>>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
On Wed, 14 Feb 2024 at 00:11, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/13/2024 1:16 PM, Dmitry Baryshkov wrote: > > On Tue, 13 Feb 2024 at 23:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/13/2024 11:31 AM, Dmitry Baryshkov wrote: > >>> On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote: > >>>>> On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote: > >>>>>>> On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@quicinc.com> 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@quicinc.com> > >>>>>>>> --- > >>>>>>>> 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 && > >>>>>>> > >>>>>>> Do we really need to check for phys_enc->hw_cdm here? > >>>>>>> > >>>>>> > >>>>>> hmmm I dont think so. If CDM was not there, then after the last patch, > >>>>>> YUV420 removes will not be present at all. > >>>>>> > >>>>>> The only other case I could think of was, if for some reason CDM was > >>>>>> used by some other interface such as WB, then hw_cdm will not be assigned. > >>>>>> > >>>>>> But, I think even for that msm_dp_needs_periph_flush() will take care of > >>>>>> it because we use the cached_mode which is assigned only in mode_set(). > >>>>>> > >>>>>>>> + 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 f562beb6f7971..3f102b2813ca8 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 > >>>>>>>> @@ -413,8 +413,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); > >>>>>>> > >>>>>>> Please move if(hw_cdm) inside dpu_encoder_get_drm_fmt(). > >>>>>>> > >>>>>> > >>>>>> I think we dont need to check for if (hw_cdm) at all. > >>>>>> dpu_encoder_get_drm_fmt() was just supposed to be a helper which returns > >>>>>> the fourcc code based on the mode. It doesnt need to know if there is > >>>>>> cdm or not. > >>>>>> > >>>>>> We cannot move it inside dpu_encoder_helper_phys_setup_cdm() because for > >>>>>> WB, we do not change the fourcc based on the mode. Its specific to video > >>>>>> mode. > >>>>> > >>>>> Wait... In the case of the WB we use the fourcc + modifier from the > >>>>> framebuffer. Ok, this looks fine then. > >>>>> > >>>>> > >>>>>> > >>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>>>>> > >>>>>>> Can this be moved into dpu_encoder_helper_phys_setup_cdm() ? Or maybe > >>>>>>> we can move both calls into the helper? I mean, fmt_fourcc is not used > >>>>>>> at all if the CDM is not used. > >>>>>>> > >>>>>> > >>>>>> fourcc is always used to get the fmt to setup the timing engine params. > >>>>>> Its just that it was always hard-coded to RGB. With CDM, it can change > >>>>>> based on the mode. Thats why this utility was introduced to return the > >>>>>> fourcc from the mode information. > >>>>> > >>>>> Yes, I perfectly understand that. I just disliked the idea of calling > >>>>> dpu_get_dpu_format() if the result gets unused. > >>>>> What about passing fourcc + modifier to > >>>>> dpu_encoder_helper_phys_setup_cdm() instead? > >>>>> > >>>> > >>>> Why would it be unused? Its returning YUV420 if the mode is YUV420 and > >>>> RGB otherwise. > >>> > >>> I thought about it being unused if there is no CDM in play. > >>> > >>>> > >>>> We need fourcc + modifier for video mode timing engine setup. I didnt > >>>> quite follow your suggestion of passing these to > >>>> dpu_encoder_helper_phys_setup_cdm(). > >>> > >>> Ah, this is modified in the next patch... > >>> > >>> Maybe passing fmt_fourcc from this function to setup_timing_engine > >>> will solve my troubles. > >>> > >> > >> Sorry but isnt that what the next patch is doing? > > > > I thought about passing it from dpu_encoder_phys_vid_enable(). Unless > > I miss something. > > > > So you want to pass fourcc_fmt to dpu_encoder_phys_vid_enable()? > > But then the dpu_encoder_phys_ops->enable() syntax will need to be > changed to accept the fourcc but that is not needed for WB. > > So I dont think its optimal. > > I think you can post your suggested idea as a cleanup on top of the > feature and we can review it that time. I think this one is fine as it is. Ack, let's do it this way. > > >> > >> dpu_encoder_phys_vid_setup_timing_engine() gets the fourcc from > >> dpu_encoder_get_drm_fmt() now. > >> > >> There should be no link between setup_cdm() and setup_timing_engine(). > >> > >> What we have right now makes looks fine. > >> > >>>> > >>>> > >>>>>> > >>>>>>>> > >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>>>>>> > >>>>>>>> @@ -423,6 +430,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); > >>>>>>>> > >>>>>>>> /* > >>>>>>>> @@ -438,6 +447,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 > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
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 f562beb6f7971..3f102b2813ca8 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 @@ -413,8 +413,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); + fmt = dpu_get_dpu_format(fmt_fourcc); DPU_DEBUG_VIDENC(phys_enc, "\n"); @@ -423,6 +430,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); /* @@ -438,6 +447,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;
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@quicinc.com> --- 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(-)