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

[Thierry Reding]

On Mon, Feb 06, 2017 at 11:11:27PM +0100, Noralf TrÃnnes wrote:
> 
> 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.

I may have come across as a bit rough. You've been doing great work,
there's no doubt about that. I was merely raising this issue because I
think it's something that we need to keep an eye out for. We don't want
to end up in a situation where we duplicate code or keep code to DRM
that would be useful outside of DRM.

The link that Rob pointed to is very insightful, and I think it's very
easy to loose touch with other parts of the kernel when you work on one
subsystem (almost) exclusively. It's completely normal, but because of
that I think it's all the more important that we remind ourselves to
look beyond our noses and refactor beyond just DRM where possible.

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

I don't think that's true in general. If graphics, and especially the
lowlevel parts of it, is something that you're interested in, then
dri-devel is the perfect place for you. It doesn't matter if you work
on it in your spare time or on the day job.

It just so happens that most people end up working on it on the day job
because there's so much work to do that spare time isn't usually enough
to do all you want.

This is probably true for most of the other areas of the kernel, too.
It's not unusual for regular contributors to be offered a job to keep
doing what they do. That's probably why we have a higher degree of
professionalization in the kernel community as a whole. I don't think
that's a bad thing, quite the contrary, it's what we had all been
wishing for for a very long time. I can understand how that might be
frustrating to hobbyists at times, but I hope we're at least not making
things worse by actively discouraging hobbyists.

> 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 :-)

Been there, done that. I think the problem that triggers the kind of
response that I gave is that it's often a handful of people that end up
doing the refactorings. Usually that handful of people is overloaded as
it is, so refactorings end up taking very long and are very tiring. The
easy way out, of course, is to require new patches to do the right thing
to begin with, because people are often motivated by the fact that they
will finally get their patch in. That's of course also a very good way
of discouraging contributions because it may be more than people set out
for.

Unfortunately I don't think there's a good solution for this. But, as
Daniel and Dave have already said, it shouldn't be a blocker for your
series. It'd be really nice if you somehow found the time to follow up,
though, after TinyDRM was merged.

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

I know Mark is a very reasonable person, so I think it can't hurt to
try. Often people will end up rolling buffer dumping code of their own
and having something in the SPI core to do that, possibly hidden behind
a SPI_DEBUG_BUFFER Kconfig option or something like that, would save a
lot of people a lot of time.

Not sure if it would help, but if you end up writing such a patch and
you Cc me when you post it, I'm going to ACK it.

> 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 :-)

I think that's fine. You've done a great job so far and we can always
fine-tune later on.

Thierry

Attachment: signature.asc
Description: PGP signature