linux-kernel - Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1533210010.3516.21.camel@pengutronix.de>
Date:   Thu, 02 Aug 2018 13:40:10 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Stefan Agner <stefan@...er.ch>,
        Leonard Crestez <leonard.crestez@....com>
Cc:     Marek Vasut <marex@...x.de>, Anson Huang <Anson.Huang@....com>,
        David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        Robert Chiras <robert.chiras@....com>, linux-imx@....com,
        kernel@...gutronix.de, Fabio Estevam <fabio.estevam@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Lucas Stach <l.stach@...gutronix.de>
Subject: Re: [PATCH v2] drm/mxsfb: Fix runtime PM for unpowering lcdif block

Hi Stefan,

On Wed, 2018-08-01 at 12:00 +0200, Stefan Agner wrote:
[...]
> > @@ -307,14 +335,15 @@ void mxsfb_plane_atomic_update(struct
> > mxsfb_drm_private *mxsfb,
> >  			drm_crtc_send_vblank_event(crtc, event);
> >  		}
> >  	}
> >  	spin_unlock_irq(&crtc->dev->event_lock);
> >  
> > -	if (!fb)
> > +	if (!mxsfb->enabled)
> >  		return;
> >  
> 
> I think this is the wrong thing to do.
> 
> The simple KMS helper callback ->update() is called by the
> ->atomic_update() callback of drm_plane_helper_funcs.
> 
> And the documentation of atomic_update() states:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_plane_helper_funcs
> 
> "Note that the power state of the display pipe when this function is
> called depends upon the exact helpers and calling sequence the driver
> has picked. See drm_atomic_helper_commit_planes() for a discussion of
> the tradeoffs and variants of plane commit helpers."
> 
> The documentation of drm_atomic_helper_commit_planes() then has more
> information:
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#c.drm_atomic_helper_commit_planes
> 
> Bottom line, we should be using drm_atomic_helper_commit_tail_rpm() for
> runtime pm...
> 
> So adding something like:
> 
> static const struct drm_mode_config_helper_funcs
> mxsfb_drm_mode_config_helpers = {
> 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> };
> 
> And add something like this in mxsfb_load:
> 
> 	drm->mode_config.funcs		= &mxsfb_mode_config_funcs;
> +	dev->mode_config.helper_private = &mxsfb_drm_mode_config_helpers;
> ...
> 
> Should make the stack not calling update while the pipe is disabled.
> 
> With that you do not have to keep state locally and can always apply the
> new state in ->enable().

Yes, thank you for the explanation. That is exactly what I would have
expected.

regards
Philipp