From patchwork Thu Jun 29 10:58:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 9816393 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E56C86035F for ; Thu, 29 Jun 2017 10:58:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D9638286C8 for ; Thu, 29 Jun 2017 10:58:10 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CE0B4286F3; Thu, 29 Jun 2017 10:58:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 39D81286EC for ; Thu, 29 Jun 2017 10:58:09 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 245F96E0EA; Thu, 29 Jun 2017 10:58:08 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 143826E0EA; Thu, 29 Jun 2017 10:58:07 +0000 (UTC) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2017 03:58:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,280,1496127600"; d="scan'208";a="120720082" Received: from okremnyo-mobl1.ccr.corp.intel.com (HELO patser.lan) ([10.249.35.3]) by fmsmga006.fm.intel.com with ESMTP; 29 Jun 2017 03:58:04 -0700 Subject: Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths To: Daniel Vetter References: <20170627145936.18983-1-daniel.vetter@ffwll.ch> <20170627145936.18983-10-daniel.vetter@ffwll.ch> <28764010-74e0-5049-ee9c-9ddc944efb20@linux.intel.com> From: Maarten Lankhorst Message-ID: <45b24a01-ade9-f8df-d6c9-99ea9cc4309a@linux.intel.com> Date: Thu, 29 Jun 2017 12:58:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Cc: Daniel Vetter , Intel Graphics Development , Thierry Reding , Peter Rosin , DRI Development X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Op 29-06-17 om 12:31 schreef Daniel Vetter: > On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst > wrote: >>> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode) >>> +{ >>> + struct drm_device *dev = fb_helper->dev; >>> + struct drm_atomic_state *state; >>> + int i, ret; >>> + >>> + struct drm_modeset_acquire_ctx ctx; >>> + >>> + drm_modeset_acquire_init(&ctx, 0); >>> + >>> + state = drm_atomic_state_alloc(dev); >>> + if (!state) { >>> + ret = -ENOMEM; >>> + goto out_ctx; >>> + } >>> + >>> + state->acquire_ctx = &ctx; >>> +retry: >>> + for (i = 0; i < fb_helper->crtc_count; i++) { >>> + struct drm_crtc_state *crtc_state; >>> + struct drm_crtc *crtc; >>> + >>> + if (!fb_helper->crtc_info[i].mode_set.mode) >>> + continue; >>> + >>> + crtc = fb_helper->crtc_info[i].mode_set.crtc; >>> + >>> + crtc_state = drm_atomic_get_crtc_state(state, crtc); >>> + if (IS_ERR(crtc_state)) { >>> + ret = PTR_ERR(crtc_state); >>> + goto out_state; >>> + } >> Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ? >> >> I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state, >> similar to how DPMS on is ignored without CRTC. > I just blindly smashed the old fbdev code in with the helper and moved > the locking out. Not sure what would be better here, since the > continue is in a way just part of a non-existent > drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth > it to overpolish this code to such an extent :-) > -Daniel Well checking for crtc_state->enable instead of mode_set.mode probably means we're at least bug for bug compatible vs legacy. It might be better to add a bool active to restore_fbdev_mode_atomic. What about something like this? The less atomic commits the better. :) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e49bae10f0ee..2c401fe8531c 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -378,7 +378,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info) } EXPORT_SYMBOL(drm_fb_helper_debug_leave); -static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) +static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active) { struct drm_device *dev = fb_helper->dev; struct drm_plane *plane; @@ -427,6 +427,17 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) ret = __drm_atomic_helper_set_config(mode_set, state); if (ret != 0) goto out_state; + + /* + * __drm_atomic_helper_set_config() sets active when a + * mode is set, unconditionally clear it if we force DPMS off + */ + if (!active) { + struct drm_crtc *crtc = mode_set->crtc; + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + + crtc_state->active = false; + } } ret = drm_atomic_commit(state); @@ -497,7 +508,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) struct drm_device *dev = fb_helper->dev; if (drm_drv_uses_atomic_modeset(dev)) - return restore_fbdev_mode_atomic(fb_helper); + return restore_fbdev_mode_atomic(fb_helper, true); else return restore_fbdev_mode_legacy(fb_helper); } @@ -714,7 +669,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) } if (drm_drv_uses_atomic_modeset(fb_helper->dev)) - dpms_atomic(fb_helper, dpms_mode); + restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON); else dpms_legacy(fb_helper, dpms_mode); mutex_unlock(&fb_helper->lock);