From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A5DC91DA106 for ; Fri, 13 Sep 2024 12:21:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726230079; cv=none; b=ZdTiKngeMNCl9kdeistTI8/JLK6NYxZxOfKCU+EIyQqSRNO6DfrQ8RpyreMujodgNsApUnGvLCJ9/wwncoIA9kcdcZSwk3B2gub7FV1Ln57N1NPRLam5KZTXGfwTS/Taz4BsLFlObnHXjqt9C4eAy6lZEdgZ5M6HGQ8Lr/6UjOU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1726230079; c=relaxed/simple; bh=8fGbJEAot3YIDsB1Y/oZ33VF8cPl4VKopjOCG8P1X4I=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=PnXDN4r4InvFE47Zy8jD1AOQsLkuDr1pzb8HS6j7BuVlyR8QiQL8C6bPCQJsO+xKoMAuq6OPnSmE/OiOxcHNSGnFOwjO1yyr5U8wd9jogwgb6Xj7r7lbwy3LYij8dyJ/qOf26WMiuRp6s1qrU8bbyrD+vl2NWVP5n8sGM/dhaYQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net; spf=none smtp.mailfrom=ursulin.net; dkim=pass (2048-bit key) header.d=ursulin-net.20230601.gappssmtp.com header.i=@ursulin-net.20230601.gappssmtp.com header.b=yfmfE6VF; arc=none smtp.client-ip=209.85.128.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ursulin.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ursulin-net.20230601.gappssmtp.com header.i=@ursulin-net.20230601.gappssmtp.com header.b="yfmfE6VF" Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-42cbface8d6so11303655e9.3 for ; Fri, 13 Sep 2024 05:21:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ursulin-net.20230601.gappssmtp.com; s=20230601; t=1726230076; x=1726834876; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=C2xf8rE0I0Vf7hYWniCA9aZSnwb/1pY5PjnZIFr0SCw=; b=yfmfE6VFBiC2zPBemlX3Sc1HDKQ9mfkVxyYMzNjHOpB9s/tGDlktD0vs7Byt5Y3JJt bJRRIl0Yz2ktGDYpAtJLlKJgYkEWG4uDzwmFOvcO+8fmjMsGSl33K7NRwl0XUawr3qZ8 j2k6T+LB7c+jFNJSPQwTxCcWDr38pb5twLuVcHemD43haGJZcNr392qBG2lzyu/ao6pt Mxjv+oJh09XYwcHS3RRLpTWEaaMCuxkysQLzihiR0eXHqIeAXBN3JiFfokWcgyeZfmWw 7pn6rTD6PB+UHDBr5oV84tarDzOMpxOHUuCy4krnuCKBBIVMw4f3EiPjaHf/ZCtIcIDg Ai7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726230076; x=1726834876; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=C2xf8rE0I0Vf7hYWniCA9aZSnwb/1pY5PjnZIFr0SCw=; b=u4z5ltKjV6KsqvZLt7kb3tpRvGBKtKTOFapE+wcnaKjvHlDZL/r8i296voGqbvcArs 7zadWJPsCbU4/0F/PLOVHL3OpnEaimUUThH7nBiTj8wEhH1tldBBh/D3RJiRqKoPc727 i/irhRyaJbyTDcJxI8yHddoPf4eDdDCAdqiY9PV1u6SA5toog9jS4uQ7QV2K2XZdvxUt Z7mbHc76pcucGF2o9bBeqz/0Ujepa4D4eb4SpeBPv7VxdYU7TkY57e38MlQP469K1Jpr WwMgF5UniAd1ZnEf012XTO14IoFd4r3pLrEZlatB+l7qbxOrVHS0eS1HitOhhG4lKrIo oMQQ== X-Forwarded-Encrypted: i=1; AJvYcCXhA6TLuVTXwfOXoKHHFBkiSb5efH6e4y471UoLJPh9l2hLHNEyzNTn3rhZjooA+uL3Dej8lz/wtXOnd5g=@vger.kernel.org X-Gm-Message-State: AOJu0YxEgSEGvS2XNwF3eN6T9d8syRTTQFVwENmI2Of1eLBtD1OhTFvj r9ix7htG06BJ1q3loj8ZSU2L5OVTIIiAK/LE9ILQ2yZghFJfIe7GjI7bd+6Vp0I= X-Google-Smtp-Source: AGHT+IHIKarTEBaRn58s3UvWoCwjM6vihgga95P3kKVtfHdcVOOvOS/CWQUaLXKidGqAASKMpnC9CA== X-Received: by 2002:a05:600c:4590:b0:42c:bb58:a077 with SMTP id 5b1f17b1804b1-42d907221a9mr26390825e9.14.1726230075344; Fri, 13 Sep 2024 05:21:15 -0700 (PDT) Received: from [192.168.0.101] ([90.241.98.187]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42d9b05d6fcsm22995435e9.15.2024.09.13.05.21.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 13 Sep 2024 05:21:14 -0700 (PDT) Message-ID: <4e85edc5-a155-460a-b470-aa7247b83230@ursulin.net> Date: Fri, 13 Sep 2024 13:21:14 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] drm/sched: warn about drm_sched_job_init()'s partial init To: Philipp Stanner , Luben Tuikov , Matthew Brost , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Danilo Krummrich Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20240828094133.17402-2-pstanner@redhat.com> <20240828094133.17402-4-pstanner@redhat.com> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: <20240828094133.17402-4-pstanner@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 28/08/2024 10:41, Philipp Stanner wrote: > drm_sched_job_init()'s name suggests that after the function succeeded, > parameter "job" will be fully initialized. This is not the case; some > members are only later set, notably "job->sched" by drm_sched_job_arm(). > > Document that drm_sched_job_init() does not set all struct members. > > Document that job->sched in particular is uninitialized before > drm_sched_job_arm(). > > Signed-off-by: Philipp Stanner > --- > Changes in v2: > - Change grammar in the new comments a bit. > --- > drivers/gpu/drm/scheduler/sched_main.c | 4 ++++ > include/drm/gpu_scheduler.h | 7 +++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > index b0c8ad10b419..721373938c1e 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -781,6 +781,10 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs); > * Drivers must make sure drm_sched_job_cleanup() if this function returns > * successfully, even when @job is aborted before drm_sched_job_arm() is called. > * > + * Note that this function does not assign a valid value to each struct member > + * of struct drm_sched_job. Take a look at that struct's documentation to see > + * who sets which struct member with what lifetime. First sentence is fine, but the second I don't see the those details in struct drm_sched_job. (And I am not saying that they must be listed. IMO at some point it is better to have a high level overview than describe the lifetime rules with individual members.) > + * > * WARNING: amdgpu abuses &drm_sched.ready to signal when the hardware > * has died, which can mean that there's no valid runqueue for a @entity. > * This function returns -ENOENT in this case (which probably should be -EIO as > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 5acc64954a88..04a268cd22f1 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -337,6 +337,13 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); > struct drm_sched_job { > struct spsc_node queue_node; > struct list_head list; > + > + /* > + * The scheduler this job is or will be scheduled on. > + * > + * Gets set by drm_sched_arm(). Valid until the scheduler's backend_ops > + * callback "free_job()" has been called. This is interesting - I was not sure where lifetime for job->sched is defined and couldn't find it browsing around. Where did you find the clues to tie it to the free_job() callback? Regards, Tvrtko > + */ > struct drm_gpu_scheduler *sched; > struct drm_sched_fence *s_fence; >