From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751805AbdBFWLg (ORCPT ); Mon, 6 Feb 2017 17:11:36 -0500 Received: from smtp.domeneshop.no ([194.63.252.55]:55912 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751172AbdBFWLe (ORCPT ); Mon, 6 Feb 2017 17:11:34 -0500 Subject: Re: [PATCH v3 2/7] drm/tinydrm: Add helper functions To: Thierry Reding , Daniel Vetter References: <20170131160319.9695-1-noralf@tronnes.org> <20170131160319.9695-3-noralf@tronnes.org> <20170206085629.GD27607@ulmo.ba.sec> <20170206090918.6rqr6l7pd62znl5j@phenom.ffwll.local> <20170206093556.GF27607@ulmo.ba.sec> <20170206110847.GH27607@ulmo.ba.sec> <20170206155303.2fwihmlh6ln4eskt@phenom.ffwll.local> Cc: Thomas Petazzoni , "devicetree@vger.kernel.org" , Linux Kernel Mailing List , dri-devel From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <95beedf6-4414-4ec2-5b5d-c17497c7bbe0@tronnes.org> Date: Mon, 6 Feb 2017 23:11:27 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170206155303.2fwihmlh6ln4eskt@phenom.ffwll.local> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> 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.