Re: [PATCH v6 5/7] drm/sched: Split free_job into own work item
Hi,
On 2023-10-19 12:55, Matthew Brost wrote:
> On Wed, Oct 18, 2023 at 09:25:36PM -0400, Luben Tuikov wrote:
>> Hi,
>>
>> On 2023-10-17 11:09, Matthew Brost wrote:
>>> Rather than call free_job and run_job in same work item have a dedicated
>>> work item for each. This aligns with the design and intended use of work
>>> queues.
>>>
>>> v2:
>>>- Test for DMA_FENCE_FLAG_TIMESTAMP_BIT before setting
>>> timestamp in free_job() work item (Danilo)
>>> v3:
>>> - Drop forward dec of drm_sched_select_entity (Boris)
>>> - Return in drm_sched_run_job_work if entity NULL (Boris)
>>> v4:
>>> - Replace dequeue with peek and invert logic (Luben)
>>> - Wrap to 100 lines (Luben)
>>> - Update comments for *_queue / *_queue_if_ready functions (Luben)
>>>
>>> Signed-off-by: Matthew Brost
>>> ---
>>> drivers/gpu/drm/scheduler/sched_main.c | 287 +++--
>>> include/drm/gpu_scheduler.h| 8 +-
>>> 2 files changed, 178 insertions(+), 117 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 273e0fbc4eab..b1b8d9f96da5 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq
>>> *rq,
>>> * drm_sched_rq_select_entity_rr - Select an entity which could provide a
>>> job to run
>>> *
>>> * @rq: scheduler run queue to check.
>>> + * @peek: Just find, don't set to current.
>>
>> The "peek" rename is good--thanks!
>>
>>> *
>>> * Try to find a ready entity, returns NULL if none found.
>>> */
>>> static struct drm_sched_entity *
>>> -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>> +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool peek)
>>> {
>>> struct drm_sched_entity *entity;
>>>
>>> @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>> if (entity) {
>>> list_for_each_entry_continue(entity, >entities, list) {
>>> if (drm_sched_entity_is_ready(entity)) {
>>> - rq->current_entity = entity;
>>> - reinit_completion(>entity_idle);
>>> + if (!peek) {
>>> + rq->current_entity = entity;
>>> + reinit_completion(>entity_idle);
>>> + }
>>> spin_unlock(>lock);
>>> return entity;
>>> }
>>> @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>> list_for_each_entry(entity, >entities, list) {
>>>
>>> if (drm_sched_entity_is_ready(entity)) {
>>> - rq->current_entity = entity;
>>> - reinit_completion(>entity_idle);
>>> + if (!peek) {
>>> + rq->current_entity = entity;
>>> + reinit_completion(>entity_idle);
>>> + }
>>> spin_unlock(>lock);
>>> return entity;
>>> }
>>> @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>> * drm_sched_rq_select_entity_fifo - Select an entity which provides a job
>>> to run
>>> *
>>> * @rq: scheduler run queue to check.
>>> + * @peek: Just find, don't set to current.
>>> *
>>> * Find oldest waiting ready entity, returns NULL if none found.
>>> */
>>> static struct drm_sched_entity *
>>> -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>> +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool peek)
>>> {
>>> struct rb_node *rb;
>>>
>>> @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
>>> *rq)
>>>
>>> entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
>>> if (drm_sched_entity_is_ready(entity)) {
>>> - rq->current_entity = entity;
>>> - reinit_completion(>entity_idle);
>>> + if (!peek) {
>>> + rq->current_entity = entity;
>>> + reinit_completion(>entity_idle);
>>> + }
>>> break;
>>> }
>>> }
>>> @@ -282,13 +290,98 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
>>> *rq)
>>> }
>>>
>>> /**
>>> - * drm_sched_wqueue_enqueue - enqueue scheduler submission
>>> + * drm_sched_run_job_queue - enqueue run-job work
>>
>> Hmm... so this removes the change from patch 1 in this series, and as such
>> obviates patch 1.
>>
>> Do you want to go with "run_job_queue" and the names introduced here?
>>
>
> Yes, I like the run_job_queue name here.
>
>> In this case, we can have that in patch 1 instead, and this patch
>> would only split the "free job" into its own work item.
>>
>
> Sure, so s/drm_sched_wqueue_enqueue/drm_sched_run_job_queue
Re: [PATCH] drm/amdgpu: Remove redundant call to priority_is_valid()
On 2023-10-20 12:37, Alex Deucher wrote:
> On Tue, Oct 17, 2023 at 9:22 PM Luben Tuikov wrote:
>>
>> Remove a redundant call to amdgpu_ctx_priority_is_valid() from
>> amdgpu_ctx_priority_permit(), which is called from amdgpu_ctx_init() which is
>> called from amdgpu_ctx_alloc() which is called from amdgpu_ctx_ioctl(), where
>> we've called amdgpu_ctx_priority_is_valid() already first thing in the
>> function.
>>
>> Cc: Alex Deucher
>> Cc: Christian König
>> Signed-off-by: Luben Tuikov
>
> Please push this to drm-misc since it depends on your previous patches.
Done!
Pushed to drm-misc-fixes, where the other two landed.
Regards,
Luben
>
> Alex
>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 15 ---
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 68db924161ef66..4c6ffca97c4512 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -56,6 +56,10 @@ bool amdgpu_ctx_priority_is_valid(int32_t ctx_prio)
>> return true;
>> default:
>> case AMDGPU_CTX_PRIORITY_UNSET:
>> + /* UNSET priority is not valid and we don't carry that
>> +* around, but set it to NORMAL in the only place this
>> +* function is called, amdgpu_ctx_ioctl().
>> +*/
>> return false;
>> }
>> }
>> @@ -96,9 +100,6 @@ amdgpu_ctx_to_drm_sched_prio(int32_t ctx_prio)
>> static int amdgpu_ctx_priority_permit(struct drm_file *filp,
>> int32_t priority)
>> {
>> - if (!amdgpu_ctx_priority_is_valid(priority))
>> - return -EINVAL;
>> -
>> /* NORMAL and below are accessible by everyone */
>> if (priority <= AMDGPU_CTX_PRIORITY_NORMAL)
>> return 0;
>> @@ -625,8 +626,6 @@ static int amdgpu_ctx_query2(struct amdgpu_device *adev,
>> return 0;
>> }
>>
>> -
>> -
>> static int amdgpu_ctx_stable_pstate(struct amdgpu_device *adev,
>> struct amdgpu_fpriv *fpriv, uint32_t id,
>> bool set, u32 *stable_pstate)
>> @@ -669,8 +668,10 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
>> id = args->in.ctx_id;
>> priority = args->in.priority;
>>
>> - /* For backwards compatibility reasons, we need to accept
>> -* ioctls with garbage in the priority field */
>> + /* For backwards compatibility, we need to accept ioctls with garbage
>> +* in the priority field. Garbage values in the priority field,
>> result
>> +* in the priority being set to NORMAL.
>> +*/
>> if (!amdgpu_ctx_priority_is_valid(priority))
>> priority = AMDGPU_CTX_PRIORITY_NORMAL;
>>
>>
>> base-commit: 915718484b8fa1eede4499a939e2e4fc0d85caa4
>> prerequisite-patch-id: a36f628997d923f66da5342e760e8b45ff959fb8
>> prerequisite-patch-id: f15148c302329c0c60d86040571c61d367bd05e7
>> --
>> 2.42.0
>>
Re: [PATCH] drm/panfrost: Remove incorrect IS_ERR() check
On 20.10.2023 11:44, Steven Price wrote:
> sg_page_iter_page() doesn't return an error code, so the IS_ERR() check
> is wrong and the error path will never be executed. This also allows
> simplifying the code to remove the local variable 'page'.
>
> CC: Adrián Larumbe
> Reported-by: Dan Carpenter
> Closes:
> https://lore.kernel.org/r/376713ff-9a4f-4ea3-b097-fb5efb685d95@moroto.mountain
> Signed-off-by: Steven Price
Reviewed-by: Adrián Larumbe
Tested-by: Adrián Larumbe
> ---
> drivers/gpu/drm/panfrost/panfrost_dump.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_dump.c
> b/drivers/gpu/drm/panfrost/panfrost_dump.c
> index e7942ac449c6..47751302f1bc 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_dump.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_dump.c
> @@ -220,16 +220,8 @@ void panfrost_core_dump(struct panfrost_job *job)
>
> iter.hdr->bomap.data[0] = bomap - bomap_start;
>
> - for_each_sgtable_page(bo->base.sgt, _iter, 0) {
> - struct page *page = sg_page_iter_page(_iter);
> -
> - if (!IS_ERR(page)) {
> - *bomap++ = page_to_phys(page);
> - } else {
> - dev_err(pfdev->dev, "Panfrost Dump: wrong
> page\n");
> - *bomap++ = 0;
> - }
> - }
> + for_each_sgtable_page(bo->base.sgt, _iter, 0)
> + *bomap++ = page_to_phys(sg_page_iter_page(_iter));
>
> iter.hdr->bomap.iova = mapping->mmnode.start << PAGE_SHIFT;
>
> --
> 2.34.1
[Bug 201957] amdgpu: ring gfx timeout
https://bugzilla.kernel.org/show_bug.cgi?id=201957 jeremy boyd (j...@jerdboyd.com) changed: What|Removed |Added CC||j...@jerdboyd.com --- Comment #97 from jeremy boyd (j...@jerdboyd.com) --- Hello, I'm having this same issue with my thinkpad z16 laptop, Ryzen 6850H and Radeon RX 6500M graphics card. I do not use the laptop for gaming but for audio and video editing. I have not had trouble with any video editing software but I can easily reproduce the issue by loading up Ardour or Mixbus32C and either leaving it alone or working. After 15 minutes the screen freezes although audio will continue for a time. At this point Ardour or Mixbus will close and I can continue using the machine. If I load up either program again it will fail again, usually within a couple minutes and the whole laptop will freeze up until I ctrl-alt-F2 to get to a terminal prompt. The issue always happens when Im recording audio with an HDMI device attached and 90% of the time without HDMI I will attempt to set this kernel parameter amdgpu.mcbp=0 and report back. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] fbdev/offb: Simplify offb_init_fb()
On 10/21/23 08:53, Christophe JAILLET wrote: Turn a strcpy()+strncat()+'\0' into an equivalent snprintf(). Signed-off-by: Christophe JAILLET applied. Thanks! Helge
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Uwe, On 10/19/23 12:51, Uwe Kleine-König wrote: > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: >> Hi Sean, >> >> On 10/17/23 11:17, Sean Young wrote: >>> Some drivers require sleeping, for example if the pwm device is connected >>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc >>> with the generated IR signal when sleeping occurs. >>> >>> This patch makes it possible to use pwm when the driver does not sleep, >>> by introducing the pwm_can_sleep() function. >>> >>> Signed-off-by: Sean Young >> >> I have no objection to this patch by itself, but it seems a bit >> of unnecessary churn to change all current callers of pwm_apply_state() >> to a new API. > > The idea is to improve the semantic of the function name, see > https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rly...@pengutronix.de > for more context. Hmm, so the argument here is that the GPIO API has this, but GPIOs generally speaking can be set atomically, so there not being able to set it atomically is special. OTOH we have many many many other kernel functions which may sleep and we don't all postfix them with _can_sleep. And for PWM controllers pwm_apply_state is IMHO sorta expected to sleep. Many of these are attached over I2C so things will sleep, others have a handshake to wait for the current dutycycle to end before you can apply a second change on top of an earlier change during the current dutycycle which often also involves sleeping. So the natural/expeected thing for pwm_apply_state() is to sleep and thus it does not need a postfix for this IMHO. > I think it's very subjective if you consider this > churn or not. I consider it churn because I don't think adding a postfix for what is the default/expected behavior is a good idea (with GPIOs not sleeping is the expected behavior). I agree that this is very subjective and very much goes into the territory of bikeshedding. So please consider the above my 2 cents on this and lets leave it at that. > While it's nice to have every caller converted in a single > step, I'd go for > > #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state) > > , keep that macro for a while and convert all users step by step. This > way we don't needlessly break oot code and the changes to convert to the > new API can go via their usual trees without time pressure. I don't think there are enough users of pwm_apply_state() to warrant such an exercise. So if people want to move ahead with the _can_sleep postfix addition (still not a fan) here is my acked-by for the drivers/platform/x86 changes, for merging this through the PWM tree in a single commit: Acked-by: Hans de Goede Regards, Hans
[PATCH] fbdev/offb: Simplify offb_init_fb()
Turn a strcpy()+strncat()+'\0' into an equivalent snprintf().
Signed-off-by: Christophe JAILLET
---
This patch is *not* even compile tested because cross-compiling leads to
some errors like on my machine:
cc1: error: cannot load plugin
./scripts/gcc-plugins/randomize_layout_plugin.so:
./scripts/gcc-plugins/randomize_layout_plugin.so: undefined symbol:
_ZNK6frange6acceptERK14vrange_visitor
So review with care!
---
drivers/video/fbdev/offb.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
index dcb1b81d35db..b421b46d88ef 100644
--- a/drivers/video/fbdev/offb.c
+++ b/drivers/video/fbdev/offb.c
@@ -423,11 +423,9 @@ static void offb_init_fb(struct platform_device *parent,
const char *name,
fix = >fix;
var = >var;
- if (name) {
- strcpy(fix->id, "OFfb ");
- strncat(fix->id, name, sizeof(fix->id) - sizeof("OFfb "));
- fix->id[sizeof(fix->id) - 1] = '\0';
- } else
+ if (name)
+ snprintf(fix->id, sizeof(fix->id), "OFfb %s", name);
+ else
snprintf(fix->id, sizeof(fix->id), "OFfb %pOFn", dp);
--
2.34.1
