[PATCH v14 10/41] compositor-drm: Disable unused CRTCs/connectors

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:27 +0000
Daniel Stone <daniels at collabora.com> wrote:

> If we have an unused CRTC or connector, explicitly disable it during the
> end of the repaint cycle, or when we get VT-switched back in.
> 
> This commit moves state_invalid from an output property to a backend
> property, as the unused CRTCs or connectors are likely not tracked by
> drm_outputs. This matches the mechanics of later commits, where we move
> to a global repaint-flush hook, applying the state for all outputs in
> one go.

Hi Daniel,

I have a recollection of having a rationale that the state_invalid flag
should stay per-output, but it seems those ideas were just optimization
that's probably moot anyway. I suppose with atomic, the per-output flag
would not make any difference to a global flag, since the atomic commit
would be modeset-or-not as a whole?

I wonder, could the following happen:
- have outputs A and B at different timing phase such that they usually
  don't coalesce to the same atomic update
- output A needs a modeset
- output B repaints first, does modeset, and clears the flag
- output A does not get a modeset

Would we actually need per-output flags and a flag for the unused CRTCs?

Clone mode will bring new causes for "needs a modeset": attaching or
detaching a head from a running output.

If we were good with just a global flag, then this patch would be good.


Thanks,
pq


> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 9c7564383..87c2603c7 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -188,6 +188,7 @@ struct drm_backend {
>  
>  	void *repaint_data;
>  
> +	bool state_invalid;
>  	struct wl_array unused_connectors;
>  	struct wl_array unused_crtcs;
>  
> @@ -350,8 +351,6 @@ struct drm_output {
>  	enum dpms_enum dpms;
>  	struct backlight *backlight;
>  
> -	bool state_invalid;
> -
>  	int vblank_pending;
>  	int page_flip_pending;
>  	int destroy_pending;
> @@ -1748,7 +1747,7 @@ drm_output_repaint(struct weston_output *output_base,
>  	assert(scanout_state->dest_h == scanout_state->src_h >> 16);
>  
>  	mode = container_of(output->base.current_mode, struct drm_mode, base);
> -	if (output->state_invalid || !scanout_plane->state_cur->fb ||
> +	if (backend->state_invalid || !scanout_plane->state_cur->fb ||
>  	    scanout_plane->state_cur->fb->stride != scanout_state->fb->stride) {
>  		ret = drmModeSetCrtc(backend->drm.fd, output->crtc_id,
>  				     scanout_state->fb->fb_id,
> @@ -1760,9 +1759,7 @@ drm_output_repaint(struct weston_output *output_base,
>  			goto err;
>  		}
>  		output_base->set_dpms(output_base, WESTON_DPMS_ON);
> -
> -		output->state_invalid = false;
> -	}
> +        }
>  
>  	if (drmModePageFlip(backend->drm.fd, output->crtc_id,
>  			    scanout_state->fb->fb_id,
> @@ -1869,7 +1866,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>  	/* Need to smash all state in from scratch; current timings might not
>  	 * be what we want, page flip might not work, etc.
>  	 */
> -	if (output->state_invalid)
> +	if (backend->state_invalid)
>  		goto finish_frame;
>  
>  	assert(scanout_plane->state_cur->output == output);
> @@ -2033,12 +2030,26 @@ drm_repaint_flush(struct weston_compositor *compositor, void *repaint_data)
>  	struct drm_backend *b = to_drm_backend(compositor);
>  	struct drm_pending_state *pending_state = repaint_data;
>  	struct drm_output_state *output_state, *tmp;
> +	uint32_t *unused;
> +
> +	if (b->state_invalid) {
> +		/* If we need to reset all our state (e.g. because we've
> +		 * just started, or just been VT-switched in), explicitly
> +		 * disable all the CRTCs we aren't using. This also disables
> +		 * all connectors on these CRTCs, so we don't need to do that
> +		 * separately with the pre-atomic API. */
> +		wl_array_for_each(unused, &b->unused_crtcs)
> +			drmModeSetCrtc(b->drm.fd, *unused, 0, 0, 0, NULL, 0,
> +				       NULL);
> +	}
>  
>  	wl_list_for_each_safe(output_state, tmp, &pending_state->output_list,
>  			      link) {
>  		drm_output_assign_state(output_state, DRM_STATE_APPLY_ASYNC);
>  	}
>  
> +	b->state_invalid = false;
> +
>  	drm_pending_state_free(pending_state);
>  	b->repaint_data = NULL;
>  }
> @@ -2659,7 +2670,7 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
>  	 *      sledgehammer modeswitch first, and only later showing new
>  	 *      content.
>  	 */
> -	output->state_invalid = true;
> +	b->state_invalid = true;
>  
>  	if (b->use_pixman) {
>  		drm_output_fini_pixman(output);
> @@ -4000,8 +4011,6 @@ drm_output_enable(struct weston_output *base)
>  				    output->connector->count_modes == 0 ?
>  				    ", built-in" : "");
>  
> -	output->state_invalid = true;
> -
>  	return 0;
>  
>  err:
> @@ -4516,10 +4525,7 @@ session_notify(struct wl_listener *listener, void *data)
>  		weston_log("activating session\n");
>  		weston_compositor_wake(compositor);
>  		weston_compositor_damage_all(compositor);
> -
> -		wl_list_for_each(output, &compositor->output_list, base.link)
> -			output->state_invalid = true;
> -
> +		b->state_invalid = true;
>  		udev_input_enable(&b->input);
>  	} else {
>  		weston_log("deactivating session\n");
> @@ -4931,6 +4937,7 @@ drm_backend_create(struct weston_compositor *compositor,
>  	if (b == NULL)
>  		return NULL;
>  
> +	b->state_invalid = true;
>  	b->drm.fd = -1;
>  	wl_array_init(&b->unused_crtcs);
>  	wl_array_init(&b->unused_connectors);

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180117/b6ad00b6/attachment.sig>