Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp1588228imu; Wed, 28 Nov 2018 11:48:12 -0800 (PST) X-Google-Smtp-Source: AJdET5cOAAlRZBE+lIJbDqDD5WvX9YQ7DyMo+fwitVgC1zvCJ2i4ZAxCNQ6hO+mgL4f3ctDwaPVS X-Received: by 2002:a62:3687:: with SMTP id d129-v6mr38974465pfa.56.1543434492274; Wed, 28 Nov 2018 11:48:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1543434492; cv=none; d=google.com; s=arc-20160816; b=LU/TmsW5kVcPbJaEX+5HYEOww5+R6lIocx6nbxaI7aPIxNDZ37/NZuS2v51T5YgKIV JJMOQE4Dbm5ruU6l3J2gQzwyUgjMciTuD8f+7vrTeTILYQsbriAsYC4DSzMnOvYEGn1Y OEvAP++Gt9cldgVmkwqVcQwyltMzaPGDXoMDMsHXGFae8SEH8QCiWPdaFKDQav6G0BXs eFEZr28M0DX++jWQIeHi/aTmOsv2JcarZDxm/zs6SQdPtWkaIuSKpE5B2KCSnujvLa7F qNFVPGPQrB/3JrTFUv7y1gLsEcgcJSd39X1wxR8x/4tvqWxq58j4FmYoeqmIGbVipjJh VW2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=IAa2ozWNSDa81AAI//mxNbLsV3jPxgp5M0hCY4SUpR4=; b=s8sHLkqD6R3a6nD2QOUR6Xn85LLqvqfOi8yjtlkQrsB/dpjVAwGcaKZ9933TnEoZHF UTg74X6l0LwbrUTbc3VDfk2ZkLBZ7Wd4LZH3osrlqVJhsG0sc2IxXJ/VZr03kYH9f5Io 5limmRqvCfh59yCBgxlNCXRAaz5XrnA4EWajKVK/SZztcu89uUuZiXbcPnPKKxIDF9jm Am+EDW6kw+SvW2zrkwyC0eG5TuWRmY6+eMg3OjKxnC21DwKZKsmSP1MOyFGgzGfU1+qY NtYN/LYtlPbWTp3OZ1kfjrO3dFhkfWADVV7cXeX0mos0dTz5Yp2PtBUnOEh0+Uga+geg t92A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@broadcom.com header.s=google header.b=gtwN2KXm; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=broadcom.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 89-v6si8552687plb.405.2018.11.28.11.47.57; Wed, 28 Nov 2018 11:48:12 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@broadcom.com header.s=google header.b=gtwN2KXm; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=broadcom.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728542AbeK2Gsq (ORCPT + 99 others); Thu, 29 Nov 2018 01:48:46 -0500 Received: from mail-it1-f195.google.com ([209.85.166.195]:37306 "EHLO mail-it1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725779AbeK2Gsq (ORCPT ); Thu, 29 Nov 2018 01:48:46 -0500 Received: by mail-it1-f195.google.com with SMTP id b5so6163913iti.2 for ; Wed, 28 Nov 2018 11:46:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=IAa2ozWNSDa81AAI//mxNbLsV3jPxgp5M0hCY4SUpR4=; b=gtwN2KXmN5j/vdpOYtOiFou+I1Odk1VmsFA/fzrDlXZFLxo7DSsgm5mO6RSLlZBi3p 2h2IKcCx9l2p/bmGNytJPvxsxjFpkRB7Mn/FFe1rxxaKA5PHECQq9W4hwEWTWHngeUZE 3038QLhykp70I/EWg6FFtQt+CJuUxulx5QhWM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=IAa2ozWNSDa81AAI//mxNbLsV3jPxgp5M0hCY4SUpR4=; b=hTwCUDiEwpcvk+Ln2M7xuHYVsWCQ22eqM+IQk59yO8oalL9/dFIkuux63k6ELwtsc8 LYip7VhBgQ/TtUwDwu0tzLyOBw/m8jnaNZvzTHk8zz09Y4XjmNLchSiRZaTAZf252JXa /T11gYajSLUtApipexQxntk61pAXPtXaWRz+jYMsLWNeYBgYSCv/xyGldEawE6OYNfEx aA/RLoU1HZMLb8NMkcQBAZuCKuCOAzsZZrWhK+1Kr48bfSdRBmcOUgEjl86mu3s2TFZA 99e91+aNJWJoIFq7+oDaAgpsCwZTDEOr9Dww2fuzBhbXhbSPNAYCCiXko5wuZ+N20DWP TOFw== X-Gm-Message-State: AA+aEWYTX71LXbstSCp0ruxnoK2URQhJj94DhN5UztpdD96/dYF7c4O8 D9p8R001+crI73t0mRvL/QBNMFSLc0de6uT4DgcE2g== X-Received: by 2002:a24:fe41:: with SMTP id w62mr4436394ith.23.1543434359668; Wed, 28 Nov 2018 11:45:59 -0800 (PST) MIME-Version: 1.0 References: <20181108161654.19888-1-eric@anholt.net> <20181108161654.19888-5-eric@anholt.net> <87ftw4y6rc.fsf@anholt.net> In-Reply-To: <87ftw4y6rc.fsf@anholt.net> From: Dave Emett Date: Wed, 28 Nov 2018 19:45:47 +0000 Message-ID: Subject: Re: [PATCH 4/4] drm/v3d: Add support for submitting jobs to the TFU. To: Eric Anholt Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, boris.brezillon@bootlin.com, daniel.vetter@ffwll.ch Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org A few comments below. In particular I think USECOEF handling is a bit broken? Otherwise looks good to me. > I think one interesting question here is if TFU hangs (has it ever hung, > in our experience?) do we want to reset the whole V3D, or is the reset > flag in the TFU block enough? We've never seen the TFU hang AFAIK. Seems prudent to handle anyway; what you've done looks fine to me. I wouldn't try to reset the TFU on its own. I don't know if that TFU reset bit has ever been tested! > > @@ -251,6 +256,7 @@ static const struct drm_ioctl_desc v3d_drm_ioctls[] = { > > DRM_IOCTL_DEF_DRV(V3D_MMAP_BO, v3d_mmap_bo_ioctl, DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF_DRV(V3D_GET_PARAM, v3d_get_param_ioctl, DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF_DRV(V3D_GET_BO_OFFSET, v3d_get_bo_offset_ioctl, DRM_RENDER_ALLOW), > > + DRM_IOCTL_DEF_DRV(V3D_SUBMIT_TFU, v3d_submit_tfu_ioctl, DRM_RENDER_ALLOW | DRM_AUTH), > > }; I would extend the comment above this block to note that DRM_AUTH is currently required on SUBMIT_TFU because TFU commands are currently not validated. (The TFU does not access memory via the GMP so I assume we will want to explicitly validate commands instead?) > > static void > > v3d_unlock_bo_reservations(struct drm_device *dev, dev not used? Wouldn't be needed by v3d_lock_bo_reservations either, if it didn't need to be passed to unlock. > > +static void > > +v3d_tfu_job_cleanup(struct kref *ref) > > +{ > > + struct v3d_tfu_job *job = container_of(ref, struct v3d_tfu_job, > > + refcount); > > + struct v3d_dev *v3d = job->v3d; > > + unsigned int i; > > + > > + dma_fence_put(job->in_fence); > > + dma_fence_put(job->done_fence); > > + > > + for (i = 0; i < ARRAY_SIZE(job->bo); i++) > > + drm_gem_object_put_unlocked(&job->bo[i]->base); This is a bit questionable. job->bo[i] may be NULL. &job->bo[i]->base would work out as NULL too, but this strictly speaking invokes undefined behaviour. > > +#define V3D_TFU_INT_STS 0x00438 > > +#define V3D_TFU_INT_SET 0x0043c > > +#define V3D_TFU_INT_CLR 0x00440 > > +#define V3D_TFU_INT_MSK_STS 0x00444 > > +#define V3D_TFU_INT_MSK_SET 0x00448 > > +#define V3D_TFU_INT_MSK_CLR 0x0044c > > +#define V3D_TFU_INT_TFUC BIT(1) > > +#define V3D_TFU_INT_TFUF BIT(0) These just alias the HUB_CTL_INT registers. They shouldn't be used. I would probably avoid listing them here to avoid confusion. > > + if (job->args.coef[0] & V3D_TFU_COEF0_USECOEF) { > > + V3D_WRITE(V3D_TFU_COEF0, job->args.coef[0]); > > + V3D_WRITE(V3D_TFU_COEF1, job->args.coef[1]); > > + V3D_WRITE(V3D_TFU_COEF2, job->args.coef[2]); > > + V3D_WRITE(V3D_TFU_COEF3, job->args.coef[3]); > > + } If USECOEF isn't set, still want to write COEF0 to clear the bit? > > +#define DRM_IOCTL_V3D_SUBMIT_TFU DRM_IOWR(DRM_COMMAND_BASE + DRM_V3D_SUBMIT_TFU, struct drm_v3d_submit_tfu) Should this not be DRM_IOW? No data is returned to userspace in the drm_v3d_submit_tfu struct AFAICT? > > + /* sync object to block on before submitting the TFU job. Each TFU > > + * job will execute in the order submitted to its FD. Synchronization > > + * against rendering jobs requires using sync objects. > > + */ > > + __u32 in_sync; "Submit" is used to mean two different things here. Maybe "before submitting the TFU job" --> "before running the TFU job" to avoid confusion?