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

[Chris Wilson]

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