Quoting Michał Winiarski (2018-01-30 11:25:50)
> 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.
Hmm, means passing in a potential NULL ctx to guc_client_alloc(), which
then gets used in guc_stage_desc_init().
But I do need to perform more surgery to make preempt_client allowed to
be NULL.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
