Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752079AbcKIKSZ (ORCPT ); Wed, 9 Nov 2016 05:18:25 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34196 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751725AbcKIKST (ORCPT ); Wed, 9 Nov 2016 05:18:19 -0500 Date: Wed, 9 Nov 2016 11:18:14 +0100 From: Daniel Vetter To: Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Sumit Semwal , Maarten Lankhorst , Brian Starkey , Gustavo Padovan Subject: Re: [PATCH v7 3/3] drm/fence: add out-fences support Message-ID: <20161109101814.g5bqvpm46zz2xesg@phenom.ffwll.local> Mail-Followup-To: Gustavo Padovan , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Daniel Stone , Rob Clark , Greg Hackmann , John Harrison , laurent.pinchart@ideasonboard.com, seanpaul@google.com, marcheu@google.com, m.chehab@samsung.com, Sumit Semwal , Maarten Lankhorst , Brian Starkey , Gustavo Padovan References: <1478588090-8664-1-git-send-email-gustavo@padovan.org> <1478588090-8664-4-git-send-email-gustavo@padovan.org> <20161108131549.lvxuqvabb43esnvx@phenom.ffwll.local> <20161109023911.GO3327@joana> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161109023911.GO3327@joana> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2389 Lines: 66 On Wed, Nov 09, 2016 at 11:39:11AM +0900, Gustavo Padovan wrote: > > On Tue, Nov 08, 2016 at 03:54:50PM +0900, Gustavo Padovan wrote: > > > + if (!access_ok(VERIFY_WRITE, fence_ptr, sizeof(*fence_ptr))) > > > + return -EFAULT; > > > > Same comment about igt coverage I made for patch 1, but with > > s/in-fence/out-fence/, and s/~0ULL/8/. I picked 8 as an invalid address != > > NULL. > > > > And the testcase need to cover all possible combinations of output event > > generation, i.e. out-fence, event and out-fence+event. So 3x3=9 testcases > > for this I think. > > out-fence and event. so 2x2=4 ;) 3 different igt modes I've counted: - wrong prop after correct fence prop (early failure) - atomic_check fails (late failure) - success With 3 kinds of events: - fence only - event only - both - which might show up some bug if you bail out after e.g. handling fences, but before handling events and then leak. Hence 3x3 ;-) But if some of these aren't reasonable I'm ok with leaving them out, too. > > > +static void unprepare_crtc_signaling(struct drm_device *dev, > > > + struct drm_atomic_state *state, > > > + struct drm_out_fence_state *fence_state) > > > +{ > > > + struct drm_crtc *crtc; > > > + struct drm_crtc_state *crtc_state; > > > + int i; > > > + > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > + /* > > > + * TEST_ONLY and PAGE_FLIP_EVENT are mutually > > > + * exclusive, if they weren't, this code should be > > > + * called on success for TEST_ONLY too. > > > + */ > > > + if (crtc_state->event) > > > + drm_event_cancel_free(dev, > > > + &crtc_state->event->base); > > > + } > > > + > > > + for (i = 0; fence_state[i].out_fence_ptr; i++) { > > > > This goes boom if you have fences set for every crtc, because then this > > check will walk past the end of the array and do something undefined. You > > need to manually count how many of these slots are set (and might want to > > switch to a krealloc pattern while at it). Sounds like it needs an igt. > > On the fd_install loop I was also checking for i < > dev->mode_config.num_crtcs but forgot to add that here. However having a > num_fences is a better solution, I'll add that. And adding num_fence will be a good prep for writeback fences from Brian, too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch