<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">Maxime Ripard <maxime@cerno.tech> 于2021年4月7日周三下午6:45写道： </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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. </blockquote><div>Chunyan has helped explain it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > +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. </blockquote><div>Thks, porch parameter is obtained from the panel driver, so I think it would be</div><div>better to check on the panel side. I will remove it here. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > +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 </blockquote><div>Yeah, i need videomode info to setup display controller, so i will retrieve the videomode</div><div>in mode_set_no_fb. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > > > + > > > + 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. </blockquote><div>You mean DPI/EDPI should be check in the encoder driver? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > > > + } > > > + > > > + 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. </blockquote><div> Yeah, we have display controller interrupts setting and clear register.</div><div>But the interrupts all been enabled in bootloader(eg, lk or uboot),</div><div>if the interrupt handler is active in the probe/bind phase by request_irq,</div><div>but the whole display pipeline is not ready, there maybe have some problems.</div><div> </div><div>So we want to manually control interrupt handler enable/disable, make sure all is ready,</div><div>then enable/disable interrupt handler. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Maxime </blockquote></div></div>