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

[Maxime Ripard]

Hi,

Adding Jörg, Will and Robin,

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?

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

Attachment: signature.asc
Description: PGP signature