| Message ID | 20170928125044.32516-2-mperttunen@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Miscellaneous improvements to Host1x and TegraDRM | expand |
On 28.09.2017 15:50, Mikko Perttunen wrote: > Since Tegra186 the Host1x hardware allows syncpoints to be assigned to > specific channels, preventing any other channels from incrementing > them. > > Enable this feature where available and assign syncpoints to channels > when submitting a job. Syncpoints are currently never unassigned from > channels since that would require extra work and is unnecessary with > the current channel allocation model. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Only one minor comment below. > drivers/gpu/host1x/dev.h | 15 +++++++++++++ > drivers/gpu/host1x/hw/channel_hw.c | 3 +++ > drivers/gpu/host1x/hw/syncpt_hw.c | 46 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/host1x/syncpt.c | 8 +++++++ > 4 files changed, 72 insertions(+) > > diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h > index def802c0a6bf..502769726480 100644 > --- a/drivers/gpu/host1x/dev.h > +++ b/drivers/gpu/host1x/dev.h > @@ -79,6 +79,9 @@ struct host1x_syncpt_ops { > u32 (*load)(struct host1x_syncpt *syncpt); > int (*cpu_incr)(struct host1x_syncpt *syncpt); > int (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr); > + void (*assign_to_channel)(struct host1x_syncpt *syncpt, > + struct host1x_channel *channel); > + void (*enable_protection)(struct host1x *host); > }; > > struct host1x_intr_ops { > @@ -186,6 +189,18 @@ static inline int host1x_hw_syncpt_patch_wait(struct host1x *host, > return host->syncpt_op->patch_wait(sp, patch_addr); > } > > +static inline void host1x_hw_syncpt_assign_to_channel( > + struct host1x *host, struct host1x_syncpt *sp, > + struct host1x_channel *ch) > +{ > + return host->syncpt_op->assign_to_channel(sp, ch); > +} > + > +static inline void host1x_hw_syncpt_enable_protection(struct host1x *host) > +{ > + return host->syncpt_op->enable_protection(host); > +} > + > static inline int host1x_hw_intr_init_host_sync(struct host1x *host, u32 cpm, > void (*syncpt_thresh_work)(struct work_struct *)) > { > diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c > index 8447a56c41ca..b929d7f1e291 100644 > --- a/drivers/gpu/host1x/hw/channel_hw.c > +++ b/drivers/gpu/host1x/hw/channel_hw.c > @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job) > > syncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs); > > + /* assign syncpoint to channel */ > + host1x_hw_syncpt_assign_to_channel(host, sp, ch); > + Since you've preserved the comment, what about to extend it with a brief explanation of what actually the 'assignment' does? Like that CDMA will stop execution on touching any syncpoint other then the assigned one. > job->syncpt_end = syncval; > > /* add a setclass for modules that require it */ > diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c > index 7b0270d60742..7dfd47d74f89 100644 > --- a/drivers/gpu/host1x/hw/syncpt_hw.c > +++ b/drivers/gpu/host1x/hw/syncpt_hw.c > @@ -106,6 +106,50 @@ static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr) > return 0; > } > > +/** > + * syncpt_assign_to_channel() - Assign syncpoint to channel > + * @sp: syncpoint > + * @ch: channel > + * > + * On chips with the syncpoint protection feature (Tegra186+), assign @sp to > + * @ch, preventing other channels from incrementing the syncpoints. If @ch is > + * NULL, unassigns the syncpoint. > + * > + * On older chips, do nothing. > + */ > +static void syncpt_assign_to_channel(struct host1x_syncpt *sp, > + struct host1x_channel *ch) > +{ > +#if HOST1X_HW >= 6 > + struct host1x *host = sp->host; > + > + if (!host->hv_regs) > + return; > + > + host1x_sync_writel(host, > + HOST1X_SYNC_SYNCPT_CH_APP_CH(ch ? ch->id : 0xff), > + HOST1X_SYNC_SYNCPT_CH_APP(sp->id)); > +#endif > +} > + > +/** > + * syncpt_enable_protection() - Enable syncpoint protection > + * @host: host1x instance > + * > + * On chips with the syncpoint protection feature (Tegra186+), enable this > + * feature. On older chips, do nothing. > + */ > +static void syncpt_enable_protection(struct host1x *host) > +{ > +#if HOST1X_HW >= 6 > + if (!host->hv_regs) > + return; > + > + host1x_hypervisor_writel(host, HOST1X_HV_SYNCPT_PROT_EN_CH_EN, > + HOST1X_HV_SYNCPT_PROT_EN); > +#endif > +} > + > static const struct host1x_syncpt_ops host1x_syncpt_ops = { > .restore = syncpt_restore, > .restore_wait_base = syncpt_restore_wait_base, > @@ -113,4 +157,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = { > .load = syncpt_load, > .cpu_incr = syncpt_cpu_incr, > .patch_wait = syncpt_patch_wait, > + .assign_to_channel = syncpt_assign_to_channel, > + .enable_protection = syncpt_enable_protection, > }; > diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c > index 048ac9e344ce..bce7cd6db724 100644 > --- a/drivers/gpu/host1x/syncpt.c > +++ b/drivers/gpu/host1x/syncpt.c > @@ -398,6 +398,13 @@ int host1x_syncpt_init(struct host1x *host) > for (i = 0; i < host->info->nb_pts; i++) { > syncpt[i].id = i; > syncpt[i].host = host; > + > + /* > + * Unassign syncpt from channels for purposes of Tegra186 > + * syncpoint protection. This prevents any channel from > + * accessing it until it is reassigned. > + */ > + host1x_hw_syncpt_assign_to_channel(host, &syncpt[i], NULL); > } > > for (i = 0; i < host->info->nb_bases; i++) > @@ -408,6 +415,7 @@ int host1x_syncpt_init(struct host1x *host) > host->bases = bases; > > host1x_syncpt_restore(host); > + host1x_hw_syncpt_enable_protection(host); > > /* Allocate sync point to use for clearing waits for expired fences */ > host->nop_sp = host1x_syncpt_alloc(host, NULL, 0); > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/30/2017 05:41 AM, Dmitry Osipenko wrote: > On 28.09.2017 15:50, Mikko Perttunen wrote: >> .. >> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c >> index 8447a56c41ca..b929d7f1e291 100644 >> --- a/drivers/gpu/host1x/hw/channel_hw.c >> +++ b/drivers/gpu/host1x/hw/channel_hw.c >> @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job) >> >> syncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs); >> >> + /* assign syncpoint to channel */ >> + host1x_hw_syncpt_assign_to_channel(host, sp, ch); >> + > > Since you've preserved the comment, what about to extend it with a brief > explanation of what actually the 'assignment' does? Like that CDMA will stop > execution on touching any syncpoint other then the assigned one. Whoops, I actually forgot to remove that :) I think the best would be to remove the comment here and have a more proper description of the feature somewhere else. Mikko -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 28, 2017 at 03:50:39PM +0300, Mikko Perttunen wrote: > Since Tegra186 the Host1x hardware allows syncpoints to be assigned to > specific channels, preventing any other channels from incrementing > them. > > Enable this feature where available and assign syncpoints to channels > when submitting a job. Syncpoints are currently never unassigned from > channels since that would require extra work and is unnecessary with > the current channel allocation model. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/gpu/host1x/dev.h | 15 +++++++++++++ > drivers/gpu/host1x/hw/channel_hw.c | 3 +++ > drivers/gpu/host1x/hw/syncpt_hw.c | 46 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/host1x/syncpt.c | 8 +++++++ > 4 files changed, 72 insertions(+) Applied all of these, thanks. Thierry
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h index def802c0a6bf..502769726480 100644 --- a/drivers/gpu/host1x/dev.h +++ b/drivers/gpu/host1x/dev.h @@ -79,6 +79,9 @@ struct host1x_syncpt_ops { u32 (*load)(struct host1x_syncpt *syncpt); int (*cpu_incr)(struct host1x_syncpt *syncpt); int (*patch_wait)(struct host1x_syncpt *syncpt, void *patch_addr); + void (*assign_to_channel)(struct host1x_syncpt *syncpt, + struct host1x_channel *channel); + void (*enable_protection)(struct host1x *host); }; struct host1x_intr_ops { @@ -186,6 +189,18 @@ static inline int host1x_hw_syncpt_patch_wait(struct host1x *host, return host->syncpt_op->patch_wait(sp, patch_addr); } +static inline void host1x_hw_syncpt_assign_to_channel( + struct host1x *host, struct host1x_syncpt *sp, + struct host1x_channel *ch) +{ + return host->syncpt_op->assign_to_channel(sp, ch); +} + +static inline void host1x_hw_syncpt_enable_protection(struct host1x *host) +{ + return host->syncpt_op->enable_protection(host); +} + static inline int host1x_hw_intr_init_host_sync(struct host1x *host, u32 cpm, void (*syncpt_thresh_work)(struct work_struct *)) { diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c index 8447a56c41ca..b929d7f1e291 100644 --- a/drivers/gpu/host1x/hw/channel_hw.c +++ b/drivers/gpu/host1x/hw/channel_hw.c @@ -147,6 +147,9 @@ static int channel_submit(struct host1x_job *job) syncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs); + /* assign syncpoint to channel */ + host1x_hw_syncpt_assign_to_channel(host, sp, ch); + job->syncpt_end = syncval; /* add a setclass for modules that require it */ diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c index 7b0270d60742..7dfd47d74f89 100644 --- a/drivers/gpu/host1x/hw/syncpt_hw.c +++ b/drivers/gpu/host1x/hw/syncpt_hw.c @@ -106,6 +106,50 @@ static int syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr) return 0; } +/** + * syncpt_assign_to_channel() - Assign syncpoint to channel + * @sp: syncpoint + * @ch: channel + * + * On chips with the syncpoint protection feature (Tegra186+), assign @sp to + * @ch, preventing other channels from incrementing the syncpoints. If @ch is + * NULL, unassigns the syncpoint. + * + * On older chips, do nothing. + */ +static void syncpt_assign_to_channel(struct host1x_syncpt *sp, + struct host1x_channel *ch) +{ +#if HOST1X_HW >= 6 + struct host1x *host = sp->host; + + if (!host->hv_regs) + return; + + host1x_sync_writel(host, + HOST1X_SYNC_SYNCPT_CH_APP_CH(ch ? ch->id : 0xff), + HOST1X_SYNC_SYNCPT_CH_APP(sp->id)); +#endif +} + +/** + * syncpt_enable_protection() - Enable syncpoint protection + * @host: host1x instance + * + * On chips with the syncpoint protection feature (Tegra186+), enable this + * feature. On older chips, do nothing. + */ +static void syncpt_enable_protection(struct host1x *host) +{ +#if HOST1X_HW >= 6 + if (!host->hv_regs) + return; + + host1x_hypervisor_writel(host, HOST1X_HV_SYNCPT_PROT_EN_CH_EN, + HOST1X_HV_SYNCPT_PROT_EN); +#endif +} + static const struct host1x_syncpt_ops host1x_syncpt_ops = { .restore = syncpt_restore, .restore_wait_base = syncpt_restore_wait_base, @@ -113,4 +157,6 @@ static const struct host1x_syncpt_ops host1x_syncpt_ops = { .load = syncpt_load, .cpu_incr = syncpt_cpu_incr, .patch_wait = syncpt_patch_wait, + .assign_to_channel = syncpt_assign_to_channel, + .enable_protection = syncpt_enable_protection, }; diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index 048ac9e344ce..bce7cd6db724 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -398,6 +398,13 @@ int host1x_syncpt_init(struct host1x *host) for (i = 0; i < host->info->nb_pts; i++) { syncpt[i].id = i; syncpt[i].host = host; + + /* + * Unassign syncpt from channels for purposes of Tegra186 + * syncpoint protection. This prevents any channel from + * accessing it until it is reassigned. + */ + host1x_hw_syncpt_assign_to_channel(host, &syncpt[i], NULL); } for (i = 0; i < host->info->nb_bases; i++) @@ -408,6 +415,7 @@ int host1x_syncpt_init(struct host1x *host) host->bases = bases; host1x_syncpt_restore(host); + host1x_hw_syncpt_enable_protection(host); /* Allocate sync point to use for clearing waits for expired fences */ host->nop_sp = host1x_syncpt_alloc(host, NULL, 0);
Since Tegra186 the Host1x hardware allows syncpoints to be assigned to specific channels, preventing any other channels from incrementing them. Enable this feature where available and assign syncpoints to channels when submitting a job. Syncpoints are currently never unassigned from channels since that would require extra work and is unnecessary with the current channel allocation model. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/gpu/host1x/dev.h | 15 +++++++++++++ drivers/gpu/host1x/hw/channel_hw.c | 3 +++ drivers/gpu/host1x/hw/syncpt_hw.c | 46 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/host1x/syncpt.c | 8 +++++++ 4 files changed, 72 insertions(+)