Messages in this thread | | | Date | Tue, 14 Feb 2023 09:54:57 -0800 | Subject | Re: [RFC PATCH 5/7] drm/msm/dpu: Document and enable TEAR interrupts on DSI interfaces | From | Abhinav Kumar <> |
| |
On 2/14/2023 5:06 AM, Marijn Suijten wrote: > On 2023-02-13 19:09:32, Abhinav Kumar wrote: >> >> >> On 2/13/2023 1:46 PM, Dmitry Baryshkov wrote: >>> On 13/02/2023 21:37, Jessica Zhang wrote: >>>> >>>> >>>> On 12/31/2022 1:50 PM, Marijn Suijten wrote: >>>>> All SoCs since DPU 5.0.0 (and seemingly up until and including 6.0.0, >>>>> but excluding 7.x.x) have the tear interrupt and control registers moved >>>>> out of the PINGPONG block and into the INTF block. Wire up the >>>>> necessary interrupts and IRQ masks on all supported hardware. >>>> >>>> Hi Marijn, >>>> >>>> Thanks for the patch. >>>> >>>> I saw that in your commit msg, you mentioned that 7.x doesn't have >>>> tearcheck in the INTF block -- can you double check that this is correct? > > It wasn't correct and has already been removed for v2 [1] after rebasing > on top of SM8[345]50 support, where the registers reside at a different > (named 7xxxx downstream) offset. > > [1] https://github.com/SoMainline/linux/commit/886d3fb9eed925e7e9c8d6ca63d2439eaec1c702 > >>>> I'm working on SM8350 (DPU v7) and I'm seeing that it does have >>>> tearcheck in INTF block. >>> >>> I confirm, according to the vendor drivers INTF TE should be used for >>> all DPU >= 5.0, including 7.x and 8.x >>> >>> However I think I know what Marijn meant here. For 5.x and 6.x these >>> IRQs are handled at the address MDSS + 0x6e800 / + 0x6e900 (which means >>> offset here should 0x6d800 and 0x6d900) for INTF_1 and INTF_2. Since DPU >>> 7.x these IRQ registers were moved close to the main INTF block (0x36800 >>> and 0x37800 = INTF + 0x800). > > That might have been the case. > >> Got it, then the commit text should remove "control" and just say tear >> interrupt registers. It got a bit confusing. > > The wording here points to both the interrupt (MDP_INTFx_TEAR_INTR) > registers and control (INTF_TEAR_xxx) registers separately. Feel free > to bikeshed the wording in preliminary v2 [1]; should I drop the mention > of the control registers being "moved" from PP to INTF entirely, leaving > just the wording about the interrupt registers moving from > MDP_SSPP_TOP0_INTR to a dedicated MDP_INTFx_TEAR_INTR region?
Yes, that makes more sense to me. Drop the mention on control registers. > >> We will add the 7xxx intf tear check support on top of this series. > > No need, that is already taken care of in an impending v2 [1] (unless > additional changes are required beyond the moved register offset). >
Ok, we will wait till you post v2 and see if that works for us without any of our local changes.
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>>> --- >>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 78 +++++++++++-------- >>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 6 +- >>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 12 +++ >>>>> .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 + >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 3 + >>>>> 5 files changed, 68 insertions(+), 33 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> index 1cfe94494135..b9b9b5b0b615 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>>>> @@ -86,6 +86,15 @@ >>>>> #define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) >>>>> +#define IRQ_MSM8998_MASK (BIT(MDP_SSPP_TOP0_INTR) | \ >>>>> + BIT(MDP_SSPP_TOP0_INTR2) | \ >>>>> + BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> + BIT(MDP_INTF0_INTR) | \ >>>>> + BIT(MDP_INTF1_INTR) | \ >>>>> + BIT(MDP_INTF2_INTR) | \ >>>>> + BIT(MDP_INTF3_INTR) | \ >>>>> + BIT(MDP_INTF4_INTR)) >>>>> + >>>>> #define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \ >>>>> BIT(MDP_SSPP_TOP0_INTR2) | \ >>>>> BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> @@ -100,13 +109,15 @@ >>>>> #define IRQ_QCM2290_MASK (BIT(MDP_SSPP_TOP0_INTR) | \ >>>>> BIT(MDP_SSPP_TOP0_INTR2) | \ >>>>> BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> - BIT(MDP_INTF1_INTR)) >>>>> + BIT(MDP_INTF1_INTR) | \ >>>>> + BIT(MDP_INTF1_TEAR_INTR)) >>>>> #define IRQ_SC7180_MASK (BIT(MDP_SSPP_TOP0_INTR) | \ >>>>> BIT(MDP_SSPP_TOP0_INTR2) | \ >>>>> BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> BIT(MDP_INTF0_INTR) | \ >>>>> - BIT(MDP_INTF1_INTR)) >>>>> + BIT(MDP_INTF1_INTR) | \ >>>>> + BIT(MDP_INTF1_TEAR_INTR)) >>>>> #define IRQ_SC7280_MASK (BIT(MDP_SSPP_TOP0_INTR) | \ >>>>> BIT(MDP_SSPP_TOP0_INTR2) | \ >>>>> @@ -120,7 +131,9 @@ >>>>> BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> BIT(MDP_INTF0_INTR) | \ >>>>> BIT(MDP_INTF1_INTR) | \ >>>>> + BIT(MDP_INTF1_TEAR_INTR) | \ >>>>> BIT(MDP_INTF2_INTR) | \ >>>>> + BIT(MDP_INTF2_TEAR_INTR) | \ >>>>> BIT(MDP_INTF3_INTR) | \ >>>>> BIT(MDP_INTF4_INTR)) >>>>> @@ -129,7 +142,9 @@ >>>>> BIT(MDP_SSPP_TOP0_HIST_INTR) | \ >>>>> BIT(MDP_INTF0_INTR) | \ >>>>> BIT(MDP_INTF1_INTR) | \ >>>>> + BIT(MDP_INTF1_TEAR_INTR) | \ >>>>> BIT(MDP_INTF2_INTR) | \ >>>>> + BIT(MDP_INTF2_TEAR_INTR) | \ >>>>> BIT(MDP_INTF3_INTR) | \ >>>>> BIT(MDP_INTF4_INTR) | \ >>>>> BIT(MDP_INTF5_INTR) | \ >>>>> @@ -1300,63 +1315,64 @@ static struct dpu_dsc_cfg sdm845_dsc[] = { >>>>> /************************************************************* >>>>> * INTF sub blocks config >>>>> *************************************************************/ >>>>> -#define INTF_BLK(_name, _id, _base, _type, _ctrl_id, _progfetch, >>>>> _features, _reg, _underrun_bit, _vsync_bit) \ >>>>> +#define INTF_BLK(_name, _id, _base, _len, _type, _ctrl_id, >>>>> _progfetch, _features, _reg, _underrun_bit, _vsync_bit, _tear_reg, >>>>> _tear_rd_ptr_bit) \ >>>>> {\ >>>>> .name = _name, .id = _id, \ >>>>> - .base = _base, .len = 0x280, \ >>>>> + .base = _base, .len = _len, \ >>>>> .features = _features, \ >>>>> .type = _type, \ >>>>> .controller_id = _ctrl_id, \ >>>>> .prog_fetch_lines_worst_case = _progfetch, \ >>>>> .intr_underrun = DPU_IRQ_IDX(_reg, _underrun_bit), \ >>>>> .intr_vsync = DPU_IRQ_IDX(_reg, _vsync_bit), \ >>>>> + .intr_tear_rd_ptr = DPU_IRQ_IDX(_tear_reg, _tear_rd_ptr_bit), \ >>>>> } >>>>> static const struct dpu_intf_cfg msm8998_intf[] = { >>>>> - INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 25, >>>>> INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25), >>>>> - INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 25, >>>>> INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27), >>>>> - INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 25, >>>>> INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29), >>>>> - INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_HDMI, 0, 25, >>>>> INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31), >>>>> + INTF_BLK("intf_0", INTF_0, 0x6A000, 0x268, INTF_DP, 0, 25, >>>>> INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1), >>>> >>>> Just wondering, how were the lengths calculated for the INTF blocks? >>>> The values in general seem a little off to me. > > These (for MSM8998) have been taken from downstream specifically; my > series starts using INTF_STATUS at 0x26C which conveniently is the > register right after 0x268, matching the fact that INTF TE and these > registers weren't supported/available yet on MSM8998. > >>>> For example, I'm looking downstream and it seems to me that the length >>>> for the INTF_0 on MSM8998 should be 0x280. Similarly for SC7280, I'm >>>> seeing that length for INTF + tearcheck should be 0x2c4. > > There are many different downstream sources and tags with seemingly > conflicting/confusing information. For v2 [2] I've picked the highest > register used by the driver which is INTF_TEAR_AUTOREFRESH_CONFIG at > 0x2B4 (but there might always be more registers that don't need to be > poked at by the driver, but contain magic debug information and the > like... those would be useful to capture in the dump going forward). > > [2]: https://github.com/SoMainline/linux/commit/2bbc609dd28aa0bd0a2dede20163e521912d0072 >
Not entirely correct.TEAR_AUTOREFRESH_STATUS is at 0x2c0 for sm8350 and sm8450 as well so 0x2b4 is a bit short. DPU code uses autorefresh status today.Esp after your changes it will use the autorefresh status from intf te which is at offset 0x2c0
>>> We have discussed INTF lengths in [1]. The current understanding of the >>> block lengths can be found at [2]. Please comment there if any of the >>> fixed lengths sounds incorrect to you. >>> >>> [1] https://patchwork.freedesktop.org/patch/522187/ >>> [2] https://patchwork.freedesktop.org/patch/522227/ >>> >>> [skipped the rest] >>> >> >> Please correct my understanding here, it was agreed to fix intf blocks >> to 0x2c4 here https://patchwork.freedesktop.org/patch/522227/ but I dont >> see this was merged? >> >> It was agreed to first land INTF_TE and then add the higher addresses > > Seems like it, at least if I interpret [3] correctly. My series adds a > new define that will hardcode _len to 0x2B8 for now, and Dmitry/Konrad > can later extend it to whatever is stated by the correct downstream > source. >
Like mentioned above it should be 0x2c0 for intf block.
If you face any conflicting information in downstream code, you can always check with me on IRC.
> [3]: https://lore.kernel.org/linux-arm-msm/6ad96cff-b91b-a4c7-4573-7bb8de7194f8@linaro.org/ > >> but I dont see such a change, am i missing something? > > This was discussed just yesterday. And it wouldn't make much sense to > make such a change now, knowing that my v2 for this series - which isn't > even on the lists yet - will already change the INTF_BLK macro resulting > in unneeded conflicts. As requested by Dmitry, let's get INTF TE > processed first before rebasing the block length change? >
Please see above comment on why it should be 0x2c4.
> - Marijn
| |