[PATCH v2 2/4] drm/scheduler: implement a function to modify sched list

[Luben Tuikov]

On 2020-03-05 01:23, Nirmoy wrote:
> 
> On 3/4/20 11:00 PM, Luben Tuikov wrote:
>> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>>> implement drm_sched_entity_modify_sched() which can modify existing
>>> sched_list with a different one. This is going to be helpful when
>>> userspace changes priority of a ctx/entity then driver can switch to
>>> corresponding hw shced list for that priority
>>>
>> This commit message should start with a capitalized sentence:
>> "implement" --> "Implement".  "can modify" --> "modifies"
>>
>> Also a spell check should be done on it before committing:
>> "shced" --> "sched".
>>
>> "then the driver", "to the corresponding", "HW scheduler".
>>
>> And the commit message paragraph should end with a period.
> Thanks!
>>

A difficult to see comment of yours above?
Why not add an empty line before and above your comments?
Are you trying to make them hard to find?

>>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>>> Reviewed-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c | 19 +++++++++++++++++++
>>>   include/drm/gpu_scheduler.h              |  4 ++++
>>>   2 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 63bccd201b97..b94312154e56 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>>   }
>>>   EXPORT_SYMBOL(drm_sched_entity_init);
>>>
>>> +/**
>>> + * drm_sched_entity_modify_sched - Modify sched of an entity
>>> + *
>> This empty line is unncessary.
>>
>>> + * @entity: scheduler entity to init
>>> + * @sched_list: the list of new drm scheds which will replace
>>> + *		existing entity->sched_list
>>> + * @num_sched_list: number of drm sched in sched_list
>>> + */
>>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> +				  struct drm_gpu_scheduler **sched_list,
>>> +				  unsigned int num_sched_list)
>>> +{
>>> +	WARN_ON(!num_sched_list || !sched_list);
>>> +
>>> +	entity->sched_list = sched_list;
>>> +	entity->num_sched_list = num_sched_list;
>>> +}
>>> +EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>>> +
>>>   /**
>>>    * drm_sched_entity_is_idle - Check if entity is idle
>>>    *
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 589be851f8a1..f70a84aaaf7a 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
>>>   int drm_sched_job_init(struct drm_sched_job *job,
>>>   		       struct drm_sched_entity *entity,
>>>   		       void *owner);
>>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> +				  struct drm_gpu_scheduler **sched_list,
>>> +                                  unsigned int num_sched_list);
>>> +
> This generally doesn't happen with my current vi config. I will be extra 
> careful.
>> Again, the argument list here is unaligned. Please align it

Another hard to see and find comment from you. Why? Why not
take the extra care and diligence to add an empty line before
and after your comment, so that it is easy to see? Why do you do this?

I think it is "generally" unaligned coming from you as you can
see in the reviews in this list. Configure "vi" or use something
else, "Emacs", etc.

Regards,
Luben

>> correctly. The correct indentation would add the maximum number of
>> leading TAB chars followed by the 0 to 7 spaces, to align
>> the argument list to the first argument in the opening parenthesis.
>> The LKCS describes this and also has the settings for Emacs.
>> In Emacs, pressing the TAB key aligns, after which pressing it more
>> does nothing (on an aligned line).
>>
>> Regards,
>> Luben
>>
>>
>>>   void drm_sched_job_cleanup(struct drm_sched_job *job);
>>>   void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>>> --
>>> 2.25.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cbe8445945c194bd8b3cc08d7bf7109c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364557354217&sdata=UAxV2NtkNcj6VXAPhDbk4GliUOnPEfuLOH4BVuOrqdw%3D&reserved=0
>>>