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 :-) > 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. > I need to first power on and configure the controller, then it can > receive the initial framebuffer, and lastly the backlight is enabled. > > No problem with you shredding this, if I can do this with much less > code, then all the better. Yeah, I think we don't need more abstraction here, with atomic helpers, simple_pipe helpers and tinydrm there's enough. And even if there is eventually a real gpu reusing these panels in their own IP (but still over an spi/i2c bus ofc), then I think we can always reuse at the drm_device level: The gpu ip exposes the spi/i2c bus (plus all the dt entries needed), and the relevant tinydrm driver binds against that. Compositor ties it all together with buffer sharing. So going over the top with making code shareable with drm_panel probably won't ever be needed anyway. This might change if there's an spi/dbi encoder chip which does it all in hw ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
