Linux Virtualization - Dec 2017 - [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

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

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

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

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