[PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

Den 12.03.2017 19.55, skrev Daniel Vetter:
> On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
>> Hi Noralf,
>>
>> On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
>>> Add support for displays that have a register interface and can be
>>> operated using a simple vtable.
>>>
>>> I have looked through the staging/fbtft drivers and it seems that,
>>> except the MIPI controllers, most if not all controllers are operated
>>> through a register. And since most controllers have more than one bus
>>> interface option, regmap seems like a good choice to describe the
>>> interface (tested[1,2]).
>>> MIPI DCS can't be represented using regmap since some commands doesn't
>>> have a parameter. That would be like a register without a value, which
>>> doesn't make sense.
>>>
>>> In my second RFC of tinydrm I used drm_panel to decribe the panels
>>> since it was a good match to the fbtft displays. I was then told that
>>> drm_panel wasn't supposed to used like that, so I dropped it and have
>>> tried to use the drm_simple_display_pipe_funcs vtable directly. This
>>> hasn't been all successful, since I ended up using devm_add_action() to
>>> power down the controller at the right time. Thierry Reding wasn't
>>> happy with this and suggested "to add an explicit callback somewhere".
>>> My solution has been to copy the drm_panel_funcs vtable.
>>> Since I now have a vtable, I also added a callback to flush the
>>> framebuffer. So presumably all the fbtft drivers can now be operated
>>> through the tinydrm_panel_funcs vtable.
>> Ehrm, what? I admit I didn't follow the discussion in-depth, but if
>> drm_panel can't be used to describe a panel, it's not fit for the job and
>> needs to be extended. Adding even more abstraction, matroschka-style,
>> isn't a solution.
>>
>> Personally I think tinydrm itself is already a bit much wrapping, but I
>> see that for really simple drivers it has it's uses. But more layers feels
>> like going in the wrong direction.
>>
>> For the callback you're looking for (i.e. the regulator_disable call) I
>> think the correct place is to enable/disable the regulator in the
>> enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in
>> their equivalent in drm_panel (well, probably pre_enable and post_disable,
>> since I guess you need that regulator to driver anything). Same for _init,
>> if the display is completely off there's no point in keeping the hw
>> running. Enabling/disabling the entire hw is pretty much what ->enable and
>> ->disable are for. This also gives you proper runtime pm for almost for
>> free ...
>>
>> Also, since the regulator is actually stored in struct mipi_dbi, it's
>> probably best to handle it in the mipi_dbi helpers too. You do that
>> already with the backlight anyway.
>>
>> I noticed that the version of tinydrm that landed doesn't use drm_panel
>> anymore, I thought that was the case once, and for the version I acked?
>>
>>> After having done this the question arises:
>>> Why not extend tinydrm_device instead of subclassing it?
>>>
>>> The benefit of subclassing is that it keeps the door open for drivers
>>> that can use tinydrm_device, but not tinydrm_panel. But I don't know of
>>> such a driver now, then again who knows what the future brings.
>>> Something that might or might not happen isn't a good reason, so it
>>> seems that I want it this way because I just like it. And it's easy to
>>> merge the two should it be that no one uses tinydrm_device directly
>>> three years down the line. But I'm actually not sure what's best.
>> As a rule of thumb, never design code for future use that you don't know
>> yet will happen. No one can reliably predict the future, which means
>> you'll get it wrong, and in the future we'll have to do 2x the work: Once
>> do unto the code resulting from the wrong prediction, then redoing it the
>> way we need to. Not including the on-going burden of maintaining unused
>> functionally.
>>
>> So let's pls merge first, split later when there's a clean need.
>>
>>> To recap:
>>>
>>> tinydrm_device
>>> - Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev.
>>> - Optional pipe setup with a connector with one mode, but the driver
>>>    can do it's own.
>>>
>>> tinydrm_panel
>>> - All drm operations are distilled down to tinydrm_panel_funcs.
>>> - Some common driver properties
>> So overal sorry I'm shredding this a bit, but I don't see the point. Imo
>> much more useful would be:
>>
>> 1. Extract the non-tinydrm specific helpers from tinydrm and put them into
>> the right places. I think from our discussions this was:
>>
>> - backlight helpers, probably best to put them into a new drm_backlight.c.
>>    This is because drivers/video is defactor unmaintained. We could also
>>    move drivers/video/backlight to drivers/gpu/backlight and take it all
>>    over within drm-misc, but that's more work.
>>
>> - spi helpers, probably best put into spi core/helper code. Thierry said
>>    the spi maintainer is fast&reactive, so shouldn't be a big issue.
>>
>> - extract the mipi-dbi helper (well, the non-tinydrm specific parts at
>>    least) into a separate helper, like we have for mipi-dsi already.
> A large chunk of the tinydrm functions should probably be moved into
> relevant existin drm helpers, e.g.
>
> - tinydrm_lastclose could be drm_fb_helper_lastclose. Only thing we need
>    for that is to store the drm_fb_helper pointer somewhere in
>    drm_device->mode_config. And thenc we could roll that out to all the
>    drivers.
>
> - tinydrm_gem_cma_prime_import_sg_table should probably go into the cma
>    helpers, as a _vmapped variant (since not every driver needs the vmap).
>    And tinydrm_gem_cma_free_object could the be merged into
>    drm_gem_cma_free_object().
>
> - tinydrm_fb_create we could move into drm_simple_pipe, only need to add
>    the fb_create hook there, which would again simplify a bunch of things
>    (since it gives you a one-stop vfunc for simple drivers).
>
> - Quick aside: The unregister devm stuff is kinda getting the lifetimes of
>    a drm_device wrong. Doesn't matter, since everyone else gets it wrong
>    too :-)
>   
> - With the fbdev pointer in dev->mode_config we could also make
>    suspend/resume helpers entirely generic, at least if we add a
>    dev->mode_config.suspend_state.
>
> Just a bunch of ideas. If you don't feel like doing those, ok if I add
> them to todo.rst as tinydrm refactorings?

Please add them to todo.rst.

I have a fatigue illness that has been getting worse the last months,
which means that I have to cut back on programming to not wreck the
rest of my waking hours. Up until now I have primarily needed to keep
my physical activity to a minimum, but computer work hasn't been that
much affected. But double sadly computer work is now also something
that affects my energy levels in a major way. A problem with this
illness is that the full effect of energy overuse comes after a few
days, making it very difficult to find a balance. And with programming
being so much fun it will be a challenge to slow down enough.

So until I find this new energy balance, I don't know how much time I
can spend on tinydrm.

Noralf.

> -Daniel
>
>> 2. Add tons more panel drivers. Personally I'd do that first :-)
>>
>> Cheers, Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch