[PATCH v5 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU

[Jeffrey Hugo]

On 1/13/2023 1:23 AM, Jacek Lawrynowicz wrote:
> Hi,
> 
> On 12.01.2023 18:34, Jeffrey Hugo wrote:
>> On 1/9/2023 5:23 AM, Jacek Lawrynowicz wrote:
>>> VPU stands for Versatile Processing Unit and it's a CPU-integrated
>>> inference accelerator for Computer Vision and Deep Learning
>>> applications.
>>>
>>> The VPU device consist of following components:
>>>     - Buttress - provides CPU to VPU integration, interrupt, frequency and
>>>       power management.
>>>     - Memory Management Unit (based on ARM MMU-600) - translates VPU to
>>>       host DMA addresses, isolates user workloads.
>>>     - RISC based microcontroller - executes firmware that provides job
>>>       execution API for the kernel-mode driver
>>>     - Neural Compute Subsystem (NCS) - does the actual work, provides
>>>       Compute and Copy engines.
>>>     - Network on Chip (NoC) - network fabric connecting all the components
>>>
>>> This driver supports VPU IP v2.7 integrated into Intel Meteor Lake
>>> client CPUs (14th generation).
>>>
>>> Module sources are at drivers/accel/ivpu and module name is
>>> "intel_vpu.ko".
>>>
>>> This patch includes only very besic functionality:
>>>     - module, PCI device and IRQ initialization
>>>     - register definitions and low level register manipulation functions
>>>     - SET/GET_PARAM ioctls
>>>     - power up without firmware
>>>
>>> Co-developed-by: Krystian Pradzynski <krystian.pradzynski at linux.intel.com>
>>> Signed-off-by: Krystian Pradzynski <krystian.pradzynski at linux.intel.com>
>>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz at linux.intel.com>
>>
>> Reviewed-by: Jeffrey Hugo <quic_jhugo2quicinc.com>
>>
>>> +static const struct file_operations ivpu_fops = {
>>> +    .owner        = THIS_MODULE,
>>> +    .open        = accel_open,
>>> +    .release    = drm_release,
>>> +    .unlocked_ioctl    = drm_ioctl,
>>> +    .compat_ioctl    = drm_compat_ioctl,
>>> +    .poll        = drm_poll,
>>> +    .read        = drm_read,
>>> +    .llseek        = noop_llseek,
>>> +    .mmap           = drm_gem_mmap
>>> +};
>>
>> Hmm DEFINE_DRM_ACCEL_FOPS is not usable here because it doesn't define .mmap
>> Feels like we should fix that and then simplify this.  Seems like a good todo item.
> 
> I think this should rather be on accel/drm TODO and not ivpu TODO list.
> For the moment I can simplify the code to this:
> static const struct file_operations ivpu_fops = {
> 	.owner		= THIS_MODULE,
> 	.mmap           = drm_gem_mmap,
> 	DRM_ACCEL_FOPS,
> };
> 
> Still not perfect but nicer.
> 
> Regards,
> Jacek
> 

Yep, it looked like you could do that simplification but it didn't seem 
worth it to spin a v6 for this.  However since you seem to be planning a 
v6 anyways, rolling in this simplification would be nice.

Also, I'm sorry, I didn't mean to imply that the DEFINE_DRM_ACCEL_FOPS 
in on the ivpu todo.  I was thinking more in general terms.

Actually, I'll take that up and post a patch next week.  We'll see what 
the reaction is.  I don't intend for it to be a dependency for you. 
Feels like a quick thing to get done so it is not something the next 
person trips over (probably me) and we can link up all the pieces when 
both FOPS and ivpu are available.

-Jeff