On 2/13/2024 11:31 AM, Dmitry Baryshkov wrote:
On Tue, 13 Feb 2024 at 20:46, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote:
On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 2/13/2024 3:18 AM, Dmitry Baryshkov wrote:
On Sat, 10 Feb 2024 at 03:53, Paloma Arellano <quic_parellan@xxxxxxxxxxx> wrote:
Adjust the encoder format programming in the case of video mode for DP
to accommodate CDM related changes.
Changes in v2:
- Move timing engine programming to a separate patch from this
one
- Move update_pending_flush_periph() invocation completely to
this patch
- Change the logic of dpu_encoder_get_drm_fmt() so that it only
calls drm_mode_is_420_only() instead of doing additional
unnecessary checks
- Create new functions msm_dp_needs_periph_flush() and it's
supporting function dpu_encoder_needs_periph_flush() to check
if the mode is YUV420 and VSC SDP is enabled before doing a
peripheral flush
Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 +++++++++++++++++++
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 13 +++++++
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 19 ++++++++++
drivers/gpu/drm/msm/dp/dp_display.c | 18 ++++++++++
drivers/gpu/drm/msm/msm_drv.h | 17 ++++++++-
5 files changed, 101 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 7e7796561009a..6280c6be6dca9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -222,6 +222,41 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+u32 dpu_encoder_get_drm_fmt(struct dpu_encoder_phys *phys_enc)
+{
+ struct drm_encoder *drm_enc;
+ struct dpu_encoder_virt *dpu_enc;
+ struct drm_display_info *info;
+ struct drm_display_mode *mode;
+
+ drm_enc = phys_enc->parent;
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ info = &dpu_enc->connector->display_info;
+ mode = &phys_enc->cached_mode;
+
+ if (drm_mode_is_420_only(info, mode))
+ return DRM_FORMAT_YUV420;
+
+ return DRM_FORMAT_RGB888;
+}
+
+bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys *phys_enc)
+{
+ struct drm_encoder *drm_enc;
+ struct dpu_encoder_virt *dpu_enc;
+ struct msm_display_info *disp_info;
+ struct msm_drm_private *priv;
+ struct drm_display_mode *mode;
+
+ drm_enc = phys_enc->parent;
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ disp_info = &dpu_enc->disp_info;
+ priv = drm_enc->dev->dev_private;
+ mode = &phys_enc->cached_mode;
+
+ return phys_enc->hw_intf->cap->type == INTF_DP && phys_enc->hw_cdm &&
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
