[PATCH 2/7] drm/msm/dpu: simplify intf allocation code
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Sat Feb 12 00:23:32 UTC 2022
On 12/02/2022 03:17, Abhinav Kumar wrote:
>
>
> On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
>> On 12/02/2022 02:38, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>>>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to
>>>> guess,
>>>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>>>
>>>
>>> Typically, I am only seeing a 1:1 mapping like
>>>
>>> DRM_MODE_ENCODER_DSI means DSI
>>> DRM_MODE_ENCODER_VIRTUAL means WB
>>>
>>> So I am not seeing any guessing for the encoder.
>>
>> s/guessing/deriving/
>>
>> Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison,
>> then I noticed that there is a misnaming, we were talking about the
>> intf_type (which clearly belongs to INTF_foo namespace), while passing
>> DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type
>> actually contain INTF_type and let DRM_ENCODER live in encoder's code.
>>
>>
>>>
>>> The only conflict I am seeing is between DP and EDP as both use
>>> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
>>>
>>> But that has been marked as a "FIXME" below.
>>>
>>> I am suggesting an approach to handle that as well below.
>>> Let me know if you agree with that.
>>
>> Actually this brings a question to me. Do we need to distinguish
>> between INTF_DP and INTF_EDP from the DPU driver point of view? Are
>> there any differences? Or we'd better always use INTF_DP here and make
>> INTF_EDP a memorial of old eDP IP found in 8x74/8x84?
>>
>> So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but
>> declares it as INTF_DP. Most probably we should stick to this idea and
>> drop INTF_EDP from dpu1?
>
> I believe you are referring to this part:
>
> static const struct dpu_intf_cfg sc7280_intf[] = {
> INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0,
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1,
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> };
Yep, this one.
>
> This is interesting ... this should have been marked as INTF_eDP unless
> I am missing some reason why.
>
> My take on this is that, since are re-using DP driver now for DP and
> eDP, less confusion the better in terms of naming of DP/eDP.
>
> I think we should fix that in the catalog to INTF_eDP. And in case some
> place is missed out due to this change fix that too.
Well, we need to fix that if there is a difference from the DPU point of
view (not from the DP IP block). If there is no difference between
INTF_0 and INTF_5, it might be easier to just use INTF_DP for both of them.
In case we change it, your suggestion seems fine to me, I'll include it
in v2.
>
> Meanwhile I can check with sankeerth if there was any specific reason
> for not using INTF_eDP in the original commit:
>
> commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
> Author: Sankeerth Billakanti <quic_sbillaka at quicinc.com>
> Date: Tue Nov 2 13:18:42 2021 +0530
>
> drm/msm/dp: Add DP controllers for sc7280
>
> The eDP controller on SC7280 is similar to the eDP/DP controllers
> supported by the current driver implementation.
>
> SC7280 supports one EDP and one DP controller which can operate
> concurrently.
>
> This change adds the support for eDP and DP controller on sc7280.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka at quicinc.com>
>
>>
>>>
>>>
>>>> While we are at it, fix the DP audio enablement code which was
>>>> comparing
>>>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>>>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>>>> Which would never succeed.
>>>
>>> This is a surprising catch for me and left me thinking for a while
>>> about how DP audio is working with this bug because that piece of
>>> code was done to program a register which is needed for DP audio.
>>
>> So did I!
>>
>>>
>>> This bug happened due to difference in the meaning of intf_type between
>>> upstream and downstream.
>>>
>>> After checking more, we found that the register in question has been
>>> deprecated on newer chipsets so I have asked Kuogee to selectively
>>> program it. Here is the change for that:
>>>
>>> https://patchwork.freedesktop.org/patch/473869/
>>
>> I'll further comment on it in the respective thread.
>>
>>>
>>>>
>>>
>>>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port
>>>> on MSM")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28
>>>> +++++++--------------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 +--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +--
>>>> 3 files changed, 13 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 1e648db439f9..e8fc029ad607 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>>> hw_mdptop = phys_enc->hw_mdptop;
>>>> disp_info = &dpu_enc->disp_info;
>>>> - if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>>>> + if (disp_info->intf_type != INTF_DSI)
>>>> return;
>>>> /**
>>>> @@ -555,7 +555,7 @@ static struct msm_display_topology
>>>> dpu_encoder_get_topology(
>>>> else
>>>> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ?
>>>> 2 : 1;
>>>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>>> + if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>>> if (dpu_kms->catalog->dspp &&
>>>> (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>>> topology.num_dspp = topology.num_lm;
>>>> @@ -1099,7 +1099,7 @@ static void
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>> }
>>>> - if (dpu_enc->disp_info.intf_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort &&
>>>> + if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>>> dpu_enc->cur_master->hw_mdptop &&
>>>> dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>>> dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>>>> @@ -1107,7 +1107,7 @@ static void
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>> _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>>>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>>>> + if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>>> !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>>> unsigned bpc =
>>>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>>> for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct
>>>> dpu_encoder_virt *dpu_enc,
>>>> {
>>>> int ret = 0;
>>>> int i = 0;
>>>> - enum dpu_intf_type intf_type = INTF_NONE;
>>>> struct dpu_enc_phys_init_params phys_params;
>>>> if (!dpu_enc) {
>>>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct
>>>> dpu_encoder_virt *dpu_enc,
>>>> phys_params.parent_ops = &dpu_encoder_parent_ops;
>>>> phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>>>> - switch (disp_info->intf_type) {
>>>> - case DRM_MODE_ENCODER_DSI:
>>>> - intf_type = INTF_DSI;
>>>> - break;
>>>> - case DRM_MODE_ENCODER_TMDS:
>>>> - intf_type = INTF_DP;
>>>> - break;
>>>> - }
>>>> -
>>>> WARN_ON(disp_info->num_of_h_tiles < 1);
>>>> DPU_DEBUG("dsi_info->num_of_h_tiles %d\n",
>>>> disp_info->num_of_h_tiles);
>>>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct
>>>> dpu_encoder_virt *dpu_enc,
>>>> i, controller_id, phys_params.split_role);
>>>> phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>>> - intf_type,
>>>> - controller_id);
>>>> + disp_info->intf_type,
>>>> + controller_id);
>>>> if (phys_params.intf_idx == INTF_MAX) {
>>>> DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d,
>>>> id %d\n",
>>>> - intf_type, controller_id);
>>>> + disp_info->intf_type, controller_id);
>>>> ret = -EINVAL;
>>>> }
>>>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device
>>>> *dev, struct drm_encoder *enc,
>>>> timer_setup(&dpu_enc->frame_done_timer,
>>>> dpu_encoder_frame_done_timeout, 0);
>>>> - if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>>>> + if (disp_info->intf_type == INTF_DSI)
>>>> timer_setup(&dpu_enc->vsync_event_timer,
>>>> dpu_encoder_vsync_event_handler,
>>>> 0);
>>>> - else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>> + else if (disp_info->intf_type == INTF_DP ||
>>>> disp_info->intf_type == INTF_EDP)
>>>> dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index ebe3944355bb..3891bcbbe5a4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct
>>>> drm_encoder *encoder,
>>>> /**
>>>> * struct msm_display_info - defines display properties
>>>> - * @intf_type: DRM_MODE_ENCODER_ type
>>>> + * @intf_type: INTF_ type
>>>> * @capabilities: Bitmask of display flags
>>>> * @num_of_h_tiles: Number of horizontal tiles in case of
>>>> split interface
>>>> * @h_tile_instance: Controller instance used per tile. Number
>>>> of elements is
>>>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct
>>>> drm_encoder *encoder,
>>>> * used instead of panel TE in cmd mode panels
>>>> */
>>>> struct msm_display_info {
>>>> - int intf_type;
>>>> + enum dpu_intf_type intf_type;
>>>> uint32_t capabilities;
>>>> uint32_t num_of_h_tiles;
>>>> uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 47fe11a84a77..f4028be9e2e2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct
>>>> drm_device *dev,
>>>> priv->encoders[priv->num_encoders++] = encoder;
>>>> memset(&info, 0, sizeof(info));
>>>> - info.intf_type = encoder->encoder_type;
>>>> + info.intf_type = INTF_DSI;
>>>> rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>> if (rc) {
>>>> @@ -630,7 +630,7 @@ static int
>>>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>> info.num_of_h_tiles = 1;
>>>> info.h_tile_instance[0] = i;
>>>> info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>>> - info.intf_type = encoder->encoder_type;
>>>
>>> You can query the connector type from the DP driver like this:
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 7cc4d21..0fae0fc 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp
>>> *dp_display, struct drm_minor *minor)
>>> }
>>>
>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct
>>> drm_device *dev,
>>> - struct drm_encoder *encoder)
>>> + struct drm_encoder *encoder, int
>>> **connector_type)
>>> {
>>> struct msm_drm_private *priv;
>>> int ret;
>>> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp
>>> *dp_display, struct drm_device *dev,
>>>
>>> priv->bridges[priv->num_bridges++] = dp_display->bridge;
>>>
>>> + *connector_type = dp_display->connector_type;
>>> +
>>> return 0;
>>> }
>>>
>>> Then you can do something like:
>>>
>>> if (connector_type == eDP)
>>> info.intf_type = INTF_eDP;
>>> else if (connector_type == DP)
>>> info.intf_type = INTF_DP;
>>>> + info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>>> rc = dpu_encoder_setup(dev, encoder, &info);
>>>> if (rc) {
>>>> DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list