| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2f22ea19-1b97-6a21-99c0-9ba1bb322af4@linaro.org>
Date: Mon, 2 Jan 2023 17:48:05 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Marijn Suijten <marijn.suijten@...ainline.org>
Cc: phone-devel@...r.kernel.org,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
~postmarketos/upstreaming@...ts.sr.ht,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Martin Botka <martin.botka@...ainline.org>,
Jami Kettunen <jami.kettunen@...ainline.org>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Stephen Boyd <swboyd@...omium.org>,
Vinod Koul <vkoul@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Kuogee Hsieh <quic_khsieh@...cinc.com>,
Jessica Zhang <quic_jesszhan@...cinc.com>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Loic Poulain <loic.poulain@...aro.org>,
Vinod Polimera <quic_vpolimer@...cinc.com>,
Adam Skladowski <a39.skl@...il.com>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 6/7] drm/msm/dpu: Implement tearcheck support on INTF
block
On 02/01/2023 13:06, Marijn Suijten wrote:
> On 2023-01-01 15:32:11, Dmitry Baryshkov wrote:
>> On 31/12/2022 23:50, Marijn Suijten wrote:
>>> Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
>>> PINGPONG block and into the INTF. Implement the necessary callbacks in
>>> the INTF block, and use these callbacks together with the INTF_TEAR
>>> interrupts
>>>
>>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
>>
>> Generally I have the same question as for the patch 2. Can we have some
>> higher level functions in the hw_pp and hw_intf files?
>
> That is mostly because patch 2 only cleaned up non-optional handling of
> hw_pp callbacks in dpu_encoder_phys_cmd_prepare_commit, which utilizes
> hw_intf's autorefresh callbacks since this patch. I don't think there's
> any logic in the encoder currently that is unique to either PP or INTF?
I think it would be better to duplicate the logic rather than having a
spaghetti of hw_pp and hw_intf calls.
> There are quite a few functions that check for NULL hw_pp only, while -
> especially after this patch - should also check hw_intf to raise
> "invalid encoder". Should I extend those checks as well?
I think so. However in most of cases these checks should be void, since
cmd encoder can not be instantiated without intf.
>> Moreover, as I
>> review your patch I have the feeling that it would make sense to have to
>> two sets of encoder callbacks, one for the hw_pp tearing handling and
>> another set for hw_intf-based one.
>
> Do you mean to duplicate most phy_cmd functions and switch them based on
> has_intf_te in dpu_encoder_phys_cmd_init_ops? Or introduce an entirely
> new set of callbacks that simply hide / abstract away the check on
> has_intf_te? The former would duplicate a bunch of code unless that is
> abstracted away into other functions, mainly in
> dpu_encoder_phys_cmd_tearcheck_config and
> dpu_encoder_phys_cmd_prepare_commit.
For the dpu_encoder_phys_cmd_tearcheck_config() it seems logical to fill
in the tc_cfg and then to call either the single hw_pp callback or a
single hw_intf callback.
I'd move most of the code from prepare_commit to dpu_hw_pp and then
duplicate it to dpu_hw_intf. This seems like the lesser evil.
This function really stands out, since if you inline
_dpu_encoder_phys_cmd_connect_te() and
dpu_encoder_phys_cmd_is_ongoing_pptx() it becomes obvious that the whole
function is a mixture of linked calls to hw_pp ops. And judging from
your patch it doesn't make sense to check them one by one. Either we
have all of them, or none.
>
> Alternatively, could we find a way where these PP and INTF ops share the
> same struct and function signature? That might be tricky for passing in
> the hw_pp or hw_intf struct without leaking those details to the
> callback and/or have the switching logic in there.
>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +
>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 10 +-
>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 113 +++++++---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 206 ++++++++++++++++++
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 29 +++
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 +
>>> 6 files changed, 340 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 9c6817b5a194..8b9070220ab2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -673,6 +673,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>>> struct dpu_kms *dpu_kms;
>>> struct dpu_hw_mdp *hw_mdptop;
>>> struct drm_encoder *drm_enc;
>>> + struct dpu_encoder_phys *phys_enc;
>>> int i;
>>>
>>> if (!dpu_enc || !disp_info) {
>>> @@ -703,12 +704,22 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
>>> vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
>>>
>>> vsync_cfg.pp_count = dpu_enc->num_phys_encs;
>>> + vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
>>> +
>>> if (disp_info->is_te_using_watchdog_timer)
>>> vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
>>> else
>>> vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
>>>
>>> hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
>>> +
>>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>> + phys_enc = dpu_enc->phys_encs[i];
>>> +
>>> + if (phys_enc->has_intf_te && phys_enc->hw_intf->ops.vsync_sel)
>>> + phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
>>> + vsync_cfg.vsync_source);
>>> + }
>>> }
>>> }
>>>
>>> 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 f2af07d87f56..47e79401032c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -148,10 +148,10 @@ struct dpu_encoder_phys_ops {
>>> /**
>>> * enum dpu_intr_idx - dpu encoder interrupt index
>>> * @INTR_IDX_VSYNC: Vsync interrupt for video mode panel
>>> - * @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
>>> - * @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode panel
>>> - * @INTR_IDX_RDPTR: Readpointer done unterrupt for cmd mode panel
>>> - * @INTR_IDX_WB_DONE: Writeback fone interrupt for virtual connector
>>> + * @INTR_IDX_PINGPONG: Pingpong done interrupt for cmd mode panel
>>> + * @INTR_IDX_UNDERRUN: Underrun interrupt for video and cmd mode panel
>>> + * @INTR_IDX_RDPTR: Readpointer done interrupt for cmd mode panel
>>> + * @INTR_IDX_WB_DONE: Writeback done interrupt for virtual connector
>>> */
>>> enum dpu_intr_idx {
>>> INTR_IDX_VSYNC,
>>> @@ -195,6 +195,7 @@ enum dpu_intr_idx {
>>> * pending.
>>> * @pending_kickoff_wq: Wait queue for blocking until kickoff completes
>>> * @irq: IRQ indices
>>> + * @has_intf_te: Interface TE configuration support
>>> */
>>> struct dpu_encoder_phys {
>>> struct drm_encoder *parent;
>>> @@ -220,6 +221,7 @@ struct dpu_encoder_phys {
>>> atomic_t pending_kickoff_cnt;
>>> wait_queue_head_t pending_kickoff_wq;
>>> int irq[INTR_IDX_MAX];
>>> + bool has_intf_te;
>>> };
>>>
>>> static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index 7e5ba52197cd..ca44a8087f01 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -100,12 +100,12 @@ static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
>>> DPU_ATRACE_END("pp_done_irq");
>>> }
>>>
>>> -static void dpu_encoder_phys_cmd_pp_rd_ptr_irq(void *arg, int irq_idx)
>>> +static void dpu_encoder_phys_cmd_te_rd_ptr_irq(void *arg, int irq_idx)
>>> {
>>> struct dpu_encoder_phys *phys_enc = arg;
>>> struct dpu_encoder_phys_cmd *cmd_enc;
>>>
>>> - if (!phys_enc->hw_pp)
>>> + if (!phys_enc->hw_pp || !phys_enc->hw_intf)
>>> return;
>>>
>>> DPU_ATRACE_BEGIN("rd_ptr_irq");
>>> @@ -147,11 +147,19 @@ static void dpu_encoder_phys_cmd_atomic_mode_set(
>>> struct drm_crtc_state *crtc_state,
>>> struct drm_connector_state *conn_state)
>>> {
>>> + if (phys_enc->has_intf_te && !phys_enc->hw_intf) {
>>> + DPU_ERROR("invalid intf configuration\n");
>>> + return;
>>> + }
>>> +
>>> phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
>>>
>>> phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
>>>
>>> - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
>>> + if (phys_enc->has_intf_te)
>>> + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
>>> + else
>>> + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
>>>
>>> phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>>> }
>>> @@ -264,7 +272,7 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
>>> if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1)
>>> ret = dpu_core_irq_register_callback(phys_enc->dpu_kms,
>>> phys_enc->irq[INTR_IDX_RDPTR],
>>> - dpu_encoder_phys_cmd_pp_rd_ptr_irq,
>>> + dpu_encoder_phys_cmd_te_rd_ptr_irq,
>>> phys_enc);
>>> else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0)
>>> ret = dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
>
> Fwiw looks like this function is a prime candidate to get updated with
> hw_intf information (in error checking and logging), as this callback is
> now shared between PP and INTF.
>
>>> @@ -336,10 +344,18 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>>>
>>> DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
>>>
>>> - if (!phys_enc->hw_pp->ops.setup_tearcheck ||
>>> - !phys_enc->hw_pp->ops.enable_tearcheck) {
>>> - DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> - return;
>>> + if (phys_enc->has_intf_te) {
>>> + if (!phys_enc->hw_intf->ops.setup_tearcheck ||
>>> + !phys_enc->hw_intf->ops.enable_tearcheck) {
>>> + DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> + return;
>>> + }
>>> + } else {
>>> + if (!phys_enc->hw_pp->ops.setup_tearcheck ||
>>> + !phys_enc->hw_pp->ops.enable_tearcheck) {
>>> + DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
>>> + return;
>>> + }
>>> }
>>>
>>> dpu_kms = phys_enc->dpu_kms;
>>> @@ -392,8 +408,13 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>>> phys_enc->hw_pp->idx - PINGPONG_0, tc_cfg.sync_cfg_height,
>>> tc_cfg.sync_threshold_start, tc_cfg.sync_threshold_continue);
>>>
>>> - phys_enc->hw_pp->ops.setup_tearcheck(phys_enc->hw_pp, &tc_cfg);
>>> - phys_enc->hw_pp->ops.enable_tearcheck(phys_enc->hw_pp, tc_enable);
>>
>> A simple random example: setup_tearcheck is always followed with the
>> enable_tearcheck. If we merge them, the code would be simpler.
>
> setup_tearcheck could include this functionality, but note that
> dpu_encoder_phys_cmd_disable currently calls enable_tearcheck(false)
> without setup_tearcheck.
Yes, so enable_tearcheck() and disable_tearcheck() sound more useful.
>
> - Marijn
>
>>> <snip>
--
With best wishes
Dmitry
Powered by blists - more mailing lists