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

["Noralf Trønnes" <noralf@tronnes.org>]

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel