Re: [PATCH v7 10/13] drm/i915/guc: Extract GuC error capture lists on G2H notification.

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@xxxxxxxxx>]

Thanks Umesh for reviewing and for the conditional Rb ...  a follow up 
on the conditions for #1 below (as per our offline conversation)... and 
i will fix #2 as expected.

On 3/11/2022 10:16 AM, Umesh Nerlige Ramappa wrote:
> On Sat, Feb 26, 2022 at 01:55:38AM -0800, Alan Previn wrote: +static int
> > +guc_capture_log_remove_dw(struct intel_guc *guc, struct 
> > __guc_capture_bufstate *buf,
> > +              u32 *dw)
> > +{
> > +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > +    int tries = 2;
> > +    int avail = 0;
> > +    u32 *src_data;
> > +
> > +    if (!guc_capture_buf_cnt(buf))
> > +        return 0;
> > +
> > +    while (tries--) {
> > +        avail = guc_capture_buf_cnt_to_end(buf);
> > +        if (avail >= sizeof(u32)) {
> > +            src_data = (u32 *)(buf->data + buf->rd);
> > +            *dw = *src_data;
> > +            buf->rd += 4;
> > +            return 4;
> > +        }
> > +        if (avail)
> > +            drm_dbg(&i915->drm, "GuC-Cap-Logs not dword aligned, 
> > skipping.\n");
> > +        buf->rd = 0;
>
> (1) This looks like an understanding between GuC and KMD that GuC will 
> not write partial headers at the end of the log buffer if there is no 
> space to fit the header. If you encounter such a scenario, you go back 
> to the beginning. Right? If so, please add a comment indicating the 
> same here.
Thanks Umesh for reviewing, and as per the offline conversation, we 
resolved a bit of a misunderstanding in intent of the function above.

As per what we agreed, we should furnish some additional comments -thus, 
this will go into the code as a comment:

     The GuC Log buffer region for error-capture gets populated in a 
byte-stream flow. However, right now, as per the current and foreseeable 
future, all packed error-capture output structures are dword aligned. 
That said, if the GuC firmware is in the midst of writing a structure 
that is larger than one dword but the tail end of the err-capture 
buffer-region has lesser space left, we would need to extract that 
structure one dword at a time across the end of the ring buffer and onto 
the start. The above function, guc_capture_log_remove_dw is responsible 
for that. All callers of this function would usually do a straight-up 
memcpy and only only call above function if their structure-extraction 
is straddling across the end of the region. GuC firmware does not add 
padding. Once above function has extracted a structure that stradles 
across the end, all extraction functions would return to the norm of 
straight up memcpy's. The reason for the no-padding is to ease 
scalability for future expansion of output data types without requiring 
a redesign of the flow controls.

An additional note wrt above function, .. the while loop always attempts 
up to two tries because if we have exhausted all the dwords, i.e. we've 
this the end, we will reset buf->rd = zero and try again from the 
beginning (this wont be in the comment).


> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static bool
> > +guc_capture_data_extracted(struct __guc_capture_bufstate *b,
> > +               int size, void *dest)
> > +{
> > +    if (guc_capture_buf_cnt_to_end(b) >= size) {
> > +        memcpy(dest, (b->data + b->rd), size);
> > +        b->rd += size;
> > +        return true;
> > +    }
> > +    return false;
> > +}
> > +
> > +static int
> > +guc_capture_log_get_group_hdr(struct intel_guc *guc, struct 
> > __guc_capture_bufstate *buf,
> > +                  struct guc_state_capture_group_header_t *ghdr)
> > +{
> > +    int read = 0;
> > +    int fullsize = sizeof(struct guc_state_capture_group_header_t);
> > +
> > +    if (fullsize > guc_capture_buf_cnt(buf))
> > +        return -1;
> > +
> > +    if (guc_capture_data_extracted(buf, fullsize, (void *)ghdr))
> > +        return 0;
> > +
> > +    read += guc_capture_log_remove_dw(guc, buf, &ghdr->owner);
> > +    read += guc_capture_log_remove_dw(guc, buf, &ghdr->info);
> > +    if (read != fullsize)
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +static int
> > +guc_capture_log_get_data_hdr(struct intel_guc *guc, struct 
> > __guc_capture_bufstate *buf,
> > +                 struct guc_state_capture_header_t *hdr)
> > +{
> > +    int read = 0;
> > +    int fullsize = sizeof(struct guc_state_capture_header_t);
> > +
> > +    if (fullsize > guc_capture_buf_cnt(buf))
> > +        return -1;
> > +
> > +    if (guc_capture_data_extracted(buf, fullsize, (void *)hdr))
> > +        return 0;
> > +
> > +    read += guc_capture_log_remove_dw(guc, buf, &hdr->owner);
> > +    read += guc_capture_log_remove_dw(guc, buf, &hdr->info);
> > +    read += guc_capture_log_remove_dw(guc, buf, &hdr->lrca);
> > +    read += guc_capture_log_remove_dw(guc, buf, &hdr->guc_id);
> > +    read += guc_capture_log_remove_dw(guc, buf, &hdr->num_mmios);
> > +    if (read != fullsize)
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +static int
> > +guc_capture_log_get_register(struct intel_guc *guc, struct 
> > __guc_capture_bufstate *buf,
> > +                 struct guc_mmio_reg *reg)
> > +{
> > +    int read = 0;
> > +    int fullsize = sizeof(struct guc_mmio_reg);
> > +
> > +    if (fullsize > guc_capture_buf_cnt(buf))
> > +        return -1;
> > +
> > +    if (guc_capture_data_extracted(buf, fullsize, (void *)reg))
> > +        return 0;
> > +
> > +    read += guc_capture_log_remove_dw(guc, buf, &reg->offset);
> > +    read += guc_capture_log_remove_dw(guc, buf, &reg->value);
> > +    read += guc_capture_log_remove_dw(guc, buf, &reg->flags);
> > +    read += guc_capture_log_remove_dw(guc, buf, &reg->mask);
> > +    if (read != fullsize)
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +static void
> > +guc_capture_delete_one_node(struct intel_guc *guc, struct 
> > __guc_capture_parsed_output *node)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < GUC_CAPTURE_LIST_TYPE_MAX; ++i) {
> > +        if (node->reginfo[i].regs)
> > +            kfree(node->reginfo[i].regs);
>
> (2) Check before kfree is not needed here and other places that you 
> use kfree.
>
> Ref: https://www.kernel.org/doc/htmldocs/kernel-api/API-kfree.html
Yes, i realize i have been holding back against changing my coding style 
(not used to freeing memory without checking).

However, as per offline chat, I shall change this here and in other patches.


> > +    }
> > +    list_del(&node->link);
> > +    kfree(node);
> > +}
> > +
> > +static void
> > +guc_capture_delete_nodes(struct intel_guc *guc)
> > +{
> > +    /*
> > +     * NOTE: At the end of driver operation, we must assume that we
> > +     * have nodes in outlist from unclaimed error capture events
> > +     * that occurred prior to shutdown.
> > +     */
> > +    if (!list_empty(&guc->capture.priv->outlist)) {
> > +        struct __guc_capture_parsed_output *n, *ntmp;
> > +
> > +        list_for_each_entry_safe(n, ntmp, 
> > &guc->capture.priv->outlist, link)
> > +            guc_capture_delete_one_node(guc, n);
> > +    }
> > +}
> > +
> > +static void
> > +guc_capture_add_node_to_list(struct __guc_capture_parsed_output *node,
> > +                 struct list_head *list)
> > +{
> > +    list_add_tail(&node->link, list);
> > +}
> > +
> > +static void
> > +guc_capture_add_node_to_outlist(struct __guc_state_capture_priv *gc,
> > +                struct __guc_capture_parsed_output *node)
> > +{
> > +    guc_capture_add_node_to_list(node, &gc->outlist);
> > +}
> > +
> > +static void
> > +guc_capture_init_node(struct intel_guc *guc, struct 
> > __guc_capture_parsed_output *node)
> > +{
> > +    INIT_LIST_HEAD(&node->link);
> > +}
> > +
> > +static struct __guc_capture_parsed_output *
> > +guc_capture_alloc_one_node(struct intel_guc *guc)
> > +{
> > +    struct __guc_capture_parsed_output *new;
> > +
> > +    new = kzalloc(sizeof(*new), GFP_KERNEL);
> > +    if (!new)
> > +        return NULL;
> > +
> > +    guc_capture_init_node(guc, new);
> > +
> > +    return new;
> > +}
> > +
> > +static struct __guc_capture_parsed_output *
> > +guc_capture_clone_node(struct intel_guc *guc, struct 
> > __guc_capture_parsed_output *ori,
> > +               u32 keep_reglist_mask)
> > +{
> > +    struct __guc_capture_parsed_output *new;
> > +    int i;
> > +
> > +    new = guc_capture_alloc_one_node(guc);
> > +    if (!new)
> > +        return NULL;
> > +    if (!ori)
> > +        return new;
>
> For readability, I would s/ori/orignal/ if that's what you mean.
>
> > +
> > +    new->is_partial = ori->is_partial;
> > +
> > +    /* copy reg-lists that we want to clone */
> > +    for (i = 0; i < GUC_CAPTURE_LIST_TYPE_MAX; ++i) {
> > +        if (keep_reglist_mask & BIT(i)) {
> > +            new->reginfo[i].regs = kcalloc(ori->reginfo[i].num_regs,
> > +                               sizeof(struct guc_mmio_reg), 
> > GFP_KERNEL);
> > +            if (!new->reginfo[i].regs)
> > +                goto bail_clone;
> > +
> > +            memcpy(new->reginfo[i].regs, ori->reginfo[i].regs,
> > +                   ori->reginfo[i].num_regs * sizeof(struct 
> > guc_mmio_reg));
> > +            new->reginfo[i].num_regs = ori->reginfo[i].num_regs;
> > +            new->reginfo[i].vfid  = ori->reginfo[i].vfid;
> > +
> > +            if (i == GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS) {
> > +                new->eng_class = ori->eng_class;
> > +            } else if (i == GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE) {
> > +                new->eng_inst = ori->eng_inst;
> > +                new->guc_id = ori->guc_id;
> > +                new->lrca = ori->lrca;
> > +            }
> > +        }
> > +    }
> > +
> > +    return new;
> > +
> > +bail_clone:
> > +    for (i = 0; i < GUC_CAPTURE_LIST_TYPE_MAX; ++i) {
> > +        if (new->reginfo[i].regs)
> > +            kfree(new->reginfo[i].regs);
> > +    }
> > +    kfree(new);
> > +    return NULL;
> > +}
> > +
> > +static int
> > +guc_capture_extract_reglists(struct intel_guc *guc, struct 
> > __guc_capture_bufstate *buf)
> > +{
> > +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > +    struct guc_state_capture_group_header_t ghdr = {0};
> > +    struct guc_state_capture_header_t hdr = {0};
> > +    struct __guc_capture_parsed_output *node = NULL;
> > +    struct guc_mmio_reg *regs = NULL;
> > +    int i, numlists, numregs, ret = 0;
> > +    enum guc_capture_type datatype;
> > +    struct guc_mmio_reg tmp;
> > +    bool is_partial = false;
> > +
> > +    i = guc_capture_buf_cnt(buf);
> > +    if (!i)
> > +        return -ENODATA;
> > +    if (i % sizeof(u32)) {
> > +        drm_warn(&i915->drm, "GuC Capture new entries unaligned\n");
> > +        ret = -EIO;
> > +        goto bailout;
> > +    }
> > +
> > +    /* first get the capture group header */
> > +    if (guc_capture_log_get_group_hdr(guc, buf, &ghdr)) {
> > +        ret = -EIO;
> > +        goto bailout;
> > +    }
> > +    /*
> > +     * we would typically expect a layout as below where n would be 
> > expected to be
> > +     * anywhere between 3 to n where n > 3 if we are seeing multiple 
> > dependent engine
> > +     * instances being reset together.
> > +     * ____________________________________________
> > +     * | Capture Group                            |
> > +     * | ________________________________________ |
> > +     * | | Capture Group Header:                | |
> > +     * | |  - num_captures = 5                  | |
> > +     * | |______________________________________| |
> > +     * | ________________________________________ |
> > +     * | | Capture1:                            | |
> > +     * | |  Hdr: GLOBAL, numregs=a              | |
> > +     * | | ____________________________________ | |
> > +     * | | | Reglist                          | | |
> > +     * | | | - reg1, reg2, ... rega           | | |
> > +     * | | |__________________________________| | |
> > +     * | |______________________________________| |
> > +     * | ________________________________________ |
> > +     * | | Capture2:                            | |
> > +     * | |  Hdr: CLASS=RENDER/COMPUTE, numregs=b| |
> > +     * | | ____________________________________ | |
> > +     * | | | Reglist                          | | |
> > +     * | | | - reg1, reg2, ... regb           | | |
> > +     * | | |__________________________________| | |
> > +     * | |______________________________________| |
> > +     * | ________________________________________ |
> > +     * | | Capture3:                            | |
> > +     * | |  Hdr: INSTANCE=RCS, numregs=c        | |
> > +     * | | ____________________________________ | |
> > +     * | | | Reglist                          | | |
> > +     * | | | - reg1, reg2, ... regc           | | |
> > +     * | | |__________________________________| | |
> > +     * | |______________________________________| |
> > +     * | ________________________________________ |
> > +     * | | Capture4:                            | |
> > +     * | |  Hdr: CLASS=RENDER/COMPUTE, numregs=d| |
> > +     * | | ____________________________________ | |
> > +     * | | | Reglist                          | | |
> > +     * | | | - reg1, reg2, ... regd           | | |
> > +     * | | |__________________________________| | |
> > +     * | |______________________________________| |
> > +     * | ________________________________________ |
> > +     * | | Capture5:                            | |
> > +     * | |  Hdr: INSTANCE=CCS0, numregs=e       | |
> > +     * | | ____________________________________ | |
> > +     * | | | Reglist                          | | |
> > +     * | | | - reg1, reg2, ... rege           | | |
> > +     * | | |__________________________________| | |
> > +     * | |______________________________________| |
> > +     * |__________________________________________|
> > +     */
> > +    is_partial = FIELD_GET(CAP_GRP_HDR_CAPTURE_TYPE, ghdr.info);
> > +    numlists = FIELD_GET(CAP_GRP_HDR_NUM_CAPTURES, ghdr.info);
> > +
> > +    while (numlists--) {
> > +        if (guc_capture_log_get_data_hdr(guc, buf, &hdr)) {
> > +            ret = -EIO;
> > +            break;
> > +        }
> > +
> > +        datatype = FIELD_GET(CAP_HDR_CAPTURE_TYPE, hdr.info);
> > +        if (datatype > GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE) {
> > +            /* unknown capture type - skip over to next capture set */
> > +            numregs = FIELD_GET(CAP_HDR_NUM_MMIOS, hdr.num_mmios);
> > +            while (numregs--) {
> > +                if (guc_capture_log_get_register(guc, buf, &tmp)) {
> > +                    ret = -EIO;
> > +                    break;
> > +                }
> > +            }
> > +            continue;
> > +        } else if (node) {
> > +            /*
> > +             * Based on the current capture type and what we have so 
> > far,
> > +             * decide if we should add the current node into the 
> > internal
> > +             * linked list for match-up when i915_gpu_coredump calls 
> > later
> > +             * (and alloc a blank node for the next set of reglists)
> > +             * or continue with the same node or clone the current node
> > +             * but only retain the global or class registers (such 
> > as the
> > +             * case of dependent engine resets).
> > +             */
> > +            if (datatype == GUC_CAPTURE_LIST_TYPE_GLOBAL) {
> > + guc_capture_add_node_to_outlist(guc->capture.priv, node);
> > +                node = NULL;
> > +            } else if (datatype == 
> > GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS &&
> > + node->reginfo[GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS].regs) {
> > +                /* Add to list, clone node and duplicate global list */
> > + guc_capture_add_node_to_outlist(guc->capture.priv, node);
> > +                node = guc_capture_clone_node(guc, node,
> > + GCAP_PARSED_REGLIST_INDEX_GLOBAL);
> > +            } else if (datatype == 
> > GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE &&
> > + node->reginfo[GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE].regs) {
> > +                /* Add to list, clone node and duplicate global + 
> > class lists */
> > + guc_capture_add_node_to_outlist(guc->capture.priv, node);
> > +                node = guc_capture_clone_node(guc, node,
> > + (GCAP_PARSED_REGLIST_INDEX_GLOBAL |
> > + GCAP_PARSED_REGLIST_INDEX_ENGCLASS));
> > +            }
> > +        }
> > +
> > +        if (!node) {
> > +            node = guc_capture_alloc_one_node(guc);
> > +            if (!node) {
> > +                ret = -ENOMEM;
> > +                break;
> > +            }
> > +            if (datatype != GUC_CAPTURE_LIST_TYPE_GLOBAL)
> > +                drm_dbg(&i915->drm, "GuC Capture missing global 
> > dump: %08x!\n",
> > +                    datatype);
> > +        }
> > +        node->is_partial = is_partial;
> > +        node->reginfo[datatype].vfid = 
> > FIELD_GET(CAP_HDR_CAPTURE_VFID, hdr.owner);
> > +        switch (datatype) {
> > +        case GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE:
> > +            node->eng_class = FIELD_GET(CAP_HDR_ENGINE_CLASS, 
> > hdr.info);
> > +            node->eng_inst = FIELD_GET(CAP_HDR_ENGINE_INSTANCE, 
> > hdr.info);
> > +            node->lrca = hdr.lrca;
> > +            node->guc_id = hdr.guc_id;
> > +            break;
> > +        case GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS:
> > +            node->eng_class = FIELD_GET(CAP_HDR_ENGINE_CLASS, 
> > hdr.info);
> > +            break;
> > +        default:
> > +            break;
> > +        }
> > +
> > +        regs = NULL;
> > +        numregs = FIELD_GET(CAP_HDR_NUM_MMIOS, hdr.num_mmios);
> > +        if (numregs) {
> > +            regs = kcalloc(numregs, sizeof(struct guc_mmio_reg), 
> > GFP_KERNEL);
> > +            if (!regs) {
> > +                ret = -ENOMEM;
> > +                break;
> > +            }
> > +        }
> > +        node->reginfo[datatype].num_regs = numregs;
> > +        node->reginfo[datatype].regs = regs;
> > +        i = 0;
> > +        while (numregs--) {
> > +            if (guc_capture_log_get_register(guc, buf, &regs[i++])) {
> > +                ret = -EIO;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +bailout:
> > +    if (node) {
> > +        /* If we have data, add to linked list for match-up when 
> > i915_gpu_coredump calls */
> > +        for (i = GUC_CAPTURE_LIST_TYPE_GLOBAL; i < 
> > GUC_CAPTURE_LIST_TYPE_MAX; ++i) {
> > +            if (node->reginfo[i].regs) {
> > + guc_capture_add_node_to_outlist(guc->capture.priv, node);
> > +                node = NULL;
> > +                break;
> > +            }
> > +        }
> > +        if (node) /* else free it */
> > +            kfree(node);
> > +    }
> > +    return ret;
> > +}
> > +
> > +static int __guc_capture_flushlog_complete(struct intel_guc *guc)
> > +{
> > +    u32 action[] = {
> > +        INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE,
> > +        2
> > +    };
> > +
> > +    return intel_guc_send(guc, action, ARRAY_SIZE(action));
> > +}
> > +
> > +static void __guc_capture_process_output(struct intel_guc *guc)
> > +{
> > +    unsigned int buffer_size, read_offset, write_offset, full_count;
> > +    struct intel_uc *uc = container_of(guc, typeof(*uc), guc);
> > +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > +    struct guc_log_buffer_state log_buf_state_local;
> > +    struct guc_log_buffer_state *log_buf_state;
> > +    struct __guc_capture_bufstate buf;
> > +    void *src_data = NULL;
> > +    bool new_overflow;
> > +    int ret;
> > +
> > +    log_buf_state = guc->log.buf_addr +
> > +            (sizeof(struct guc_log_buffer_state) * 
> > GUC_CAPTURE_LOG_BUFFER);
> > +    src_data = guc->log.buf_addr + 
> > intel_guc_get_log_buffer_offset(GUC_CAPTURE_LOG_BUFFER);
> > +
> > +    /*
> > +     * Make a copy of the state structure, inside GuC log buffer
> > +     * (which is uncached mapped), on the stack to avoid reading
> > +     * from it multiple times.
> > +     */
> > +    memcpy(&log_buf_state_local, log_buf_state, sizeof(struct 
> > guc_log_buffer_state));
> > +    buffer_size = 
> > intel_guc_get_log_buffer_size(GUC_CAPTURE_LOG_BUFFER);
> > +    read_offset = log_buf_state_local.read_ptr;
> > +    write_offset = log_buf_state_local.sampled_write_ptr;
> > +    full_count = log_buf_state_local.buffer_full_cnt;
> > +
> > +    /* Bookkeeping stuff */
> > +    guc->log.stats[GUC_CAPTURE_LOG_BUFFER].flush += 
> > log_buf_state_local.flush_to_file;
> > +    new_overflow = intel_guc_check_log_buf_overflow(&guc->log, 
> > GUC_CAPTURE_LOG_BUFFER,
> > +                            full_count);
> > +
> > +    /* Now copy the actual logs. */
> > +    if (unlikely(new_overflow)) {
> > +        /* copy the whole buffer in case of overflow */
> > +        read_offset = 0;
> > +        write_offset = buffer_size;
> > +    } else if (unlikely((read_offset > buffer_size) ||
> > +            (write_offset > buffer_size))) {
> > +        drm_err(&i915->drm, "invalid GuC log capture buffer state!\n");
> > +        /* copy whole buffer as offsets are unreliable */
> > +        read_offset = 0;
> > +        write_offset = buffer_size;
> > +    }
> > +
> > +    buf.size = buffer_size;
> > +    buf.rd = read_offset;
> > +    buf.wr = write_offset;
> > +    buf.data = src_data;
> > +
> > +    if (!uc->reset_in_progress) {
> > +        do {
> > +            ret = guc_capture_extract_reglists(guc, &buf);
> > +        } while (ret >= 0);
> > +    }
> > +
> > +    /* Update the state of log buffer err-cap state */
> > +    log_buf_state->read_ptr = write_offset;
> > +    log_buf_state->flush_to_file = 0;
> > +    __guc_capture_flushlog_complete(guc);
> > +}
> > +
> > +void intel_guc_capture_process(struct intel_guc *guc)
> > +{
> > +    if (guc->capture.priv)
> > +        __guc_capture_process_output(guc);
> > +}
> > +
> > static void
> > guc_capture_free_ads_cache(struct __guc_state_capture_priv *gc)
> > {
> > @@ -715,6 +1255,8 @@ void intel_guc_capture_destroy(struct intel_guc 
> > *guc)
> >
> >     guc_capture_free_ads_cache(guc->capture.priv);
> >
> > +    guc_capture_delete_nodes(guc);
> > +
> >     if (guc->capture.priv->extlists) {
> > guc_capture_free_extlists(guc->capture.priv->extlists);
> >         kfree(guc->capture.priv->extlists);
> > @@ -732,5 +1274,7 @@ int intel_guc_capture_init(struct intel_guc *guc)
> >
> >     guc->capture.priv->reglists = guc_capture_get_device_reglist(guc);
> >
> > +    INIT_LIST_HEAD(&guc->capture.priv->outlist);
> > +
> >     return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > index 24a11f33f7d9..3c79460c7ca5 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > @@ -12,6 +12,7 @@ struct file;
> > struct guc_gt_system_info;
> > struct intel_guc;
> >
> > +void intel_guc_capture_process(struct intel_guc *guc);
> > int intel_guc_capture_output_min_size_est(struct intel_guc *guc);
> > int intel_guc_capture_getlist(struct intel_guc *guc, u32 owner, u32 
> > type, u32 classid,
> >                   struct file **fileptr);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> > index e9a865c2f4cb..8d59a11ec595 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> > @@ -158,9 +158,9 @@ static void *guc_get_write_buffer(struct 
> > intel_guc_log *log)
> >     return relay_reserve(log->relay.channel, 0);
> > }
> >
> > -static bool guc_check_log_buf_overflow(struct intel_guc_log *log,
> > -                       enum guc_log_buffer_type type,
> > -                       unsigned int full_cnt)
> > +bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log,
> > +                      enum guc_log_buffer_type type,
> > +                      unsigned int full_cnt)
> > {
> >     unsigned int prev_full_cnt = log->stats[type].sampled_overflow;
> >     bool overflow = false;
> > @@ -183,7 +183,7 @@ static bool guc_check_log_buf_overflow(struct 
> > intel_guc_log *log,
> >     return overflow;
> > }
> >
> > -static unsigned int guc_get_log_buffer_size(enum guc_log_buffer_type 
> > type)
> > +unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type 
> > type)
> > {
> >     switch (type) {
> >     case GUC_DEBUG_LOG_BUFFER:
> > @@ -199,6 +199,20 @@ static unsigned int guc_get_log_buffer_size(enum 
> > guc_log_buffer_type type)
> >     return 0;
> > }
> >
> > +size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type)
> > +{
> > +    enum guc_log_buffer_type i;
> > +    size_t offset = PAGE_SIZE;/* for the log_buffer_states */
> > +
> > +    for (i = GUC_DEBUG_LOG_BUFFER; i < GUC_MAX_LOG_BUFFER; ++i) {
> > +        if (i == type)
> > +            break;
> > +        offset += intel_guc_get_log_buffer_size(i);
> > +    }
> > +
> > +    return offset;
> > +}
> > +
> > static void _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log)
> > {
> >     unsigned int buffer_size, read_offset, write_offset, 
> > bytes_to_copy, full_cnt;
> > @@ -243,14 +257,14 @@ static void 
> > _guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log)
> >          */
> >         memcpy(&log_buf_state_local, log_buf_state,
> >                sizeof(struct guc_log_buffer_state));
> > -        buffer_size = guc_get_log_buffer_size(type);
> > +        buffer_size = intel_guc_get_log_buffer_size(type);
> >         read_offset = log_buf_state_local.read_ptr;
> >         write_offset = log_buf_state_local.sampled_write_ptr;
> >         full_cnt = log_buf_state_local.buffer_full_cnt;
> >
> >         /* Bookkeeping stuff */
> >         log->stats[type].flush += log_buf_state_local.flush_to_file;
> > -        new_overflow = guc_check_log_buf_overflow(log, type, full_cnt);
> > +        new_overflow = intel_guc_check_log_buf_overflow(log, type, 
> > full_cnt);
> >
> >         /* Update the state of shared log buffer */
> >         log_buf_state->read_ptr = write_offset;
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> > index e1345fca7729..18007e639be9 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> > @@ -67,6 +67,10 @@ struct intel_guc_log {
> > };
> >
> > void intel_guc_log_init_early(struct intel_guc_log *log);
> > +bool intel_guc_check_log_buf_overflow(struct intel_guc_log *log, 
> > enum guc_log_buffer_type type,
> > +                      unsigned int full_cnt);
> > +unsigned int intel_guc_get_log_buffer_size(enum guc_log_buffer_type 
> > type);
> > +size_t intel_guc_get_log_buffer_offset(enum guc_log_buffer_type type);
> > int intel_guc_log_create(struct intel_guc_log *log);
> > void intel_guc_log_destroy(struct intel_guc_log *log);
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index ab3cea352fb3..870b48456e9c 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -25,6 +25,7 @@
> > #include "gt/intel_ring.h"
> >
> > #include "intel_guc_ads.h"
> > +#include "intel_guc_capture.h"
> > #include "intel_guc_submission.h"
> >
> > #include "i915_drv.h"
> > @@ -4070,17 +4071,18 @@ int 
> > intel_guc_context_reset_process_msg(struct intel_guc *guc,
> > int intel_guc_error_capture_process_msg(struct intel_guc *guc,
> >                     const u32 *msg, u32 len)
> > {
> > -    int status;
> > +    u32 status;
> >
> >     if (unlikely(len != 1)) {
> >         drm_dbg(&guc_to_gt(guc)->i915->drm, "Invalid length %u", len);
> >         return -EPROTO;
> >     }
> >
> > -    status = msg[0];
> > -    drm_info(&guc_to_gt(guc)->i915->drm, "Got error capture: status 
> > = %d", status);
> > +    status = msg[0] & INTEL_GUC_STATE_CAPTURE_EVENT_STATUS_MASK;
> > +    if (status == INTEL_GUC_STATE_CAPTURE_EVENT_STATUS_NOSPACE)
> > +        drm_warn(&guc_to_gt(guc)->i915->drm, "G2H-Error capture no 
> > space");
> >
> > -    /* FIXME: Do something with the capture */
> > +    intel_guc_capture_process(guc);
> >
> >     return 0;
> > }
>
> With (1) and (2) taken care of, this is:
>
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
>
> Regards,
> Umesh
>
> > --
> > 2.25.1