Den 12.03.2017 21.40, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 09:17:00PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 20.16, 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?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
Hm, I thought I checked all the old versions of your example tinydrm submissions and didn't find any with drm_panel. Do you have a link to archives? I'd like to read Thierry's aguments, in case I'm oblivious to a bad corner case :-)
I used drm_panel in the first tinydrm RFC in March 2016: https://lists.freedesktop.org/archives/dri-devel/2016-March/102903.html
struct tinydrm_device { struct drm_device *base; struct drm_panel panel; ... };
Then you commented: https://lists.freedesktop.org/archives/dri-devel/2016-March/102921.html
In the case of tinydrm I think that means we should have a bunch of new drm helpers, or extensions for existing ones:
<snip>
- A helper to create a simple drm_connector from a drm_panel (the get_modes hooks you have here), maybe also in drm_simple_kms_helper.c.
So I made: [PATCH 4/4] drm/panel: Add helper for simple panel connector https://lists.freedesktop.org/archives/dri-devel/2016-May/106890.html
Thierry replied: https://lists.freedesktop.org/archives/dri-devel/2016-May/107023.html
Okay, that gives me a better understanding of where things are headed. That said, why would these devices even need to deal with drm_panel in the first place? Sounds to me like they are devices on some sort of control bus that you talk to directly. And they will represent essentially the panel with its built-in display controller.
drm_panel on the other hand was designed as an interface between display controllers and panels, with the goal to let any display controller talk to any panel.
While I'm sure you can support these types of simple panels using the drm_panel infrastructure I'm not sure it's necessary, since your driver will always talk to the panel directly, and hence the code to deal with the panel specifics could be part of the display pipe functions.
The reason for making this patchset was to solve a problem of power management that Thierry pointed out in the mi0283qt driver where I use devm_add_action() to disable the regulator on module/device unload. I haven't found a way to do PM in the simple drm pipeline.
I use drm_simple_display_pipe.enable to enable backlight since it's called after drm_simple_display_pipe.update. If it was called before, then I could use it to prepare the panel/controller. I remember having seen some comments in the atomic code about reordering something to make it match PM better. But if .enable() could be called before .update(), how then do I control backlight?
So what everyone else does is enable the backlight in ->enable (with the screen just displaying black) and updating the screen contents in ->update afterwards. That's what the comment in the docs about reordering stuff to make it better fit with runtime PM.
If you don't like that for tinydrm, you can insert a call to ->update in your ->enable. Slightly redundant, but then enabling a screen is not the fastest thing so not much problem if you're inefficient. And you could still fix that with a special case in ->update, but really not sure this is worth it.
Once the screen is on you just get calls to ->update, so then it doesn't matter anymore.
And with this ordering you should be able to stuff the regulator calls into ->enable. On the disable side the same thing, but inverse ordering.
Thanks, I'll try that.
Noralf.