Re: [Intel-gfx] linux-next: build failure after merge of the drm-misc tree
On 2023-11-26 18:38, Stephen Rothwell wrote:
> Hi all,
>
> After merging the drm-misc tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> drivers/gpu/drm/nouveau/nouveau_sched.c:21:41: error:
> 'DRM_SCHED_PRIORITY_MIN' undeclared here (not in a function); did you mean
> 'DRM_SCHED_PRIORITY_LOW'?
>21 | NOUVEAU_SCHED_PRIORITY_SINGLE = DRM_SCHED_PRIORITY_MIN,
> | ^~
> | DRM_SCHED_PRIORITY_LOW
>
> Caused by commit
>
> fe375c74806d ("drm/sched: Rename priority MIN to LOW")
>
> I have used the drm-misc tree from next-20231124 for today.
I posted a fix for this yesterday:
https://lore.kernel.org/r/20231125192246.87268-2-ltuiko...@gmail.com
--
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-24 08:20, Jani Nikula wrote: > On Wed, 22 Nov 2023, Luben Tuikov wrote: >> On 2023-11-22 07:00, Maxime Ripard wrote: >>> Hi Luben, >>> >>> On Thu, Nov 16, 2023 at 09:27:58AM +0100, Daniel Vetter wrote: >>>> On Thu, Nov 16, 2023 at 09:11:43AM +0100, Maxime Ripard wrote: >>>>> On Tue, Nov 14, 2023 at 06:46:21PM -0500, Luben Tuikov wrote: >>>>>> On 2023-11-13 22:08, Stephen Rothwell wrote: >>>>>>> BTW, cherry picking commits does not avoid conflicts - in fact it can >>>>>>> cause conflicts if there are further changes to the files affected by >>>>>>> the cherry picked commit in either the tree/branch the commit was >>>>>>> cheery picked from or the destination tree/branch (I have to deal with >>>>>>> these all the time when merging the drm trees in linux-next). Much >>>>>>> better is to cross merge the branches so that the patch only appears >>>>>>> once or have a shared branches that are merged by any other branch that >>>>>>> needs the changes. >>>>>>> >>>>>>> I understand that things are not done like this in the drm trees :-( >>>>>> >>>>>> Hi Stephen, >>>>>> >>>>>> Thank you for the clarification--understood. I'll be more careful in the >>>>>> future. >>>>>> Thanks again! :-) >>>>> >>>>> In this case, the best thing to do would indeed have been to ask the >>>>> drm-misc maintainers to merge drm-misc-fixes into drm-misc-next. >>>>> >>>>> We're doing that all the time, but we're not ubiquitous so you need to >>>>> ask us :) >>>>> >>>>> Also, dim should have caught that when you pushed the branch. Did you >>>>> use it? >>>> >>>> Yeah dim must be used, exactly to avoid these issues. Both for applying >>>> patches (so not git am directly, or cherry-picking from your own >>>> development branch), and for pushing. The latter is even checked for by >>>> the server (dim sets a special push flag which is very long and contains a >>>> very clear warning if you bypass it). >>>> >>>> If dim was used, this would be a bug in the dim script that we need to >>>> fix. >>> >>> It would be very useful for you to explain what happened here so we >>> improve the tooling or doc and can try to make sure it doesn't happen >>> again >>> >>> Maxime >> >> There is no problem with the tooling--I just forced the commit in. > > Wait what? > > What do you mean by forcing the commit in? Bypass dim? > > If yes, please *never* do that when you're dealing with dim managed > branches. That's part of the deal for getting commit access, along with > following all the other maintainer tools documentation. Hi Jani, I only use dim, ever. -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-22 07:00, Maxime Ripard wrote: > Hi Luben, > > On Thu, Nov 16, 2023 at 09:27:58AM +0100, Daniel Vetter wrote: >> On Thu, Nov 16, 2023 at 09:11:43AM +0100, Maxime Ripard wrote: >>> On Tue, Nov 14, 2023 at 06:46:21PM -0500, Luben Tuikov wrote: >>>> On 2023-11-13 22:08, Stephen Rothwell wrote: >>>>> BTW, cherry picking commits does not avoid conflicts - in fact it can >>>>> cause conflicts if there are further changes to the files affected by >>>>> the cherry picked commit in either the tree/branch the commit was >>>>> cheery picked from or the destination tree/branch (I have to deal with >>>>> these all the time when merging the drm trees in linux-next). Much >>>>> better is to cross merge the branches so that the patch only appears >>>>> once or have a shared branches that are merged by any other branch that >>>>> needs the changes. >>>>> >>>>> I understand that things are not done like this in the drm trees :-( >>>> >>>> Hi Stephen, >>>> >>>> Thank you for the clarification--understood. I'll be more careful in the >>>> future. >>>> Thanks again! :-) >>> >>> In this case, the best thing to do would indeed have been to ask the >>> drm-misc maintainers to merge drm-misc-fixes into drm-misc-next. >>> >>> We're doing that all the time, but we're not ubiquitous so you need to >>> ask us :) >>> >>> Also, dim should have caught that when you pushed the branch. Did you >>> use it? >> >> Yeah dim must be used, exactly to avoid these issues. Both for applying >> patches (so not git am directly, or cherry-picking from your own >> development branch), and for pushing. The latter is even checked for by >> the server (dim sets a special push flag which is very long and contains a >> very clear warning if you bypass it). >> >> If dim was used, this would be a bug in the dim script that we need to >> fix. > > It would be very useful for you to explain what happened here so we > improve the tooling or doc and can try to make sure it doesn't happen > again > > Maxime There is no problem with the tooling--I just forced the commit in. -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-16 04:22, Maxime Ripard wrote:
> Hi,
>
> On Mon, Nov 13, 2023 at 09:56:32PM -0500, Luben Tuikov wrote:
>> On 2023-11-13 21:45, Stephen Rothwell wrote:
>>> Hi Luben,
>>>
>>> On Mon, 13 Nov 2023 20:32:40 -0500 Luben Tuikov wrote:
>>>>
>>>> On 2023-11-13 20:08, Luben Tuikov wrote:
>>>>> On 2023-11-13 15:55, Stephen Rothwell wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Commit
>>>>>>
>>>>>> 0da611a87021 ("dma-buf: add dma_fence_timestamp helper")
>>>>>>
>>>>>> is missing a Signed-off-by from its committer.
>>>>>>
>>>>>
>>>>> In order to merge the scheduler changes necessary for the Xe driver,
>>>>> those changes
>>>>> were based on drm-tip, which included this change from drm-misc-fixes,
>>>>> but which
>>>>> wasn't present in drm-misc-next.
>>>>>
>>>>> I didn't want to create a merge conflict between drm-misc-next and
>>>>> drm-misc-fixes,
>>>>> when pulling that change from drm-misc-next to drm-misc-fixes, so that I
>>>>> can apply
>>>>
>>>> ... when pulling that change from from drm-misc-fixes into drm-misc-next,
>>>> so that I can apply...
>>>>
>>>>> the Xe scheduler changes on top of drm-misc-next.
>>>>
>>>> The change in drm-misc-fixes is b83ce9cb4a465b. The latter is contained
>>>> in linus-master, and in drm-misc-fixes, while the former is in
>>>> drm-misc-next.
>>>> When we merge linus-master/drm-misc-fixes into drm-misc-next, or whichever
>>>> way
>>>> it happens, I'd like to avoid a merge conflict, but wanted to expedite the
>>>> changes
>>>> for Xe.
>>>
>>> None of that is relevant ... if you commit a patch to a tree that will
>>> be in the linux kernel tree, you must add your Signed-off-by to the commit.
>>
>> Noted!
>>
>> So I always do this when I do git-am and such, but wasn't sure for this one
>> single cherry-pick whose
>> original author was the committer in drm-misc-fixes, but will add my
>> Signed-off-by in those
>> rare circumstances.
>>
>> Thanks for the clarification!
>
> In order to move forward with this, can you provide your SoB here for
> that patch so that we can at least point to it in the drm-misc-next PR?
>
> Maxime
Signed-off-by: Luben Tuikov
--
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-13 22:08, Stephen Rothwell wrote: > Hi Luben, > > BTW, cherry picking commits does not avoid conflicts - in fact it can > cause conflicts if there are further changes to the files affected by > the cherry picked commit in either the tree/branch the commit was > cheery picked from or the destination tree/branch (I have to deal with > these all the time when merging the drm trees in linux-next). Much > better is to cross merge the branches so that the patch only appears > once or have a shared branches that are merged by any other branch that > needs the changes. > > I understand that things are not done like this in the drm trees :-( Hi Stephen, Thank you for the clarification--understood. I'll be more careful in the future. Thanks again! :-) -- Regards, Luben OpenPGP_0x4C15479431A334AF.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-13 21:45, Stephen Rothwell wrote:
> Hi Luben,
>
> On Mon, 13 Nov 2023 20:32:40 -0500 Luben Tuikov wrote:
>>
>> On 2023-11-13 20:08, Luben Tuikov wrote:
>>> On 2023-11-13 15:55, Stephen Rothwell wrote:
>>>> Hi all,
>>>>
>>>> Commit
>>>>
>>>> 0da611a87021 ("dma-buf: add dma_fence_timestamp helper")
>>>>
>>>> is missing a Signed-off-by from its committer.
>>>>
>>>
>>> In order to merge the scheduler changes necessary for the Xe driver, those
>>> changes
>>> were based on drm-tip, which included this change from drm-misc-fixes, but
>>> which
>>> wasn't present in drm-misc-next.
>>>
>>> I didn't want to create a merge conflict between drm-misc-next and
>>> drm-misc-fixes,
>>> when pulling that change from drm-misc-next to drm-misc-fixes, so that I
>>> can apply
>>
>> ... when pulling that change from from drm-misc-fixes into drm-misc-next, so
>> that I can apply...
>>
>>> the Xe scheduler changes on top of drm-misc-next.
>>
>> The change in drm-misc-fixes is b83ce9cb4a465b. The latter is contained
>> in linus-master, and in drm-misc-fixes, while the former is in drm-misc-next.
>> When we merge linus-master/drm-misc-fixes into drm-misc-next, or whichever
>> way
>> it happens, I'd like to avoid a merge conflict, but wanted to expedite the
>> changes
>> for Xe.
>
> None of that is relevant ... if you commit a patch to a tree that will
> be in the linux kernel tree, you must add your Signed-off-by to the commit.
Hi Stephen,
Noted!
So I always do this when I do git-am and such, but wasn't sure for this one
single cherry-pick whose
original author was the committer in drm-misc-fixes, but will add my
Signed-off-by in those
rare circumstances.
Thanks for the clarification!
--
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-13 20:08, Luben Tuikov wrote:
> On 2023-11-13 15:55, Stephen Rothwell wrote:
>> Hi all,
>>
>> Commit
>>
>> 0da611a87021 ("dma-buf: add dma_fence_timestamp helper")
>>
>> is missing a Signed-off-by from its committer.
>>
>
> In order to merge the scheduler changes necessary for the Xe driver, those
> changes
> were based on drm-tip, which included this change from drm-misc-fixes, but
> which
> wasn't present in drm-misc-next.
>
> I didn't want to create a merge conflict between drm-misc-next and
> drm-misc-fixes,
> when pulling that change from drm-misc-next to drm-misc-fixes, so that I can
> apply
... when pulling that change from from drm-misc-fixes into drm-misc-next, so
that I can apply...
> the Xe scheduler changes on top of drm-misc-next.
The change in drm-misc-fixes is b83ce9cb4a465b. The latter is contained
in linus-master, and in drm-misc-fixes, while the former is in drm-misc-next.
When we merge linus-master/drm-misc-fixes into drm-misc-next, or whichever way
it happens, I'd like to avoid a merge conflict, but wanted to expedite the
changes
for Xe.
--
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [Intel-gfx] linux-next: Signed-off-by missing for commit in the drm-misc tree
On 2023-11-13 15:55, Stephen Rothwell wrote:
> Hi all,
>
> Commit
>
> 0da611a87021 ("dma-buf: add dma_fence_timestamp helper")
>
> is missing a Signed-off-by from its committer.
>
In order to merge the scheduler changes necessary for the Xe driver, those
changes
were based on drm-tip, which included this change from drm-misc-fixes, but which
wasn't present in drm-misc-next.
I didn't want to create a merge conflict between drm-misc-next and
drm-misc-fixes,
when pulling that change from drm-misc-next to drm-misc-fixes, so that I can
apply
the Xe scheduler changes on top of drm-misc-next.
--
Regards,
Luben
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [Intel-gfx] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On 2023-07-12 09:53, Christian König wrote: > Am 12.07.23 um 15:38 schrieb Uwe Kleine-König: >> Hello Maxime, >> >> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote: >>> On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: > Background is that this makes merge conflicts easier to handle and detect. Really? >>> FWIW, I agree with Christian here. >>> Each file (apart from include/drm/drm_crtc.h) is only touched once. So unless I'm missing something you don't get less or easier conflicts by doing it all in a single patch. But you gain the freedom to drop a patch for one driver without having to drop the rest with it. >>> Not really, because the last patch removed the union anyway. So you have >>> to revert both the last patch, plus that driver one. And then you need >>> to add a TODO to remove that union eventually. >> Yes, with a single patch you have only one revert (but 194 files changed, >> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1 >> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit >> bigger). (And maybe you get away with just reverting the last patch.) >> >> With a single patch the TODO after a revert is "redo it all again (and >> prepare for a different set of conflicts)" while with the split series >> it's only "fix that one driver that was forgotten/borked" + reapply that >> 10 line patch. > > Yeah, but for a maintainer the size of the patches doesn't matter. > That's only interesting if you need to manually review the patch, which > you hopefully doesn't do in case of something auto-generated. > > In other words if the patch is auto-generated re-applying it completely > is less work than fixing things up individually. > >> As the one who gets that TODO, I prefer the latter. > > Yeah, but your personal preferences are not a technical relevant > argument to a maintainer. > > At the end of the day Dave or Daniel need to decide, because they need > to live with it. > > Regards, > Christian. > >> >> So in sum: If your metric is "small count of reverted commits", you're >> right. If however your metric is: Better get 95% of this series' change >> in than maybe 0%, the split series is the way to do it. >> >> With me having spend ~3h on this series' changes, it's maybe >> understandable that I did it the way I did. >> >> FTR: This series was created on top of v6.5-rc1. If you apply it to >> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to >> be the responsible maintainer who applies this series, I like being able >> to just do git am --skip then. >> >> FTR#2: In drm-misc-next is a new driver >> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for >> now might indeed be a good idea. >> So I still like the split version better, but I'm open to a more verbose reasoning from your side. >>> You're doing only one thing here, really: you change the name of a >>> structure field. If it was shared between multiple maintainers, then >>> sure, splitting that up is easier for everyone, but this will go through >>> drm-misc, so I can't see the benefit it brings. >> I see your argument, but I think mine weights more. I'm with Maxime and Christian on this--a single action necessitates a single patch. One single movement. As Maxime said "either 0 or 100." As to the name, perhaps "drm_dev" is more descriptive than just "drm". What is "drm"? Ah it's a "dev", as in "drm dev"... Then why not rename it to "drm_dev"? You are renaming it from "dev" to something more descriptive after all. "dev" --> "drm" is no better, but "dev" --> "drm_dev" is just right. -- Regards, Luben
[Intel-gfx] [PATCH] drm/sched: Add missing structure comment
Add a missing structure comment for the recently
added @list member.
Cc: Stephen Rothwell
Cc: Daniel Vetter
Cc: Christian König
Fixes: 8935ff00e3b1 ("drm/scheduler: "node" --> "list"")
Reported-by: Stephen Rothwell
Signed-off-by: Luben Tuikov
---
include/drm/gpu_scheduler.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 2e0c368e19f6..975e8a67947f 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -171,10 +171,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct
dma_fence *f);
* struct drm_sched_job - A job to be run by an entity.
*
* @queue_node: used to append this struct to the queue of jobs in an entity.
+ * @list: a job participates in a "pending" and "done" lists.
* @sched: the scheduler instance on which this job is scheduled.
* @s_fence: contains the fences for the scheduling of job.
* @finish_cb: the callback for the finished fence.
- * @node: used to append this struct to the @drm_gpu_scheduler.pending_list.
* @id: a unique id assigned to each job scheduled on the scheduler.
* @karma: increment on every hang caused by this job. If this exceeds the hang
* limit of the scheduler then the job is marked guilty and will not
--
2.29.2.404.ge67fbf927d
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/sched: Add missing structure comment
On 2020-12-09 5:24 p.m., Stephen Rothwell wrote:
> Hi Luben,
>
> On Wed, 9 Dec 2020 16:58:07 -0500 Luben Tuikov wrote:
>>
>> Add a missing structure comment for the recently
>> added @list member.
>>
>> Signed-off-by: Luben Tuikov
>>
>> Cc: Stephen Rothwell
>> Cc: Daniel Vetter
>> Cc: Christian König
>
> The commit message tags should all be together at the end of the commit
> message (probably with your SoB last). Also:
>
> Fixes: 8935ff00e3b1 ("drm/scheduler: "node" --> "list"")
> Reported-by: Stephen Rothwell
Right! I was just looking into this as the empty
line up there didn't look good.
I'll resubmit.
Regards,
Luben
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/sched: Add missing structure comment
Add a missing structure comment for the recently added @list member. Signed-off-by: Luben Tuikov Cc: Stephen Rothwell Cc: Daniel Vetter Cc: Christian König --- include/drm/gpu_scheduler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 2e0c368e19f6..975e8a67947f 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -171,10 +171,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); * struct drm_sched_job - A job to be run by an entity. * * @queue_node: used to append this struct to the queue of jobs in an entity. + * @list: a job participates in a "pending" and "done" lists. * @sched: the scheduler instance on which this job is scheduled. * @s_fence: contains the fences for the scheduling of job. * @finish_cb: the callback for the finished fence. - * @node: used to append this struct to the @drm_gpu_scheduler.pending_list. * @id: a unique id assigned to each job scheduled on the scheduler. * @karma: increment on every hang caused by this job. If this exceeds the hang * limit of the scheduler then the job is marked guilty and will not -- 2.29.2.404.ge67fbf927d ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] linux-next: build warning after merge of the drm-misc tree
On 2020-12-09 05:02, Stephen Rothwell wrote:
> Hi all,
>
> After merging the drm-misc tree, today's linux-next build (htmldocs)
> produced this warning:
>
> include/drm/gpu_scheduler.h:201: warning: Function parameter or member 'list'
> not described in 'drm_sched_job'
>
> Introduced by commit
>
> 8935ff00e3b1 ("drm/scheduler: "node" --> "list"")
>
Thanks for the notification.
I'll send out a patch to fix this.
Regards,
Luben
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/amdgpu: Paper over the drm_driver mangling for virt
On 2020-10-30 08:04, Daniel Vetter wrote:
> On Fri, Oct 30, 2020 at 11:41 AM Liu, Monk wrote:
>>
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> What's the purpose of the patch sets
>>
>> e.g.: what bug can those 5 patches fix or what feature provided
>>
>> for this particular one (3/5) I didn't see how it helpful, could you give a
>> background ?
>
> It's good to make function tables const, so that they can be write
> protected. More resilience against exploits and all that. This patch
> here is needed to be able to make drm_device->driver const so that all
> other drivers can make their drm_driver structure const. Would be good
> to fully fix up amdgpu like in the comment, but I'm not going that in
> this series here.
> -Daniel
Hi Daniel,
I feel that that's a good change.
But if you can clarify this for me... Is this leading
towards a single instance of a struct drm_driver per
low-level driver, i.e. amdgpu?
And as such, being able to be defined as const?
So that we have many GPU devices driven by one
low-level driver (amdgpu_drv), represented by one
const drm_driver (and thus const)?
Which would imply that if varied devices can be handled
by a single low-level driver, whose struct drm_driver
settings cannot be shared among subset of devices (say
very old and new), then the low-level driver
would have to create more than one "const" struct drm_driver?
Regards,
Luben
>
>>
>> thanks
>> _
>> Monk Liu|GPU Virtualization Team |AMD
>>
>>
>> -Original Message-
>> From: Daniel Vetter
>> Sent: Friday, October 30, 2020 6:11 PM
>> To: DRI Development
>> Cc: Intel Graphics Development ; Daniel
>> Vetter ; Deucher, Alexander
>> ; Koenig, Christian ;
>> Quan, Evan ; Kuehling, Felix ;
>> Zhang, Hawking ; Grodzovsky, Andrey
>> ; Tuikov, Luben ; Thomas
>> Zimmermann ; Liu, Monk ; Yintian Tao
>> ; Li, Dennis ; Liu, Shaoyun
>> ; Zhang, Bokun ; Yang, Stanley
>> ; Sheng, Wenhui ; Gong, Curry
>> ; Daniel Vetter
>> Subject: [PATCH 3/5] drm/amdgpu: Paper over the drm_driver mangling for virt
>>
>> Prep work to make drm_device->driver const.
>>
>> Signed-off-by: Daniel Vetter
>> Cc: Alex Deucher
>> Cc: "Christian König"
>> Cc: Evan Quan
>> Cc: Felix Kuehling
>> Cc: Hawking Zhang
>> Cc: Andrey Grodzovsky
>> Cc: Luben Tuikov
>> Cc: Thomas Zimmermann
>> Cc: Monk Liu
>> Cc: Yintian Tao
>> Cc: Dennis Li
>> Cc: shaoyunl
>> Cc: Bokun Zhang
>> Cc: "Stanley.Yang"
>> Cc: Wenhui Sheng
>> Cc: chen gong
>> Signed-off-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8
>> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 12 +++-
>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 024c3b70b1aa..3d337f13ae4e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1093,7 +1093,7 @@ static const struct pci_device_id pciidlist[] = {
>>
>> MODULE_DEVICE_TABLE(pci, pciidlist);
>>
>> -static struct drm_driver kms_driver;
>> +struct drm_driver amdgpu_kms_driver;
>>
>> static int amdgpu_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent) @@ -1164,7 +1164,7 @@ static int
>> amdgpu_pci_probe(struct pci_dev *pdev,
>> if (ret)
>> return ret;
>>
>> -adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
>> +adev = devm_drm_dev_alloc(&pdev->dev, &amdgpu_kms_driver,
>> +typeof(*adev), ddev);
>> if (IS_ERR(adev))
>> return PTR_ERR(adev);
>>
>> @@ -1508,7 +1508,7 @@ int amdgpu_file_to_fpriv(struct file *filp, struct
>> amdgpu_fpriv **fpriv)
>> return 0;
>> }
>>
>> -static struct drm_driver kms_driver = {
>> +struct drm_driver amdgpu_kms_driver = {
>> .driver_features =
>> DRIVER_ATOMIC |
>> DRIVER_GEM |
>> @@ -1571,7 +1571,7 @@ static int __init amdgpu_init(void)
>> goto error_fence;
>>
>> DRM_INFO("amdgpu kernel modesetting enabled.\n");
>> -kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>> +amdgpu_kms_driver.num_ioctls = amdgpu_max_kms_ioctl;
>> amdgpu_register_atpx_handler();
>>
>> /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not set. */ diff
>> --git a/drivers/gp
Re: [Intel-gfx] [PATCH] drm/managed: Cleanup of unused functions and polishing docs
On 2020-09-03 10:26 a.m., Daniel Vetter wrote: > On Wed, Sep 02, 2020 at 09:26:27AM +0200, Daniel Vetter wrote: >> Following functions are only used internally, not by drivers: >> - devm_drm_dev_init >> >> Also, now that we have a very slick and polished way to allocate a >> drm_device with devm_drm_dev_alloc, update all the docs to reflect the >> new reality. Mostly this consists of deleting old and misleading >> hints. Two main ones: >> >> - it is no longer required that the drm_device base class is first in >> the structure. devm_drm_dev_alloc can cope with it being anywhere >> >> - obviously embedded now strongly recommends using devm_drm_dev_alloc >> >> v2: Fix typos (Noralf) >> >> v3: Split out the removal of drm_dev_init, that's blocked on some >> discussions on how to convert vgem/vkms/i915-selftests. Adjust commit >> message to reflect that. >> >> Cc: Noralf Trønnes >> Acked-by: Noralf Trønnes (v2) >> Acked-by: Sam Ravnborg >> Cc: Luben Tuikov >> Cc: amd-...@lists.freedesktop.org >> Signed-off-by: Daniel Vetter > > Ok pushed that now to drm-misc-next. I'm also working on getting the > remaining bits of the basic drm managed conversion resubmitted. That was > unfortunately massively sidelined for the dma-fence discussions. > > Quick heads-up: > drmm_add_final_kfree and drm_dev_init will both disappear, please use > devm_drm_dev_alloc. Hi Daniel, In drm_drv.c, in the "DOC: driver instance overview" section, it would help a lot, if you could add/summarize/clarify, succinctly, the two paths, device instantiation: devm_drm_dev_alloc(); ... drm_dev_init(); ... drm_dev_register(); ... And, device destruction, and where/how the "release" method of drm_driver plays out. The allocation part is mostly there, that's good, but the release/destruction part is not. That is, the platform driver callbacks are there, but not the DRM driver part. In this patch, there is no mention of drm_dev_init(), and the documentation update of this patch doesn't mention it, while it is being used by at least one driver. If, on the other hand, you're thinking of removing the "release" callback, a lucid explanation on kref reaching 0--what should be done by DRM and what should be done by drivers, would be very nice and helpful to low-level DRM device driver maintainers/writers. Also, consider renaming "drm_add_action()" to something qualifying the action: either a bitmask as an argument, or right in the name, "drm_add_action_release()", else one begs the question "What action?" It would be helpful, to describe the order of "release" actions on kref reaching 0, and what is expected of DRM and what of drivers, in the order of the expected callbacks. Regards, Luben > > Cheers, Daniel > >> --- >> .../driver-api/driver-model/devres.rst| 2 +- >> drivers/gpu/drm/drm_drv.c | 78 +-- >> drivers/gpu/drm/drm_managed.c | 2 +- >> include/drm/drm_device.h | 2 +- >> include/drm/drm_drv.h | 16 ++-- >> 5 files changed, 30 insertions(+), 70 deletions(-) >> >> diff --git a/Documentation/driver-api/driver-model/devres.rst >> b/Documentation/driver-api/driver-model/devres.rst >> index efc21134..aa4d2420f79e 100644 >> --- a/Documentation/driver-api/driver-model/devres.rst >> +++ b/Documentation/driver-api/driver-model/devres.rst >> @@ -263,7 +263,7 @@ DMA >>dmam_pool_destroy() >> >> DRM >> - devm_drm_dev_init() >> + devm_drm_dev_alloc() >> >> GPIO >>devm_gpiod_get() >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index d4506f7a234e..7c1689842ec0 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -240,13 +240,13 @@ void drm_minor_release(struct drm_minor *minor) >> * DOC: driver instance overview >> * >> * A device instance for a drm driver is represented by &struct drm_device. >> This >> - * is initialized with drm_dev_init(), usually from bus-specific ->probe() >> - * callbacks implemented by the driver. The driver then needs to initialize >> all >> - * the various subsystems for the drm device like memory management, vblank >> - * handling, modesetting support and intial output configuration plus >> obviously >> - * initialize all the corresponding hardware bits. Finally when everything >> is up >> - * and running and ready for userspace the device instance can be published >>
Re: [Intel-gfx] [RFC 02/17] dma-fence: basic lockdep annotations
On 2020-05-12 4:59 a.m., Daniel Vetter wrote:
> Design is similar to the lockdep annotations for workers, but with
> some twists:
>
> - We use a read-lock for the execution/worker/completion side, so that
> this explicit annotation can be more liberally sprinkled around.
> With read locks lockdep isn't going to complain if the read-side
> isn't nested the same way under all circumstances, so ABBA deadlocks
> are ok. Which they are, since this is an annotation only.
>
> - We're using non-recursive lockdep read lock mode, since in recursive
> read lock mode lockdep does not catch read side hazards. And we
> _very_ much want read side hazards to be caught. For full details of
> this limitation see
>
> commit e91498589746065e3ae95d9a00b068e525eec34f
> Author: Peter Zijlstra
> Date: Wed Aug 23 13:13:11 2017 +0200
>
> locking/lockdep/selftests: Add mixed read-write ABBA tests
>
> - To allow nesting of the read-side explicit annotations we explicitly
> keep track of the nesting. lock_is_held() allows us to do that.
>
> - The wait-side annotation is a write lock, and entirely done within
> dma_fence_wait() for everyone by default.
>
> - To be able to freely annotate helper functions I want to make it ok
> to call dma_fence_begin/end_signalling from soft/hardirq context.
> First attempt was using the hardirq locking context for the write
> side in lockdep, but this forces all normal spinlocks nested within
> dma_fence_begin/end_signalling to be spinlocks. That bollocks.
>
> The approach now is to simple check in_atomic(), and for these cases
> entirely rely on the might_sleep() check in dma_fence_wait(). That
> will catch any wrong nesting against spinlocks from soft/hardirq
> contexts.
>
> The idea here is that every code path that's critical for eventually
> signalling a dma_fence should be annotated with
> dma_fence_begin/end_signalling. The annotation ideally starts right
> after a dma_fence is published (added to a dma_resv, exposed as a
> sync_file fd, attached to a drm_syncobj fd, or anything else that
> makes the dma_fence visible to other kernel threads), up to and
> including the dma_fence_wait(). Examples are irq handlers, the
> scheduler rt threads, the tail of execbuf (after the corresponding
> fences are visible), any workers that end up signalling dma_fences and
> really anything else. Not annotated should be code paths that only
> complete fences opportunistically as the gpu progresses, like e.g.
> shrinker/eviction code.
>
> The main class of deadlocks this is supposed to catch are:
>
> Thread A:
>
> mutex_lock(A);
> mutex_unlock(A);
>
> dma_fence_signal();
>
> Thread B:
>
> mutex_lock(A);
> dma_fence_wait();
> mutex_unlock(A);
>
> Thread B is blocked on A signalling the fence, but A never gets around
> to that because it cannot acquire the lock A.
>
> Note that dma_fence_wait() is allowed to be nested within
> dma_fence_begin/end_signalling sections. To allow this to happen the
> read lock needs to be upgraded to a write lock, which means that any
> other lock is acquired between the dma_fence_begin_signalling() call and
> the call to dma_fence_wait(), and still held, this will result in an
> immediate lockdep complaint. The only other option would be to not
> annotate such calls, defeating the point. Therefore these annotations
> cannot be sprinkled over the code entirely mindless to avoid false
> positives.
>
> v2: handle soft/hardirq ctx better against write side and dont forget
> EXPORT_SYMBOL, drivers can't use this otherwise.
>
> Cc: linux-me...@vger.kernel.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: linux-r...@vger.kernel.org
> Cc: amd-...@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Chris Wilson
> Cc: Maarten Lankhorst
> Cc: Christian König
> Signed-off-by: Daniel Vetter
> ---
> drivers/dma-buf/dma-fence.c | 53 +
> include/linux/dma-fence.h | 12 +
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 6802125349fb..d5c0fd2efc70 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> }
> EXPORT_SYMBOL(dma_fence_context_alloc);
>
> +#ifdef CONFIG_LOCKDEP
> +struct lockdep_map dma_fence_lockdep_map = {
> + .name = "dma_fence_map"
> +};
> +
> +bool dma_fence_begin_signalling(void)
> +{
> + /* explicitly nesting ... */
> + if (lock_is_held_type(&dma_fence_lockdep_map, 1))
> + return true;
> +
> + /* rely on might_sleep check for soft/hardirq locks */
> + if (in_atomic())
> + return true;
> +
> + /* ... and non-recursive readlock */
> + lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> +
> + return false;
> +}
> +EXPORT_SYMBOL(dma_fence_begin_signalling);
Hi Daniel,
This is great work
