From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>
Cc: Stephen Boyd <swboyd@chromium.org>,
David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
freedreno@lists.freedesktop.org
Subject: Re: [PATCH v2 8/8] drm/msm/dpu: simplify intf allocation code
Date: Thu, 17 Feb 2022 04:18:32 +0300 [thread overview]
Message-ID: <342ac115-e200-3bb0-9092-d148ba34cefd@linaro.org> (raw)
In-Reply-To: <ddd2d3b9-2bbe-1255-a3b9-3f5a5aaae514@quicinc.com>
On 15/02/2022 20:51, Abhinav Kumar wrote:
>
>
> On 2/15/2022 6:16 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.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 26 +++++++--------------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 ++--
>> 3 files changed, 13 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index fa1dc088a672..597d40f78d38 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -490,7 +490,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;
>> /**
>> @@ -552,7 +552,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;
>> @@ -1074,7 +1074,7 @@ static void
>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>> }
>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_TMDS &&
>> + 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(
>> @@ -1082,7 +1082,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->connector->display_info.bpc;
>> for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>> @@ -1949,7 +1949,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) {
>> @@ -1965,15 +1964,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);
>> @@ -2005,11 +1995,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;
>> }
>> @@ -2092,7 +2082,7 @@ 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);
>> 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 5f0dc44119c9..bca4f05a5782 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,8 @@ 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;
>> + /* FIXME: HW catalog treats both DP and eDP interfaces as
>> INTF_DP */
>
> I thought we discussed an approach to address this FIXME.
> https://patchwork.freedesktop.org/patch/473871/?series=99651&rev=2
>
> Without addressing that, there is really no need for this change because
> the only confusion between DRM_MODE_ENCODER_*** mapping to intf_type
> mapping comes for eDP and DP as they use the same DRM_MODE_ENCODER_TMDS.
It's not that distinction which triggered me to sketch this patch.
It was more about the code.
We loop over all INTF_DSI (or INTF_DP) interfaces.
Then we deduce that for this intf we should use DRM_MODE_ENCODER_foo.
We pass it to dpu_encoder_setup_display() only to map it back to
INTF_DSI (DP). This makes completely no sense to me.
>
>> + info.intf_type = INTF_DP;
>> rc = dpu_encoder_setup(dev, encoder, &info);
>> if (rc) {
>> DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
--
With best wishes
Dmitry
prev parent reply other threads:[~2022-02-17 1:18 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-15 14:16 [PATCH v2 0/8] drm/msm/dpu: cleanup dpu encoder code Dmitry Baryshkov
2022-02-15 14:16 ` [PATCH v2 1/8] drm/msm/dpu: fix dp audio condition Dmitry Baryshkov
2022-02-15 15:37 ` Bjorn Andersson
2022-02-15 17:54 ` Abhinav Kumar
2022-02-16 1:55 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 2/8] drm/msm: move struct msm_display_info to dpu driver Dmitry Baryshkov
2022-02-16 1:56 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 3/8] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt Dmitry Baryshkov
2022-02-15 15:38 ` Bjorn Andersson
2022-02-16 1:57 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 4/8] drm/msm/dpu: drop bus_scaling_client field Dmitry Baryshkov
2022-02-15 15:38 ` Bjorn Andersson
2022-02-16 1:57 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 5/8] drm/msm/dpu: encoder: drop unused mode_fixup callback Dmitry Baryshkov
2022-02-15 15:39 ` Bjorn Andersson
2022-02-15 16:51 ` Abhinav Kumar
2022-02-16 1:58 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 6/8] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set Dmitry Baryshkov
2022-02-16 1:58 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 7/8] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt Dmitry Baryshkov
2022-02-16 2:00 ` Stephen Boyd
2022-02-16 2:13 ` Dmitry Baryshkov
2022-02-16 2:17 ` Stephen Boyd
2022-02-15 14:16 ` [PATCH v2 8/8] drm/msm/dpu: simplify intf allocation code Dmitry Baryshkov
2022-02-15 15:42 ` Bjorn Andersson
2022-02-15 17:51 ` Abhinav Kumar
2022-02-17 1:18 ` Dmitry Baryshkov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=342ac115-e200-3bb0-9092-d148ba34cefd@linaro.org \
--to=dmitry.baryshkov@linaro.org \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=quic_abhinavk@quicinc.com \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).