Re: [PATCH 03/11] drm/msm: Add hint to DRM_IOCTL_MSM_GEM_INFO to return an object IOVA

[Rob Clark <robdclark@gmail.com>]

On Mon, Feb 6, 2017 at 3:24 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 6 February 2017 at 19:57, Rob Clark <robdclark@gmail.com> wrote:
>> On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> Hi Jordan,
>>>
>>> On 6 February 2017 at 17:39, Jordan Crouse <jcrouse@codeaurora.org> wrote:
>>>> Modify the 'pad' member of struct drm_msm_gem_info to 'hint'. If the
>>>> user sets 'hint' to non-zero it means that they want a IOVA for the
>>>> GEM object instead of a mmap() offset. Return the iova in the 'offset'
>>>> member.
>>>>
>>>> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
>>>> ---
>>>>  drivers/gpu/drm/msm/msm_drv.c | 29 +++++++++++++++++++++++++----
>>>>  include/uapi/drm/msm_drm.h    |  4 ++--
>>>>  2 files changed, 27 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>>> index e29bb66..1e4e022 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>> @@ -677,6 +677,17 @@ static int msm_ioctl_gem_cpu_fini(struct drm_device *dev, void *data,
>>>>         return ret;
>>>>  }
>>>>
>>>> +static int msm_ioctl_gem_info_iova(struct drm_device *dev,
>>>> +               struct drm_gem_object *obj, uint64_t *iova)
>>>> +{
>>>> +       struct msm_drm_private *priv = dev->dev_private;
>>>> +
>>>> +       if (!priv->gpu)
>>>> +               return -EINVAL;
>>>> +
>>> Not too familiar with msm so perhaps a silly question: how can we trigger this ?
>>
>> if gpu has not loaded (for example, missing firmware, or kernel does
>> not have iommu, etc)
>>
> Thanks Rob. I was under the impression that in such cases the driver
> will/should fail to load.

radeon/nouveau/i915 will all, iirc, fail to load.  I made drm/msm
defer to loading gpu until first open() since having to constantly
rebuild initrd seemed annoying ;-)

>>>> +       __u64 offset;         /* out, mmap() offset if hint is 0, iova if 1 */
>>> Other drivers have used anonymous unions to improve the naming, in
>>> such situations.
>>>
>>> struct drm_msm_gem_info {
>>>         __u32 handle;           /* in */
>>>         __u32 hint;             /* in */
>>>         union {                 /* out */
>>>               __u64 offset;         /* offset if hint is FOO */
>>>               __u64 iova;           /* iova if hint is BAR */
>>>         };
>>> };
>>
>> is anon union legit for uabi?  I was under the impression that for
>> some reason it was not.  But I could be wrong.
>>
> Haven't seen any wording against it and we do have a few instances in
> DRM UABI land.
> Either way it was just an idea.

hmm, ok, if we are already using it in uabi (and not just ancient
ioctls) then maybe I am wrong..

BR,
-R


> Regards,
> Emil