Message-ID: <95beedf6-4414-4ec2-5b5d-c17497c7bbe0@tronnes.org>
Date:   Mon, 6 Feb 2017 23:11:27 +0100
From:   Noralf Trønnes <noralf@...nnes.org>
To:     Thierry Reding <thierry.reding@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions


Den 06.02.2017 16.53, skrev Daniel Vetter:
> On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote:
>> On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
>>> On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@...il.com>
>>> wrote:
>>>
>>>>>>> +EXPORT_SYMBOL(tinydrm_disable_backlight);
>>>>>>> +#endif
>>>>>> These look like they really should be part of the backlight subsystem.
>>>> I
>>>>>> don't see anything DRM specific about them. Well, except for the error
>>>>>> messages.
>>>>> So this is a bit an unpopular opinion with some folks, but I don't
>>>> require
>>>>> anyone to submit new code to subsystems outside of drm for new drivers.
>>>>> Simply because it takes months to get stuff landed, and in general it's
>>>>> not worth the trouble.
>>>> "Not worth the trouble" is very subjective. If you look at the Linux
>>>> kernel in general, one of the reasons why it works so well is because
>>>> the changes we make apply to the kernel as a whole. Yes, sometimes that
>>>> makes things more difficult and time-consuming, but it also means that
>>>> the end result will be much more widely usable and therefore benefits
>>>> everyone else in return. In my opinion that's a large part of why the
>>>> kernel is so successful.
>>>>
>>>>> We have piles of stuff in drm and drm drivers that should be in core but
>>>>> isn't.
>>>>>
>>>>> Imo the only reasonable way is to merge as-is, then follow-up with a
>>>> patch
>>>>> series to move the helper into the right subsystem. Most often
>>>>> unfortunately that follow-up patch series will just die.
>>>> Of course follow-up series die. That's because nobody cares to follow-up
>>>> once their code has been merged.
>>>>
>>>> Collecting our own helpers or variants of subsystems is a great way of
>>>> isolating ourselves from the rest of the community. I don't think that's
>>>> a good solution in the long run at all.
>>>>
>>> We have a bunch of patch series that we resubmit for months and they go
>>> exactly nowhere. They don't die because we stop caring, they die because
>>> they die. Some of them we even need to constantly rebase and carry around
>>> in drm-tip since our CI would Oops or spew WARNIGs all over the place.
>>> There's simply some areas of the kernel which seem overloaded under patches
>>> and no one is willing or able to fix things, and I can't fix the entire
>>> kernel. Nor expect contributors (who have much less political weight to
>>> throw around than me) to do that and succeed. And we don't end up with
>>> worse code in the drm subsystem, since we can still do the refactoring
>>> within drm helpers and end up with clean drivers.
>>>
>>> I fully agree that it's not great for the kernel's future, but when I'm
>>> stuck with the option to get shit done or burning out playing the
>>> upstreaming game, the choice is easy. And in the end I care about open
>>> source gfx much more than the kernel, and I think for open source gfx's
>>> success it's crucial that we're welcoming to new contributors and don't
>>> throw up massive roadblocks. Open source gfx is tiny and still far away
>>> from world domination, we need _lots_ more people. If that means routing
>>> around other subsystems for them, I'm all for it.
>> I can't say I fully agree with that sentiment. I do see how routing
>> around subsystems can be useful occasionally. If nobody will merge the
>> code, or if nobody cares, then by all means, let's make them DRM-
>> specific helpers.
>>
>> But I think we need to at least try to do the right thing. If only to
>> teach people what the right way is. If we start accepting such things
>> by default, how can we expect contributors to even try?
>>
>> I also think that contributors will often end up contributing not only
>> to DRM but to the kernel as a whole. As such it should be part of our
>> mentoring to teach them about how the process works as a rule, even if
>> the occasional exception is necessary to get things done.
>>
>> In this particular case, I know for a fact that both backlight and SPI
>> maintainers are very responsive, so that's not a good excuse.
> I definitely don't want that we don't attempt this. But brought from years
> of experience, I recommend to merge first (with pre-refactoring already
> applied, but helpers only extracted, not yet at the right spot), and then
> follow up with. Because on average, there's way too many trees with
> overloaded maintainers who maybe look at your patch once per kernel
> release cycle.
>
> If you know that backlight and spi isn't one of these areas (anything that
> goes through takashi/sound is a similar good experience for us on the i915
> side), then I guess we can try. But then Noralf has already written a few
> months worth of really great refactoring, and I'm seriously starting to
> feel guilty for volunteering him for all of this. Even though he seems to
> be really good at it, and seems to not mind, it's getting a bit silly.
> Given that I'd say up to Noralf.
>
> In short, there's always a balance.

Yes, it's a balance between the perfect and not's so perfect,
the professinal and the amateur, and the drm expert and the newbie.

If I knew how much time I would have to spend on tinydrm to get it
merged, then I would never have started it. I wondered, a couple of
months back, if I should just cut my losses and move to something else.
dri-devel is a friendly place and I do get help to keep moving, but I
get the feeling that drm is really a place for professionals that write
kernel code 50 hours a week. And there's nothing wrong in that, drm is
complex and maybe that kind of expertice and work-hours are needed to
do work here.

It's not that I plan on backing out now, I'm just putting down a few
words about the challenge it has been for me as a hobbyist to meet drm.


As for the specifics,

Backlight enable/disable helpers, I haven't thought about those as
backlight subsystem helpers. But I see some drm panel drivers that do
the same, so it makes sense.
But what I'm feeling is this:
If I'm expected to get everything right, then I will "never" get this 
merged.
This thing by itself isn't much, but adding up every little thing
amounts to a lot. And it's not just "make this patch", it's probably
clean up the drivers as well :-)

tinydrm_spi_transfer() can print the content of the buffer. This is
important at least until I have seen some real use of the drivers.
I have emulation code for handling bit widths not supported by the SPI
controller, so I want to see what goes over the wire. SPI core uses
trace events to track what's going on, and I don't think I can get
buffer dumping into that code.

Don't get me wrong Thierry, I do appreciate that you take the time to
look at the code. I'm just frustrated that it takes so long to get this
right. I thought that all I needed now was a DT maintainer ack :-)


Noralf.

Powered by blists - more mailing lists