Sudip Mukherjee
2017-Nov-30 23:49 UTC
[PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
Hi Daniel, On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote:> On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote: > > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote: > > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote: > > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote: > > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote: > > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Micha? Miros?aw wrote: > > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with > > > > > > > the same code. Extract common part from PCI drivers into separate > > > > > > > remove_conflicting_pci_framebuffers(). > > > > > > ><snip>> > > > > > > > Greg? > > > > > > Yes, if no one is working to get it out of staging, that means no one > > > cares about it, and it needs to be removed from the tree. > > > > Agreed. I was not working on getting it out of staging as there is no > > place for it to go. > > But, Teddy Wang told me that they have a working drm driver for it, but > > it is not atomic like Daniel was asking for. If it is ok, then I can send > > in a patch to remove the existing sm750 from staging and add the new sm750 > > drm driver to staging. And after it is ready, we can go ahead with moving > > it out of staging to drm. > > Please keep the todo item that it needs to be converted to atomic. And > tbh, it's probably faster if you just submit it to dri-devel, assuming you > have time to work on it. For small drivers we tend to be fairly quick in > getting them into good enough shape.I have received the driver from Teddy and pushed it to https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first look into it. It is not even building with next-20171130 and has lots of build warnings. I will have to do a lot of work on it before I can even submit it to dri-devel. Time will be the problem, as this is not part of my day job.> > Staging is also a major pain for drm subsystem refactorings, I really, > really, really prefer we don't add more than the vbox pain we have > already.I am hoping that after seeing it, you will agree to have it in staging. :) -- Regards Sudip
Daniel Vetter
2017-Dec-01 07:19 UTC
[PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
On Thu, Nov 30, 2017 at 11:49:53PM +0000, Sudip Mukherjee wrote:> Hi Daniel, > > On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote: > > On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote: > > > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote: > > > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote: > > > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote: > > > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote: > > > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Micha? Miros?aw wrote: > > > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with > > > > > > > > the same code. Extract common part from PCI drivers into separate > > > > > > > > remove_conflicting_pci_framebuffers(). > > > > > > > > > > <snip> > > > > > > > > > > > Greg? > > > > > > > > Yes, if no one is working to get it out of staging, that means no one > > > > cares about it, and it needs to be removed from the tree. > > > > > > Agreed. I was not working on getting it out of staging as there is no > > > place for it to go. > > > But, Teddy Wang told me that they have a working drm driver for it, but > > > it is not atomic like Daniel was asking for. If it is ok, then I can send > > > in a patch to remove the existing sm750 from staging and add the new sm750 > > > drm driver to staging. And after it is ready, we can go ahead with moving > > > it out of staging to drm. > > > > Please keep the todo item that it needs to be converted to atomic. And > > tbh, it's probably faster if you just submit it to dri-devel, assuming you > > have time to work on it. For small drivers we tend to be fairly quick in > > getting them into good enough shape. > > I have received the driver from Teddy and pushed it to > https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first > look into it. It is not even building with next-20171130 and has lots of > build warnings. I will have to do a lot of work on it before I can even > submit it to dri-devel. > > Time will be the problem, as this is not part of my day job. > > > > > Staging is also a major pain for drm subsystem refactorings, I really, > > really, really prefer we don't add more than the vbox pain we have > > already. > > I am hoping that after seeing it, you will agree to have it in staging. :)So I know Greg is willing to take anything into staging, but I'm no fan. We refactor and improve drm a lot, with a lot of cross-driver changes necessary to move things forward. We can do that since we have a generally rather active development community, and we try hard to keep most drivers in reasonable good shape and so easy to maintain. If you know throw a pile of unmaintainable stuff into staging, but without working on it, then that's just cost, no benefit to the dri-devel community. On top, staging tree is separate from drm trees, so more pain to synchronize trees (and we have to do that a few times per release cycle or drivers simply stop compiling). Where's the upside of taking this driver into staging? One is users, but ime for soc display drivers usually everything else is in worse shape (e.g. even great drivers like the one for qualcom must be tested on some vendor tree because critical core bits are missing in upstream), so "more users" is not the good reason. And it's clearly not "more developers", because no time to clean things up. So what exactly is the good reason to put this into staging instead of just waiting until someone has the time to clean it up quickly? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Sudip Mukherjee
2017-Dec-01 14:10 UTC
[PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
On Fri, Dec 01, 2017 at 08:19:16AM +0100, Daniel Vetter wrote:> On Thu, Nov 30, 2017 at 11:49:53PM +0000, Sudip Mukherjee wrote: > > Hi Daniel, > > > > <snip> > > > > > > > > > > > > > > Greg? > > > > > > > > > > Yes, if no one is working to get it out of staging, that means no one > > > > > cares about it, and it needs to be removed from the tree. > > > > > > > > Agreed. I was not working on getting it out of staging as there is no > > > > place for it to go. > > > > But, Teddy Wang told me that they have a working drm driver for it, but > > > > it is not atomic like Daniel was asking for. If it is ok, then I can send > > > > in a patch to remove the existing sm750 from staging and add the new sm750 > > > > drm driver to staging. And after it is ready, we can go ahead with moving > > > > it out of staging to drm. > > > > > > Please keep the todo item that it needs to be converted to atomic. And > > > tbh, it's probably faster if you just submit it to dri-devel, assuming you > > > have time to work on it. For small drivers we tend to be fairly quick in > > > getting them into good enough shape. > > > > I have received the driver from Teddy and pushed it to > > https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first > > look into it. It is not even building with next-20171130 and has lots of > > build warnings. I will have to do a lot of work on it before I can even > > submit it to dri-devel. > > > > Time will be the problem, as this is not part of my day job. > > > > > > > > Staging is also a major pain for drm subsystem refactorings, I really, > > > really, really prefer we don't add more than the vbox pain we have > > > already. > > > > I am hoping that after seeing it, you will agree to have it in staging. :) > > So I know Greg is willing to take anything into staging, but I'm no fan. > We refactor and improve drm a lot, with a lot of cross-driver changes > necessary to move things forward. We can do that since we have a generally > rather active development community, and we try hard to keep most drivers > in reasonable good shape and so easy to maintain. > > If you know throw a pile of unmaintainable stuff into staging, but without > working on it, then that's just cost, no benefit to the dri-devel > community. On top, staging tree is separate from drm trees, so more pain > to synchronize trees (and we have to do that a few times per release cycle > or drivers simply stop compiling). Where's the upside of taking this > driver into staging? > > One is users, but ime for soc display drivers usually everything else is > in worse shape (e.g. even great drivers like the one for qualcom must be > tested on some vendor tree because critical core bits are missing in > upstream), so "more users" is not the good reason. And it's clearly not > "more developers", because no time to clean things up. So what exactly is > the good reason to put this into staging instead of just waiting until > someone has the time to clean it up quickly?Ok, I will not try to give any more reasons now. :) I will cleanup the basic things I can and then submit it to dri-devel. Greg - Please keep the SM750 driver in staging for now. I will send a patch later to add in TODO the git location where the drm driver is being developed. And when that drm driver is ready, I will send a patch to remove the sm750fb driver from staging. -- Regards Sudip
Emil Velikov
2017-Dec-01 14:40 UTC
[PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
On 30 November 2017 at 23:49, Sudip Mukherjee <sudipm.mukherjee at gmail.com> wrote:> Hi Daniel, > > On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote: >> On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote: >> > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote: >> > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote: >> > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote: >> > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote: >> > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Micha? Miros?aw wrote: >> > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with >> > > > > > > the same code. Extract common part from PCI drivers into separate >> > > > > > > remove_conflicting_pci_framebuffers(). >> > > > > > > > > <snip> > >> > > > >> > > > Greg? >> > > >> > > Yes, if no one is working to get it out of staging, that means no one >> > > cares about it, and it needs to be removed from the tree. >> > >> > Agreed. I was not working on getting it out of staging as there is no >> > place for it to go. >> > But, Teddy Wang told me that they have a working drm driver for it, but >> > it is not atomic like Daniel was asking for. If it is ok, then I can send >> > in a patch to remove the existing sm750 from staging and add the new sm750 >> > drm driver to staging. And after it is ready, we can go ahead with moving >> > it out of staging to drm. >> >> Please keep the todo item that it needs to be converted to atomic. And >> tbh, it's probably faster if you just submit it to dri-devel, assuming you >> have time to work on it. For small drivers we tend to be fairly quick in >> getting them into good enough shape. > > I have received the driver from Teddy and pushed it to > https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first > look into it. It is not even building with next-20171130 and has lots of > build warnings. I will have to do a lot of work on it before I can even > submit it to dri-devel. >A crazy idea, mostly towards Tedd and Sudip: Start small and build gradually. An example split for separate patch series: - one HW, basic setup + atomic KMS - add second HW - more KMS features - fancy memory management - 2D/3D/other acceleration The driver as seen above tries to do all of the above (almost, it's not atomic) at once - 40k loc. Someone familiar with the code can quickly split it up and while doing so, feed it through checkpatch. Current code is _very_ far from kernel coding style, plus the copyright blurp is very disturbing: * All rights are reserved. Reproduction or in part is prohibited * without the written consent of the copyright owner. HTH Emil
Sudip Mukherjee
2017-Dec-11 21:57 UTC
[PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
Hi Emil, On Fri, Dec 01, 2017 at 02:40:11PM +0000, Emil Velikov wrote:> On 30 November 2017 at 23:49, Sudip Mukherjee > <sudipm.mukherjee at gmail.com> wrote: > > Hi Daniel, > > > > On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote:<snip>> > submit it to dri-devel. > > > A crazy idea, mostly towards Tedd and Sudip: > > Start small and build gradually. An example split for separate patch series: > > - one HW, basic setup + atomic KMS > - add second HW > - more KMS features > - fancy memory management > - 2D/3D/other acceleration > > The driver as seen above tries to do all of the above (almost, it's not atomic) > at once - 40k loc. > > Someone familiar with the code can quickly split it up and while doing > so, feed it through checkpatch.I can try but will be very tough since I have to go through the code to get familiar with it and, as mentioned before, its not part of my dayjob, so time will be a problem. Developing from scratch takes much more time than fixing something.> Current code is _very_ far from kernel coding style, plus the > copyright blurp is very disturbing:I will fix the coding style before my first submission to dri-devel, which should be in January after I have setup my desktop with the hardware for testing. The copyright thing - I am sure Teddy can talk to his company and will confirm that we can change it to the way kernel code is done. -- Regards Sudip
Seemingly Similar Threads
- [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
- [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
- [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
- [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
- [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()