From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3EAEAC4727D for ; Mon, 5 Oct 2020 12:16:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 03A7C2075A for ; Mon, 5 Oct 2020 12:16:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726404AbgJEMQG (ORCPT ); Mon, 5 Oct 2020 08:16:06 -0400 Received: from mga07.intel.com ([134.134.136.100]:64423 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725960AbgJEMQF (ORCPT ); Mon, 5 Oct 2020 08:16:05 -0400 IronPort-SDR: uh2RaNJo+xsZ80DTaLUiy8ND85NAGE9g0pXJDpyKjTkwBAbkVkqzAhm0GL8ianZn9QYnsb4tfX VlyNaUsvTBUw== X-IronPort-AV: E=McAfee;i="6000,8403,9764"; a="227548110" X-IronPort-AV: E=Sophos;i="5.77,338,1596524400"; d="scan'208";a="227548110" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Oct 2020 05:15:29 -0700 IronPort-SDR: wsBWwSr0FVKe1J6GMO9fuGVhlhCkBjZALlmfwFxxmZIyc0tu3eQVednjIO54kMtSYkYExZ7mPi pae6ZKzmmdhA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,338,1596524400"; d="scan'208";a="296094490" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by fmsmga008.fm.intel.com with SMTP; 05 Oct 2020 05:15:24 -0700 Received: by stinkbox (sSMTP sendmail emulation); Mon, 05 Oct 2020 15:15:24 +0300 Date: Mon, 5 Oct 2020 15:15:24 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rob Clark Cc: Daniel Vetter , Rob Clark , linux-arm-msm , open list , Tim Murray , dri-devel , Tejun Heo , Qais Yousef Subject: Re: [PATCH v2 0/3] drm: commit_work scheduling Message-ID: <20201005121524.GI6112@intel.com> References: <20200930211723.3028059-1-robdclark@gmail.com> <20201002105256.GA6112@intel.com> <20201002110544.GB6112@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Fri, Oct 02, 2020 at 10:55:52AM -0700, Rob Clark wrote: > On Fri, Oct 2, 2020 at 4:05 AM Ville Syrjälä > wrote: > > > > On Fri, Oct 02, 2020 at 01:52:56PM +0300, Ville Syrjälä wrote: > > > On Thu, Oct 01, 2020 at 05:25:55PM +0200, Daniel Vetter wrote: > > > > On Thu, Oct 1, 2020 at 5:15 PM Rob Clark wrote: > > > > > > > > > > On Thu, Oct 1, 2020 at 12:25 AM Daniel Vetter wrote: > > > > > > > > > > > > On Wed, Sep 30, 2020 at 11:16 PM Rob Clark wrote: > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > The android userspace treats the display pipeline as a realtime problem. > > > > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > > > > > > > it is. (See https://lwn.net/Articles/809545/ for the best explaination > > > > > > > that I found.) > > > > > > > > > > > > > > But this presents a problem with using workqueues for non-blocking > > > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > > > > > > > preempt the worker. Which is not really the outcome you want.. once > > > > > > > the required fences are scheduled, you want to push the atomic commit > > > > > > > down to hw ASAP. > > > > > > > > > > > > > > But the decision of whether commit_work should be RT or not really > > > > > > > depends on what userspace is doing. For a pure CFS userspace display > > > > > > > pipeline, commit_work() should remain SCHED_NORMAL. > > > > > > > > > > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC > > > > > > > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > > > > > > > used to avoid serializing commits when userspace is using a per-CRTC > > > > > > > update loop. And the last patch exposes the task id to userspace as > > > > > > > a CRTC property, so that userspace can adjust the priority and sched > > > > > > > policy to fit it's needs. > > > > > > > > > > > > > > > > > > > > > v2: Drop client cap and in-kernel setting of priority/policy in > > > > > > > favor of exposing the kworker tid to userspace so that user- > > > > > > > space can set priority/policy. > > > > > > > > > > > > Yeah I think this looks more reasonable. Still a bit irky interface, > > > > > > so I'd like to get some kworker/rt ack on this. Other opens: > > > > > > - needs userspace, the usual drill > > > > > > > > > > fwiw, right now the userspace is "modetest + chrt".. *probably* the > > > > > userspace will become a standalone helper or daemon, mostly because > > > > > the chrome gpu-process sandbox does not allow setting SCHED_FIFO. I'm > > > > > still entertaining the possibility of switching between rt and cfs > > > > > depending on what is in the foreground (ie. only do rt for android > > > > > apps). > > > > > > > > > > > - we need this also for vblank workers, otherwise this wont work for > > > > > > drivers needing those because of another priority inversion. > > > > > > > > > > I have a thought on that, see below.. > > > > > > > > Hm, not seeing anything about vblank worker below? > > > > > > > > > > - we probably want some indication of whether this actually does > > > > > > something useful, not all drivers use atomic commit helpers. Not sure > > > > > > how to do that. > > > > > > > > > > I'm leaning towards converting the other drivers over to use the > > > > > per-crtc kwork, and then dropping the 'commit_work` from atomic state. > > > > > I can add a patch to that, but figured I could postpone that churn > > > > > until there is some by-in on this whole idea. > > > > > > > > i915 has its own commit code, it's not even using the current commit > > > > helpers (nor the commit_work). Not sure how much other fun there is. > > > > > > I don't think we want per-crtc threads for this in i915. Seems > > > to me easier to guarantee atomicity across multiple crtcs if > > > we just commit them from the same thread. > > > > Oh, and we may have to commit things in a very specific order > > to guarantee the hw doesn't fall over, so yeah definitely per-crtc > > thread is a no go. > > If I'm understanding the i915 code, this is only the case for modeset > commits? I suppose we could achieve the same result by just deciding > to pick the kthread of the first CRTC for modeset commits. I'm not > really so much concerned about parallelism for modeset. I'm not entirely happy about the random differences between modesets and other commits. Ideally we wouldn't need any. Anyways, even if we ignore modesets we still have the issue with atomicity guarantees across multiple crtcs. So I think we still don't want per-crtc threads, rather it should be thread for each commit. Well, if the crtcs aren't running in lockstep then maybe we could shove them off to separate threads, but that'll just complicate things needlessly I think since we'd need yet another way to iterate the crtcs in each thread. With the thread-per-commit apporach we can just use the normal atomic iterators. > > > I don't even understand the serialization argument. If the commits > > are truly independent then why isn't the unbound wq enough to avoid > > the serialization? It should just spin up a new thread for each commit > > no? > > The problem with wq is prioritization and SCHED_FIFO userspace > components stomping on the feet of commit_work. That is the entire > motivation of this series in the first place, so no we cannot use > unbound wq. This is a bit dejavu of the vblank worker discussion, where I actually did want a per-crtc RT kthread but people weren't convinced they actually help. The difference is that for vblank workers we actually tried to get some numbers, here I've not seen any. -- Ville Syrjälä Intel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0AE2EC4363A for ; Mon, 5 Oct 2020 12:16:01 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9E39F2075A for ; Mon, 5 Oct 2020 12:15:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9E39F2075A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0FB0789D4B; Mon, 5 Oct 2020 12:15:58 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id E2D0089D4B for ; Mon, 5 Oct 2020 12:15:56 +0000 (UTC) IronPort-SDR: GJlEOdkoWcgOY2mV+8Clw4WHx1+HI439L0VvjqUrTBWQULF83/L4KAn1174sajSbtRMjDgJCaX bmaN9C7CqRIw== X-IronPort-AV: E=McAfee;i="6000,8403,9764"; a="248145642" X-IronPort-AV: E=Sophos;i="5.77,338,1596524400"; d="scan'208";a="248145642" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Oct 2020 05:15:29 -0700 IronPort-SDR: wsBWwSr0FVKe1J6GMO9fuGVhlhCkBjZALlmfwFxxmZIyc0tu3eQVednjIO54kMtSYkYExZ7mPi pae6ZKzmmdhA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,338,1596524400"; d="scan'208";a="296094490" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by fmsmga008.fm.intel.com with SMTP; 05 Oct 2020 05:15:24 -0700 Received: by stinkbox (sSMTP sendmail emulation); Mon, 05 Oct 2020 15:15:24 +0300 Date: Mon, 5 Oct 2020 15:15:24 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rob Clark Subject: Re: [PATCH v2 0/3] drm: commit_work scheduling Message-ID: <20201005121524.GI6112@intel.com> References: <20200930211723.3028059-1-robdclark@gmail.com> <20201002105256.GA6112@intel.com> <20201002110544.GB6112@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Patchwork-Hint: comment User-Agent: Mutt/1.10.1 (2018-07-13) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Clark , linux-arm-msm , open list , Tim Murray , dri-devel , Tejun Heo , Qais Yousef Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Fri, Oct 02, 2020 at 10:55:52AM -0700, Rob Clark wrote: > On Fri, Oct 2, 2020 at 4:05 AM Ville Syrj=E4l=E4 > wrote: > > > > On Fri, Oct 02, 2020 at 01:52:56PM +0300, Ville Syrj=E4l=E4 wrote: > > > On Thu, Oct 01, 2020 at 05:25:55PM +0200, Daniel Vetter wrote: > > > > On Thu, Oct 1, 2020 at 5:15 PM Rob Clark wrot= e: > > > > > > > > > > On Thu, Oct 1, 2020 at 12:25 AM Daniel Vetter w= rote: > > > > > > > > > > > > On Wed, Sep 30, 2020 at 11:16 PM Rob Clark wrote: > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > The android userspace treats the display pipeline as a realti= me problem. > > > > > > > And arguably, if your goal is to not miss frame deadlines (ie= . vblank), > > > > > > > it is. (See https://lwn.net/Articles/809545/ for the best ex= plaination > > > > > > > that I found.) > > > > > > > > > > > > > > But this presents a problem with using workqueues for non-blo= cking > > > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread= (s) can > > > > > > > preempt the worker. Which is not really the outcome you want= .. once > > > > > > > the required fences are scheduled, you want to push the atomi= c commit > > > > > > > down to hw ASAP. > > > > > > > > > > > > > > But the decision of whether commit_work should be RT or not r= eally > > > > > > > depends on what userspace is doing. For a pure CFS userspace= display > > > > > > > pipeline, commit_work() should remain SCHED_NORMAL. > > > > > > > > > > > > > > To handle this, convert non-blocking commit_work() to use per= -CRTC > > > > > > > kthread workers, instead of system_unbound_wq. Per-CRTC work= ers are > > > > > > > used to avoid serializing commits when userspace is using a p= er-CRTC > > > > > > > update loop. And the last patch exposes the task id to users= pace as > > > > > > > a CRTC property, so that userspace can adjust the priority an= d sched > > > > > > > policy to fit it's needs. > > > > > > > > > > > > > > > > > > > > > v2: Drop client cap and in-kernel setting of priority/policy = in > > > > > > > favor of exposing the kworker tid to userspace so that us= er- > > > > > > > space can set priority/policy. > > > > > > > > > > > > Yeah I think this looks more reasonable. Still a bit irky inter= face, > > > > > > so I'd like to get some kworker/rt ack on this. Other opens: > > > > > > - needs userspace, the usual drill > > > > > > > > > > fwiw, right now the userspace is "modetest + chrt".. *probably* t= he > > > > > userspace will become a standalone helper or daemon, mostly becau= se > > > > > the chrome gpu-process sandbox does not allow setting SCHED_FIFO.= I'm > > > > > still entertaining the possibility of switching between rt and cfs > > > > > depending on what is in the foreground (ie. only do rt for android > > > > > apps). > > > > > > > > > > > - we need this also for vblank workers, otherwise this wont wor= k for > > > > > > drivers needing those because of another priority inversion. > > > > > > > > > > I have a thought on that, see below.. > > > > > > > > Hm, not seeing anything about vblank worker below? > > > > > > > > > > - we probably want some indication of whether this actually does > > > > > > something useful, not all drivers use atomic commit helpers. No= t sure > > > > > > how to do that. > > > > > > > > > > I'm leaning towards converting the other drivers over to use the > > > > > per-crtc kwork, and then dropping the 'commit_work` from atomic s= tate. > > > > > I can add a patch to that, but figured I could postpone that churn > > > > > until there is some by-in on this whole idea. > > > > > > > > i915 has its own commit code, it's not even using the current commit > > > > helpers (nor the commit_work). Not sure how much other fun there is. > > > > > > I don't think we want per-crtc threads for this in i915. Seems > > > to me easier to guarantee atomicity across multiple crtcs if > > > we just commit them from the same thread. > > > > Oh, and we may have to commit things in a very specific order > > to guarantee the hw doesn't fall over, so yeah definitely per-crtc > > thread is a no go. > = > If I'm understanding the i915 code, this is only the case for modeset > commits? I suppose we could achieve the same result by just deciding > to pick the kthread of the first CRTC for modeset commits. I'm not > really so much concerned about parallelism for modeset. I'm not entirely happy about the random differences between modesets and other commits. Ideally we wouldn't need any. Anyways, even if we ignore modesets we still have the issue with atomicity guarantees across multiple crtcs. So I think we still don't want per-crtc threads, rather it should be thread for each = commit. Well, if the crtcs aren't running in lockstep then maybe we could shove them off to separate threads, but that'll just complicate things needlessly I think since we'd need yet another way to iterate the crtcs in each thread. With the thread-per-commit apporach we can just use the normal atomic iterators. > = > > I don't even understand the serialization argument. If the commits > > are truly independent then why isn't the unbound wq enough to avoid > > the serialization? It should just spin up a new thread for each commit > > no? > = > The problem with wq is prioritization and SCHED_FIFO userspace > components stomping on the feet of commit_work. That is the entire > motivation of this series in the first place, so no we cannot use > unbound wq. This is a bit dejavu of the vblank worker discussion, where I actually did want a per-crtc RT kthread but people weren't convinced they actually help. The difference is that for vblank workers we actually tried to get some numbers, here I've not seen any. -- = Ville Syrj=E4l=E4 Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel