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

[Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>]

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@linux.intel.com>
>> Signed-off-by: Krystian Pradzynski <krystian.pradzynski@linux.intel.com>
>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@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