[PATCH v2 16/19] drm/msm/dpu: modify encoder programming for CDM over DP
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Feb 13 18:45:56 UTC 2024
On 2/13/2024 10:23 AM, Dmitry Baryshkov wrote:
> On Tue, 13 Feb 2024 at 19:32, Abhinav Kumar <quic_abhinavk at 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 at 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 at 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
>>>>
>>>
>>>
>
>
>
More information about the Freedreno
mailing list