From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C742FC433B4 for ; Wed, 7 Apr 2021 10:46:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8A59861359 for ; Wed, 7 Apr 2021 10:46:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1351472AbhDGKqS (ORCPT ); Wed, 7 Apr 2021 06:46:18 -0400 Received: from new3-smtp.messagingengine.com ([66.111.4.229]:33957 "EHLO new3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1351427AbhDGKqI (ORCPT ); Wed, 7 Apr 2021 06:46:08 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailnew.nyi.internal (Postfix) with ESMTP id 31B07580986; Wed, 7 Apr 2021 06:45:43 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 07 Apr 2021 06:45:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=KBte6mMjoAGegGrtIFkx5zgN2cU muA0JM1y/5uroxNI=; b=b0hgm9p0rny8zQXlF5+GRqfY2LD3RwxjCz3Fa2trI5a JY6bU01PpTn8bRzGeBXMb8k724q/DhtUZp/r/ITXM2r81h4RldiwZcfSG452i+VI EY/qMvnWAR+70IkE97JBINlFtNPH3zf7d1+9+35l3lr9Eo34AeIjCidsvCDZyFiK AOKR7qh3wzvQ+vl4wApxVJgCIyEo2pi6jnA311G1DWUl/91mE0pSEcFl7utoUlFd Eql4g+e7dJQXmAceZA7aCI9Gnkx1FToPTHIg8/Ez8Eia7QOIPyl3t62dUbQg0PJG o7lwatU/f9MxmxCc3Fj0JGyiBeU+Z4LIJdH/bDEzAaw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=KBte6m MjoAGegGrtIFkx5zgN2cUmuA0JM1y/5uroxNI=; b=ZZ64in0hNmp8sj4lW0MCJZ QWvWXK2g4yqu8+7lB4+WiEZUlhAzqmg6ga/slSXNy/MjO2irnm9reObqn3SNygvi Ncyq1AkopKCy2xOdFf9dP58amAYO2kcKdcTu8WsvyLPN8v2ovOdCm0045iT5Wa+n gPEH9PRkz+0lUZRZQT8MizIwQIpsvtf7nkmAOak3m/sTLsuMimcZNE1PyPvkiQCZ f9hetfelf5082aLrtcqkS7AGuM3vaAiItjZtRjO/PhKHoD6Y3GON0jAPZPffTd66 8/SLnz6D1UOGd5CBh03TYbRUoWxyCOJC/ZD0tqTsIl/bXH5A64peJkEDRiQRmCoA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrudejjedgfedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttdejnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepuedtgfejueduheevgfevvdettdduleffgfffkeeltdffkeegudekjeeuveei gedunecukfhppeeltddrkeelrdeikedrjeeinecuvehluhhsthgvrhfuihiivgeptdenuc frrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Received: from localhost (lfbn-tou-1-1502-76.w90-89.abo.wanadoo.fr [90.89.68.76]) by mail.messagingengine.com (Postfix) with ESMTPA id 9954A24005A; Wed, 7 Apr 2021 06:45:41 -0400 (EDT) Date: Wed, 7 Apr 2021 12:45:38 +0200 From: Maxime Ripard To: Kevin Tang Cc: Maarten Lankhorst , Sean Paul , David Airlie , Daniel Vetter , Rob Herring , Mark Rutland , Orson Zhai , Chunyan Zhang , "Linux-Kernel@Vger. Kernel. Org" , ML dri-devel , devicetree@vger.kernel.org Subject: Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver Message-ID: <20210407104538.cvssdck26rejrfye@gilmour> References: <20210222132822.7830-1-kevin3.tang@gmail.com> <20210222132822.7830-5-kevin3.tang@gmail.com> <20210324111019.og6d3w47swjim2mq@gilmour> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4udzy4wtmt67hg7k" Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --4udzy4wtmt67hg7k Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, Adding J=C3=B6rg, 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 =3D &dpu->ctx; > > > + u32 mmu_mask =3D 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 =3D 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, ad= dr: > > 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. >=20 > 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 =3D &dpu->ctx; > > > + u32 int_mask =3D 0; > > > + u32 reg_val; > > > + > > > + if (ctx->if_type =3D=3D 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 =3D 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 =3D 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 *cr= tc, > > > + const struct drm_display_mode > > *mode) > > > +{ > > > + struct sprd_dpu *dpu =3D 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=E2=80=99t adjust i= t. 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 =3D=3D mode->htotal) || > > > + (mode->vdisplay =3D=3D mode->vtotal)) > > > + dpu->ctx.if_type =3D SPRD_DPU_IF_EDPI; > > > + else > > > + dpu->ctx.if_type =3D 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. >=20 > 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 =3D 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 >=20 > And I would assume that you would > have the interrupts enabled all the time, but disabled in your device? > [kevin]I don=E2=80=99t 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 --4udzy4wtmt67hg7k Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYG2NUgAKCRDj7w1vZxhR xciCAP46gAj+KXXsPheac+fk30LVvpGA59Dov03+CK9oOHW5xQEAtO9jP0S4MKvG 17audHTVg2v8ZC/IUx/t2aKDrRJRDw0= =Ieuo -----END PGP SIGNATURE----- --4udzy4wtmt67hg7k--