linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Leonard Crestez <leonard.crestez@nxp.com>
To: "stefan@agner.ch" <stefan@agner.ch>,
	"l.stach@pengutronix.de" <l.stach@pengutronix.de>,
	"marex@denx.de" <marex@denx.de>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Laurentiu Palcu <laurentiu.palcu@nxp.com>,
	Robert Chiras <robert.chiras@nxp.com>,
	Marco Antonio Franchi <marco.franchi@nxp.com>,
	Fabio Estevam <fabio.estevam@nxp.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"airlied@linux.ie" <airlied@linux.ie>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	Mirela Rabulea <mirela.rabulea@nxp.com>
Subject: Re: [PATCH 1/2] drm/mxsfb: Fix runtime PM for unpowering lcdif block
Date: Tue, 17 Jul 2018 09:54:03 +0000	[thread overview]
Message-ID: <afe143888ae3c280e27884e3d98ff22f0c267006.camel@nxp.com> (raw)
In-Reply-To: <2024fd1b31818e5f62c779a33c6011e4@agner.ch>

On Fri, 2018-07-13 at 12:57 +0200, Stefan Agner wrote:
> On 05.06.2018 19:11, Leonard Crestez wrote:
> > Adding imx6sl/sx lcdif nodes in a power domain currently does work, it
> > results in black/corrupted screens or hangs. While the driver does
> > enable runtime pm it does not deal correctly with the block being
> > unpowered.
> > 
> > Fix by adding pm_runtime_get/put_sync to mxsfb_pipe_enable/disable.
> > 
> > The mxsfb_plane_atomic_update function can get called before
> > mxsfb_pipe_enable while the block is not powered. When this happens the
> > write to LCDIF_NEXT_BUF is lost causing random corrupted pixels on
> > unblank.
> > 
> > Fix this by splitting the writing of gem->paddr to nextbuf into a
> > mxsfb_update_hw_next_buf functiona and also calling it from
> > mxsfb_crtc_mode_set_nofb.
> > 
> > Also add fields to mxsfb_drv to keep track of enabled/suspended states.

> > +static void mxsfb_update_hw_next_buf(struct mxsfb_drm_private *mxsfb)
> > +{
> > +	struct drm_framebuffer *fb = mxsfb->pipe.plane.state->fb;
> > +	struct drm_gem_cma_object *gem;
> > +
> > +	if (!fb)
> > +		return;
> > +
> > +	gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +	if (!gem)
> > +		return;
> > +
> > +	writel(gem->paddr, mxsfb->base + mxsfb->devdata->next_buf);
> > +}
> > +
> >  static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb)
> >  {
> >  	struct drm_display_mode *m = &mxsfb->pipe.crtc.state->adjusted_mode;
> >  	const u32 bus_flags = mxsfb->connector.display_info.bus_flags;
> >  	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
> > @@ -268,35 +283,41 @@ static void mxsfb_crtc_mode_set_nofb(struct
> > mxsfb_drm_private *mxsfb)
> >  	       mxsfb->base + LCDC_VDCTRL3);
> >  
> >  	writel(SET_DOTCLK_H_VALID_DATA_CNT(m->hdisplay),
> >  	       mxsfb->base + LCDC_VDCTRL4);
> >  
> > +	mxsfb_update_hw_next_buf(mxsfb);
> >  	mxsfb_disable_axi_clk(mxsfb);
> >  }
> >  
> >  void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb)
> >  {
> > +	if (mxsfb->enabled)
> > +		return;
> > +
> >  	mxsfb_crtc_mode_set_nofb(mxsfb);
> >  	mxsfb_enable_controller(mxsfb);
> > +
> > +	mxsfb->enabled = true;
> 
> Is this state keeping really necessary?

Sort of. The code is indeed a bit complicated, there are 3 possible
states: "disabled", "enabled", "enabled but suspended"

When the device resumes it should enable the controller only if it was
enabled before (not if screen is blanked). After some digging I found
that suspend/resume could be better implemented using
drm_mode_config_helper_suspend/resume. This should automatically
remember the crtc state "properly" without keeping state in the driver.

One notable issue is that the lcdif block is only powered while the
crtc is enabled but mxsfb_plane_atomic_update can be called outside
that. So we need to check if the CRTC is enabled inside
mxsfb_plane_atomic_update and avoid writing to hardware. Instead the HW
write for next is delayed for the next mxsfb_crtc_mode_set_nofb.

Does this make sense? Being unable to update the plane if the crtc is
not enabled seems like it would be a common limitation of simple
display controllers.

Writing the FB paddr from mxsfb_crtc_mode_set_nofb feels wrong and I'm
not sure it behaves correctly: when the display is resumed it seems to
output an initial frame of incorrect data. It's possible the enable
sequence needs to be adjusted.

      reply	other threads:[~2018-07-17  9:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 17:11 Leonard Crestez
2018-06-05 17:11 ` [RFC 2/2] ARM: dts: imx6sl: Convert gpc to new bindings Leonard Crestez
2018-06-06  9:24   ` Lucas Stach
2018-06-27 14:17     ` Leonard Crestez
2018-07-13 10:57 ` [PATCH 1/2] drm/mxsfb: Fix runtime PM for unpowering lcdif block Stefan Agner
2018-07-17  9:54   ` Leonard Crestez [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=afe143888ae3c280e27884e3d98ff22f0c267006.camel@nxp.com \
    --to=leonard.crestez@nxp.com \
    --cc=airlied@linux.ie \
    --cc=aisheng.dong@nxp.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=fabio.estevam@nxp.com \
    --cc=kernel@pengutronix.de \
    --cc=l.stach@pengutronix.de \
    --cc=laurentiu.palcu@nxp.com \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marco.franchi@nxp.com \
    --cc=marex@denx.de \
    --cc=mirela.rabulea@nxp.com \
    --cc=robert.chiras@nxp.com \
    --cc=shawnguo@kernel.org \
    --cc=stefan@agner.ch \
    --subject='Re: [PATCH 1/2] drm/mxsfb: Fix runtime PM for unpowering lcdif block' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).