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=-13.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 0C0D7C433B4 for ; Thu, 29 Apr 2021 17:16:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 4F7D2613F0 for ; Thu, 29 Apr 2021 17:16:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F7D2613F0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 23CC46F400; Thu, 29 Apr 2021 17:16:43 +0000 (UTC) Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by gabe.freedesktop.org (Postfix) with ESMTPS id 480776F400 for ; Thu, 29 Apr 2021 17:16:41 +0000 (UTC) Received: by mail-wr1-x436.google.com with SMTP id q9so13541108wrs.6 for ; Thu, 29 Apr 2021 10:16:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=yZg4I7GGuoymyvYn0QnJ2u8BIQYtSonX5/395f5LN1s=; b=lxPMXVCQ3w9GG8PZORQtq3/JkmnOHRTpVVrsMF4arKHt49oLDOlg8cpZMFeFINLO0G B63kOzPcl1U4jO2IVVxh12E6Lb61NZAlk84+0xOaD1lelxFnURa7/Jky6Dw5vgbIxqkx UtXeNTEZp2pwHWeW9zvrN9lDjGCAXmzVIJJr4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=yZg4I7GGuoymyvYn0QnJ2u8BIQYtSonX5/395f5LN1s=; b=FxphveASWnrEeuMWRRKknmCnHqDb7t9aOSTc1kYAjiu9uY/LvS8R33miEwToHQhX7M nOu+0MHozO80KU+4tSZDNOkMtMlUiQF5uK6pSyRx3g2EAjXMBkdIZS/2VIjvTeeSbl56 RjlhwVWu/8bSfQvGjUcC6kRsbH/El/0PAt1Vh8fyUo3yff/Ksw69pM+w8b3mXsbe5HMx 3xDO9CwepIcbXgdN4MrDB7elRK/wxGPXDFOy8pY8hcOBiYpVmIHmD3RB0Pc9INMgcHFA ZccZRhWy+SEXaCQFlT/gTCPrBtjYjBPkgvcGi1Fri3Y2VhNmxnrMzWAd8fr/kvmI7PaT W4pQ== X-Gm-Message-State: AOAM532/3soBmzIl6iJTCymg3iuldq2tpqWsNNL5AnrbskTCW3vkls0M mEtV9r4SH4/VGfD/9k+L17ePrw== X-Google-Smtp-Source: ABdhPJyWBvxkjD/S34ShuBFduvAFP3WoxCqxegWISJu5gKlCS3SjUVhRnmfaw2qBmfDbPr3HdhHwqg== X-Received: by 2002:a5d:4308:: with SMTP id h8mr873115wrq.371.1619716599931; Thu, 29 Apr 2021 10:16:39 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id z15sm5372310wrv.39.2021.04.29.10.16.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Apr 2021 10:16:39 -0700 (PDT) Date: Thu, 29 Apr 2021 19:16:37 +0200 From: Daniel Vetter To: Jason Ekstrand Subject: Re: [Intel-gfx] [PATCH 14/21] drm/i915/gem: Return an error ptr from context_lookup Message-ID: References: <20210423223131.879208-1-jason@jlekstrand.net> <20210423223131.879208-15-jason@jlekstrand.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.10.32scarlett+ X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Intel GFX , Maling list - DRI developers Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, Apr 29, 2021 at 10:29:51AM -0500, Jason Ekstrand wrote: > On Thu, Apr 29, 2021 at 8:27 AM Daniel Vetter wrote: > > > > On Fri, Apr 23, 2021 at 05:31:24PM -0500, Jason Ekstrand wrote: > > > We're about to start doing lazy context creation which means contexts > > > get created in i915_gem_context_lookup and we may start having more > > > errors than -ENOENT. > > > > > > Signed-off-by: Jason Ekstrand > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 ++++++------ > > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++-- > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > drivers/gpu/drm/i915/i915_perf.c | 4 ++-- > > > 4 files changed, 11 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > index 3e883daab93bf..7929d5a8be449 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > @@ -2105,8 +2105,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, > > > int ret = 0; > > > > > > ctx = i915_gem_context_lookup(file_priv, args->ctx_id); > > > - if (!ctx) > > > - return -ENOENT; > > > + if (IS_ERR(ctx)) > > > + return PTR_ERR(ctx); > > > > > > switch (args->param) { > > > case I915_CONTEXT_PARAM_GTT_SIZE: > > > @@ -2174,8 +2174,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, > > > int ret; > > > > > > ctx = i915_gem_context_lookup(file_priv, args->ctx_id); > > > - if (!ctx) > > > - return -ENOENT; > > > + if (IS_ERR(ctx)) > > > + return PTR_ERR(ctx); > > > > > > ret = ctx_setparam(file_priv, ctx, args); > > > > > > @@ -2194,8 +2194,8 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, > > > return -EINVAL; > > > > > > ctx = i915_gem_context_lookup(file->driver_priv, args->ctx_id); > > > - if (!ctx) > > > - return -ENOENT; > > > + if (IS_ERR(ctx)) > > > + return PTR_ERR(ctx); > > > > > > /* > > > * We opt for unserialised reads here. This may result in tearing > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > > index 7024adcd5cf15..de14b26f3b2d5 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > > @@ -739,8 +739,8 @@ static int eb_select_context(struct i915_execbuffer *eb) > > > struct i915_gem_context *ctx; > > > > > > ctx = i915_gem_context_lookup(eb->file->driver_priv, eb->args->rsvd1); > > > - if (unlikely(!ctx)) > > > - return -ENOENT; > > > + if (unlikely(IS_ERR(ctx))) > > > + return PTR_ERR(ctx); > > > > > > eb->gem_context = ctx; > > > if (rcu_access_pointer(ctx->vm)) > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 8571c5c1509a7..004ed0e59c999 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > I just realized that I think __i915_gem_context_lookup_rcu doesn't have > > users anymore. Please make sure it's deleted. > > I deleted it in "drm/i915: Stop manually RCU banging in reset_stats_ioctl" Indeed that's the case, so looks all fine. The benefits of reviewing patches one-by-one :-/ -Daniel > > > > > @@ -1851,7 +1851,7 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) > > > ctx = NULL; > > > rcu_read_unlock(); > > > > > > - return ctx; > > > + return ctx ? ctx : ERR_PTR(-ENOENT); > > > } > > > > > > /* i915_gem_evict.c */ > > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > > > index 85ad62dbabfab..b86ed03f6a705 100644 > > > --- a/drivers/gpu/drm/i915/i915_perf.c > > > +++ b/drivers/gpu/drm/i915/i915_perf.c > > > @@ -3414,10 +3414,10 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf, > > > struct drm_i915_file_private *file_priv = file->driver_priv; > > > > > > specific_ctx = i915_gem_context_lookup(file_priv, ctx_handle); > > > - if (!specific_ctx) { > > > + if (IS_ERR(specific_ctx)) { > > > DRM_DEBUG("Failed to look up context with ID %u for opening perf stream\n", > > > ctx_handle); > > > - ret = -ENOENT; > > > + ret = PTR_ERR(specific_ctx); > > > > Yeah this looks like a nice place to integrate this. > > > > Reviewed-by: Daniel Vetter > > > > One thing we need to make sure in the next patch or thereabouts is that > > lookup can only return ENOENT or ENOMEM, but never EINVAL. I'll drop some > > bikesheds on that :-) > > I believe that is the case. All -EINVAL should be handled in the > proto-context code. > > --Jason > > > -Daniel > > > > > goto err; > > > } > > > } > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel