[Intel-xe] Code review for Xe driver - Second batch of comments (1/6)

[Lucas De Marchi]

On Thu, Jun 01, 2023 at 04:44:44PM +0300, Oded Gabbay wrote:
>Hi All,
>Here is the second batch of comments, grouped by filename:
>
>General: Use of defines in the middle of functions is really making it
>hard to read the code. And it is a very uncommon practice. I can
>understand definitions of flags inside structure, and even definitions
>before functions, but just in the middle of the function makes the
>reader lose track of the code. I would like to request to modify only
>those parts.
>
>xe_vm_types.h:
>- Line 175: Either XE_VM_FLAGS_64K should be XE_VM_FLAG_64K or all
>other XE_VM_FLAG_* should be XE_VM_FLAGS_*
>    I would also love to have some documentation on what each flag
>means. It's not trivial to understand that from the code.

oh... and probably a comment that more flags can't be added without
spacing XE_VM_FLAG_GT_ID out. Should probably go ahead and s/6/16/
maybe?

>
>xe_bo.h:
>- Line 23: Where is the STOLEN define being used? I only found code
>that checks if that flag is set

there are limited uses of stolen. One place that uses it is the display
side, for example for the initial fb and display page table.

Could be added only together with display and was probably left over
when display was wiped from the branch.


>xe_guc_ads.c:
>- No need for guc_ads_regset_size function, use field directly.

true. On i915 the ordering is more intricate and there could be cases of
using the field before it got initialized. This is not the case in xe

>- Use define instead of guc_ads_capture_size.

even if the FIXME needs to be fixed na in which it would be similar to
the guc_ads_golden_lrc_size()?

>- calculate_golden_lrc_size() - you are calculating golden ring size
>for all possible engine types but not taking into account if engines
>have several hw instances. Is it ok ?

yes, we don't have different golden state per instance, it's per class
of engine. See the loop in guc_populate_golden_lrc() also happening by
engine class.

>- Line 452: not all struct fields (which are used later) are initialized.

extra_regs? they are initialized to 0. After the xe_reg refactor,
commit 748a140d5c63 ("drm/xe/guc: Handle RCU_MODE as masked from definition")
it's also simplified as the "masked" information is kept on the register
rather than on that table.


>
>xe_guc_ct_types.h:
>- Line 17: XE_GUC_CT_SELFTEST is always defined, why do we need it?
>- mutex lock in xe_guc_ct: same lock used for G2H and H2G actions so
>these CTs can't work in parallel. Is there a reason why this can't be
>separated into two different locks ? If so, I think it is worth
>commenting somewhere in the code.
>
>xe_guc_pc.c
>- pc_gucrc_disable(): disabling registers which were never enabled
>(lines 748-750).
>- Line 896: Add mutex destroy for pc->freq_lock.
>
>xe_guc_ct.c
>- Line 45, 50: Please use define for uninitialized fence.
>- Line 139: need to add mutex_destroy for ct->lock.

several other places in xe moved to drmm_mutex_init(). Maybe follow suit
here and in the other comments below?

thanks
Lucas De Marchi

>- Line 142: It appears that ct->fence_context is initialized but never
>used. If so, please remove
>- Although it's explained at the top, please add defines for offsets
>in the CT buffer:
>  H2G_CTB_DESC_OFFSET, G2H_CTB_DESC_OFFSET, H2G_CTB_BUF_OFFSET and
>G2H_CTB_BUF_OFFSET.
>- parse_g2h_event(): add default handling to switch.
>- parse_g2h_msg(): header variable is unused.
>- Lines 927: can't you use CIRC_SPACE instead of doing this manually ?
>- xe_guc_ct_fast_path(): problematic use of ct->fast_lock spinlock.
>g2h_read() can take some time to execute. Same for dequeue_one_g2h
>which also calls to g2h_read(). spinlock protected code should execute
>fast. Must we have a spinlock here ?
>- xe_guc_tlb_invalidation_done_handler(): it asserts that the ct mutex
>is locked, but I didn't see anywhere in the call stack of this
>function that the mutex is locked
>
>xe_guc_submit.c
>- Line 230: mutex_init(&guc->submission_state.lock); without mutex_destroy.
>- Lines 387, 396(!) and 399: please add defines which hold the
>calculations and use them in the structure definitions.
>- wq_wait_for_space(), Line 526: no handling in case there is no
>available space. Actually, it looks like this if statement is
>redundant since you check it again two lines after. This code is very
>confusing, I recommend refactoring this.
>- wq_item_append(): Lines 571 - 572 - Please add define instead of 3.
>Leaving it to the user to understand where this +3 came from is not
>helpful. Also there is no indication on success/fail, need to add
>return code.
>- submit_engine(): There is no indication of success/failure, need to
>add return code.
>- guc_engine_run_job(): maybe it's possible to merge assertions? There
>are 7 assertions before emitting the job. Maybe add
>engine_operational() which takes care of these assertions?
>  Also, if job is not submitted to the engine, I think this function
>should return ERR_VALUE to the drm scheduler.
>
>xe_pci.c:
>- Line 462: Consider using pcim_enable_device ? I see other drm
>drivers (drivers/gpu/drm/tiny/bochs.c for example) using it.
>
>xe_force_wake.c
>- Line 44: mutex_init(&fw->lock); without mutex_destroy.
>
>xe_pcode.c:
>- Line 278: mutex_init(&gt->pcode.lock); without mutex_destroy.
>
>xe_ttm_vram_mgr.c:
>- Line 342: mutex_init(&mgr->lock); without mutex_destroy.
>
>Thanks,
>Oded