All of lore.kernel.org
 help / color / mirror / Atom feed
From: Luben Tuikov <ltuikov89@gmail.com>
To: Danilo Krummrich <dakr@redhat.com>,
	airlied@gmail.com, daniel@ffwll.ch, matthew.brost@intel.com,
	boris.brezillon@collabora.com, christian.koenig@amd.com,
	faith@gfxstrand.net
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control
Date: Thu, 9 Nov 2023 20:25:04 -0500	[thread overview]
Message-ID: <42d3b0c0-bad6-4ac2-b755-6cbfc5ec5524@gmail.com> (raw)
In-Reply-To: <e2ec5036-81bd-490b-bdc2-fc215e14b8b1@gmail.com>


[-- Attachment #1.1.1: Type: text/plain, Size: 2005 bytes --]

On 2023-11-09 19:57, Luben Tuikov wrote:
> On 2023-11-09 19:16, Danilo Krummrich wrote:
[snip]
>> @@ -667,6 +771,8 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   * drm_sched_job_init - init a scheduler job
>>   * @job: scheduler job to init
>>   * @entity: scheduler entity to use
>> + * @credits: the number of credits this job contributes to the schedulers
>> + * credit limit
>>   * @owner: job owner for debugging
>>   *
>>   * Refer to drm_sched_entity_push_job() documentation
>> @@ -684,7 +790,7 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   */
>>  int drm_sched_job_init(struct drm_sched_job *job,
>>  		       struct drm_sched_entity *entity,
>> -		       void *owner)
>> +		       u32 credits, void *owner)
>>  {
>>  	if (!entity->rq) {
>>  		/* This will most likely be followed by missing frames
>> @@ -695,7 +801,11 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>  		return -ENOENT;
>>  	}
>>  
>> +	if (unlikely(!credits))
>> +		return -EINVAL;
>> +
> 
> This will most likely result in bad user experience (read: blank screen),
> and debugging this would be really hard without something to go by
> in the kernel log.
> 
> (This was exactly the case with amdgpu when 56e449603f0ac5
> ("drm/sched: Convert the GPU scheduler to variable number of run-queues") 
> was being worked on and merged. Without the drm_err() on missing rq in
> the lines immediately before the hunk above returning -ENOENT, there
> was no indication why setting up an fb was failing very early on (blank screen).)
> 
> So it is best to print a "[drm] *ERROR* "-equivalent string in the logs,
> so that we can make a note of this, without relying on drivers, old and new, logging
> that drm_sched_job_init() failed.

If you add _exactly_ this,

	if (unlikely(!credits)) {
		pr_err("*ERROR* %s: credits cannot be 0!\n", __func__)
		return -EINVAL;
	}

You can add my,

Reviewed-by: Luben Tuikov <ltuikov89@gmail.com>

and push it.
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Luben Tuikov <ltuikov89@gmail.com>
To: Danilo Krummrich <dakr@redhat.com>,
	airlied@gmail.com, daniel@ffwll.ch, matthew.brost@intel.com,
	boris.brezillon@collabora.com, christian.koenig@amd.com,
	faith@gfxstrand.net
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control
Date: Thu, 9 Nov 2023 20:25:04 -0500	[thread overview]
Message-ID: <42d3b0c0-bad6-4ac2-b755-6cbfc5ec5524@gmail.com> (raw)
In-Reply-To: <e2ec5036-81bd-490b-bdc2-fc215e14b8b1@gmail.com>


[-- Attachment #1.1.1: Type: text/plain, Size: 2005 bytes --]

On 2023-11-09 19:57, Luben Tuikov wrote:
> On 2023-11-09 19:16, Danilo Krummrich wrote:
[snip]
>> @@ -667,6 +771,8 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   * drm_sched_job_init - init a scheduler job
>>   * @job: scheduler job to init
>>   * @entity: scheduler entity to use
>> + * @credits: the number of credits this job contributes to the schedulers
>> + * credit limit
>>   * @owner: job owner for debugging
>>   *
>>   * Refer to drm_sched_entity_push_job() documentation
>> @@ -684,7 +790,7 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs);
>>   */
>>  int drm_sched_job_init(struct drm_sched_job *job,
>>  		       struct drm_sched_entity *entity,
>> -		       void *owner)
>> +		       u32 credits, void *owner)
>>  {
>>  	if (!entity->rq) {
>>  		/* This will most likely be followed by missing frames
>> @@ -695,7 +801,11 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>  		return -ENOENT;
>>  	}
>>  
>> +	if (unlikely(!credits))
>> +		return -EINVAL;
>> +
> 
> This will most likely result in bad user experience (read: blank screen),
> and debugging this would be really hard without something to go by
> in the kernel log.
> 
> (This was exactly the case with amdgpu when 56e449603f0ac5
> ("drm/sched: Convert the GPU scheduler to variable number of run-queues") 
> was being worked on and merged. Without the drm_err() on missing rq in
> the lines immediately before the hunk above returning -ENOENT, there
> was no indication why setting up an fb was failing very early on (blank screen).)
> 
> So it is best to print a "[drm] *ERROR* "-equivalent string in the logs,
> so that we can make a note of this, without relying on drivers, old and new, logging
> that drm_sched_job_init() failed.

If you add _exactly_ this,

	if (unlikely(!credits)) {
		pr_err("*ERROR* %s: credits cannot be 0!\n", __func__)
		return -EINVAL;
	}

You can add my,

Reviewed-by: Luben Tuikov <ltuikov89@gmail.com>

and push it.
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

  reply	other threads:[~2023-11-10  1:25 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-10  0:16 [PATCH drm-misc-next v6] drm/sched: implement dynamic job-flow control Danilo Krummrich
2023-11-10  0:16 ` Danilo Krummrich
2023-11-10  0:57 ` Luben Tuikov
2023-11-10  0:57   ` Luben Tuikov
2023-11-10  1:25   ` Luben Tuikov [this message]
2023-11-10  1:25     ` Luben Tuikov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=42d3b0c0-bad6-4ac2-b755-6cbfc5ec5524@gmail.com \
    --to=ltuikov89@gmail.com \
    --cc=airlied@gmail.com \
    --cc=boris.brezillon@collabora.com \
    --cc=christian.koenig@amd.com \
    --cc=dakr@redhat.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=faith@gfxstrand.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.brost@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.