Re: [Intel-xe] [PATCH v3 26/30] drm/xe/uapi: Add pad to drm_xe_engine_class_instance

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Oct 04, 2023 at 07:35:06PM -0700, Umesh Nerlige Ramappa wrote:
>On Wed, Oct 04, 2023 at 12:55:19PM +0200, Francois Dugast wrote:
>>On Tue, Oct 03, 2023 at 11:15:00AM -0700, Umesh Nerlige Ramappa wrote:
>>>On Fri, Sep 29, 2023 at 09:45:35AM -0700, Souza, Jose wrote:
>>>> On Fri, 2023-09-29 at 09:00 -0700, Umesh Nerlige Ramappa wrote:
>>>> > On Fri, Sep 29, 2023 at 11:06:53AM +0200, Francois Dugast wrote:
>>>> > > Hi Umesh,
>>>> > >
>>>> > > On Thu, Sep 28, 2023 at 05:36:39PM -0700, Umesh Nerlige Ramappa wrote:
>>>> > > > On Tue, Sep 26, 2023 at 12:55:36PM +0000, Francois Dugast wrote:
>>>> > > > > Add pad to reserve space for future uses such as special configurations
>>>> > > > > for media.
>>>> > > > >
>>>> > > > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>>>> > > >
>>>> > > > I saw a comment in v4 that you dropped the patches that are not reviewed, so
>>>> > > > this is:
>>>> > > >
>>>> > > > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>> > > >
>>>> > > > Hoping this will be part of the final series that you merge since the
>>>> > > > ENGINE_CYCLES query is using this structure and relies on this padding.
>>>> > >
>>>> > > Actually using your original submission [1] this patch should not longer
>>>> > > be necessary, right? It includes:
>>>> > >
>>>> > >   __u16 rsvd;
>>>> > >
>>>> > > [1] https://lore.kernel.org/intel-xe/20230814223734.375449-5-umesh.nerlige.ramappa@intel.com/
>>>> >
>>>> > Hmmm, I failed to notice that earlier. My bad. v4 looks good, you can
>>>> > ignore this.
>>>>
>>>> It is named as rsvd but it should be a pad.
>>>>
>>>> We can't leave as reserved to be future used to return media engine capabilities because drm_xe_engine_class_instance is also used as input to other
>>>> uAPIs and that is useless information after initialization that UMDs should not need to carry around.
>>>>
>>>> Instead DRM_XE_DEVICE_QUERY_ENGINES should return something like
>>>>
>>>> struct drm_xe_engine_info {
>>>> 	struct drm_xe_engine_class_instance instance;
>>>> 	__u32 rsvd;/* future could be __u16 capabilities; __u16 rsvd; */
>>>> };
>>>>
>>>> This can be fixed in uAPI take 2.
>>>
>>>Hi Francois,
>>>
>>>As per Jose's inputs, I posted a new version of my series here -
>>>https://patchwork.freedesktop.org/patch/560614/?series=124565&rev=1.
>>>
>>>Can you please pick up patch 3/3 from here. Note that just adding the pad
>>>does not resolve the issue I was mentioning in my original series.  We also
>>>need to explicitly initialize the pad to 0 in this case. I have included
>>>that change as well in the above patch.
>>>
>>>Thanks,
>>>Umesh
>>
>>Hi Umesh,
>>
>>Thanks for the update. An earlier version was already included in take 1
>>of the uAPI changes [1],
>
>Not sure if the below link in [1] is what you wanted to share. It 
>points to the series I posted. I am guessing you were intending to 
>point to a take 1 series.

fixup here - https://patchwork.freedesktop.org/series/124690/

Thanks,
Umesh
>
>Thanks,
>Umesh
>
>>against which UMDs are currently aligning, so it
>>would be best to leave take 1 as it is and include your fix as a fixup in
>>take 2. Could you please send a fixup instead?
>>
>>[1] https://patchwork.freedesktop.org/patch/560614/?series=124565&rev=1
>>
>>Francois
>>
>>>>
>>>> >
>>>> > Thanks,
>>>> > Umesh
>>>> > >
>>>> > > Francois
>>>> > >
>>>> > > >
>>>> > > > Thanks,
>>>> > > > Umesh
>>>> > > > > ---
>>>> > > > > include/uapi/drm/xe_drm.h | 2 ++
>>>> > > > > 1 file changed, 2 insertions(+)
>>>> > > > >
>>>> > > > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>>> > > > > index 79e47708d526..84091860c7d2 100644
>>>> > > > > --- a/include/uapi/drm/xe_drm.h
>>>> > > > > +++ b/include/uapi/drm/xe_drm.h
>>>> > > > > @@ -143,6 +143,8 @@ struct drm_xe_engine_class_instance {
>>>> > > > >
>>>> > > > > 	__u16 engine_instance;
>>>> > > > > 	__u16 gt_id;
>>>> > > > > +	/** @pad: MBZ */
>>>> > > > > +	__u32 pad;
>>>> > > > > };
>>>> > > > >
>>>> > > > > /**
>>>> > > > > --
>>>> > > > > 2.34.1
>>>> > > > >
>>>>