| Message ID | 1465579766-31595-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 10, 2016 at 06:29:25PM +0100, Dave Gordon wrote: > -static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev, > - u32 size) > +static struct drm_i915_gem_object * > +gem_allocate_guc_obj(struct drm_i915_private *dev_priv, u32 size) Whilst you are looking at these, this is not GEM allocating an object for the guc, but guc allocating a GEM object for itself. -Chris
On 10/06/16 18:29, Dave Gordon wrote: > Convert all static functions in i915_guc_submission.c that currently > take a 'dev' pointer to take 'dev_priv' instead (there are three, > guc_client_alloc(), guc_client_free(), and gem_allocate_guc_obj(). > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 39 +++++++++++++++--------------- > 1 file changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 2db1182..1bd0fac 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -591,7 +591,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq) > > /** > * gem_allocate_guc_obj() - Allocate gem object for GuC usage > - * @dev: drm device > + * @dev_priv: driver private data structure > * @size: size of object > * > * This is a wrapper to create a gem obj. In order to use it inside GuC, the > @@ -600,13 +600,12 @@ int i915_guc_submit(struct drm_i915_gem_request *rq) > * > * Return: A drm_i915_gem_object if successful, otherwise NULL. > */ > -static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev, > - u32 size) > +static struct drm_i915_gem_object * > +gem_allocate_guc_obj(struct drm_i915_private *dev_priv, u32 size) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_object *obj; > > - obj = i915_gem_object_create(dev, size); > + obj = i915_gem_object_create(dev_priv->dev, size); > if (IS_ERR(obj)) > return NULL; > > @@ -642,10 +641,10 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) > drm_gem_object_unreference(&obj->base); > } > > -static void guc_client_free(struct drm_device *dev, > - struct i915_guc_client *client) > +static void > +guc_client_free(struct drm_i915_private *dev_priv, > + struct i915_guc_client *client) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc *guc = &dev_priv->guc; > > if (!client) > @@ -688,7 +687,7 @@ static void guc_client_free(struct drm_device *dev, > > /** > * guc_client_alloc() - Allocate an i915_guc_client > - * @dev: drm device > + * @dev_priv: driver private data structure > * @priority: four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW > * The kernel client to replace ExecList submission is created with > * NORMAL priority. Priority of a client for scheduler can be HIGH, > @@ -698,12 +697,12 @@ static void guc_client_free(struct drm_device *dev, > * > * Return: An i915_guc_client object if success, else NULL. > */ > -static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > - uint32_t priority, > - struct i915_gem_context *ctx) > +static struct i915_guc_client * > +guc_client_alloc(struct drm_i915_private *dev_priv, > + uint32_t priority, > + struct i915_gem_context *ctx) > { > struct i915_guc_client *client; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc *guc = &dev_priv->guc; > struct drm_i915_gem_object *obj; > > @@ -724,7 +723,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > } > > /* The first page is doorbell/proc_desc. Two followed pages are wq. */ > - obj = gem_allocate_guc_obj(dev, GUC_DB_SIZE + GUC_WQ_SIZE); > + obj = gem_allocate_guc_obj(dev_priv, GUC_DB_SIZE + GUC_WQ_SIZE); > if (!obj) > goto err; > > @@ -768,7 +767,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, > err: > DRM_ERROR("FAILED to create priority %u GuC client!\n", priority); > > - guc_client_free(dev, client); > + guc_client_free(dev_priv, client); > return NULL; > } > > @@ -793,7 +792,7 @@ static void guc_create_log(struct intel_guc *guc) > > obj = guc->log_obj; > if (!obj) { > - obj = gem_allocate_guc_obj(dev_priv->dev, size); > + obj = gem_allocate_guc_obj(dev_priv, size); > if (!obj) { > /* logging will be off */ > i915.guc_log_level = -1; > @@ -853,7 +852,7 @@ static void guc_create_ads(struct intel_guc *guc) > > obj = guc->ads_obj; > if (!obj) { > - obj = gem_allocate_guc_obj(dev_priv->dev, PAGE_ALIGN(size)); > + obj = gem_allocate_guc_obj(dev_priv, PAGE_ALIGN(size)); > if (!obj) > return; > > @@ -925,7 +924,7 @@ int i915_guc_submission_init(struct drm_device *dev) > if (guc->ctx_pool_obj) > return 0; /* already allocated */ > > - guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv->dev, gemsize); > + guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv, gemsize); > if (!guc->ctx_pool_obj) > return -ENOMEM; > > @@ -943,7 +942,7 @@ int i915_guc_submission_enable(struct drm_device *dev) > struct i915_guc_client *client; > > /* client for execbuf submission */ > - client = guc_client_alloc(dev, > + client = guc_client_alloc(dev_priv, > GUC_CTX_PRIORITY_KMD_NORMAL, > dev_priv->kernel_context); > if (!client) { > @@ -963,7 +962,7 @@ void i915_guc_submission_disable(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_guc *guc = &dev_priv->guc; > > - guc_client_free(dev, guc->execbuf_client); > + guc_client_free(dev_priv, guc->execbuf_client); > guc->execbuf_client = NULL; > } > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> And you can keep the r-b if you rename gem_allocate_guc_obj as Chris has suggested. Regards, Tvrtko
On 11/06/16 06:50, Patchwork wrote: > == Series Details == > > Series: series starting with [1/2] drm/i915/guc: prefer 'dev_priv' to 'dev' for static functions > URL : https://patchwork.freedesktop.org/series/8556/ > State : warning > > == Summary == > > Series 8556v1 Series without cover letter > http://patchwork.freedesktop.org/api/1.0/series/8556/revisions/1/mbox > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-cmd: > fail -> PASS (ro-byt-n2820) > Test kms_pipe_crc_basic: > Subgroup hang-read-crc-pipe-a: > dmesg-warn -> PASS (ro-hsw-i3-4010u) > Subgroup nonblocking-crc-pipe-b-frame-sequence: > skip -> PASS (fi-skl-i5-6260u) > Subgroup suspend-read-crc-pipe-a: > skip -> DMESG-WARN (ro-bdw-i5-5250u) > Subgroup suspend-read-crc-pipe-b: > skip -> DMESG-WARN (ro-bdw-i5-5250u) These are both an existing issue, logged as https://bugs.freedesktop.org/show_bug.cgi?id=96448 > Subgroup suspend-read-crc-pipe-c: > skip -> PASS (fi-skl-i5-6260u) > > fi-bdw-i7-5557u total:213 pass:201 dwarn:0 dfail:0 fail:0 skip:12 > fi-skl-i5-6260u total:213 pass:202 dwarn:0 dfail:0 fail:0 skip:11 > fi-skl-i7-6700k total:213 pass:188 dwarn:0 dfail:0 fail:0 skip:25 > fi-snb-i7-2600 total:213 pass:174 dwarn:0 dfail:0 fail:0 skip:39 > ro-bdw-i5-5250u total:213 pass:197 dwarn:4 dfail:0 fail:0 skip:12 > ro-bdw-i7-5600u total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28 > ro-bsw-n3050 total:213 pass:172 dwarn:0 dfail:0 fail:2 skip:39 > ro-byt-n2820 total:213 pass:174 dwarn:0 dfail:0 fail:2 skip:37 > ro-hsw-i3-4010u total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23 > ro-hsw-i7-4770r total:213 pass:190 dwarn:0 dfail:0 fail:0 skip:23 > ro-ilk-i7-620lm total:177 pass:120 dwarn:2 dfail:2 fail:1 skip:51 > ro-ilk1-i5-650 total:208 pass:150 dwarn:0 dfail:0 fail:1 skip:57 > ro-ivb-i7-3770 total:213 pass:181 dwarn:0 dfail:0 fail:0 skip:32 > ro-ivb2-i7-3770 total:213 pass:185 dwarn:0 dfail:0 fail:0 skip:28 > ro-skl3-i5-6260u total:213 pass:201 dwarn:1 dfail:0 fail:0 skip:11 > ro-snb-i7-2620M total:213 pass:174 dwarn:0 dfail:0 fail:1 skip:38 > fi-hsw-i7-4770k failed to connect after reboot > ro-bdw-i7-5557U failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1162/ > > 94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration manifest > 4c20b95 drm/i915/guc: prefer 'dev_priv' to 'dev' for intra-module functions > b0b12a6 drm/i915/guc: prefer 'dev_priv' to 'dev' for static functions >
On 13/06/16 16:42, Dave Gordon wrote: > On 11/06/16 06:50, Patchwork wrote: >> == Series Details == >> >> Series: series starting with [1/2] drm/i915/guc: prefer 'dev_priv' to >> 'dev' for static functions >> URL : https://patchwork.freedesktop.org/series/8556/ >> State : warning >> >> == Summary == >> >> Series 8556v1 Series without cover letter >> http://patchwork.freedesktop.org/api/1.0/series/8556/revisions/1/mbox >> >> Test gem_exec_flush: >> Subgroup basic-batch-kernel-default-cmd: >> fail -> PASS (ro-byt-n2820) >> Test kms_pipe_crc_basic: >> Subgroup hang-read-crc-pipe-a: >> dmesg-warn -> PASS (ro-hsw-i3-4010u) >> Subgroup nonblocking-crc-pipe-b-frame-sequence: >> skip -> PASS (fi-skl-i5-6260u) >> Subgroup suspend-read-crc-pipe-a: >> skip -> DMESG-WARN (ro-bdw-i5-5250u) >> Subgroup suspend-read-crc-pipe-b: >> skip -> DMESG-WARN (ro-bdw-i5-5250u) > > These are both an existing issue, logged as > https://bugs.freedesktop.org/show_bug.cgi?id=96448 > >> Subgroup suspend-read-crc-pipe-c: >> skip -> PASS (fi-skl-i5-6260u) >> >> fi-bdw-i7-5557u total:213 pass:201 dwarn:0 dfail:0 fail:0 >> skip:12 >> fi-skl-i5-6260u total:213 pass:202 dwarn:0 dfail:0 fail:0 >> skip:11 >> fi-skl-i7-6700k total:213 pass:188 dwarn:0 dfail:0 fail:0 >> skip:25 >> fi-snb-i7-2600 total:213 pass:174 dwarn:0 dfail:0 fail:0 >> skip:39 >> ro-bdw-i5-5250u total:213 pass:197 dwarn:4 dfail:0 fail:0 >> skip:12 >> ro-bdw-i7-5600u total:213 pass:185 dwarn:0 dfail:0 fail:0 >> skip:28 >> ro-bsw-n3050 total:213 pass:172 dwarn:0 dfail:0 fail:2 >> skip:39 >> ro-byt-n2820 total:213 pass:174 dwarn:0 dfail:0 fail:2 >> skip:37 >> ro-hsw-i3-4010u total:213 pass:190 dwarn:0 dfail:0 fail:0 >> skip:23 >> ro-hsw-i7-4770r total:213 pass:190 dwarn:0 dfail:0 fail:0 >> skip:23 >> ro-ilk-i7-620lm total:177 pass:120 dwarn:2 dfail:2 fail:1 >> skip:51 >> ro-ilk1-i5-650 total:208 pass:150 dwarn:0 dfail:0 fail:1 >> skip:57 >> ro-ivb-i7-3770 total:213 pass:181 dwarn:0 dfail:0 fail:0 >> skip:32 >> ro-ivb2-i7-3770 total:213 pass:185 dwarn:0 dfail:0 fail:0 >> skip:28 >> ro-skl3-i5-6260u total:213 pass:201 dwarn:1 dfail:0 fail:0 >> skip:11 >> ro-snb-i7-2620M total:213 pass:174 dwarn:0 dfail:0 fail:1 >> skip:38 >> fi-hsw-i7-4770k failed to connect after reboot >> ro-bdw-i7-5557U failed to connect after reboot >> >> Results at /archive/results/CI_IGT_test/RO_Patchwork_1162/ >> >> 94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration >> manifest >> 4c20b95 drm/i915/guc: prefer 'dev_priv' to 'dev' for intra-module >> functions >> b0b12a6 drm/i915/guc: prefer 'dev_priv' to 'dev' for static functions Merged, thanks for the patches and review! :) Regards, Tvrtko
On 13/06/16 10:13, Tvrtko Ursulin wrote: > > On 10/06/16 18:29, Dave Gordon wrote: >> Convert all static functions in i915_guc_submission.c that currently >> take a 'dev' pointer to take 'dev_priv' instead (there are three, >> guc_client_alloc(), guc_client_free(), and gem_allocate_guc_obj(). >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 39 >> +++++++++++++++--------------- >> 1 file changed, 19 insertions(+), 20 deletions(-) [snip] > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > And you can keep the r-b if you rename gem_allocate_guc_obj as Chris has > suggested. > > Regards, > Tvrtko I was thinking more of doing a bulk-renaming patch later to make all the GuC functions use a more consistent naming scheme, probably along the lines of "<prefix><topic>_<object>_<operation>", where <prefix> is empty for local functions or intel_/i915_ for externally-visible ones, <topic> is probably "guc" for all the functions in the loader and submission code, <object> is the class we're manipulating (if it's not the GuC itself) and <operation> is what we're doing to it. Fairly standard object-based RPN naming, in fact. Many functions are already named this way, but there's still some legacy of Alex's original style which is more topic-verb-object. So gem_allocate_guc_obj() would become guc_gem_object_alloc(). But I probably won't do it until I've finished all the other GuC enhancements ;) .Dave.
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 2db1182..1bd0fac 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -591,7 +591,7 @@ int i915_guc_submit(struct drm_i915_gem_request *rq) /** * gem_allocate_guc_obj() - Allocate gem object for GuC usage - * @dev: drm device + * @dev_priv: driver private data structure * @size: size of object * * This is a wrapper to create a gem obj. In order to use it inside GuC, the @@ -600,13 +600,12 @@ int i915_guc_submit(struct drm_i915_gem_request *rq) * * Return: A drm_i915_gem_object if successful, otherwise NULL. */ -static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev, - u32 size) +static struct drm_i915_gem_object * +gem_allocate_guc_obj(struct drm_i915_private *dev_priv, u32 size) { - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_object *obj; - obj = i915_gem_object_create(dev, size); + obj = i915_gem_object_create(dev_priv->dev, size); if (IS_ERR(obj)) return NULL; @@ -642,10 +641,10 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj) drm_gem_object_unreference(&obj->base); } -static void guc_client_free(struct drm_device *dev, - struct i915_guc_client *client) +static void +guc_client_free(struct drm_i915_private *dev_priv, + struct i915_guc_client *client) { - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc *guc = &dev_priv->guc; if (!client) @@ -688,7 +687,7 @@ static void guc_client_free(struct drm_device *dev, /** * guc_client_alloc() - Allocate an i915_guc_client - * @dev: drm device + * @dev_priv: driver private data structure * @priority: four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW * The kernel client to replace ExecList submission is created with * NORMAL priority. Priority of a client for scheduler can be HIGH, @@ -698,12 +697,12 @@ static void guc_client_free(struct drm_device *dev, * * Return: An i915_guc_client object if success, else NULL. */ -static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, - uint32_t priority, - struct i915_gem_context *ctx) +static struct i915_guc_client * +guc_client_alloc(struct drm_i915_private *dev_priv, + uint32_t priority, + struct i915_gem_context *ctx) { struct i915_guc_client *client; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc *guc = &dev_priv->guc; struct drm_i915_gem_object *obj; @@ -724,7 +723,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, } /* The first page is doorbell/proc_desc. Two followed pages are wq. */ - obj = gem_allocate_guc_obj(dev, GUC_DB_SIZE + GUC_WQ_SIZE); + obj = gem_allocate_guc_obj(dev_priv, GUC_DB_SIZE + GUC_WQ_SIZE); if (!obj) goto err; @@ -768,7 +767,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev, err: DRM_ERROR("FAILED to create priority %u GuC client!\n", priority); - guc_client_free(dev, client); + guc_client_free(dev_priv, client); return NULL; } @@ -793,7 +792,7 @@ static void guc_create_log(struct intel_guc *guc) obj = guc->log_obj; if (!obj) { - obj = gem_allocate_guc_obj(dev_priv->dev, size); + obj = gem_allocate_guc_obj(dev_priv, size); if (!obj) { /* logging will be off */ i915.guc_log_level = -1; @@ -853,7 +852,7 @@ static void guc_create_ads(struct intel_guc *guc) obj = guc->ads_obj; if (!obj) { - obj = gem_allocate_guc_obj(dev_priv->dev, PAGE_ALIGN(size)); + obj = gem_allocate_guc_obj(dev_priv, PAGE_ALIGN(size)); if (!obj) return; @@ -925,7 +924,7 @@ int i915_guc_submission_init(struct drm_device *dev) if (guc->ctx_pool_obj) return 0; /* already allocated */ - guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv->dev, gemsize); + guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv, gemsize); if (!guc->ctx_pool_obj) return -ENOMEM; @@ -943,7 +942,7 @@ int i915_guc_submission_enable(struct drm_device *dev) struct i915_guc_client *client; /* client for execbuf submission */ - client = guc_client_alloc(dev, + client = guc_client_alloc(dev_priv, GUC_CTX_PRIORITY_KMD_NORMAL, dev_priv->kernel_context); if (!client) { @@ -963,7 +962,7 @@ void i915_guc_submission_disable(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_guc *guc = &dev_priv->guc; - guc_client_free(dev, guc->execbuf_client); + guc_client_free(dev_priv, guc->execbuf_client); guc->execbuf_client = NULL; }
Convert all static functions in i915_guc_submission.c that currently take a 'dev' pointer to take 'dev_priv' instead (there are three, guc_client_alloc(), guc_client_free(), and gem_allocate_guc_obj(). Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 39 +++++++++++++++--------------- 1 file changed, 19 insertions(+), 20 deletions(-)