[PATCH V6 2/8] drm/panel: Add support for prepare and unprepare routines

[Ajay kumar]

On Wed, Jul 30, 2014 at 4:02 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Sat, Jul 26, 2014 at 12:52:04AM +0530, Ajay Kumar wrote:
>> Now that we have 2 new callbacks(prepare and unprepare) for drm_panel,
>> make changes in all the drm drivers which use the drm_panel framework
>> to support the new callbacks.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dpi.c |    8 +++--
>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c |    7 +++++
>>  drivers/gpu/drm/panel/panel-ld9040.c    |   18 +++++++++--
>>  drivers/gpu/drm/panel/panel-s6e8aa0.c   |   18 +++++++++--
>>  drivers/gpu/drm/panel/panel-simple.c    |   51 ++++++++++++++++++++++++-------
>>  drivers/gpu/drm/tegra/output.c          |    2 ++
>>  6 files changed, 85 insertions(+), 19 deletions(-)
>
> I'd prefer for this to be split up into patches per panel and per
> display driver. The reason is that if this patch is merged as-is, then
> if it's ever determined to cause a regression it'll be difficult to find
> out which change exactly caused this.
>
> But to not break bisectability you'll need to be careful in how you
> break up the patches. I think the following should work:
>
>         - for each panel driver, implement dummy .prepare() and
>           .unprepare() that return 0
>         - for each display driver, make use of drm_panel_prepare() and
>           drm_panel_unprepare()
>         - for each panel driver, properly implement .prepare() and
>           .unprepare() (presumably by moving code out of .enable() and
>           .disable(), respectively)
>
I will try this. With this approach, compilation should not fail.

