From patchwork Thu Jun 29 11:23:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 9816445 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 1ECB5603D7 for ; Thu, 29 Jun 2017 11:24:14 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 032312861C for ; Thu, 29 Jun 2017 11:24:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EAA7628650; Thu, 29 Jun 2017 11:24:13 +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 3842B2861C for ; Thu, 29 Jun 2017 11:24:13 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D76916E656; Thu, 29 Jun 2017 11:24:11 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3FEB86E62F; Thu, 29 Jun 2017 11:24:10 +0000 (UTC) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2017 04:23:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,280,1496127600"; d="scan'208";a="120727789" 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 04:23:46 -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> <45b24a01-ade9-f8df-d6c9-99ea9cc4309a@linux.intel.com> From: Maarten Lankhorst Message-ID: <01e2d2d9-cf1b-c9e0-425f-ffa90e351457@linux.intel.com> Date: Thu, 29 Jun 2017 13:23:46 +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 13:00 schreef Daniel Vetter: > On Thu, Jun 29, 2017 at 12:58 PM, Maarten Lankhorst > wrote: >> 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. :) > Hm yeah, that sounds like a useful simplification. I don't think we > should do too much into extremes in fbdev and only have 1 atomic > commit, because fbdev fundamentally isn't atomic. But merging them > where it makes sense is reasonable. > -Daniel Agreed, perhaps with pan_display_atomic too, if we no longer use modeset locks, then it becomes very simple.. What about getting rid of that one too, in favor of this? diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 0cd2035ac1d1..b4356fdf5e6d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1557,70 +1557,36 @@ int drm_fb_helper_set_par(struct fb_info *info) } EXPORT_SYMBOL(drm_fb_helper_set_par); -static int pan_display_atomic(struct fb_var_screeninfo *var, - struct fb_info *info) +static void pan_set(struct drm_fb_helper *fb_helper, int x, int y) { - struct drm_fb_helper *fb_helper = info->par; - struct drm_device *dev = fb_helper->dev; - struct drm_atomic_state *state; - struct drm_plane *plane; - int i, ret; - unsigned int plane_mask; - 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; - } + int i; - state->acquire_ctx = &ctx; -retry: - plane_mask = 0; for (i = 0; i < fb_helper->crtc_count; i++) { struct drm_mode_set *mode_set; mode_set = &fb_helper->crtc_info[i].mode_set; - mode_set->x = var->xoffset; - mode_set->y = var->yoffset; - - ret = __drm_atomic_helper_set_config(mode_set, state); - if (ret != 0) - goto out_state; - - plane = mode_set->crtc->primary; - plane_mask |= (1 << drm_plane_index(plane)); - plane->old_fb = plane->fb; + mode_set->x = x; + mode_set->y = y; } +} - ret = drm_atomic_commit(state); - if (ret != 0) - goto out_state; - - info->var.xoffset = var->xoffset; - info->var.yoffset = var->yoffset; +static int pan_display_atomic(struct fb_var_screeninfo *var, + struct fb_info *info) +{ + struct drm_fb_helper *fb_helper = info->par; + int ret; -out_state: - drm_atomic_clean_old_fb(dev, plane_mask, ret); + pan_set(fb_helper, var->xoffset, var->yoffset); - if (ret == -EDEADLK) - goto backoff; - - drm_atomic_state_put(state); -out_ctx: - drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); + ret = restore_fbdev_mode(fb_helper); + if (!ret) { + info->var.xoffset = var->xoffset; + info->var.yoffset = var->yoffset; + } else + pan_set(fb_helper, info->var.xoffset, info->var.yoffset); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_modeset_backoff(&ctx); - - goto retry; } static int pan_display_legacy(struct fb_var_screeninfo *var,