[Intel-gfx] [PATCH 4/6] drm/i915: Use to_i915() instead of guc_to_i915()
Dave Gordon
david.s.gordon at intel.com
Tue Mar 22 10:55:40 UTC 2016
On 18/03/16 21:16, Chris Wilson wrote:
> The convenience of saving a few characters by using a consistent
> interface to obtain our drm_i915_private struct from intel_guc.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 +++-
> drivers/gpu/drm/i915/i915_guc_submission.c | 23 ++++++++++-------------
> 2 files changed, 13 insertions(+), 14 deletions(-)
Generally: I don't mind this, though I don't think there's any huge
advantage ...
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 92365f047e53..d5fa42c96110 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1982,7 +1982,7 @@ static inline struct drm_i915_private *dev_to_i915(struct device *dev)
> return __to_i915(dev_get_drvdata(dev));
> }
>
> -static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
> +static inline struct drm_i915_private *__guc_to_i915(struct intel_guc *guc)
> {
> return container_of(guc, struct drm_i915_private, guc);
> }
> @@ -2463,6 +2463,8 @@ struct drm_i915_cmd_table {
> __p = __to_i915((struct drm_device *)p); \
> else if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_gem_object)) \
> __p = __obj_to_i915((struct drm_i915_gem_object *)p); \
> + else if (__builtin_types_compatible_p(typeof(*p), struct intel_guc)) \
> + __p = __guc_to_i915((struct intel_guc *)p); \
... so yes, this is OK ...
> else \
> BUILD_BUG(); \
> __p; \
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index ae1f58d073f2..850aee78c40f 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -77,7 +77,7 @@ static inline bool host2guc_action_response(struct drm_i915_private *dev_priv,
>
> static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + struct drm_i915_private *dev_priv = to_i915(guc);
... and these ...
> u32 status;
> int i;
> int ret;
> @@ -152,7 +152,7 @@ static int host2guc_release_doorbell(struct intel_guc *guc,
> static int host2guc_sample_forcewake(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + struct drm_i915_private *dev_priv = to_i915(guc);
> struct drm_device *dev = dev_priv->dev;
> u32 data[2];
>
> @@ -254,7 +254,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
> static void guc_disable_doorbell(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + struct drm_i915_private *dev_priv = to_i915(guc);
> struct guc_doorbell_info *doorbell;
> void *base;
> i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
> @@ -376,7 +376,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
> static void guc_init_ctx_desc(struct intel_guc *guc,
> struct i915_guc_client *client)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_engine_cs *engine;
> struct intel_context *ctx = client->owner;
> struct guc_context_desc desc;
> @@ -390,7 +389,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> desc.priority = client->priority;
> desc.db_id = client->doorbell_id;
>
> - for_each_engine(engine, dev_priv, i) {
> + for_each_engine(engine, guc, i) {
... but not this (see earlier mail), although, the objection is less
here because the GuC is singular and associated with all engines, so
there isn't much else that we could expect to iterate over.
OTOH this may actually be less efficient, because the conversion of the
"struct intel_guc" to the thing(s) actually needed for the iteration
will (or at least may) occur on each iteration of the loop. Generally
I'd prefer to pull all such conversions out to the head of the function,
as the original code did.
> struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
> struct drm_i915_gem_object *obj;
> uint64_t ctx_desc;
> @@ -772,7 +771,6 @@ err:
>
> static void guc_create_log(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct drm_i915_gem_object *obj;
> unsigned long offset;
> uint32_t size, flags;
> @@ -791,7 +789,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(to_i915(guc)->dev, size);
Should we have to_dev(any) as well as to_i915()?
> if (!obj) {
> /* logging will be off */
> i915.guc_log_level = -1;
> @@ -835,7 +833,6 @@ static void init_guc_policies(struct guc_policies *policies)
>
> static void guc_create_ads(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
This dev_priv is used more than once (in fact, it's used in a
for_each_engine() loop below), so I'd think it worth keeping -- and
therefore none of the changes below would be applicable.
.Dave.
> struct drm_i915_gem_object *obj;
> struct guc_ads *ads;
> struct guc_policies *policies;
> @@ -851,7 +848,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(to_i915(guc)->dev, PAGE_ALIGN(size));
> if (!obj)
> return;
>
> @@ -868,10 +865,10 @@ static void guc_create_ads(struct intel_guc *guc)
> * so its address won't change after we've told the GuC where
> * to find it.
> */
> - engine = &dev_priv->engine[RCS];
> - ads->golden_context_lrca = engine->status_page.gfx_addr;
> + ads->golden_context_lrca =
> + to_i915(guc)->engine[RCS].status_page.gfx_addr;
>
> - for_each_engine(engine, dev_priv, i)
> + for_each_engine(engine, guc, i)
> ads->eng_state_size[engine->guc_id] = intel_lr_context_size(engine);
>
> /* GuC scheduling policies */
> @@ -884,7 +881,7 @@ static void guc_create_ads(struct intel_guc *guc)
> /* MMIO reg state */
> reg_state = (void *)policies + sizeof(struct guc_policies);
>
> - for_each_engine(engine, dev_priv, i) {
> + for_each_engine(engine, guc, i) {
> reg_state->mmio_white_list[engine->guc_id].mmio_start =
> engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
More information about the Intel-gfx
mailing list