[Intel-gfx] [PATCH 2/2] drm/i915/guc: Convert ct buffer to iosys_map

[Lucas De Marchi]

On Mon, Mar 14, 2022 at 11:23:07AM +0530, Siva Mullati wrote:
>
>On 09/03/22 07:08, Lucas De Marchi wrote:
>> On Tue, Mar 08, 2022 at 03:08:05PM +0530, Mullati Siva wrote:
>>> From: Siva Mullati <siva.mullati at intel.com>
>>>
>>> Convert CT commands and descriptors to use iosys_map rather
>>> than plain pointer and save it in the intel_guc_ct_buffer struct.
>>> This will help with ct_write and ct_read for cmd send and receive
>>> after the initialization by abstracting the IO vs system memory.
>>>
>>> Signed-off-by: Siva Mullati <siva.mullati at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 170 +++++++++++++---------
>>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |   9 +-
>>> 2 files changed, 110 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> index f01325cd1b62..457deca1c25a 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>> @@ -44,6 +44,11 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
>>> #define CT_PROBE_ERROR(_ct, _fmt, ...) \
>>>     i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
>>>
>>> +#define ct_desc_read(desc_map_, field_) \
>>> +    iosys_map_rd_field(desc_map_, 0, struct guc_ct_buffer_desc, field_)
>>
>> one thing I found useful in intel_guc_ads, was to use the first argument
>> as the struct type instead of map. That's because then I enforce the
>> right type to be passed rather than a random iosys_map. See :
>>
>>     #define ads_blob_read(guc_, field_)                                     \
>>         iosys_map_rd_field(&(guc_)->ads_map, 0, struct __guc_ads_blob, field_)
>>
>> First arg is expected to be `struct intel_guc *`. Yes, I didn't do this
>> for info_map_{read,write}, because there were situation in which I had
>> to pass a info from outside (forcefully from system memory). If you
>> don't have such case,  I think it would be better to pass the typed
>> pointer.
>>
>understood, will change it as a "typed pointer".
>>> +#define ct_desc_write(desc_map_, field_, val_) \
>>> +    iosys_map_wr_field(desc_map_, 0, struct guc_ct_buffer_desc, field_, val_)
>>> +
>>> /**
>>>  * DOC: CTB Blob
>>>  *
>>> @@ -113,9 +118,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>>>     init_waitqueue_head(&ct->wq);
>>> }
>>>
>>> -static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc)
>>> +static void guc_ct_buffer_desc_init(struct iosys_map *desc)
>>
>> if you can apply the comment above, then leave it as
>> struct intel_guc_ct_buffer.
>>
>yes
>>> {
>>> -    memset(desc, 0, sizeof(*desc));
>>> +    iosys_map_memset(desc, 0, 0, sizeof(struct guc_ct_buffer_desc));
>>> }
>>>
>>> static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb)
>>> @@ -128,17 +133,24 @@ static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb)
>>>     space = CIRC_SPACE(ctb->tail, ctb->head, ctb->size) - ctb->resv_space;
>>>     atomic_set(&ctb->space, space);
>>>
>>> -    guc_ct_buffer_desc_init(ctb->desc);
>>> +    guc_ct_buffer_desc_init(&ctb->desc_map);
>>> }
>>>
>>> static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,
>>> -                   struct guc_ct_buffer_desc *desc,
>>> -                   u32 *cmds, u32 size_in_bytes, u32 resv_space)
>>> +                   void *desc, void *cmds, u32 size_in_bytes,
>>> +                   u32 resv_space, bool lmem)
>>
>> bool arguments are really hard to read, because you always have to
>> lookup the function prototype to understand what that is about.
>>
>Tried to avoid bool but could not find a better alternative code path.
>Please suggest, if you have something.

humn... suggestion as as below:

>
>> Here we could turn struct guc_ct_buffer_desc *desc into the map, and let
>> the caller prepare it for iomem or system memory.

In other words, maybe instead of receiving guc_ct_buffer_desc as
parameter, receiving a struct iosys_map *desc_map. That map can be
created by the caller and then you do:

	ctb->desc_map = *desc_map;


Lucas De Marchi