| Message ID | 1491384637-971-4-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote: > > On 05/04/2017 10:30, Oscar Mateo wrote: >> Not really needed, but makes the next change a little bit more compact. >> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++++-- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++- >> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +- >> 3 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >> b/drivers/gpu/drm/i915/intel_engine_cs.c >> index abc0a9c..530f822 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -71,7 +71,7 @@ >> .init_legacy = intel_init_bsd_ring_buffer, >> }, >> [VCS2] = { >> - .name = "vcs2", >> + .name = "vcs", > > Rename the field to class_name perhaps? > In the following patch, when I move .name to the class table, it becomes much more obvious what it refers to, but I can change it to class_name if you feel strongly about it (or change it here and then back to name in the next patch?). >> .hw_id = VCS2_HW, >> .exec_id = I915_EXEC_BSD, >> .class = VIDEO_DECODE_CLASS, >> @@ -100,6 +100,7 @@ >> { >> const struct engine_info *info = &intel_engines[id]; >> struct intel_engine_cs *engine; >> + char instance[3] = ""; >> >> GEM_BUG_ON(dev_priv->engine[id]); >> engine = kzalloc(sizeof(*engine), GFP_KERNEL); >> @@ -108,7 +109,10 @@ >> >> engine->id = id; >> engine->i915 = dev_priv; >> - engine->name = info->name; >> + /* For historical reasons the engines are called: name, name2... */ >> + if (info->instance) >> + snprintf(instance, sizeof(instance), "%u", info->instance + 1); >> + snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, >> instance); > > Since Chris has recently renamed all the engines, I'd say who cares > about the numbering scheme. Just drop it for simplicity. > Oooohh! Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1 ... xcsN?) >> engine->exec_id = info->exec_id; >> engine->hw_id = engine->guc_id = info->hw_id; >> engine->mmio_base = info->mmio_base; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >> b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 5c1a27f..d617049 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -187,9 +187,11 @@ enum intel_engine_id { >> VECS >> }; >> >> +#define INTEL_ENGINE_CS_MAX_NAME 8 >> + >> struct intel_engine_cs { >> struct drm_i915_private *i915; >> - const char *name; >> + char name[INTEL_ENGINE_CS_MAX_NAME]; >> enum intel_engine_id id; >> unsigned int exec_id; >> unsigned int hw_id; >> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c >> b/drivers/gpu/drm/i915/selftests/mock_engine.c >> index b89050e..4a1ffca 100644 >> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c >> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c >> @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct >> drm_i915_private *i915, >> >> /* minimal engine setup for requests */ >> engine->base.i915 = i915; >> - engine->base.name = name; >> + strncpy(engine->base.name, name, sizeof(engine->base.name) - 1); > > Can this miss to null-terminate? Or it relies on the mock_engine being > kzalloced? > It relies on the kzalloc, but I can add a \0 at the end for extra security? >> engine->base.id = id++; >> engine->base.status_page.page_addr = (void *)(engine + 1); >> >> > > Regards, > > Tvrtko
On 05/04/2017 10:30, Oscar Mateo wrote: > Not really needed, but makes the next change a little bit more compact. > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++++-- > drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++- > drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +- > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index abc0a9c..530f822 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -71,7 +71,7 @@ > .init_legacy = intel_init_bsd_ring_buffer, > }, > [VCS2] = { > - .name = "vcs2", > + .name = "vcs", Rename the field to class_name perhaps? > .hw_id = VCS2_HW, > .exec_id = I915_EXEC_BSD, > .class = VIDEO_DECODE_CLASS, > @@ -100,6 +100,7 @@ > { > const struct engine_info *info = &intel_engines[id]; > struct intel_engine_cs *engine; > + char instance[3] = ""; > > GEM_BUG_ON(dev_priv->engine[id]); > engine = kzalloc(sizeof(*engine), GFP_KERNEL); > @@ -108,7 +109,10 @@ > > engine->id = id; > engine->i915 = dev_priv; > - engine->name = info->name; > + /* For historical reasons the engines are called: name, name2... */ > + if (info->instance) > + snprintf(instance, sizeof(instance), "%u", info->instance + 1); > + snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance); Since Chris has recently renamed all the engines, I'd say who cares about the numbering scheme. Just drop it for simplicity. > engine->exec_id = info->exec_id; > engine->hw_id = engine->guc_id = info->hw_id; > engine->mmio_base = info->mmio_base; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 5c1a27f..d617049 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -187,9 +187,11 @@ enum intel_engine_id { > VECS > }; > > +#define INTEL_ENGINE_CS_MAX_NAME 8 > + > struct intel_engine_cs { > struct drm_i915_private *i915; > - const char *name; > + char name[INTEL_ENGINE_CS_MAX_NAME]; > enum intel_engine_id id; > unsigned int exec_id; > unsigned int hw_id; > diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c > index b89050e..4a1ffca 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_engine.c > +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c > @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915, > > /* minimal engine setup for requests */ > engine->base.i915 = i915; > - engine->base.name = name; > + strncpy(engine->base.name, name, sizeof(engine->base.name) - 1); Can this miss to null-terminate? Or it relies on the mock_engine being kzalloced? > engine->base.id = id++; > engine->base.status_page.page_addr = (void *)(engine + 1); > > Regards, Tvrtko
On Thu, Apr 06, 2017 at 07:02:10PM +0100, Tvrtko Ursulin wrote: > > On 05/04/2017 10:30, Oscar Mateo wrote: > >@@ -100,6 +100,7 @@ > > { > > const struct engine_info *info = &intel_engines[id]; > > struct intel_engine_cs *engine; > >+ char instance[3] = ""; > > > > GEM_BUG_ON(dev_priv->engine[id]); > > engine = kzalloc(sizeof(*engine), GFP_KERNEL); > >@@ -108,7 +109,10 @@ > > > > engine->id = id; > > engine->i915 = dev_priv; > >- engine->name = info->name; > >+ /* For historical reasons the engines are called: name, name2... */ > >+ if (info->instance) > >+ snprintf(instance, sizeof(instance), "%u", info->instance + 1); > >+ snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance); > > Since Chris has recently renamed all the engines, I'd say who cares > about the numbering scheme. Just drop it for simplicity. Seconded. class0 is ok, or if you are being fancy only use the classN format if there are multiples in the class. Though kiss says just use classN and be done with it. -Chris
On 06/04/2017 12:22, Oscar Mateo wrote: > On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote: >> On 05/04/2017 10:30, Oscar Mateo wrote: >>> Not really needed, but makes the next change a little bit more compact. >>> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++++-- >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++- >>> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +- >>> 3 files changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c >>> b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index abc0a9c..530f822 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -71,7 +71,7 @@ >>> .init_legacy = intel_init_bsd_ring_buffer, >>> }, >>> [VCS2] = { >>> - .name = "vcs2", >>> + .name = "vcs", >> >> Rename the field to class_name perhaps? >> > > In the following patch, when I move .name to the class table, it becomes > much more obvious what it refers to, but I can change it to class_name > if you feel strongly about it (or change it here and then back to name > in the next patch?). No it's fine like it is, just a consequence of not looking-ahead enough. > >>> .hw_id = VCS2_HW, >>> .exec_id = I915_EXEC_BSD, >>> .class = VIDEO_DECODE_CLASS, >>> @@ -100,6 +100,7 @@ >>> { >>> const struct engine_info *info = &intel_engines[id]; >>> struct intel_engine_cs *engine; >>> + char instance[3] = ""; >>> >>> GEM_BUG_ON(dev_priv->engine[id]); >>> engine = kzalloc(sizeof(*engine), GFP_KERNEL); >>> @@ -108,7 +109,10 @@ >>> >>> engine->id = id; >>> engine->i915 = dev_priv; >>> - engine->name = info->name; >>> + /* For historical reasons the engines are called: name, name2... */ >>> + if (info->instance) >>> + snprintf(instance, sizeof(instance), "%u", info->instance + 1); >>> + snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, >>> instance); >> >> Since Chris has recently renamed all the engines, I'd say who cares >> about the numbering scheme. Just drop it for simplicity. >> > > Oooohh! > Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1 > ... xcsN?) Sounds like the verdict is yes. > >>> engine->exec_id = info->exec_id; >>> engine->hw_id = engine->guc_id = info->hw_id; >>> engine->mmio_base = info->mmio_base; >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> index 5c1a27f..d617049 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >>> @@ -187,9 +187,11 @@ enum intel_engine_id { >>> VECS >>> }; >>> >>> +#define INTEL_ENGINE_CS_MAX_NAME 8 >>> + >>> struct intel_engine_cs { >>> struct drm_i915_private *i915; >>> - const char *name; >>> + char name[INTEL_ENGINE_CS_MAX_NAME]; >>> enum intel_engine_id id; >>> unsigned int exec_id; >>> unsigned int hw_id; >>> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c >>> b/drivers/gpu/drm/i915/selftests/mock_engine.c >>> index b89050e..4a1ffca 100644 >>> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c >>> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c >>> @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct >>> drm_i915_private *i915, >>> >>> /* minimal engine setup for requests */ >>> engine->base.i915 = i915; >>> - engine->base.name = name; >>> + strncpy(engine->base.name, name, sizeof(engine->base.name) - 1); >> >> Can this miss to null-terminate? Or it relies on the mock_engine being >> kzalloced? >> > > It relies on the kzalloc, but I can add a \0 at the end for extra security? Not sure at the moment without checking in detail how much mock_engine already depends on being zeroed. But I guess null-terminating it at the end wouldn't harm. Or maybe snprintf to eliminate the dilemma if it creates neater code? Regards, Tvrtko
On Thu, Apr 06, 2017 at 07:37:20PM +0100, Tvrtko Ursulin wrote: > > On 06/04/2017 12:22, Oscar Mateo wrote: > >On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote: > >>On 05/04/2017 10:30, Oscar Mateo wrote: > >>>Not really needed, but makes the next change a little bit more compact. > >>> > >>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >>>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > >>>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >>>Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > >>>--- > >>> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++++-- > >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++- > >>> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +- > >>> 3 files changed, 10 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c > >>>b/drivers/gpu/drm/i915/intel_engine_cs.c > >>>index abc0a9c..530f822 100644 > >>>--- a/drivers/gpu/drm/i915/intel_engine_cs.c > >>>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >>>@@ -71,7 +71,7 @@ > >>> .init_legacy = intel_init_bsd_ring_buffer, > >>> }, > >>> [VCS2] = { > >>>- .name = "vcs2", > >>>+ .name = "vcs", > >> > >>Rename the field to class_name perhaps? > >> > > > >In the following patch, when I move .name to the class table, it becomes > >much more obvious what it refers to, but I can change it to class_name > >if you feel strongly about it (or change it here and then back to name > >in the next patch?). > > No it's fine like it is, just a consequence of not looking-ahead enough. > > > > >>> .hw_id = VCS2_HW, > >>> .exec_id = I915_EXEC_BSD, > >>> .class = VIDEO_DECODE_CLASS, > >>>@@ -100,6 +100,7 @@ > >>> { > >>> const struct engine_info *info = &intel_engines[id]; > >>> struct intel_engine_cs *engine; > >>>+ char instance[3] = ""; > >>> > >>> GEM_BUG_ON(dev_priv->engine[id]); > >>> engine = kzalloc(sizeof(*engine), GFP_KERNEL); > >>>@@ -108,7 +109,10 @@ > >>> > >>> engine->id = id; > >>> engine->i915 = dev_priv; > >>>- engine->name = info->name; > >>>+ /* For historical reasons the engines are called: name, name2... */ > >>>+ if (info->instance) > >>>+ snprintf(instance, sizeof(instance), "%u", info->instance + 1); > >>>+ snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, > >>>instance); > >> > >>Since Chris has recently renamed all the engines, I'd say who cares > >>about the numbering scheme. Just drop it for simplicity. > >> > > > >Oooohh! > >Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1 > >... xcsN?) > > Sounds like the verdict is yes. > > > > >>> engine->exec_id = info->exec_id; > >>> engine->hw_id = engine->guc_id = info->hw_id; > >>> engine->mmio_base = info->mmio_base; > >>>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>index 5c1a27f..d617049 100644 > >>>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > >>>@@ -187,9 +187,11 @@ enum intel_engine_id { > >>> VECS > >>> }; > >>> > >>>+#define INTEL_ENGINE_CS_MAX_NAME 8 > >>>+ > >>> struct intel_engine_cs { > >>> struct drm_i915_private *i915; > >>>- const char *name; > >>>+ char name[INTEL_ENGINE_CS_MAX_NAME]; > >>> enum intel_engine_id id; > >>> unsigned int exec_id; > >>> unsigned int hw_id; > >>>diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c > >>>b/drivers/gpu/drm/i915/selftests/mock_engine.c > >>>index b89050e..4a1ffca 100644 > >>>--- a/drivers/gpu/drm/i915/selftests/mock_engine.c > >>>+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c > >>>@@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct > >>>drm_i915_private *i915, > >>> > >>> /* minimal engine setup for requests */ > >>> engine->base.i915 = i915; > >>>- engine->base.name = name; > >>>+ strncpy(engine->base.name, name, sizeof(engine->base.name) - 1); > >> > >>Can this miss to null-terminate? Or it relies on the mock_engine being > >>kzalloced? > >> > > > >It relies on the kzalloc, but I can add a \0 at the end for extra security? > > Not sure at the moment without checking in detail how much > mock_engine already depends on being zeroed. It does depend on the kzalloc. > But I guess > null-terminating it at the end wouldn't harm. Or maybe snprintf to > eliminate the dilemma if it creates neater code? Indeed, I want my mockN! We may want multiple instances of the mock engine class in the future. -Chris
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index abc0a9c..530f822 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -71,7 +71,7 @@ .init_legacy = intel_init_bsd_ring_buffer, }, [VCS2] = { - .name = "vcs2", + .name = "vcs", .hw_id = VCS2_HW, .exec_id = I915_EXEC_BSD, .class = VIDEO_DECODE_CLASS, @@ -100,6 +100,7 @@ { const struct engine_info *info = &intel_engines[id]; struct intel_engine_cs *engine; + char instance[3] = ""; GEM_BUG_ON(dev_priv->engine[id]); engine = kzalloc(sizeof(*engine), GFP_KERNEL); @@ -108,7 +109,10 @@ engine->id = id; engine->i915 = dev_priv; - engine->name = info->name; + /* For historical reasons the engines are called: name, name2... */ + if (info->instance) + snprintf(instance, sizeof(instance), "%u", info->instance + 1); + snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance); engine->exec_id = info->exec_id; engine->hw_id = engine->guc_id = info->hw_id; engine->mmio_base = info->mmio_base; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 5c1a27f..d617049 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -187,9 +187,11 @@ enum intel_engine_id { VECS }; +#define INTEL_ENGINE_CS_MAX_NAME 8 + struct intel_engine_cs { struct drm_i915_private *i915; - const char *name; + char name[INTEL_ENGINE_CS_MAX_NAME]; enum intel_engine_id id; unsigned int exec_id; unsigned int hw_id; diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c index b89050e..4a1ffca 100644 --- a/drivers/gpu/drm/i915/selftests/mock_engine.c +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915, /* minimal engine setup for requests */ engine->base.i915 = i915; - engine->base.name = name; + strncpy(engine->base.name, name, sizeof(engine->base.name) - 1); engine->base.id = id++; engine->base.status_page.page_addr = (void *)(engine + 1);
Not really needed, but makes the next change a little bit more compact. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 4 +++- drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-)