On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhin...@quicinc.com> wrote:
>
>
>
> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote:
> > On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parel...@quicinc.com>
> > wrote:
> >>
> >>
> >> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote:
> >>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>> Adjust the encoder format programming in the case of video mode for DP
> >>>> to accommodate CDM related changes.
> >>>>
> >>>> Signed-off-by: Paloma Arellano <quic_parel...@quicinc.com>
> >>>> ---
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++
> >>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++---
> >>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++
> >>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++-
> >>>> 5 files changed, 75 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> index b0896814c1562..99ec53446ad21 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> >>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> >>>> };
> >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
> >>>> const struct drm_display_mode *mode)
> >>>> +{
> >>>> + const struct dpu_encoder_virt *dpu_enc;
> >>>> + const struct msm_display_info *disp_info;
> >>>> + struct msm_drm_private *priv;
> >>>> +
> >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> >>>> + disp_info = &dpu_enc->disp_info;
> >>>> + priv = drm_enc->dev->dev_private;
> >>>> +
> >>>> + if (disp_info->intf_type == INTF_DP &&
> >>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]],
> >>>> mode))
> >>>
> >>> This should not require interacting with DP. If we got here, we must
> >>> be sure that 4:2:0 is supported and can be configured.
> >> Ack. Will drop this function and only check for if the mode is YUV420.
> >>>
> >>>> + return DRM_FORMAT_YUV420;
> >>>> +
> >>>> + return DRM_FORMAT_RGB888;
> >>>> +}
> >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> >>>> *drm_enc)
> >>>> {
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >>>> index 7b4afa71f1f96..62255d0aa4487 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> >>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct
> >>>> drm_encoder *drm_enc);
> >>>> */
> >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder
> >>>> *drm_enc);
> >>>> +/**
> >>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format
> >>>> + * @drm_enc: Pointer to previously created drm encoder structure
> >>>> + * @mode: Corresponding drm_display_mode for dpu encoder
> >>>> + */
> >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc,
> >>>> + const struct drm_display_mode *mode);
> >>>> +
> >>>> /**
> >>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders
> >>>> contained
> >>>> * in virtual encoder that can collect CRC values
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> >>>> index e284bf448bdda..a1dde0ff35dc8 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> >>>> @@ -234,6 +234,7 @@ static void
> >>>> dpu_encoder_phys_vid_setup_timing_engine(
> >>>> {
> >>>> struct drm_display_mode mode;
> >>>> struct dpu_hw_intf_timing_params timing_params = { 0 };
> >>>> + struct dpu_hw_cdm *hw_cdm;
> >>>> const struct dpu_format *fmt = NULL;
> >>>> u32 fmt_fourcc = DRM_FORMAT_RGB888;
> >>>> unsigned long lock_flags;
> >>>> @@ -254,17 +255,26 @@ static void
> >>>> dpu_encoder_phys_vid_setup_timing_engine(
> >>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n");
> >>>> drm_mode_debug_printmodeline(&mode);
> >>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) {
> >>>> + hw_cdm = phys_enc->hw_cdm;
> >>>> + if (hw_cdm) {
> >>>> + intf_cfg.cdm = hw_cdm->idx;
> >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode);
> >>>> + }
> >>>> +
> >>>> + if (phys_enc->split_role != ENC_ROLE_SOLO ||
> >>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) ==
> >>>> DRM_FORMAT_YUV420) {
> >>>> mode.hdisplay >>= 1;
> >>>> mode.htotal >>= 1;
> >>>> mode.hsync_start >>= 1;
> >>>> mode.hsync_end >>= 1;
> >>>> + mode.hskew >>= 1;
> >>>
> >>> Separate patch.
> >> Ack.
> >>>
> >>>> DPU_DEBUG_VIDENC(phys_enc,
> >>>> - "split_role %d, halve horizontal %d %d %d %d\n",
> >>>> + "split_role %d, halve horizontal %d %d %d %d %d\n",
> >>>> phys_enc->split_role,
> >>>> mode.hdisplay, mode.htotal,
> >>>> - mode.hsync_start, mode.hsync_end);
> >>>> + mode.hsync_start, mode.hsync_end,
> >>>> + mode.hskew);
> >>>> }
> >>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params);
> >>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq(
> >>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys
> >>>> *phys_enc)
> >>>> {
> >>>> struct dpu_hw_ctl *ctl;
> >>>> + struct dpu_hw_cdm *hw_cdm;
> >>>> + const struct dpu_format *fmt = NULL;
> >>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888;
> >>>> ctl = phys_enc->hw_ctl;
> >>>> + hw_cdm = phys_enc->hw_cdm;
> >>>> + if (hw_cdm)
> >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent,
> >>>> &phys_enc->cached_mode);
> >>>> + fmt = dpu_get_dpu_format(fmt_fourcc);
> >>>> DPU_DEBUG_VIDENC(phys_enc, "\n");
> >>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct
> >>>> dpu_encoder_phys *phys_enc)
> >>>> dpu_encoder_helper_split_config(phys_enc,
> >>>> phys_enc->hw_intf->idx);
> >>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt,
> >>>> CDM_CDWN_OUTPUT_HDMI);
> >>>
> >>> If there is no CDM, why do we need to call this?
> >> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if
> >> there is a hw_cdm. If there is not, then it immediately exits the function.
> >>>
> >>>> +
> >>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc);
> >>>> /*
> >>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct
> >>>> dpu_encoder_phys *phys_enc)
> >>>> if (ctl->ops.update_pending_flush_merge_3d &&
> >>>> phys_enc->hw_pp->merge_3d)
> >>>> ctl->ops.update_pending_flush_merge_3d(ctl,
> >>>> phys_enc->hw_pp->merge_3d->idx);
> >>>> - if (ctl->ops.update_pending_flush_periph &&
> >>>> phys_enc->hw_intf->cap->type == INTF_DP)
> >>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm)
> >>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx);
> >>>> +
> >>>> + /*
> >>>> + * Peripheral flush must be updated whenever flushing SDP
> >>>> packets is needed.
> >>>> + * SDP packets are required for any YUV format (YUV420, YUV422,
> >>>> YUV444).
> >>>> + */
> >>>> + if (ctl->ops.update_pending_flush_periph &&
> >>>> phys_enc->hw_intf->cap->type == INTF_DP &&
> >>>> + phys_enc->hw_cdm)
> >>>> ctl->ops.update_pending_flush_periph(ctl,
> >>>> phys_enc->hw_intf->idx);
> >>>
> >>> Should there be a flush if we are switching from YUV 420 to RGB mode?
> >> We only need to flush for the sdp packet, but for msa we do not need to
> >> flush.
> >
> > What about having SDP with RGB as colorimetry? In other words, if
> > there is a decision point, this one looks incorrect.
> >
>
> 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()?
>
> >>>
> >>> Also, I'd say, we should move update_pending_flush_periph invocation
> >>> to this patch.
> >> Ack
> >>>
> >>>> skip_flush:
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index 6d764f5b08727..4329435518351 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void)
> >>>> platform_driver_unregister(&dp_display_driver);
> >>>> }
> >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
> >>>> + const struct drm_display_mode *mode)
> >>>> +{
> >>>> + struct dp_display_private *dp;
> >>>> + const struct drm_display_info *info;
> >>>> +
> >>>> + dp = container_of(dp_display, struct dp_display_private,
> >>>> dp_display);
> >>>> + info = &dp_display->connector->display_info;
> >>>> +
> >>>> + return dp_panel_vsc_sdp_supported(dp->panel) &&
> >>>> drm_mode_is_420_only(info, mode);
> >>>
> >>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not
> >>> supported.
> >> Ack. Will change
> >>>
> >>>> +}
> >>>> +
> >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
> >>>> {
> >>>> struct dp_display_private *dp;
> >>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
> >>>> b/drivers/gpu/drm/msm/msm_drv.h
> >>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644
> >>>> --- a/drivers/gpu/drm/msm/msm_drv.h
> >>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
> >>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void);
> >>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct
> >>>> drm_device *dev,
> >>>> struct drm_encoder *encoder);
> >>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
> >>>> msm_dp *dp_display);
> >>>> -
> >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
> >>>> + const struct drm_display_mode *mode);
> >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
> >>>> #else
> >>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct
> >>>> msm_disp_state *disp_state, struct msm
> >>>> {
> >>>> }
> >>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp
> >>>> *dp_display,
> >>>> + const struct drm_display_mode *mode)
> >>>> +{
> >>>> + return false;
> >>>> +}
> >>>> +
> >>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp
> >>>> *dp_display)
> >>>> {
> >>>> return false;
> >>>
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
