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

[Daniel Vetter]

On Mon, Mar 13, 2017 at 04:12:37PM +0100, Noralf Trønnes wrote:
> 
> 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

Ugh, that's bad, review has come full circle :(

Given that I'd say let's not bother more with panel frameworks for now,
but just add the simple panel drivers as-is. Once we have someone who
wants to reuse a panel (probably a mipi-dbi one), but where the dbi bus
isn't exposed to software but behind some fancy hw encoder, then we can
ponder whether/how (and probably just for the affected drivers) we should
re-introduce drm_panel into tinydrm.

I'll try and collect everything we've discussed here into a tinydrm
todo.rst entry.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch