On Tue, Jan 30, 2018 at 09:02:45AM +0000, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2018-01-30 00:24:04)
> > <snip>
> >
> > > @@ -979,17 +979,19 @@ static int guc_clients_create(struct intel_guc *guc)
> > > }
> > > guc->execbuf_client = client;
> > >
> > > - client = guc_client_alloc(dev_priv,
> > > - INTEL_INFO(dev_priv)->ring_mask,
> > > - GUC_CLIENT_PRIORITY_KMD_HIGH,
> > > - dev_priv->preempt_context);
> > > - if (IS_ERR(client)) {
> > > - DRM_ERROR("Failed to create GuC client for preemption!\n");
> > > - guc_client_free(guc->execbuf_client);
> > > - guc->execbuf_client = NULL;
> > > - return PTR_ERR(client);
> > > + if (dev_priv->preempt_context) {
> > > + client = guc_client_alloc(dev_priv,
> > > + INTEL_INFO(dev_priv)->ring_mask,
> > > + GUC_CLIENT_PRIORITY_KMD_HIGH,
> > > + dev_priv->preempt_context);
> > > + if (IS_ERR(client)) {
> > > + DRM_ERROR("Failed to create GuC client for
> > > preemption!\n");
> > > + guc_client_free(guc->execbuf_client);
> > > + guc->execbuf_client = NULL;
> > > + return PTR_ERR(client);
> > > + }
> > > + guc->preempt_client = client;
> > > }
> >
> > I was having another look at this patch while rebasing my patches on top
> > of it and I noticed that although guc->preempt_client is now not always
> > created other code still assumes that it is always there. All platforms
> > with GuC also have preemption, so i think the only case in which we can
> > have issues is when we fail to create the preempt context and keep going
> > with the driver loading assuming preemption disabled.
>
> At the time we landed guc preemption we intended for it to be defensive
> and allow us to disable it if need by throwing a single switch
> (has_logical_ring_preemption = false).
It controls whether 'inject_preempt_context' is called.
> When creating this patch, I thought that was still true; we could use
> the propagation of guc->preempt_client = NULL /
> dev_priv->preempt_context = NULL to simply not trigger preemption.
Using dev_priv->preempt_context = NULL should be fine. You could just drop
the changes to preempt_client allocation from this patch. We can add "do not
allocate guc preempt client if preemption is not supported" as a separate patch
later on if extra preempt client bothers anyone.
-Michał
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
