Re: [PATCH v2 3/5] soc: visconti: Add Toshiba Visconti DNN image processing accelerator

[Greg KH]

On Tue, Jul 26, 2022 at 06:10:37AM +0000, yuji2.ishikawa@xxxxxxxxxxxxx wrote:
> Hi Greg
> 
> Thank you for your comments.
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Monday, July 25, 2022 9:51 PM
> > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> > <yuji2.ishikawa@xxxxxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Hans Verkuil <hverkuil@xxxxxxxxx>;
> > iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> > <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>;
> > Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Christian König
> > <christian.koenig@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx; linaro-mm-sig@xxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 3/5] soc: visconti: Add Toshiba Visconti DNN image
> > processing accelerator
> > 
> > On Fri, Jul 22, 2022 at 05:28:56PM +0900, Yuji Ishikawa wrote:
> > > --- /dev/null
> > > +++ b/drivers/soc/visconti/uapi/dnn.h
> > > @@ -0,0 +1,77 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/* Toshiba Visconti DNN Accelerator Support
> > > + *
> > > + * (C) Copyright 2022 TOSHIBA CORPORATION
> > > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage
> > > +Corporation  */
> > > +
> > > +#ifndef _UAPI_LINUX_DNN_H
> > > +#define _UAPI_LINUX_DNN_H
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/types.h>
> > > +#include "ipa.h"
> > > +
> > > +#define DRV_DNN_BIT_CONFIG_DESC_FINAL (0x8000U)
> > > +#define DRV_DNN_BUFFER_INDEX_MAX      (15)
> > > +
> > > +#define DRV_DNN_BASE_ADDR_NUM (8U) /* DNN number of base
> > address */
> > > +
> > > +#define DRV_DNN_BASE_ADDR_PURPOSE_INPUT	    (1U)
> > > +#define DRV_DNN_BASE_ADDR_PURPOSE_OUTPUT    (2U)
> > > +#define DRV_DNN_BASE_ADDR_PURPOSE_AWB	    (3U)
> > > +#define DRV_DNN_BASE_ADDR_PURPOSE_TEMPORARY (4U)
> > > +
> > > +/**
> > > + * struct drv_dnn_status - DNN IPA status for IOC_IPA_GET_STATUS
> > > + *
> > > + * @state:     State of driver
> > > + * @eer_cmd:   Execution error command
> > > + * @eer:       Execution error
> > > + * @reserved:  Padding
> > > + * @eer_flags: Execution error flags
> > > + */
> > > +struct drv_dnn_status {
> > > +	enum drv_ipa_state state;
> > > +	__u32 eer_cmd;
> > > +	__u32 eer : 1;
> > > +	__u32 reserved : 31;
> > 
> > bitfields will not work like this for uapi files, sorry.
> 
> I'll change the type of the member eer from bitfield to bool.

bool will not work for a user/kernel api structure at all, sorry.

> > > +	__u32 eer_flags[32];
> > 
> > What endian is all of these?  Big?  Little?  Unknown?
> 
> The processors and accelerators are little endian in Visconti SoC.
> Do I have to use more specific type such as __le32 ?

Of course, this has to be defined as to how the hardware sees it.  Why
wouldn't you specify this?

> > > +};
> > > +
> > > +struct drv_dnn_base_addr {
> > > +	__u32 purpose;
> > > +	union {
> > > +		struct drv_ipa_addr ipa_addr;
> > > +		uintptr_t list_addr;
> > 
> > You really do not ever want a uintptr_t in a uapi file, that's not going to be
> > portable at all.  It's also not a valid kernel type :(
> 
> I understand. The member list_addr should be typed "struct drv_ipa_addr*".

No, not at all, that too will not work and is not portable.  Please read
the documentation in the kernel for how to write correct user/kernel
apis with ioctl structures.  It is all documented there, please do not
ignore it and create an api that will be broken.

> > > + * @config_done:          Flags of called configuration
> > > + * @buffer_info:          Table of buffer information
> > > + * @buffer_info_num:      Number of buffer_info
> > > + */
> > > +struct drv_dnn_descriptor {
> > > +	struct drv_ipa_addr configuration;
> > > +	__u32 configuration_offset;
> > 
> > What endian are any of these?
> 
> They are little endian as processors and accelerators are LE.
> Do I have to use specific type such as __le32?

Yes, as that is defined by your hardware, not the processor the kernel
is running as.

> Do we need special care for endianness	when userland and kernel are sharing data (a drv_dnn_descriptor instance) ?

Yes, why wouldn't you?

> I thought there're no endianness problem when the driver is reading/writing HW's 32bit registers.

Is that what you are doing here?  It's impossible to tell.

For data that only crosses the user/kernel boundry, you can use the
native processor endian, but when it crosses the kernel/hardware
boundry, you HAVE to specify it as to what the hardware expects.

thanks,

greg k-h