On to, 2017-02-16 at 06:18 -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
<SNIP>
> -static void guc_client_free(struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client **p_client)
> {
> + struct i915_guc_client *client;
> +
> + client = fetch_and_zero(p_client);
> + if (!client)
> + return;
> +
Might be useful to wrap the reminder of this function into
__guc_client_free, which can be called directly. But that can be added
later, when code described by Daniele appears.
> @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state) +
> GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>
> - vma = guc->ads_vma;
> - if (!vma) {
> - vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> - if (IS_ERR(vma))
> - return;
> + vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
>
> - guc->ads_vma = vma;
> - }
> + guc->ads_vma = vma;
Only now realized the connection, may I suggest s/ads_vma/addon_vma/ as
a follow-up patch :P
> @@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> dev_priv->kernel_context);
> if (IS_ERR(guc->execbuf_client)) {
> DRM_ERROR("Failed to create GuC client for execbuf!\n");
> - goto err;
> + ret = PTR_ERR(guc->execbuf_client);
> + goto err_ads;
> }
>
> return 0;
\n here to make Chris happy.
> +err_ads:
> + guc_addon_destroy(guc);
> +err_log:
> + intel_guc_log_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +err_vma:
> + i915_vma_unpin_and_release(&guc->ctx_pool);
>
> -err:
> - i915_guc_submission_fini(dev_priv);
> - return -ENOMEM;
> + return ret;
> +}
<SNIP>
> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)
If guc_log_relay_channel is going to be a thing, then this is the right
thing to do.
> @@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
> return 0;
> }
>
> -static int guc_log_create_relay_file(struct intel_guc *guc)
> +static void guc_log_relay_channel_destroy(struct intel_guc *guc)
> +{
> + relay_close(guc->log.relay_chan);
> + guc->log.relay_chan = NULL;
If the relay channel is a dynamic thing which gets recreated and
destroyed in runtime, then this is OKish, but if it's only created at
driver init and destroyed at shutdown, then don't bother assigning
NULL.
> +static int guc_log_relay_file_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct dentry *log_dir;
> int ret;
>
> + if (i915.guc_log_level < 0)
> + return 0; /* nothing to do */
The comment is not necessary, the check is rather self-explaining.
> @@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
> > }
>
> ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> - if (ret) {
> + if (ret == -EEXIST) {
> + /* Ignore "file already exists" */
Comment again is redundant. Just;
if (ret < 0 && ret != -EEXISTS)
> + }
> + else if (ret < 0) {
> DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> return ret;
> }
<SNIP>
> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)
The naming convention we have is rather difficult, here
guc_log_create_extras would be the right name, as "guc_log" is the
structure, and "create extras" is our verb. If we had "guc_log_extras",
then "guc_log_extras" would be the structure and "create" the verb. But
if you intend to break out guc_log_extras, then it's good.
Although, the purpose of this function sounds more like init_extras.
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> - int ret;
> + int ret = 0;
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - /* Nothing to do */
> if (i915.guc_log_level < 0)
> - return 0;
> + return 0; /* nothing to do */
Don't be shy to nuke such comments.
>
> - if (!guc->log.buf_addr) {
> - /* Create a WC (Uncached for read) vmalloc mapping of log
> - * buffer pages, so that we can directly get the data
> - * (up-to-date) from memory.
> - */
> - vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> - return ret;
> - }
> + if (guc->log.buf_addr)
> + return 0; /* already allocated */
This check can be hoisted to calling function and if you feel like so,
add "guc_has_log_extras" helper. (Comment is redundant again).
Generally speaking, the calls should not be idempotent, so instead of
checking, add GEM_BUG_ON(guc->log.buf_addr); The more "if"s we have,
the harder it's to get good testing coverage.
<SNIP>
> + guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
> + if (guc->log.flush_wq == NULL) {
While around, make it if (!guc->log.flush_wq)
> + DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> + ret = -ENOMEM;
> + goto err_relaychan;
> }
>
> return 0;
\n here and other spots.
> +err_relaychan:
> + guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> > + i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> > + return ret;
> }
<SNIP>
> @@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> return ret;
> }
>
> - i915.guc_log_level = log_param.verbosity;
> + if (log_param.logging_enabled)
Extra newline here to be removed.
A much wanted improvement. I might move the function renames to
follow-up patches when new structs get introduced. I'd also like some
clarity between "extras", "addon" and "ads" in follow-up :)
With above changes, this patch is looking good to me.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
