Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Chunyan Zhang <zhang.lyra@gmail.com>]

On Wed, 7 Apr 2021 at 18:45, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> Adding Jörg, Will and Robin,

You forgot to add them actually :)
I've added Robin and Joerg.

>
> On Wed, Mar 31, 2021 at 09:21:19AM +0800, Kevin Tang wrote:
> > > > +static u32 check_mmu_isr(struct sprd_dpu *dpu, u32 reg_val)
> > > > +{
> > > > +     struct dpu_context *ctx = &dpu->ctx;
> > > > +     u32 mmu_mask = BIT_DPU_INT_MMU_VAOR_RD |
> > > > +                     BIT_DPU_INT_MMU_VAOR_WR |
> > > > +                     BIT_DPU_INT_MMU_INV_RD |
> > > > +                     BIT_DPU_INT_MMU_INV_WR;
> > > > +     u32 val = reg_val & mmu_mask;
> > > > +     int i;
> > > > +
> > > > +     if (val) {
> > > > +             drm_err(dpu->drm, "--- iommu interrupt err: 0x%04x ---\n",
> > > val);
> > > > +
> > > > +             if (val & BIT_DPU_INT_MMU_INV_RD)
> > > > +                     drm_err(dpu->drm, "iommu invalid read error, addr:
> > > 0x%08x\n",
> > > > +                             readl(ctx->base + REG_MMU_INV_ADDR_RD));
> > > > +             if (val & BIT_DPU_INT_MMU_INV_WR)
> > > > +                     drm_err(dpu->drm, "iommu invalid write error,
> > > addr: 0x%08x\n",
> > > > +                             readl(ctx->base + REG_MMU_INV_ADDR_WR));
> > > > +             if (val & BIT_DPU_INT_MMU_VAOR_RD)
> > > > +                     drm_err(dpu->drm, "iommu va out of range read
> > > error, addr: 0x%08x\n",
> > > > +                             readl(ctx->base + REG_MMU_VAOR_ADDR_RD));
> > > > +             if (val & BIT_DPU_INT_MMU_VAOR_WR)
> > > > +                     drm_err(dpu->drm, "iommu va out of range write
> > > error, addr: 0x%08x\n",
> > > > +                             readl(ctx->base + REG_MMU_VAOR_ADDR_WR));
> > >
> > > Is that the IOMMU page fault interrupt? I would expect it to be in the
> > > iommu driver.
> >
> > Our iommu driver is indeed an separate driver, and also in upstreaming,
> > but iommu fault interrupts reporting by display controller, not iommu
> >  itself,
> > if use iommu_set_fault_handler() to hook in our reporting function, there
> > must be cross-module access to h/w registers.
>
> Can you explain a bit more the design of the hardware then? Each device
> connected to the IOMMU has a status register (and an interrupt) that
> reports when there's a fault?

On Unisoc's platforms, one IOMMU serves one master device only, and interrupts
are handled by master devices rather than IOMMUs, since the registers are in the
physical address range of master devices.

>
> I'd like to get an ack at least from the IOMMU maintainers and
> reviewers.
>
> > > > +static void sprd_dpi_init(struct sprd_dpu *dpu)
> > > > +{
> > > > +     struct dpu_context *ctx = &dpu->ctx;
> > > > +     u32 int_mask = 0;
> > > > +     u32 reg_val;
> > > > +
> > > > +     if (ctx->if_type == SPRD_DPU_IF_DPI) {
> > > > +             /* use dpi as interface */
> > > > +             dpu_reg_clr(ctx, REG_DPU_CFG0, BIT_DPU_IF_EDPI);
> > > > +             /* disable Halt function for SPRD DSI */
> > > > +             dpu_reg_clr(ctx, REG_DPI_CTRL, BIT_DPU_DPI_HALT_EN);
> > > > +             /* select te from external pad */
> > > > +             dpu_reg_set(ctx, REG_DPI_CTRL,
> > > BIT_DPU_EDPI_FROM_EXTERNAL_PAD);
> > > > +
> > > > +             /* set dpi timing */
> > > > +             reg_val = ctx->vm.hsync_len << 0 |
> > > > +                       ctx->vm.hback_porch << 8 |
> > > > +                       ctx->vm.hfront_porch << 20;
> > > > +             writel(reg_val, ctx->base + REG_DPI_H_TIMING);
> > > > +
> > > > +             reg_val = ctx->vm.vsync_len << 0 |
> > > > +                       ctx->vm.vback_porch << 8 |
> > > > +                       ctx->vm.vfront_porch << 20;
> > > > +             writel(reg_val, ctx->base + REG_DPI_V_TIMING);
> > > > +
> > > > +             if (ctx->vm.vsync_len + ctx->vm.vback_porch < 32)
> > > > +                     drm_warn(dpu->drm, "Warning: (vsync + vbp) < 32, "
> > > > +                             "underflow risk!\n");
> > >
> > > I don't think a warning is appropriate here. Maybe we should just
> > > outright reject any mode that uses it?
> > >
> >  This issue has been fixed on the new soc, maybe I should remove it.
>
> If it still requires a workaround on older SoC, you can definitely add
> it but we should prevent any situation where the underflow might occur
> instead of reporting it once we're there.
>
> > > > +static enum drm_mode_status sprd_crtc_mode_valid(struct drm_crtc *crtc,
> > > > +                                     const struct drm_display_mode
> > > *mode)
> > > > +{
> > > > +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > > +
> > > > +     drm_dbg(dpu->drm, "%s() mode: "DRM_MODE_FMT"\n", __func__,
> > > DRM_MODE_ARG(mode));
> > > > +
> > > > +     if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > > +             drm_display_mode_to_videomode(mode, &dpu->ctx.vm);
> > >
> > > You don't seem to use that anywhere else? And that's a bit fragile,
> > > nothing really guarantees that it's the mode you're going to use, and
> > > even then it can be adjusted.
> > >
> >  drm_mode convert to video_mode is been use in "sprd_dpu_init" and
> > "sprd_dpi_init "
> >  Preferred mode should be fixed mode, we generally don’t adjust it.
>
> That's not really the assumption DRM is built upon though. The userspace
> is even allowed to setup its own mode and try to configure it, and your
> driver should take that into account.
>
> I'd just drop that mode_valid hook, and retrieve the videomode if you
> need it in atomic_enable or mode_set_no_fb
>
> > >
> > > > +
> > > > +             if ((mode->hdisplay == mode->htotal) ||
> > > > +                 (mode->vdisplay == mode->vtotal))
> > > > +                     dpu->ctx.if_type = SPRD_DPU_IF_EDPI;
> > > > +             else
> > > > +                     dpu->ctx.if_type = SPRD_DPU_IF_DPI;
> > >
> > > From an API PoV, you would want that to be in atomic_check. However, I'm
> > > not even sure what it's doing in the first place?
> > >
> > dpi interface mode: DPI(dsi video mode panel) and EDPI(dsi cmd mode panel)
> > dpi interface mode has been used on crtc atomic_enable foo, so we need
> > check dpi interface
> > mode before atomic_enable.
> >
> > Must be put it in atomic_check? Here is the dpi interface mode selection,
> > maybe here is better?
>
> This doesn't have any relationship to the htotal and vtotal though? it's
> something that is carried over by the MIPI-DSI functions and struct
> mipi_dsi_device.
>
> > >
> > > > +     }
> > > > +
> > > > +     return MODE_OK;
> > > > +}
> > > > +
> > > > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > +                                struct drm_atomic_state *state)
> > > > +{
> > > > +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > > +
> > > > +     sprd_dpu_init(dpu);
> > > > +
> > > > +     sprd_dpi_init(dpu);
> > > > +
> > > > +     enable_irq(dpu->ctx.irq);
> > >
> > > Shouldn't this be in enable_vblank? And I would assume that you would
> > > have the interrupts enabled all the time, but disabled in your device?
> > >
> > It seems better to put in enable_vblank, i will try and test it... Thks
> >
> >   And I would assume that you would
> > have the interrupts enabled all the time, but disabled in your device?
> > [kevin]I don’t quite understand this, can you help me explain it in
> > detail?
>
> You seem to have a register that enables and disables the interrupt in
> that device. The way we usually deal with them in this case is just to
> call request_irq in your bind/probe with the interrupts enabled at the
> controller level, and mask them when needed at the device level by
> clearing / setting that bit.
>
> Maxime