> I have a couple more comments below about the ordering of the .prepare()
> vs. .enable() and .disable() vs. .unprepare() calls.
>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index 5e78d45..2f58e45 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -1333,6 +1333,12 @@ static int exynos_dsi_enable(struct exynos_dsi *dsi)
>>       if (ret < 0)
>>               return ret;
>>
>> +     ret = drm_panel_prepare(dsi->panel);
>> +     if (ret < 0) {
>> +             exynos_dsi_poweroff(dsi);
>> +             return ret;
>> +     }
>> +
>>       ret = drm_panel_enable(dsi->panel);
>>       if (ret < 0) {
>>               exynos_dsi_poweroff(dsi);
>> @@ -1354,6 +1360,7 @@ static void exynos_dsi_disable(struct exynos_dsi *dsi)
>>
>>       exynos_dsi_set_display_enable(dsi, false);
>>       drm_panel_disable(dsi->panel);
>> +     drm_panel_unprepare(dsi->panel);
>>       exynos_dsi_poweroff(dsi);
>
> I don't know Exynos very well, so this may be completely wrong, but
> should disable_panel_prepare() be called much earlier than
> drm_panel_enable() and drm_panel_unprepare() much later than
> drm_panel_disable()? With the above the result is still the same, so
> you'll get the same glitches as without their separation.
Actually, I have not worked on DSI panels.
And till now, these DSI panels have been working with just the
enable/disable callback. So, I thought it might not really cause a
glitch/garbage on the screen.
I just placed the new panel calls so that basic working will not be broken.
It would be great if someone can test this on exynos boards with DSI panels :(
Inki/Andrej?

Anyways, now there is a kerneldoc which explains all these calls,
I will rearrange the panel calls.

> Without knowing exactly what Exynos does in the above, I'd expect the
> correct sequence to be something like this:
>
>         ret = exynos_dsi_power_on(dsi);
>         if (ret < 0)
>                 return ret;
>
>         ret = drm_panel_prepare(dsi->panel);
>         if (ret < 0) {
>                 exynos_dsi_poweroff(dsi);
>                 return ret;
>         }
>
>         exynos_dsi_set_display_mode(dsi);
>         exynos_dsi_set_display_enable(dsi, true);
>
>         ret = drm_panel_enable(dsi->panel);
>         if (ret < 0) {
>                 /* perhaps undo exynos_dsi_set_display_enable() here? */
>                 exynos_dsi_poweroff(dsi);
>                 return ret;
>         }
>
> And for disable:
>
>         drm_panel_disable(dsi->panel);
>         exynos_dsi_set_display_enable(dsi, false);
>         drm_panel_unprepare(dsi->panel);
>         exynos_dsi_poweroff(dsi);
>
> Perhaps I should quote the kerneldoc that I added for drm_panel_funcs to
> underline why I think this is important:
>
> /**
>  * struct drm_panel_funcs - perform operations on a given panel
>  * @disable: disable panel (turn off back light, etc.)
>  * @unprepare: turn off panel
>  * @prepare: turn on panel and perform set up
>  * @enable: enable panel (turn on back light, etc.)
>  * @get_modes: add modes to the connector that the panel is attached to and
>  * return the number of modes added
>  *
>  * The .prepare() function is typically called before the display controller
>  * starts to transmit video data. Panel drivers can use this to turn the panel
>  * on and wait for it to become ready. If additional configuration is required
>  * (via a control bus such as I2C, SPI or DSI for example) this is a good time
>  * to do that.
>  *
>  * After the display controller has started transmitting video data, it's safe
>  * to call the .enable() function. This will typically enable the backlight to
>  * make the image on screen visible. Some panels require a certain amount of
>  * time or frames before the image is displayed. This function is responsible
>  * for taking this into account before enabling the backlight to avoid visual
>  * glitches.
>  *
>  * Before stopping video transmission from the display controller it can be
>  * necessary to turn off the panel to avoid visual glitches. This is done in
>  * the .disable() function. Analogously to .enable() this typically involves
>  * turning off the backlight and waiting for some time to make sure no image
>  * is visible on the panel. It is then safe for the display controller to
>  * cease transmission of video data.
>  *
>  * To save power when no video data is transmitted, a driver can power down
>  * the panel. This is the job of the .unprepare() function.
>  */
>
> As you see, .prepare() should do whatever is necessary to make the panel
> accept a stream of video data, then the display driver can start sending
> video data and call .enable() to make the transmitted data visible.
>
> Analogously .disable() should turn off the display, so that the user can
> no longer see what's going on, then the display controller can cease
> transmission of video data (and since the panel is disabled this should
> no longer cause visual glitches) and then call .unprepare() to turn the
> panel off.
>
> I know that this isn't immediately relevant just yet because currently
> the backlight will already turn on by default, but like we discussed
> earlier I have ideas on how to properly fix it. As a matter of fact I'll
> go and send out another mail about that when I'm through this series.
>
>>  static const struct drm_panel_funcs ld9040_drm_funcs = {
>> +     .unprepare = ld9040_unprepare,
>>       .disable = ld9040_disable,
>> +     .prepare = ld9040_prepare,
>>       .enable = ld9040_enable,
>>       .get_modes = ld9040_get_modes,
>>  };
>
> The patch I applied for .prepare() and .unprepare() I've reordered the
> functions slightly to give a more natural sequence. .disable() is now
> first, while .unprepare() is next, since that's the sequence that they
> should be called in.
>
> Patches for the panel drivers should follow this same ordering.
Ok. I will follow the same order for all panel drivers.

>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index a251361..fb0cfe2 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -45,7 +45,8 @@ struct panel_desc {
>>
>>  struct panel_simple {
>>       struct drm_panel base;
>> -     bool enabled;
>> +     bool panel_enabled;
>> +     bool backlight_enabled;
>
> Perhaps this should rather be:
>
>         bool prepared;
>         bool enabled;
>
More generic. Will change it!

Ajay

>
>> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
>> index 446837e..b574ee6 100644
>> --- a/drivers/gpu/drm/tegra/output.c
>> +++ b/drivers/gpu/drm/tegra/output.c
>> @@ -139,9 +139,11 @@ static void tegra_encoder_dpms(struct drm_encoder *encoder, int mode)
>>
>>       if (mode != DRM_MODE_DPMS_ON) {
>>               drm_panel_disable(panel);
>> +             drm_panel_unprepare(panel);
>>               tegra_output_disable(output);
>
> Similarly to my comments for the Exynos drivers, this should be:
>
>                 drm_panel_disable(panel);
>                 tegra_output_disable(output);
>                 drm_panel_unprepare(panel);
>
>>       } else {
>>               tegra_output_enable(output);
>> +             drm_panel_prepare(panel);
>>               drm_panel_enable(panel);
>>       }
>
> and
>
>                 drm_panel_prepare(panel);
>                 tegra_output_enable(output);
>                 drm_panel_enable(panel);
>
> Thierry