[{"id":1763357,"web_url":"http://patchwork.ozlabs.org/comment/1763357/","msgid":"<5cbadb84-31aa-ee3d-7b4b-e83afdfdcf7e@gmail.com>","list_archive_url":null,"date":"2017-09-05T13:33:13","subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 05.09.2017 11:10, Mikko Perttunen wrote:\n> Since Tegra186 the Host1x hardware allows syncpoints to be assigned to\n> specific channels, preventing any other channels from incrementing\n> them.\n> \n> Enable this feature where available and assign syncpoints to channels\n> when submitting a job. Syncpoints are currently never unassigned from\n> channels since that would require extra work and is unnecessary with\n> the current channel allocation model.\n> \n> Signed-off-by: Mikko Perttunen \n> ---\n> \n> Notes:\n> v2:\n> - Changed from set_protection(bool) to enable_protection\n> - Added some comments\n> - Added missing check for hv_regs being NULL in\n> enable_protection\n> \n> drivers/gpu/host1x/dev.h | 15 +++++++++++++\n> drivers/gpu/host1x/hw/channel_hw.c | 3 +++\n> drivers/gpu/host1x/hw/syncpt_hw.c | 46 ++++++++++++++++++++++++++++++++++++++\n> drivers/gpu/host1x/syncpt.c | 8 +++++++\n> 4 files changed, 72 insertions(+)\n> \n> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h\n> index def802c0a6bf..7497cc5ead9e 100644\n> --- a/drivers/gpu/host1x/dev.h\n> +++ b/drivers/gpu/host1x/dev.h\n> @@ -79,6 +79,9 @@ struct host1x_syncpt_ops {\n> \tu32 (*load)(struct host1x_syncpt *syncpt);\n> \tint (*cpu_incr)(struct host1x_syncpt *syncpt);\n> \tint (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr);\n> +\tvoid (*assign_channel)(struct host1x_syncpt *syncpt,\n> +\t struct host1x_channel *channel);\n> +\tvoid (*enable_protection)(struct host1x *host);\n> };\n> \n> struct host1x_intr_ops {\n> @@ -186,6 +189,18 @@ static inline int host1x_hw_syncpt_patch_wait(struct host1x *host,\n> \treturn host->syncpt_op->patch_wait(sp, patch_addr);\n> }\n> \n> +static inline void host1x_hw_syncpt_assign_channel(struct host1x *host,\n> +\t\t\t\t\t\t struct host1x_syncpt *sp,\n> +\t\t\t\t\t\t struct host1x_channel *ch)\n> +{\n> +\treturn host->syncpt_op->assign_channel(sp, ch);\n> +}\n> +\n> +static inline void host1x_hw_syncpt_enable_protection(struct host1x *host)\n> +{\n> +\treturn host->syncpt_op->enable_protection(host);\n> +}\n> +\n> static inline int host1x_hw_intr_init_host_sync(struct host1x *host, u32 cpm,\n> \t\t\tvoid (*syncpt_thresh_work)(struct work_struct *))\n> {\n> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c\n> index 8447a56c41ca..0161da331702 100644\n> --- a/drivers/gpu/host1x/hw/channel_hw.c\n> +++ b/drivers/gpu/host1x/hw/channel_hw.c\n> @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job)\n> \n> \tsyncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs);\n> \n> +\t/* assign syncpoint to channel */\n> +\thost1x_hw_syncpt_assign_channel(host, sp, ch);\n\nThis function could be renamed to host1x_hw_assign_syncpt_to_channel() and then\ncomment to it won't be needed.\n\nIt is not very nice that channel would be re-assigned on each submit. Maybe that\nassignment should be done by host1x_syncpt_request() ?\n\n> +\n> \tjob->syncpt_end = syncval;\n> \n> \t/* add a setclass for modules that require it */\n> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c\n> index 7b0270d60742..dc7a44614fef 100644\n> --- a/drivers/gpu/host1x/hw/syncpt_hw.c\n> +++ b/drivers/gpu/host1x/hw/syncpt_hw.c\n> @@ -106,6 +106,50 @@ static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)\n> \treturn 0;\n> }\n> \n> +/**\n> + * syncpt_assign_channel() - Assign syncpoint to channel\n> + * @sp: syncpoint\n> + * @ch: channel\n> + *\n> + * On chips with the syncpoint protection feature (Tegra186+), assign @sp to\n> + * @ch, preventing other channels from incrementing the syncpoints. If @ch is\n> + * NULL, unassigns the syncpoint.\n> + *\n> + * On older chips, do nothing.\n> + */\n> +static void syncpt_assign_channel(struct host1x_syncpt *sp,\n> +\t\t\t\t struct host1x_channel *ch)\n> +{\n> +#if HOST1X_HW >= 6\n> +\tstruct host1x *host = sp->host;\n> +\n> +\tif (!host->hv_regs)\n> +\t\treturn;\n> +\n> +\thost1x_sync_writel(host,\n> +\t\t\t HOST1X_SYNC_SYNCPT_CH_APP_CH(ch ? ch->id : 0xff),\n> +\t\t\t HOST1X_SYNC_SYNCPT_CH_APP(sp->id));\n> +#endif\n> +}\n> +\n> +/**\n> + * syncpt_enable_protection() - Enable syncpoint protection\n> + * @host: host1x instance\n> + *\n> + * On chips with the syncpoint protection feature (Tegra186+), enable this\n> + * feature. On older chips, do nothing.\n> + */\n> +static void syncpt_enable_protection(struct host1x *host)\n> +{\n> +#if HOST1X_HW >= 6\n> +\tif (!host->hv_regs)\n> +\t\treturn;\n> +\n> +\thost1x_hypervisor_writel(host, HOST1X_HV_SYNCPT_PROT_EN_CH_EN,\n> +\t\t\t\t HOST1X_HV_SYNCPT_PROT_EN);\n> +#endif\n> +}\n> +\n> static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n> \t.restore = syncpt_restore,\n> \t.restore_wait_base = syncpt_restore_wait_base,\n> @@ -113,4 +157,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n> \t.load = syncpt_load,\n> \t.cpu_incr = syncpt_cpu_incr,\n> \t.patch_wait = syncpt_patch_wait,\n> +\t.assign_channel = syncpt_assign_channel,\n> +\t.enable_protection = syncpt_enable_protection,\n> };\n> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c\n> index 048ac9e344ce..4c7a4c8b2ad2 100644\n> --- a/drivers/gpu/host1x/syncpt.c\n> +++ b/drivers/gpu/host1x/syncpt.c\n> @@ -398,6 +398,13 @@ int host1x_syncpt_init(struct host1x *host)\n> \tfor (i = 0; i < host->info->nb_pts; i++) {\n> \t\tsyncpt[i].id = i;\n> \t\tsyncpt[i].host = host;\n> +\n> +\t\t/*\n> +\t\t * Unassign syncpt from channels for purposes of Tegra186\n> +\t\t * syncpoint protection. This prevents any channel from\n> +\t\t * accessing it until it is reassigned.\n> +\t\t */\n> +\t\thost1x_hw_syncpt_assign_channel(host, &syncpt[i], NULL);\n> \t}\n> \n> \tfor (i = 0; i < host->info->nb_bases; i++)\n> @@ -408,6 +415,7 @@ int host1x_syncpt_init(struct host1x *host)\n> \thost->bases = bases;\n> \n> \thost1x_syncpt_restore(host);\n> +\thost1x_hw_syncpt_enable_protection(host);\n> \n> \t/* Allocate sync point to use for clearing waits for expired fences */\n> \thost->nop_sp = host1x_syncpt_alloc(host, NULL, 0);\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"WjunpBtl\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmnjC0nq0z9t2S\n\tfor ;\n\tTue, 5 Sep 2017 23:33:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751679AbdIENdR (ORCPT );\n\tTue, 5 Sep 2017 09:33:17 -0400","from mail-lf0-f67.google.com ([209.85.215.67]:38588 \"EHLO\n\tmail-lf0-f67.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750932AbdIENdQ (ORCPT\n\t); Tue, 5 Sep 2017 09:33:16 -0400","by mail-lf0-f67.google.com with SMTP id m199so1745413lfe.5;\n\tTue, 05 Sep 2017 06:33:15 -0700 (PDT)","from [192.168.1.145] (ppp109-252-91-9.pppoe.spdop.ru.\n\t[109.252.91.9]) by smtp.googlemail.com with ESMTPSA id\n\tx26sm132072ljd.88.2017.09.05.06.33.13\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tTue, 05 Sep 2017 06:33:14 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=Ef77a6WZN0e7PvaTkwxi55+19LCMYsIzWr5DLQmUPg4=;\n\tb=WjunpBtlnM6jYVMzKIKc7E5odWOSS5PKlgAdvGA7maHvGud4pIpcCoCg6LY37eUSeU\n\tIvvKrRCp+zshYePouRB3HIfzj8GF+mww89SmuSuHXM5Vf7SWT7LbbjbIcOiXReMvIQmN\n\t7DmluxSPhiUMQKTiZu2++SOpzwag3rNsXwF9nei3UcjR6IAs88Tv4LHxK6XO+PbZDRhr\n\t2IrIdJdJmjtOCWUAGfXXhWvqx0cDEbINCIu6/NvYpSbO1FpWFPlBubr4QowiKi0eX5qm\n\tDcZO0OaTxChYHxFp/1w7RxCiTHG3hzIv/WnJZPZ7JNkiA77+yF6a/iBYINWlMNAu2I2w\n\tFt8Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=Ef77a6WZN0e7PvaTkwxi55+19LCMYsIzWr5DLQmUPg4=;\n\tb=E8lf//4XLUMe0+2QikHOX7KvUpAEZYawJtU/9HrYuKmYiVN3KY5aBv+8OAWmEleA6G\n\tybO0rd0oNSr5ZtYiz55pHzWS1fQoXSEsqmmtZuihMkV1r/xgZgt3WJJK+5OhIDUXaemA\n\tQkMcMzZwKLXBE26g6uMby4YyIlfkdKg49n377Aoe12ncVXr1vKMLrPtwPhIFlGL4GLma\n\tdbSvEyG4usP5qwFQVn4UKTV/hXwu5bGRmbURKlXdQZZ7kDilZPbiot7CwXosWxr8raqJ\n\tuB4QTlJHy4u5BRWyXbnCbSWzcNtR964932sbJMFNn5rcJWJVi/DQUb0AzzXkbAmSP/G/\n\tQHhg==","X-Gm-Message-State":"AHPjjUj0AfnrkGBnk6GyXczAlTniUWvboSDlTSNQIOUBBzFxFHCRcGhS\n\tJ67Azk4OECvyZOtNRzI=","X-Google-Smtp-Source":"ADKCNb5iquowVP5CtT8H1mHyL3xAARZP6ux7ZjKlKD/N/rWa5t6gdqfQ63/3tEJJtvrbXb2bdWUjUA==","X-Received":"by 10.46.5.148 with SMTP id 142mr1269471ljf.157.1504618394906;\n\tTue, 05 Sep 2017 06:33:14 -0700 (PDT)","Subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","To":"Mikko Perttunen , thierry.reding@gmail.com,\n\tjonathanh@nvidia.com","Cc":"dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"<20170905081029.19769-1-mperttunen@nvidia.com>\n\t<20170905081029.19769-2-mperttunen@nvidia.com>","From":"Dmitry Osipenko ","Message-ID":"<5cbadb84-31aa-ee3d-7b4b-e83afdfdcf7e@gmail.com>","Date":"Tue, 5 Sep 2017 16:33:13 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170905081029.19769-2-mperttunen@nvidia.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1773618,"web_url":"http://patchwork.ozlabs.org/comment/1773618/","msgid":"<179df5f9-86d5-b373-fda8-8523b4d13fe3@kapsi.fi>","list_archive_url":null,"date":"2017-09-22T14:02:28","subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","submitter":{"id":64745,"url":"http://patchwork.ozlabs.org/api/people/64745/","name":"Mikko Perttunen","email":"cyndis@kapsi.fi"},"content":"On 09/05/2017 04:33 PM, Dmitry Osipenko wrote:\n> On 05.09.2017 11:10, Mikko Perttunen wrote:\n>> ... >> diff --git a/drivers/gpu/host1x/hw/channel_hw.c \nb/drivers/gpu/host1x/hw/channel_hw.c\n>> index 8447a56c41ca..0161da331702 100644\n>> --- a/drivers/gpu/host1x/hw/channel_hw.c\n>> +++ b/drivers/gpu/host1x/hw/channel_hw.c\n>> @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job)\n>> \n>> \tsyncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs);\n>> \n>> +\t/* assign syncpoint to channel */\n>> +\thost1x_hw_syncpt_assign_channel(host, sp, ch);\n> \n> This function could be renamed to host1x_hw_assign_syncpt_to_channel() and then\n> comment to it won't be needed.\n\nMaybe host1x_hw_syncpt_assign_to_channel? I'd like to keep the current \nnoun_verb format. Though IMHO even the current name is pretty \ndescriptive in itself.\n\n> \n> It is not very nice that channel would be re-assigned on each submit. Maybe that\n> assignment should be done by host1x_syncpt_request() ?\n\nhost1x_syncpt_request doesn't know about the channel so we'd have to \nthread this information there and through each client driver in \ndrm/tegra/, so I decided not to do this at this time. I'm planning a new \nchannel allocation model which will change that side of the code anyway, \nso I'd like to revisit this at that point. For our current channel \nmodel, the current implementation doesn't have any functional downsides \nanyway.\n\n> \n>> +\n>> \tjob->syncpt_end = syncval;\n>> \n>> \t/* add a setclass for modules that require it */\n>> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c\n>> index 7b0270d60742..dc7a44614fef 100644\n>> --- a/drivers/gpu/host1x/hw/syncpt_hw.c\n>> +++ b/drivers/gpu/host1x/hw/syncpt_hw.c\n>> @@ -106,6 +106,50 @@ static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)\n>> \treturn 0;\n>> }\n>> \n>> +/**\n>> + * syncpt_assign_channel() - Assign syncpoint to channel\n>> + * @sp: syncpoint\n>> + * @ch: channel\n>> + *\n>> + * On chips with the syncpoint protection feature (Tegra186+), assign @sp to\n>> + * @ch, preventing other channels from incrementing the syncpoints. If @ch is\n>> + * NULL, unassigns the syncpoint.\n>> + *\n>> + * On older chips, do nothing.\n>> + */\n>> +static void syncpt_assign_channel(struct host1x_syncpt *sp,\n>> +\t\t\t\t struct host1x_channel *ch)\n>> +{\n>> +#if HOST1X_HW >= 6\n>> +\tstruct host1x *host = sp->host;\n>> +\n>> +\tif (!host->hv_regs)\n>> +\t\treturn;\n>> +\n>> +\thost1x_sync_writel(host,\n>> +\t\t\t HOST1X_SYNC_SYNCPT_CH_APP_CH(ch ? ch->id : 0xff),\n>> +\t\t\t HOST1X_SYNC_SYNCPT_CH_APP(sp->id));\n>> +#endif\n>> +}\n>> +\n>> +/**\n>> + * syncpt_enable_protection() - Enable syncpoint protection\n>> + * @host: host1x instance\n>> + *\n>> + * On chips with the syncpoint protection feature (Tegra186+), enable this\n>> + * feature. On older chips, do nothing.\n>> + */\n>> +static void syncpt_enable_protection(struct host1x *host)\n>> +{\n>> +#if HOST1X_HW >= 6\n>> +\tif (!host->hv_regs)\n>> +\t\treturn;\n>> +\n>> +\thost1x_hypervisor_writel(host, HOST1X_HV_SYNCPT_PROT_EN_CH_EN,\n>> +\t\t\t\t HOST1X_HV_SYNCPT_PROT_EN);\n>> +#endif\n>> +}\n>> +\n>> static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n>> \t.restore = syncpt_restore,\n>> \t.restore_wait_base = syncpt_restore_wait_base,\n>> @@ -113,4 +157,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n>> \t.load = syncpt_load,\n>> \t.cpu_incr = syncpt_cpu_incr,\n>> \t.patch_wait = syncpt_patch_wait,\n>> +\t.assign_channel = syncpt_assign_channel,\n>> +\t.enable_protection = syncpt_enable_protection,\n>> };\n>> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c\n>> index 048ac9e344ce..4c7a4c8b2ad2 100644\n>> --- a/drivers/gpu/host1x/syncpt.c\n>> +++ b/drivers/gpu/host1x/syncpt.c\n>> @@ -398,6 +398,13 @@ int host1x_syncpt_init(struct host1x *host)\n>> \tfor (i = 0; i < host->info->nb_pts; i++) {\n>> \t\tsyncpt[i].id = i;\n>> \t\tsyncpt[i].host = host;\n>> +\n>> +\t\t/*\n>> +\t\t * Unassign syncpt from channels for purposes of Tegra186\n>> +\t\t * syncpoint protection. This prevents any channel from\n>> +\t\t * accessing it until it is reassigned.\n>> +\t\t */\n>> +\t\thost1x_hw_syncpt_assign_channel(host, &syncpt[i], NULL);\n>> \t}\n>> \n>> \tfor (i = 0; i < host->info->nb_bases; i++)\n>> @@ -408,6 +415,7 @@ int host1x_syncpt_init(struct host1x *host)\n>> \thost->bases = bases;\n>> \n>> \thost1x_syncpt_restore(host);\n>> +\thost1x_hw_syncpt_enable_protection(host);\n>> \n>> \t/* Allocate sync point to use for clearing waits for expired fences */\n>> \thost->nop_sp = host1x_syncpt_alloc(host, NULL, 0);\n>>\n> \n> \n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-tegra\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tsecure) header.d=kapsi.fi header.i=@kapsi.fi header.b=\"ZiZTXA3N\";\n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzFY51zPyz9sRW\n\tfor ;\n\tSat, 23 Sep 2017 00:02:33 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752324AbdIVOCc (ORCPT );\n\tFri, 22 Sep 2017 10:02:32 -0400","from mail.kapsi.fi ([91.232.154.25]:48872 \"EHLO mail.kapsi.fi\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752231AbdIVOCb (ORCPT );\n\tFri, 22 Sep 2017 10:02:31 -0400","from dsl-hkibng22-54f983-249.dhcp.inet.fi ([84.249.131.249])\n\tby mail.kapsi.fi with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dvOXE-00063S-Li; Fri, 22 Sep 2017 17:02:28 +0300"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=kapsi.fi;\n\ts=20161220; \n\th=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:From:References:Cc:To:Subject;\n\tbh=tvuXsYvFx9cfyW8SmrV83PdR99FapxPEsrUBuxw6Cm4=; \n\tb=ZiZTXA3NNcO5oatf+clZoj4yg+y22IVcNdqM3agNJWULXsOD5irZz3FGCRgR1yRA30yY+ly7IUMcXAY1pr3734Ry0v8CypEgUFuQyEL317BNnl+S5TBaa0kdoTOaiSnR2FtSlYkNMMEzDAKneIZEJf+QKrfETBvThGZKwr/KGYw23FeBh9FcOJS1EkUC0z9mFbIBIojoSmrMvhqj5vYiwcTq+Yri1r1WxHWK8NRn7wf9Cwy6NyxOlAKW9hl8RCWfl4woYpHD5b3S+EYDDukQcWKrbrLuEKl6ULgeMDZe+kro/YtR5PgYWAKuV1R+bz1WcZsHKeZXWEDSw7gqizqycw==;","Subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","To":"Dmitry Osipenko ,\n\tMikko Perttunen ,\n\tthierry.reding@gmail.com, jonathanh@nvidia.com","Cc":"dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"<20170905081029.19769-1-mperttunen@nvidia.com>\n\t<20170905081029.19769-2-mperttunen@nvidia.com>\n\t<5cbadb84-31aa-ee3d-7b4b-e83afdfdcf7e@gmail.com>","From":"Mikko Perttunen ","Message-ID":"<179df5f9-86d5-b373-fda8-8523b4d13fe3@kapsi.fi>","Date":"Fri, 22 Sep 2017 17:02:28 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<5cbadb84-31aa-ee3d-7b4b-e83afdfdcf7e@gmail.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-SA-Exim-Connect-IP":"84.249.131.249","X-SA-Exim-Mail-From":"cyndis@kapsi.fi","X-SA-Exim-Scanned":"No (on mail.kapsi.fi); SAEximRunCond expanded to false","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}},{"id":1773932,"web_url":"http://patchwork.ozlabs.org/comment/1773932/","msgid":"<806c8cbc-151f-ee43-13f2-3a5438e93599@gmail.com>","list_archive_url":null,"date":"2017-09-22T23:17:55","subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","submitter":{"id":18124,"url":"http://patchwork.ozlabs.org/api/people/18124/","name":"Dmitry Osipenko","email":"digetx@gmail.com"},"content":"On 22.09.2017 17:02, Mikko Perttunen wrote:\n> On 09/05/2017 04:33 PM, Dmitry Osipenko wrote:\n>> On 05.09.2017 11:10, Mikko Perttunen wrote:\n>>> ... >> diff --git a/drivers/gpu/host1x/hw/channel_hw.c \n> b/drivers/gpu/host1x/hw/channel_hw.c\n>>> index 8447a56c41ca..0161da331702 100644\n>>> --- a/drivers/gpu/host1x/hw/channel_hw.c\n>>> +++ b/drivers/gpu/host1x/hw/channel_hw.c\n>>> @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job)\n>>>         syncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs);\n>>>   +    /* assign syncpoint to channel */\n>>> +    host1x_hw_syncpt_assign_channel(host, sp, ch);\n>>\n>> This function could be renamed to host1x_hw_assign_syncpt_to_channel() and then\n>> comment to it won't be needed.\n> \n> Maybe host1x_hw_syncpt_assign_to_channel? I'd like to keep the current noun_verb\n> format. Though IMHO even the current name is pretty descriptive in itself.\n> \n\nThat variant sounds good to me as well.\n\n>>\n>> It is not very nice that channel would be re-assigned on each submit. Maybe that\n>> assignment should be done by host1x_syncpt_request() ?\n> \n> host1x_syncpt_request doesn't know about the channel so we'd have to thread this\n> information there and through each client driver in drm/tegra/, so I decided not\n> to do this at this time. I'm planning a new channel allocation model which will\n> change that side of the code anyway, so I'd like to revisit this at that point.\n> For our current channel model, the current implementation doesn't have any\n> functional downsides anyway.\n> \n\nAnother very minor downside is that it causes an extra dummy invocation on\npre-Tegra186. Of course that could be changed later in a follow-up patch, not a\nbig deal :)\n\n>>\n>>> +\n>>>       job->syncpt_end = syncval;\n>>>         /* add a setclass for modules that require it */\n>>> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c\n>>> b/drivers/gpu/host1x/hw/syncpt_hw.c\n>>> index 7b0270d60742..dc7a44614fef 100644\n>>> --- a/drivers/gpu/host1x/hw/syncpt_hw.c\n>>> +++ b/drivers/gpu/host1x/hw/syncpt_hw.c\n>>> @@ -106,6 +106,50 @@ static int syncpt_patch_wait(struct host1x_syncpt *sp,\n>>> void *patch_addr)\n>>>       return 0;\n>>>   }\n>>>   +/**\n>>> + * syncpt_assign_channel() - Assign syncpoint to channel\n>>> + * @sp: syncpoint\n>>> + * @ch: channel\n>>> + *\n>>> + * On chips with the syncpoint protection feature (Tegra186+), assign @sp to\n>>> + * @ch, preventing other channels from incrementing the syncpoints. If @ch is\n>>> + * NULL, unassigns the syncpoint.\n>>> + *\n>>> + * On older chips, do nothing.\n>>> + */\n>>> +static void syncpt_assign_channel(struct host1x_syncpt *sp,\n>>> +                  struct host1x_channel *ch)\n>>> +{\n>>> +#if HOST1X_HW >= 6\n>>> +    struct host1x *host = sp->host;\n>>> +\n>>> +    if (!host->hv_regs)\n>>> +        return;\n>>> +\n>>> +    host1x_sync_writel(host,\n>>> +               HOST1X_SYNC_SYNCPT_CH_APP_CH(ch ? ch->id : 0xff),\n>>> +               HOST1X_SYNC_SYNCPT_CH_APP(sp->id));\n>>> +#endif\n>>> +}\n>>> +\n>>> +/**\n>>> + * syncpt_enable_protection() - Enable syncpoint protection\n>>> + * @host: host1x instance\n>>> + *\n>>> + * On chips with the syncpoint protection feature (Tegra186+), enable this\n>>> + * feature. On older chips, do nothing.\n>>> + */\n>>> +static void syncpt_enable_protection(struct host1x *host)\n>>> +{\n>>> +#if HOST1X_HW >= 6\n>>> +    if (!host->hv_regs)\n>>> +        return;\n>>> +\n>>> +    host1x_hypervisor_writel(host, HOST1X_HV_SYNCPT_PROT_EN_CH_EN,\n>>> +                 HOST1X_HV_SYNCPT_PROT_EN);\n>>> +#endif\n>>> +}\n>>> +\n>>>   static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n>>>       .restore = syncpt_restore,\n>>>       .restore_wait_base = syncpt_restore_wait_base,\n>>> @@ -113,4 +157,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = {\n>>>       .load = syncpt_load,\n>>>       .cpu_incr = syncpt_cpu_incr,\n>>>       .patch_wait = syncpt_patch_wait,\n>>> +    .assign_channel = syncpt_assign_channel,\n>>> +    .enable_protection = syncpt_enable_protection,\n>>>   };\n>>> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c\n>>> index 048ac9e344ce..4c7a4c8b2ad2 100644\n>>> --- a/drivers/gpu/host1x/syncpt.c\n>>> +++ b/drivers/gpu/host1x/syncpt.c\n>>> @@ -398,6 +398,13 @@ int host1x_syncpt_init(struct host1x *host)\n>>>       for (i = 0; i < host->info->nb_pts; i++) {\n>>>           syncpt[i].id = i;\n>>>           syncpt[i].host = host;\n>>> +\n>>> +        /*\n>>> +         * Unassign syncpt from channels for purposes of Tegra186\n>>> +         * syncpoint protection. This prevents any channel from\n>>> +         * accessing it until it is reassigned.\n>>> +         */\n>>> +        host1x_hw_syncpt_assign_channel(host, &syncpt[i], NULL);\n>>>       }\n>>>         for (i = 0; i < host->info->nb_bases; i++)\n>>> @@ -408,6 +415,7 @@ int host1x_syncpt_init(struct host1x *host)\n>>>       host->bases = bases;\n>>>         host1x_syncpt_restore(host);\n>>> +    host1x_hw_syncpt_enable_protection(host);\n>>>         /* Allocate sync point to use for clearing waits for expired fences */\n>>>       host->nop_sp = host1x_syncpt_alloc(host, NULL, 0);\n>>>\n>>\n>>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-tegra-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"GwqSTyLt\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzTt21FVmz9sRm\n\tfor ;\n\tSat, 23 Sep 2017 09:18:02 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751995AbdIVXSB (ORCPT );\n\tFri, 22 Sep 2017 19:18:01 -0400","from mail-lf0-f41.google.com ([209.85.215.41]:49712 \"EHLO\n\tmail-lf0-f41.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751845AbdIVXR7 (ORCPT\n\t);\n\tFri, 22 Sep 2017 19:17:59 -0400","by mail-lf0-f41.google.com with SMTP id r17so2415901lff.6;\n\tFri, 22 Sep 2017 16:17:58 -0700 (PDT)","from [192.168.1.145] (ppp109-252-90-109.pppoe.spdop.ru.\n\t[109.252.90.109]) by smtp.googlemail.com with ESMTPSA id\n\ty20sm109093lfk.2.2017.09.22.16.17.55\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 22 Sep 2017 16:17:56 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=98vkanieAfkfU7KuaxTiFXsXHe+BCXjBeS5i7Zzs16Y=;\n\tb=GwqSTyLtNq9pevWhB4tVVUXwSPcGkJUdgyf5r/NzsELckAfjh46Pzga73VDksMGio5\n\t4brf+Q/yv0yJ1UFfpZ4vrEBkb52WuGfbBzveI2zM9ES8XBEOGrODGffzH5d9pefaqq+i\n\t1JPJ4vUBRffy+qLoSriJwlCZ/jlnZQeBKB50FISPF4p7CT1bjJYv12eLd/I5HDsSqEp3\n\t/PvAIRhtyYuCdSheneuHTFMaUThFT5agrf11IESRFGAP4V8+SlpHY3w3vY1IX7G4CMqf\n\tn7LQncvqcmBEB13cyQNKRIZ+NlsoUHV/zXjD0IT8UMj8WPBdYlwBFK+D8JKUx30F2ipY\n\tXS/A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=98vkanieAfkfU7KuaxTiFXsXHe+BCXjBeS5i7Zzs16Y=;\n\tb=i4I9JsdU+feP6DB+8qeSLmb5adRSyUKyxA7lJw2LezeQMLPBHOM67oP9Y9LHJSIdCB\n\thKXxfUejm+ajk0MWFSVQyTmLJmA5/KeuPj716F+mzHt/Yja4GT5CvConjD69VY4Quai+\n\tp++6FnZXxgYmDskCj9j9vvK/lDiu0Pl+ZbTFMYrfJ1+0+/aaEpICOlzsY5rBaJmf6VK8\n\tJoCRdfeqo3guesKzsSqi0GNGLQbR25hccByQAHeNwhRnTRj1BW+bm2IxD53oCLEEnuwO\n\tIJnt8ibRB9mOL0g2vHhHnW+kLd8oqov/sxT22q2QR0gMZ2a4oPysqEgHUVxRigchtGCr\n\tvctw==","X-Gm-Message-State":"AHPjjUigh7bK9jM4Wbse4kOMW7OSK3lUm25Pg6a+sREHuXxWEZpEFqzY\n\tcIbxbNm41QU5RjKQGAsX4+/qeHZC","X-Google-Smtp-Source":"AOwi7QD/TKBAuVq4UMLnTr8LHfqqHCc0CYBGQ+s4FyqooaYrZyQMHKQ/6PrYsdJtf71HNXp0BjkwDA==","X-Received":"by 10.46.80.17 with SMTP id e17mr268909ljb.78.1506122277864;\n\tFri, 22 Sep 2017 16:17:57 -0700 (PDT)","Subject":"Re: [PATCH v2 1/6] gpu: host1x: Enable Tegra186 syncpoint protection","To":"Mikko Perttunen ,\n\tMikko Perttunen ,\n\tthierry.reding@gmail.com, jonathanh@nvidia.com","Cc":"dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org","References":"<20170905081029.19769-1-mperttunen@nvidia.com>\n\t<20170905081029.19769-2-mperttunen@nvidia.com>\n\t<5cbadb84-31aa-ee3d-7b4b-e83afdfdcf7e@gmail.com>\n\t<179df5f9-86d5-b373-fda8-8523b4d13fe3@kapsi.fi>","From":"Dmitry Osipenko ","Message-ID":"<806c8cbc-151f-ee43-13f2-3a5438e93599@gmail.com>","Date":"Sat, 23 Sep 2017 02:17:55 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<179df5f9-86d5-b373-fda8-8523b4d13fe3@kapsi.fi>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"linux-tegra-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-tegra@vger.kernel.org"}}]