On 1/31/2024 7:17 PM, Dmitry Baryshkov wrote:
On Thu, 1 Feb 2024 at 03:30, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote:
On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@xxxxxxxxxxx> wrote:
On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote:
On 25/01/2024 21:38, Paloma Arellano wrote:
Adjust the encoder format programming in the case of video mode for DP
to accommodate CDM related changes.
Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++---
drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++
drivers/gpu/drm/msm/msm_drv.h | 9 ++++-
5 files changed, 75 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b0896814c1562..99ec53446ad21 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
const struct drm_display_mode *mode)
+{
+ const struct dpu_encoder_virt *dpu_enc;
+ const struct msm_display_info *disp_info;
+ struct msm_drm_private *priv;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ disp_info = &dpu_enc->disp_info;
+ priv = drm_enc->dev->dev_private;
+
+ if (disp_info->intf_type == INTF_DP &&
+ msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]],
mode))
This should not require interacting with DP. If we got here, we must
be sure that 4:2:0 is supported and can be configured.
Ack. Will drop this function and only check for if the mode is YUV420.
+ return DRM_FORMAT_YUV420;
+
+ return DRM_FORMAT_RGB888;
+}
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
*drm_enc)
{
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 7b4afa71f1f96..62255d0aa4487 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct
drm_encoder *drm_enc);
*/
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
*drm_enc);
+/**
+ * dpu_encoder_get_drm_fmt - return DRM fourcc format
+ * @drm_enc: Pointer to previously created drm encoder structure
+ * @mode: Corresponding drm_display_mode for dpu encoder
+ */
+u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
+ const struct drm_display_mode *mode);
+
/**
* dpu_encoder_get_crc_values_cnt - get number of physical encoders
contained
* in virtual encoder that can collect CRC values
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index e284bf448bdda..a1dde0ff35dc8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -234,6 +234,7 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
{
struct drm_display_mode mode;
struct dpu_hw_intf_timing_params timing_params = { 0 };
+ struct dpu_hw_cdm *hw_cdm;
const struct dpu_format *fmt = NULL;
u32 fmt_fourcc = DRM_FORMAT_RGB888;
unsigned long lock_flags;
@@ -254,17 +255,26 @@ static void
dpu_encoder_phys_vid_setup_timing_engine(
DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n");
drm_mode_debug_printmodeline(&mode);
- if (phys_enc->split_role != ENC_ROLE_SOLO) {
+ hw_cdm = phys_enc->hw_cdm;
+ if (hw_cdm) {
+ intf_cfg.cdm = hw_cdm->idx;
+ fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode);
+ }
+
+ if (phys_enc->split_role != ENC_ROLE_SOLO ||
+ dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) ==
DRM_FORMAT_YUV420) {
mode.hdisplay >>= 1;
mode.htotal >>= 1;
mode.hsync_start >>= 1;
mode.hsync_end >>= 1;
+ mode.hskew >>= 1;
Separate patch.
Ack.
DPU_DEBUG_VIDENC(phys_enc,
- "split_role %d, halve horizontal %d %d %d %d\n",
+ "split_role %d, halve horizontal %d %d %d %d %d\n",
phys_enc->split_role,
mode.hdisplay, mode.htotal,
- mode.hsync_start, mode.hsync_end);
+ mode.hsync_start, mode.hsync_end,
+ mode.hskew);
}
drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params);
@@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq(
static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys
*phys_enc)
{
struct dpu_hw_ctl *ctl;
+ struct dpu_hw_cdm *hw_cdm;
+ const struct dpu_format *fmt = NULL;
+ u32 fmt_fourcc = DRM_FORMAT_RGB888;
ctl = phys_enc->hw_ctl;
+ hw_cdm = phys_enc->hw_cdm;
+ if (hw_cdm)
+ fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent,
&phys_enc->cached_mode);
+ fmt = dpu_get_dpu_format(fmt_fourcc);
DPU_DEBUG_VIDENC(phys_enc, "\n");
@@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct
dpu_encoder_phys *phys_enc)
dpu_encoder_helper_split_config(phys_enc,
phys_enc->hw_intf->idx);
+ dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt,
CDM_CDWN_OUTPUT_HDMI);
If there is no CDM, why do we need to call this?
Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if
there is a hw_cdm. If there is not, then it immediately exits the function.
+
dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
/*
@@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct
dpu_encoder_phys *phys_enc)
if (ctl->ops.update_pending_flush_merge_3d &&
phys_enc->hw_pp->merge_3d)
ctl->ops.update_pending_flush_merge_3d(ctl,
phys_enc->hw_pp->merge_3d->idx);
- if (ctl->ops.update_pending_flush_periph &&
phys_enc->hw_intf->cap->type == INTF_DP)
+ if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm)
+ ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx);
+
+ /*
+ * Peripheral flush must be updated whenever flushing SDP
packets is needed.
+ * SDP packets are required for any YUV format (YUV420, YUV422,
YUV444).
+ */
+ if (ctl->ops.update_pending_flush_periph &&
phys_enc->hw_intf->cap->type == INTF_DP &&
+ phys_enc->hw_cdm)
ctl->ops.update_pending_flush_periph(ctl,
phys_enc->hw_intf->idx);
Should there be a flush if we are switching from YUV 420 to RGB mode?
We only need to flush for the sdp packet, but for msa we do not need to
flush.
What about having SDP with RGB as colorimetry? In other words, if
there is a decision point, this one looks incorrect.
There are two ways to do it:
1) Use SDP for both RGB and YUV as that supports both. If we implement
this policy, then what you are asking for is correct that we will need
SDP even to switch back to RGB. But to implement this we will also need
to have some sort of state management in the encoder layer about what is
the current encoder fmt Vs what is the prev fmt and then trigger
peripheral flush only during transitions from RGB to YUV and vice-versa
2) Use SDP only for YUV because MSA does not support YUV formats and use
MSA for RGB
We decided to implement (2) and there is no significant impact of
switching between MSA and SDPs but state management becomes easier.
Yes. However as you wrote, there might be other usecases concerning
SDP. Having this in mind, it sounds like the driver should decide
whether to flush peripheral at a different place (when the SDP
infoframe is being updated?). And the dpu_encoder_phys_vid_enable()
should use this previous decision. Maybe this should be a part of
msm_dp_ API, something like msm_dp_needs_peripheral_flush()?
Correct. The decision to flush peripheral or not certainly comes from
the peripheral itself . In this case its DP.
I think perhaps the usage of hw_cdm here makes it hard to understand
that but the idea behind this was that in the change "drm/msm/dpu:
reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only
if msm_dp_is_yuv_420_enabled() returns true.
Yes.
So in some sense, the API you are asking for is already
msm_dp_is_yuv_420_enabled() but we can rename that to
msm_dp_needs_periph_flush().
No. This leaks details. We might need peripheral flush for other
reasons. So this decision should be made inside the DP driver.
BTW, is there a need to flush-peripheral each time the INTF gets
enabled? When some bits of configuration were updated (like SDP
infoframe or CDM config)?
Not really leaking any details. Interface decides when DPU's flush bit
needs to be invoked. All the conditions needing the flush can be
absorbed within msm_dp_needs_periph_flush() which will be within DP
driver. So I dont see any issue with that.
As long as we have two distinct functions: msm_dp_needs_periph_flush()
and msm_dp_is_yuv420_enabled(), it's fine with me.
I am okay with it being distinct but in this series and till DP DSC is
posted, we have only YUV420 SDP use-case for peripheral flush.
So are you okay with the fact that the code will look something like
bool msm_dp_needs_periph_flush() {
return msm_dp_is_yuv420_enabled();
}
Yes. The reason is that if at some point conditions for periph flush
get changed, you won't have to change the API between DPU and DP
drivers.
Ack, I am okay with this.
The peripheral flush needs to be invoked whenever SDP/PPS packets need
to be sent out at the very least. Those are the two main use-cases so far.
Not necessarily each time the INTF is enabled.
But ....
For PPS, we would typically enable DSC from the first frame itself and
we dont support dynamically turning DSC ON/OFF so its a fair assumption
that we need to the peripheral flush for DSC only during enable.
For SDP, so today we enforce a modeset when we switch from a case of
needing cdm / not needing cdm and vice-versa. So even this use-case
should be accompanied by a disable()/enable().
My point was that there might be other changes to the SDP infoframe.
So are you suggesting that we unconditionally just do peripheral_flush
from enable()?
I need to do some checking but maybe ....
This might be an option too. I don't know what delays / issues it can cause.
I will check this if the other option we discussed above does not work
for you.
The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled()
so its kind of round-about by checking hw_cdm.
Which maybe means that I should finish phys / encoder rework.
Ugh no. Not another rework just for this.
It is pending for the CTL flush story.
The check is not entirely wrong because for DP, we need hw_cdm for all
YUV formats and not just 420 but the relationship is maybe not clear but
I thought the comment above that would help a bit:
/*
* Peripheral flush must be updated whenever flushing SDP packets is
needed.
* SDP packets are required for any YUV format (YUV420, YUV422, YUV444).
*/
The only other way I can think of is maybe we need to introduce a new
phys variable called phys_enc->needs_update_periph_flush and we can set
that in dpu_encoder.c as that can call msm_dp_needs_periph_flush().
What do you think of this way?
Let me understand first the requirements for peripheral flush (see the
questions above).
