LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Sebastian Reichel <sebastian.reichel@collabora.co.uk>,
	Sebastian Reichel <sre@kernel.org>,
	Tony Lindgren <tony@atomide.com>, Pavel Machek <pavel@ucw.cz>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	<dri-devel@lists.freedesktop.org>, <devicetree@vger.kernel.org>,
	<linux-omap@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<kernel@collabora.com>
Subject: Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays
Date: Fri, 20 Apr 2018 10:09:38 +0300	[thread overview]
Message-ID: <a078d8c3-9e65-cc7b-737a-cbd5cf141f95@ti.com> (raw)
In-Reply-To: <20180330171822.25896-4-sebastian.reichel@collabora.co.uk>

Hi Sebastian,

On 30/03/18 20:18, Sebastian Reichel wrote:
> This adds the required infrastructure for manually
> updated displays, such as DSI command mode panels.
> 
> While those panels often support partial updates
> we currently always do a full refresh. Display
> will be refreshed when something calls the dirty
> callback, such as libdrm's drmModeDirtyFB().
> 
> This is currently being implemented for the kernel
> console and for Xorg. Weston currently does not
> implement this and is known not to work on manually
> updated displays.
> 
> Tested-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 107 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/omapdrm/omap_crtc.h |   1 +
>  drivers/gpu/drm/omapdrm/omap_fb.c   |  20 +++++++
>  3 files changed, 120 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index b893985e4efb..1b91bff5bac6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -51,6 +51,7 @@ struct omap_crtc {
>  	bool pending;
>  	wait_queue_head_t pending_wait;
>  	struct drm_pending_vblank_event *event;
> +	struct delayed_work update_work;
>  
>  	void (*framedone_handler)(void *);
>  	void *framedone_handler_data;
> @@ -146,6 +147,25 @@ static void omap_crtc_dss_disconnect(struct omap_drm_private *priv,
>  static void omap_crtc_dss_start_update(struct omap_drm_private *priv,
>  				       enum omap_channel channel)
>  {
> +	priv->dispc_ops->mgr_enable(priv->dispc, channel, true);
> +}
> +
> +static bool omap_crtc_is_manually_updated(struct drm_crtc *crtc)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	bool result = false;
> +
> +	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (connector->state->crtc != crtc)
> +			continue;
> +		result = omap_connector_get_manually_updated(connector);
> +		break;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);

It would be much nicer if the is-manual flag was somehow conveyed from
connector/encoder to the crtc when doing modesetting. I don't know how
or where, so just thinking out loud. However, if we need to do such loop
as above, I think we should just do it once, perhaps in
omap_crtc_atomic_enable, and store the value in omap_crtc's private data.

> +
> +	return result;
>  }
>  
>  /* Called only from the encoder enable/disable and suspend/resume handlers. */
> @@ -157,12 +177,17 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  	enum omap_channel channel = omap_crtc->channel;
>  	struct omap_irq_wait *wait;
>  	u32 framedone_irq, vsync_irq;
> +	bool is_manual = omap_crtc_is_manually_updated(crtc);
> +	enum omap_display_type type = omap_crtc_output[channel]->output_type;
>  	int ret;
>  
>  	if (WARN_ON(omap_crtc->enabled == enable))
>  		return;
>  
> -	if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) {
> +	if (is_manual)
> +		omap_irq_enable_framedone(crtc, enable);
> +
> +	if (is_manual || type == OMAP_DISPLAY_TYPE_HDMI) {
>  		priv->dispc_ops->mgr_enable(priv->dispc, channel, enable);
>  		omap_crtc->enabled = enable;
>  		return;

This doesn't look correct, omap_crtc_dss_start_update() already sets the
enable bit for manual update displays. And you don't want to set the
enable to false with manual update displays.

HDMI handling here is already a special case due to HW issues, so I'd
rather see the manual update handled separately from the HDMI.

> @@ -214,7 +239,6 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  	}
>  }
>  
> -
>  static int omap_crtc_dss_enable(struct omap_drm_private *priv,
>  				enum omap_channel channel)
>  {
> @@ -378,6 +402,53 @@ void omap_crtc_framedone_irq(struct drm_crtc *crtc, uint32_t irqstatus)
>  	wake_up(&omap_crtc->pending_wait);
>  }
>  
> +void omap_crtc_flush(struct drm_crtc *crtc)
> +{
> +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +
> +	if (!omap_crtc_is_manually_updated(crtc))
> +		return;
> +
> +	if (!delayed_work_pending(&omap_crtc->update_work))
> +		schedule_delayed_work(&omap_crtc->update_work, 0);
> +}

Why delayed work here?

It's actually not quite clear to me how manual update displays work with
DRM...

As far as I see, we have essentially two cases: 1) single buffering,
where the userspace must set an area in the fb dirty, which then
triggers the update, 2) multi buffering, which doesn't need fb dirty,
but just a page flip which triggers the update.

In the 2) case (which I think is the optimal case which all the modern
apps should use), there's no need for delayed work or any work, and the
code flow should be very similar to the auto-update model.

Also, as Daniel mentioned in "drm/omapdrm: Nuke
omap_framebuffer_get_next_connector()" thread, the dirtying mechanism in
the series is not valid.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

  reply	other threads:[~2018-04-20  7:10 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-30 17:18 [PATCHv3 0/8] omapdrm: DSI command mode panel support Sebastian Reichel
2018-03-30 17:18 ` [PATCHv3 1/8] drm/omap: add framedone interrupt support Sebastian Reichel
2018-03-30 17:18 ` [PATCHv3 2/8] drm/omap: add manual update detection helper Sebastian Reichel
2018-03-30 17:18 ` [PATCHv3 3/8] drm/omap: add support for manually updated displays Sebastian Reichel
2018-04-20  7:09   ` Tomi Valkeinen [this message]
2018-04-20  8:11     ` Daniel Vetter
2018-04-20 10:19     ` Daniel Stone
2018-04-20 14:25       ` Tony Lindgren
2018-04-20 14:39         ` Daniel Stone
2018-04-20 14:44           ` Tony Lindgren
2018-03-30 17:18 ` [PATCHv3 4/8] drm/omap: make omap_framebuffer_get_next_connector static Sebastian Reichel
2018-03-30 17:18 ` [PATCHv3 5/8] dt-bindings: panel: common: document orientation property Sebastian Reichel
2018-04-03 10:49   ` Pavel Machek
2018-04-09 20:29   ` Rob Herring
2018-03-30 17:18 ` [PATCHv3 6/8] drm/omap: add support for rotation hints from display drivers Sebastian Reichel
2018-03-30 17:18 ` [PATCHv3 7/8] drm/omap: panel-dsi-cm: add rotation support Sebastian Reichel
2018-04-03 10:49   ` Pavel Machek
2018-03-30 17:18 ` [PATCHv3 8/8] ARM: dts: omap4-droid4: Add LCD panel rotation property Sebastian Reichel
2018-04-03 10:49   ` Pavel Machek

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=a078d8c3-9e65-cc7b-737a-cbd5cf141f95@ti.com \
    --to=tomi.valkeinen@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=sebastian.reichel@collabora.co.uk \
    --cc=sre@kernel.org \
    --cc=tony@atomide.com \
    --subject='Re: [PATCHv3 3/8] drm/omap: add support for manually updated displays' \
    /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).