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