[Intel-gfx] [PATCH] drm/i915: Only allocate preempt context when required

[Michał Winiarski]

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