Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions

[Noralf TrÃnnes]

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@xxxxxxxxx>
> > > 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.