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=-11.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable 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 145BAC43461 for ; Tue, 8 Sep 2020 12:52:55 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C1E252222E for ; Tue, 8 Sep 2020 12:52:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="pm0wBa35" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C1E252222E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YUNssoWyLCd7RGLq0Hv5RiL5YpRWZsxA3M5iNVtgSQo=; b=pm0wBa35CY4Oxsh8tZWPILMsu M0RKckAxA3N2nSLPnkgE0sTu322awnoWif1E9m2Ma3Pk5BeEOUJa8rGpoPBuZkDB7Y0hH3FNW+7up XDR4df4lvpYwLsrrtLd87StX+ovmnJ9BrO7eOfHphxpheS+VXkOS/5tqor3R5xXbhs4MxxM1O3Ric MFyS6lTclhr6p4s1FdqFDMuC0Ng4U8THypLUuRM4KpGKUdg7w+poyfvL3ZtUDa/mcPDgnKRlYs3YM Ne+uPvl26KJVN0Z3h8dW9j7Gt37vT2IdLrE/GMwGRgfXjX1cYc8n3B4RjgMdLBqGSupOOr8AVJgsI vMSH6yAXw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFd5w-0007B8-CN; Tue, 08 Sep 2020 12:51:32 +0000 Received: from mga09.intel.com ([134.134.136.24]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFd5t-0006qE-8F for linux-arm-kernel@lists.infradead.org; Tue, 08 Sep 2020 12:51:30 +0000 IronPort-SDR: cpsieJo+UTzML42ezCQEIk6h/eNh3SNqtZPdaFji1s8qozhPBXMvymM5TAYM6D4Lt7zpUQjCay oAGe9xEvwsoA== X-IronPort-AV: E=McAfee;i="6000,8403,9737"; a="159092007" X-IronPort-AV: E=Sophos;i="5.76,405,1592895600"; d="scan'208";a="159092007" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Sep 2020 05:49:24 -0700 IronPort-SDR: nj5/OcTe06e0EVi7mUiEzjkOSEWW3X/AM5XuICu+e2zJkh+3sdwK5BnVoz1RNV+EIShorkPOi3 VEApXgzFLKMA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.76,405,1592895600"; d="scan'208";a="304081783" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by orsmga006.jf.intel.com with SMTP; 08 Sep 2020 05:49:20 -0700 Received: by stinkbox (sSMTP sendmail emulation); Tue, 08 Sep 2020 15:49:19 +0300 Date: Tue, 8 Sep 2020 15:49:19 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Laurent Pinchart Subject: Re: [PATCH] drm: mxsfb: check framebuffer pitch Message-ID: <20200908124919.GI6112@intel.com> References: <20200907160343.124405-1-stefan@agner.ch> <20200907161712.GF6047@pendragon.ideasonboard.com> <20200907181855.GE2352366@phenom.ffwll.local> <86615b4b1551d4a6f1cfcc13b38e616c@agner.ch> <20200908084855.GH2352366@phenom.ffwll.local> <20200908123304.GG6047@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200908123304.GG6047@pendragon.ideasonboard.com> X-Patchwork-Hint: comment User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200908_085129_422204_9CDA3B74 X-CRM114-Status: GOOD ( 47.19 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marek Vasut , Sascha Hauer , Dave Airlie , Sascha Hauer , Linux Kernel Mailing List , dri-devel , Tomi Valkeinen , Jyri Sarha , Daniel Vetter , Shawn Guo , Linux ARM , dl-linux-imx Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Sep 08, 2020 at 03:33:04PM +0300, Laurent Pinchart wrote: > On Tue, Sep 08, 2020 at 02:29:02PM +0200, Daniel Vetter wrote: > > On Tue, Sep 8, 2020 at 2:07 PM Stefan Agner wrote: > > > On 2020-09-08 10:48, Daniel Vetter wrote: > > >> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote: > > >>> On 08/09/2020 10:55, Stefan Agner wrote: > > >>>> On 2020-09-07 20:18, Daniel Vetter wrote: > > >>>>> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote: > > >>>>>> Hi Stefan, > > >>>>>> > > >>>>>> Thank you for the patch. > > >>>>>> > > >>>>>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote: > > >>>>>>> The lcdif IP does not support a framebuffer pitch (stride) othe= r than > > >>>>>>> the CRTC width. Check for equality and reject the state otherwi= se. > > >>>>>>> > > >>>>>>> This prevents a distorted picture when using 640x800 and runnin= g the > > >>>>>>> Mesa graphics stack. Mesa tires to use a cache aligned stride, = which > > >>>>>> > > >>>>>> s/tires/tries/ > > >>>>>> > > >>>>>>> leads at that particular resolution to width !=3D stride. Curre= ntly > > >>>>>>> Mesa has no fallback behavior, but rejecting this configuration= allows > > >>>>>>> userspace to handle the issue correctly. > > >>>>>> > > >>>>>> I'm increasingly impressed by how featureful this IP core is :-) > > >>>>>> > > >>>>>>> Signed-off-by: Stefan Agner > > >>>>>>> --- > > >>>>>>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++++++++++++++++++---- > > >>>>>>> 1 file changed, 18 insertions(+), 4 deletions(-) > > >>>>>>> > > >>>>>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/dr= m/mxsfb/mxsfb_kms.c > > >>>>>>> index b721b8b262ce..79aa14027f91 100644 > > >>>>>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > >>>>>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > >>>>>>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struc= t drm_plane *plane, > > >>>>>>> { > > >>>>>>> struct mxsfb_drm_private *mxsfb =3D to_mxsfb_drm_privat= e(plane->dev); > > >>>>>>> struct drm_crtc_state *crtc_state; > > >>>>>>> + unsigned int pitch; > > >>>>>>> + int ret; > > >>>>>>> > > >>>>>>> crtc_state =3D drm_atomic_get_new_crtc_state(plane_stat= e->state, > > >>>>>>> &mxsfb->crtc= ); > > >>>>>>> > > >>>>>>> - return drm_atomic_helper_check_plane_state(plane_state,= crtc_state, > > >>>>>>> - DRM_PLANE_HE= LPER_NO_SCALING, > > >>>>>>> - DRM_PLANE_HE= LPER_NO_SCALING, > > >>>>>>> - false, true); > > >>>>>>> + ret =3D drm_atomic_helper_check_plane_state(plane_state= , crtc_state, > > >>>>>>> + DRM_PLANE_HEL= PER_NO_SCALING, > > >>>>>>> + DRM_PLANE_HEL= PER_NO_SCALING, > > >>>>>>> + false, true); > > >>>>>>> + if (ret || !plane_state->visible) > > >>>>>> > > >>>>>> Would it be more explict to check for !plane_state->fb ? Otherwi= se I'll > > >>>>>> have to verify that !fb always implies !visible :-) > > >>>>>> > > >>>>>>> + return ret; > > >>>>>>> + > > >>>>>>> + pitch =3D crtc_state->mode.hdisplay * > > >>>>>>> + plane_state->fb->format->cpp[0]; > > >>>>>> > > >>>>>> This holds on a single line. > > >>>>>> > > >>>>>>> + if (plane_state->fb->pitches[0] !=3D pitch) { > > >>>>>>> + dev_err(plane->dev->dev, > > >>>>>>> + "Invalid pitch: fb and crtc widths must= be the same"); > > >>>>>> > > >>>>>> I'd turn this into a dev_dbg(), printing error messages to the k= ernel > > >>>>>> log in response to user-triggered conditions is a bit too verbos= e and > > >>>>>> could flood the log. > > >>>>>> > > >>>>>> Wouldn't it be best to catch this issue when creating the frameb= uffer ? > > >>>>> > > >>>>> Yeah this should be verified at addfb time. We try to validate as= early as > > >>>>> possible. > > >>>>> -Daniel > > >>>>> > > >>>> > > >>>> Sounds sensible. From what I can tell fb_create is the proper call= back > > >>>> to implement this at addfb time. Will give this a try. > > >>>> > > >>>> FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. M= aybe > > >>>> should be moved to addfb there too? > > >>> > > >>> But you don't know the crtc width when creating the framebuffer. > > >> > > >> Hm right this is a different check. What we could check in fb_create= for > > >> both is that the logical fb size matches exactly the pitch. That's n= ot > > >> sufficient criteria, but it will at least catch some of them already. > > >> > > >> But yeah we'd need both here. > > > > > > After validating width of framebuffer against pitch, the only thing we > > > need to check here is that the width matches. From what I can tell, > > > least for mxsfb, this should be covered by > > > drm_atomic_helper_check_plane_state's can_position parameter set to > > > false. > > = > > This only checks against the src rectangle of the crtc state, there's > > nothing forcing that the size of the fb matches the src rectangle > > exactly. I guess we could maybe add that as another parameter for hw > > like yours or tilcdc. Naming is a bit tricky, maybe > > require_matching_fb or src_must_match_fb or something like that. > = > Can we turn those parameters into flags ? false, true, false is hard to > read. Even the flags approach doesn't really scale past some point. Is there a particularly convincing reason for stuffing yet another check into this function as opposed to just introducing a separate function? I prefer clear single purpose functions over swiss army knives. > = > > > So I think in my case I can get away by only checking the framebuffer. > > = > > You still need both I think. > = > -- = > Regards, > = > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- = Ville Syrj=E4l=E4 Intel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel