Hi Dan,
thank you for your comments. I have to delay this change until I
cleanup our staging.
Regards,
Roman.
On Thu, Jun 28, 2018 at 8:51 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Try building this on a 32 bit system. I bet that's the build warnings
> which Greg saw...
>
> On Wed, Jun 27, 2018 at 02:50:52PM -0700, rkir@xxxxxxxxxx wrote:
> > From: Roman Kiryanov <rkir@xxxxxxxxxx>
> >
> > This is kernel driver for controlling the Goldfish sync
> > device on the host. It is used to maintain ordering
> > in critical OpenGL state changes while using
> > GPU emulation.
> >
> > The guest open()'s the Goldfish sync device to create
> > a context for possibly maintaining sync timeline and fences.
> > There is a 1:1 correspondence between such sync contexts
> > and OpenGL contexts in the guest that need synchronization
> > (which in turn, is anything involving swapping buffers,
> > SurfaceFlinger, or Hardware Composer).
> >
> > The ioctl QUEUE_WORK takes a handle to a sync object
> > and attempts to tell the host GPU to wait on the sync object
> > and deal with signaling it. It possibly outputs
> > a fence FD on which the Android systems that use them
> > (GLConsumer, SurfaceFlinger, anything employing
> > EGL_ANDROID_native_fence_sync) can use to wait.
> >
> > Signed-off-by: Roman Kiryanov <rkir@xxxxxxxxxx>
> > ---
> > drivers/staging/goldfish/Kconfig | 7 +
> > drivers/staging/goldfish/Makefile | 4 +
> > .../staging/goldfish/goldfish_sync_timeline.c | 985 ++++++++++++++++++
> > .../goldfish/goldfish_sync_timeline_fence.c | 260 +++++
> > .../goldfish/goldfish_sync_timeline_fence.h | 58 ++
> > include/uapi/linux/goldfish/goldfish_sync.h | 49 +
> > 6 files changed, 1363 insertions(+)
> > create mode 100644 drivers/staging/goldfish/goldfish_sync_timeline.c
> > create mode 100644 drivers/staging/goldfish/goldfish_sync_timeline_fence.c
> > create mode 100644 drivers/staging/goldfish/goldfish_sync_timeline_fence.h
> > create mode 100644 include/uapi/linux/goldfish/goldfish_sync.h
> >
> > diff --git a/drivers/staging/goldfish/Kconfig b/drivers/staging/goldfish/Kconfig
> > index 9165385df9de..59a7d26f5d3f 100644
> > --- a/drivers/staging/goldfish/Kconfig
> > +++ b/drivers/staging/goldfish/Kconfig
> > @@ -4,3 +4,10 @@ config GOLDFISH_AUDIO
> > help
> > Emulated audio channel for the Goldfish Android Virtual Device
> >
> > +config GOLDFISH_SYNC
> > + tristate "Goldfish AVD Sync Driver"
> > + depends on GOLDFISH
> > + depends on SW_SYNC
> > + depends on SYNC_FILE
> > + ---help---
> > + Emulated sync fences for the Goldfish Android Virtual Device
> > diff --git a/drivers/staging/goldfish/Makefile b/drivers/staging/goldfish/Makefile
> > index 054eeb82151e..781a81730d15 100644
> > --- a/drivers/staging/goldfish/Makefile
> > +++ b/drivers/staging/goldfish/Makefile
> > @@ -3,3 +3,7 @@
> > #
> >
> > obj-$(CONFIG_GOLDFISH_AUDIO) += goldfish_audio.o
> > +
> > +ccflags-y := -Idrivers/staging/android
> > +goldfish_sync-objs := goldfish_sync_timeline_fence.o goldfish_sync_timeline.o
> > +obj-$(CONFIG_GOLDFISH_SYNC) += goldfish_sync.o
> > diff --git a/drivers/staging/goldfish/goldfish_sync_timeline.c b/drivers/staging/goldfish/goldfish_sync_timeline.c
> > new file mode 100644
> > index 000000000000..33b17fd97499
> > --- /dev/null
> > +++ b/drivers/staging/goldfish/goldfish_sync_timeline.c
> > @@ -0,0 +1,985 @@
> > +/*
> > + * Copyright (C) 2018 Google, Inc.
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/fdtable.h>
> > +#include <linux/file.h>
> > +#include <linux/init.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/kref.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/io.h>
> > +#include <linux/mm.h>
> > +#include <linux/acpi.h>
> > +
> > +#include <linux/string.h>
> > +
> > +#include <linux/fs.h>
> > +#include <linux/syscalls.h>
> > +#include <linux/sync_file.h>
> > +#include <linux/dma-fence.h>
> > +
> > +#include <linux/goldfish.h>
> > +
> > +#include <uapi/linux/goldfish/goldfish_sync.h>
> > +
> > +#include "goldfish_sync_timeline_fence.h"
> > +
> > +/* The Goldfish sync driver is designed to provide a interface
> > + * between the underlying host's sync device and the kernel's
> > + * fence sync framework..
> > + * The purpose of the device/driver is to enable lightweight
> > + * creation and signaling of timelines and fences
> > + * in order to synchronize the guest with host-side graphics events.
> > + *
> > + * Each time the interrupt trips, the driver
> > + * may perform a sync operation.
> > + */
> > +
> > +/* The operations are: */
> > +enum cmd_id {
> > + /* Ready signal - used to mark when irq should lower */
> > + CMD_SYNC_READY = 0,
> > +
> > + /* Create a new timeline. writes timeline handle */
> > + CMD_CREATE_SYNC_TIMELINE = 1,
> > +
> > + /* Create a fence object. reads timeline handle and time argument.
> > + * Writes fence fd to the SYNC_REG_HANDLE register.
> > + */
> > + CMD_CREATE_SYNC_FENCE = 2,
> > +
> > + /* Increments timeline. reads timeline handle and time argument */
> > + CMD_SYNC_TIMELINE_INC = 3,
> > +
> > + /* Destroys a timeline. reads timeline handle */
> > + CMD_DESTROY_SYNC_TIMELINE = 4,
> > +
> > + /* Starts a wait on the host with
> > + * the given glsync object and sync thread handle.
> > + */
> > + CMD_TRIGGER_HOST_WAIT = 5,
> > +};
> > +
> > +/* The register layout is: */
> > +enum sync_reg_id {
> > + /* host->guest batch commands */
> > + SYNC_REG_BATCH_COMMAND = 0x00,
> > +
> > + /* guest->host batch commands */
> > + SYNC_REG_BATCH_GUESTCOMMAND = 0x04,
> > +
> > + /* communicate physical address of host->guest batch commands */
> > + SYNC_REG_BATCH_COMMAND_ADDR = 0x08,
> > +
> > + /* 64-bit part */
> > + SYNC_REG_BATCH_COMMAND_ADDR_HIGH = 0x0c,
> > +
> > + /* communicate physical address of guest->host commands */
> > + SYNC_REG_BATCH_GUESTCOMMAND_ADDR = 0x10,
> > +
> > + /* 64-bit part */
> > + SYNC_REG_BATCH_GUESTCOMMAND_ADDR_HIGH = 0x14,
> > +
> > + /* signals that the device has been probed */
> > + SYNC_REG_INIT = 0x18,
> > +};
> > +
> > +/* The above definitions (command codes, register layout, ioctl definitions)
> > + * need to be in sync with the following files:
> > + *
> > + * Host-side (emulator):
> > + * external/qemu/android/emulation/goldfish_sync.h
> > + * external/qemu-android/hw/misc/goldfish_sync.c
> > + *
> > + * Guest-side (system image):
> > + * device/generic/goldfish-opengl/system/egl/goldfish_sync.h
> > + * device/generic/goldfish/ueventd.ranchu.rc
> > + * platform/build/target/board/generic/sepolicy/file_contexts
> > + */
> > +struct goldfish_sync_hostcmd {
> > + /* sorted for alignment */
> > + u64 handle;
> > + u64 hostcmd_handle;
> > + u32 cmd;
> > + u32 time_arg;
> > +};
> > +
> > +struct goldfish_sync_guestcmd {
> > + u64 host_command;
> > + u64 glsync_handle;
> > + u64 thread_handle;
> > + u64 guest_timeline_handle;
> > +};
> > +
> > +#define GOLDFISH_SYNC_MAX_CMDS 32
> > +
> > +struct goldfish_sync_state {
> > + char __iomem *reg_base;
> > + int irq;
> > +
> > + /* Spinlock protects |to_do| / |to_do_end|. */
> > + spinlock_t lock;
> > +
> > + /* |mutex_lock| protects all concurrent access
> > + * to timelines for both kernel and user space.
> > + */
> > + struct mutex mutex_lock;
> > +
> > + /* Buffer holding commands issued from host. */
> > + struct goldfish_sync_hostcmd to_do[GOLDFISH_SYNC_MAX_CMDS];
> > + u32 to_do_end;
> > +
> > + /* Addresses for the reading or writing
> > + * of individual commands. The host can directly write
> > + * to |batch_hostcmd| (and then this driver immediately
> > + * copies contents to |to_do|). This driver either replies
> > + * through |batch_hostcmd| or simply issues a
> > + * guest->host command through |batch_guestcmd|.
> > + */
> > + struct goldfish_sync_hostcmd *batch_hostcmd;
> > + struct goldfish_sync_guestcmd *batch_guestcmd;
> > +
> > + /* Used to give this struct itself to a work queue
> > + * function for executing actual sync commands.
> > + */
> > + struct work_struct work_item;
> > +
> > + /* A pointer to struct device to use for logging */
> > + struct device *dev;
> > +};
> > +
> > +static struct goldfish_sync_state global_sync_state;
> > +
> > +struct goldfish_sync_timeline_obj {
> > + struct goldfish_sync_timeline *sync_tl;
> > + u32 current_time;
> > + /* We need to be careful about when we deallocate
> > + * this |goldfish_sync_timeline_obj| struct.
> > + * In order to ensure proper cleanup, we need to
> > + * consider the triggered host-side wait that may
> > + * still be in flight when the guest close()'s a
> > + * goldfish_sync device's sync context fd (and
> > + * destroys the |sync_tl| field above).
> > + * The host-side wait may raise IRQ
> > + * and tell the kernel to increment the timeline _after_
> > + * the |sync_tl| has already been set to null.
> > + *
> > + * From observations on OpenGL apps and CTS tests, this
> > + * happens at some very low probability upon context
> > + * destruction or process close, but it does happen
> > + * and it needs to be handled properly. Otherwise,
> > + * if we clean up the surrounding |goldfish_sync_timeline_obj|
> > + * too early, any |handle| field of any host->guest command
> > + * might not even point to a null |sync_tl| field,
> > + * but to garbage memory or even a reclaimed |sync_tl|.
> > + * If we do not count such "pending waits" and kfree the object
> > + * immediately upon |goldfish_sync_timeline_destroy|,
> > + * we might get mysterous RCU stalls after running a long
> > + * time because the garbage memory that is being read
> > + * happens to be interpretable as a |spinlock_t| struct
> > + * that is currently in the locked state.
> > + *
> > + * To track when to free the |goldfish_sync_timeline_obj|
> > + * itself, we maintain a kref.
> > + * The kref essentially counts the timeline itself plus
> > + * the number of waits in flight. kref_init/kref_put
> > + * are issued on
> > + * |goldfish_sync_timeline_create|/|goldfish_sync_timeline_destroy|
> > + * and kref_get/kref_put are issued on
> > + * |goldfish_sync_fence_create|/|goldfish_sync_timeline_inc|.
> > + *
> > + * The timeline is destroyed after reference count
> > + * reaches zero, which would happen after
> > + * |goldfish_sync_timeline_destroy| and all pending
> > + * |goldfish_sync_timeline_inc|'s are fulfilled.
> > + *
> > + * NOTE (1): We assume that |fence_create| and
> > + * |timeline_inc| calls are 1:1, otherwise the kref scheme
> > + * will not work. This is a valid assumption as long
> > + * as the host-side virtual device implementation
> > + * does not insert any timeline increments
> > + * that we did not trigger from here.
> > + *
> > + * NOTE (2): The use of kref by itself requires no locks,
> > + * but this does not mean everything works without locks.
> > + * Related timeline operations do require a lock of some sort,
> > + * or at least are not proven to work without it.
> > + * In particualr, we assume that all the operations
> > + * done on the |kref| field above are done in contexts where
> > + * |global_sync_state->mutex_lock| is held. Do not
> > + * remove that lock until everything is proven to work
> > + * without it!!!
> > + */
> > + struct kref kref;
> > +};
> > +
> > +/* We will call |delete_timeline_obj| when the last reference count
> > + * of the kref is decremented. This deletes the sync
> > + * timeline object along with the wrapper itself.
> > + */
> > +static void delete_timeline_obj(struct kref *kref)
> > +{
> > + struct goldfish_sync_timeline_obj *obj =
> > + container_of(kref, struct goldfish_sync_timeline_obj, kref);
> > +
> > + goldfish_sync_timeline_put_internal(obj->sync_tl);
> > + obj->sync_tl = NULL;
> ^^^^^^^^^^^^^^^^^^^
> Not required.
>
> > + kfree(obj);
> > +}
> > +
> > +static void gensym(char *dst, int size)
> > +{
> > + static u64 counter;
> > +
> > + snprintf(dst, size, "goldfish_sync:%s:%llu", __func__, counter);
> > + counter++;
> > +}
> > +
> > +/* |goldfish_sync_timeline_create| assumes that |global_sync_state->mutex_lock|
> > + * is held.
> > + */
> > +static struct goldfish_sync_timeline_obj*
> > +goldfish_sync_timeline_create(struct goldfish_sync_state *sync_state)
> > +{
> > + char timeline_name[64];
> > + struct goldfish_sync_timeline *res_sync_tl = NULL;
> > + struct goldfish_sync_timeline_obj *res;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + gensym(timeline_name, sizeof(timeline_name));
> > +
> > + res_sync_tl = goldfish_sync_timeline_create_internal(timeline_name);
> > + if (!res_sync_tl) {
> > + dev_err(sync_state->dev,
> > + "Failed to create goldfish_sw_sync timeline\n");
> > + return NULL;
> > + }
> > +
> > + res = kzalloc(sizeof(*res), GFP_KERNEL);
>
> if (!res) {
> goldfish_sync_timeline_put_internal(res_sync_tl);
> return NULL;
> }
>
> > + res->sync_tl = res_sync_tl;
> > + res->current_time = 0;
> > + kref_init(&res->kref);
> > +
> > + return res;
> > +}
> > +
> > +/* |goldfish_sync_fence_create| assumes that |global_sync_state->mutex_lock|
> > + * is held.
> > + */
> > +static int
> > +goldfish_sync_fence_create(struct goldfish_sync_state *sync_state,
> > + struct goldfish_sync_timeline_obj *obj,
> > + u32 val)
> > +{
> > + int fd;
> > + struct sync_pt *syncpt;
> > + struct sync_file *sync_file_obj;
> > + struct goldfish_sync_timeline *tl;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + if (!obj)
> > + return -1;
>
> This function is returning file handles... So I guess -1 is fine. None
> of the callers check for error. It makes me nervous and there are no
> comments.
>
> > +
> > + tl = obj->sync_tl;
> > +
> > + syncpt =
> > + goldfish_sync_pt_create_internal(tl,
> > + sizeof(struct sync_pt) + 4,
> > + val);
>
> The white space is whack.
>
> syncpt = goldfish_sync_pt_create_internal(tl,
> sizeof(struct sync_pt) + 4,
> val);
>
> > + if (!syncpt) {
> > + dev_err(sync_state->dev,
> > + "Could not create sync point, val=%d\n", val);
> > + return -1;
> > + }
> > +
> > + fd = get_unused_fd_flags(O_CLOEXEC);
> > + if (fd < 0) {
> > + dev_err(sync_state->dev,
> > + "Could not get unused fd for sync fence, "
> > + "errno=%d\n", fd);
> > + goto err_cleanup_pt;
> > + }
> > +
> > + sync_file_obj = sync_file_create(&syncpt->base);
> > + if (!sync_file_obj) {
> > + dev_err(sync_state->dev,
> > + "Could not create sync fence! val=%d\n", val);
> > + goto err_cleanup_fd_pt;
> > + }
> > +
> > + dev_info(sync_state->dev, "Installing sync fence into fd=%d\n", fd);
> > + fd_install(fd, sync_file_obj->file);
> > + kref_get(&obj->kref);
> > +
> > + return fd;
> > +
> > +err_cleanup_fd_pt:
> > + put_unused_fd(fd);
> > +err_cleanup_pt:
> > + dma_fence_put(&syncpt->base);
>
> We need to free "syncpt" as well. The goldfish_sync_pt_create_internal()
> should have a mirror free function instead of messing around with
> internals like &syncpt->base.
>
> > + return -1;
> > +}
> > +
> > +/* |goldfish_sync_timeline_inc| assumes that |global_sync_state->mutex_lock|
> > + * is held.
> > + */
> > +static void
> > +goldfish_sync_timeline_inc(struct goldfish_sync_state *sync_state,
> > + struct goldfish_sync_timeline_obj *obj, u32 inc)
> > +{
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + /* Just give up if someone else nuked the timeline.
> > + * Whoever it was won't care that it doesn't get signaled.
> > + */
> > + if (!obj)
> > + return;
> > +
> > + goldfish_sync_timeline_signal_internal(obj->sync_tl, inc);
> > + dev_info(sync_state->dev, "Incremented timeline, increment max_time\n");
> > + obj->current_time += inc;
> > +
> > + /* Here, we will end up deleting the timeline object if it
> > + * turns out that this call was a pending increment after
> > + * |goldfish_sync_timeline_destroy| was called.
> > + */
> > + kref_put(&obj->kref, delete_timeline_obj);
> > + dev_info(sync_state->dev, "done\n");
> > +}
> > +
> > +/* |goldfish_sync_timeline_destroy| assumes
> > + * that |global_sync_state->mutex_lock| is held.
> > + */
> > +static void
> > +goldfish_sync_timeline_destroy(struct goldfish_sync_state *sync_state,
> > + struct goldfish_sync_timeline_obj *obj)
> > +{
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > + /* See description of |goldfish_sync_timeline_obj| for why we
> > + * should not immediately destroy |obj|
> > + */
> > + kref_put(&obj->kref, delete_timeline_obj);
> > +}
> > +
> > +static int
> > +goldfish_sync_cmd_queue(struct goldfish_sync_state *sync_state,
> > + u32 cmd,
> > + u64 handle,
> > + u32 time_arg,
> > + u64 hostcmd_handle)
> > +{
> > + u32 to_do_end = sync_state->to_do_end;
>
> We don't need the "to_do_end" variable. Just use sync_state->to_do_end
> directly.
>
> > + struct goldfish_sync_hostcmd *to_add;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + if (to_do_end >= GOLDFISH_SYNC_MAX_CMDS)
> > + return -1;
>
> I really hate -1 as an error code. Except it's not really an error code
> here, it just means we're full.
>
> > +
> > + to_add = &sync_state->to_do[to_do_end];
> > +
> > + to_add->cmd = cmd;
> > + to_add->handle = handle;
> > + to_add->time_arg = time_arg;
> > + to_add->hostcmd_handle = hostcmd_handle;
> > +
> > + sync_state->to_do_end = to_do_end + 1;
> > + return 0;
> > +}
> > +
> > +static void
> > +goldfish_sync_hostcmd_reply(struct goldfish_sync_state *sync_state,
> > + u32 cmd,
> > + u64 handle,
> > + u32 time_arg,
> > + u64 hostcmd_handle)
> > +{
> > + struct goldfish_sync_hostcmd *batch_hostcmd =
> > + sync_state->batch_hostcmd;
> > + unsigned long irq_flags;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + spin_lock_irqsave(&sync_state->lock, irq_flags);
> > +
> > + batch_hostcmd->cmd = cmd;
> > + batch_hostcmd->handle = handle;
> > + batch_hostcmd->time_arg = time_arg;
> > + batch_hostcmd->hostcmd_handle = hostcmd_handle;
> > + writel(0, sync_state->reg_base + SYNC_REG_BATCH_COMMAND);
> > +
> > + spin_unlock_irqrestore(&sync_state->lock, irq_flags);
> > +}
> > +
> > +static void
> > +goldfish_sync_send_guestcmd(struct goldfish_sync_state *sync_state,
> > + u32 cmd,
> > + u64 glsync_handle,
> > + u64 thread_handle,
> > + u64 timeline_handle)
> > +{
> > + unsigned long irq_flags;
> > + struct goldfish_sync_guestcmd *batch_guestcmd =
> > + sync_state->batch_guestcmd;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + spin_lock_irqsave(&sync_state->lock, irq_flags);
> > +
> > + batch_guestcmd->host_command = cmd;
> > + batch_guestcmd->glsync_handle = glsync_handle;
> > + batch_guestcmd->thread_handle = thread_handle;
> > + batch_guestcmd->guest_timeline_handle = timeline_handle;
> > + writel(0, sync_state->reg_base + SYNC_REG_BATCH_GUESTCOMMAND);
> > +
> > + spin_unlock_irqrestore(&sync_state->lock, irq_flags);
> > +}
> > +
> > +/* |goldfish_sync_interrupt| handles IRQ raises from the virtual device.
> > + * In the context of OpenGL, this interrupt will fire whenever we need
> > + * to signal a fence fd in the guest, with the command
> > + * |CMD_SYNC_TIMELINE_INC|.
> > + * However, because this function will be called in an interrupt context,
> > + * it is necessary to do the actual work of signaling off of interrupt context.
> > + * The shared work queue is used for this purpose. At the end when
> > + * all pending commands are intercepted by the interrupt handler,
> > + * we call |schedule_work|, which will later run the actual
> > + * desired sync command in |goldfish_sync_work_item_fn|.
> > + */
> > +static irqreturn_t goldfish_sync_interrupt(int irq, void *dev_id)
> > +{
> > + struct goldfish_sync_state *sync_state = dev_id;
> > + int has_cmds = 0;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + spin_lock(&sync_state->lock);
> > + while (true) {
> > + struct goldfish_sync_hostcmd *batch_hostcmd;
> > + u32 nextcmd;
> > +
> > + readl(sync_state->reg_base + SYNC_REG_BATCH_COMMAND);
> > + batch_hostcmd = sync_state->batch_hostcmd;
> > + nextcmd = batch_hostcmd->cmd;
> > +
> > + if (nextcmd == CMD_SYNC_READY)
> > + break;
> > +
> > + if (goldfish_sync_cmd_queue(sync_state,
> > + nextcmd,
> > + batch_hostcmd->handle,
> > + batch_hostcmd->time_arg,
> > + batch_hostcmd->hostcmd_handle))
> > + break;
> > +
> > + has_cmds = 1;
> > + }
> > + spin_unlock(&sync_state->lock);
> > +
> > + if (has_cmds) {
> > + schedule_work(&sync_state->work_item);
> > + return IRQ_HANDLED;
> > + } else {
> > + return IRQ_NONE;
> > + }
>
> These days checkpatch.pl --strict insists that we write it like so:
>
> if (has_cmds) {
> schedule_work(&sync_state->work_item);
> return IRQ_HANDLED;
> }
> return IRQ_NONE;
>
> > +}
> > +
> > +static u32 get_commands_todo_locked(struct goldfish_sync_state *sync_state,
> > + struct goldfish_sync_hostcmd *to_run)
> > +{
> > + unsigned long irq_flags;
> > + u32 to_do_end;
> > + u32 i;
> > +
> > + spin_lock_irqsave(&sync_state->lock, irq_flags);
> > + to_do_end = sync_state->to_do_end;
> > +
> > + dev_info(sync_state->dev, "Num sync todos: %u\n", to_do_end);
> > +
> > + for (i = 0; i < to_do_end; i++)
> > + to_run[i] = sync_state->to_do[i];
> > +
> > + /* We expect that commands will come in at a slow enough rate
> > + * so that incoming items will not be more than
> > + * GOLDFISH_SYNC_MAX_CMDS.
> > + *
> > + * This is because the way the sync device is used,
> > + * it's only for managing buffer data transfers per frame,
> > + * with a sequential dependency between putting things in
> > + * to_do and taking them out. Once a set of commands is
> > + * queued up in to_do, the user of the device waits for
> > + * them to be processed before queuing additional commands,
> > + * which limits the rate at which commands come in
> > + * to the rate at which we take them out here.
> > + *
> > + * We also don't expect more than MAX_CMDS to be issued
> > + * at once; there is a correspondence between
> > + * which buffers need swapping to the (display / buffer queue)
> > + * to particular commands, and we don't expect there to be
> > + * enough display or buffer queues in operation at once
> > + * to overrun GOLDFISH_SYNC_MAX_CMDS.
> > + */
> > + sync_state->to_do_end = 0;
> > +
> > + spin_unlock_irqrestore(&sync_state->lock, irq_flags);
> > +
> > + return to_do_end;
> > +}
> > +
> > +void run_command_locked(const struct goldfish_sync_hostcmd *todo,
> > + struct goldfish_sync_state *sync_state)
> > +{
>
> It would simplify a lot of this function if you just did this at the
> start:
> void *handle = (void *)(unsigned long)todo->handle;
>
> Btw, I think your casting u64 to pointer will cause warnings on 32 bit
> compiles but I haven't tested.
>
>
> > + switch (todo->cmd) {
> > + case CMD_SYNC_READY:
> > + break;
> > +
> > + case CMD_CREATE_SYNC_TIMELINE:
> > + dev_info(sync_state->dev, "CMD_CREATE_SYNC_TIMELINE\n");
> > + {
> > + struct goldfish_sync_timeline_obj *timeline =
> > + goldfish_sync_timeline_create(sync_state);
> > +
> > + goldfish_sync_hostcmd_reply(sync_state,
> > + CMD_CREATE_SYNC_TIMELINE,
> > + (u64)timeline,
> > + 0,
> > + todo->hostcmd_handle);
> > + }
> > + break;
> > +
> > + case CMD_CREATE_SYNC_FENCE:
> > + dev_info(sync_state->dev, "CMD_CREATE_SYNC_FENCE\n");
> > + {
> > + struct goldfish_sync_timeline_obj *timeline =
> > + (struct goldfish_sync_timeline_obj *)
> > + todo->handle;
> > +
> > + int sync_fence_fd =
> > + goldfish_sync_fence_create(sync_state,
> > + timeline,
> > + todo->time_arg);
> > +
> > + goldfish_sync_hostcmd_reply(sync_state,
> > + CMD_CREATE_SYNC_FENCE,
> > + sync_fence_fd,
> > + 0,
> > + todo->hostcmd_handle);
> > + }
> > + break;
>
>
> We don't really need to dev_info() since most of this can be found with
> ftrace... We could pull it in a tab. Then it would look like this:
>
> case CMD_CREATE_SYNC_FENCE: {
> int fd = goldfish_sync_fence_create(sync_state, handle,
> todo->time_arg);
> goldfish_sync_hostcmd_reply(sync_state,
> CMD_CREATE_SYNC_FENCE,
> fd, 0, todo->hostcmd_handle);
> break;
> }
> case CMD_SYNC_TIMELINE_INC: {
>
>
>
> > +
> > + case CMD_SYNC_TIMELINE_INC:
> > + dev_info(sync_state->dev, "CMD_SYNC_TIMELINE_INC\n");
> > + {
> > + struct goldfish_sync_timeline_obj *timeline =
> > + (struct goldfish_sync_timeline_obj *)
> > + todo->handle;
> > +
> > + goldfish_sync_timeline_inc(sync_state,
> > + timeline,
> > + todo->time_arg);
> > + }
> > + break;
> > +
> > + case CMD_DESTROY_SYNC_TIMELINE:
> > + dev_info(sync_state->dev, "CMD_DESTROY_SYNC_TIMELINE\n");
> > + {
> > + struct goldfish_sync_timeline_obj *timeline =
> > + (struct goldfish_sync_timeline_obj *)
> > + todo->handle;
> > +
> > + goldfish_sync_timeline_destroy(sync_state, timeline);
> > + }
> > + break;
> > +
> > + default:
> > + dev_err(sync_state->dev, "Unexpected command: %u\n", todo->cmd);
> > + break;
> > + }
> > +
> > + dev_info(sync_state->dev, "Done executing sync command\n");
>
> This should be dev_dbg() or deleted.
>
> > +}
> > +
> > +void run_commands_locked(struct goldfish_sync_state *sync_state,
> > + struct goldfish_sync_hostcmd *to_run,
> > + u32 to_do_end)
> > +{
> > + u32 i;
> > +
> > + for (i = 0; i < to_do_end; ++i) {
> > + dev_info(sync_state->dev, "todo index: %u\n", i);
>
> Too spammy.
>
> > + run_command_locked(&to_run[i], sync_state);
> > + }
> > +}
> > +
> > +/* |goldfish_sync_work_item_fn| does the actual work of servicing
> > + * host->guest sync commands. This function is triggered whenever
> > + * the IRQ for the goldfish sync device is raised. Once it starts
> > + * running, it grabs the contents of the buffer containing the
> > + * commands it needs to execute (there may be multiple, because
> > + * our IRQ is active high and not edge triggered), and then
> > + * runs all of them one after the other.
> > + */
> > +static void goldfish_sync_work_item_fn(struct work_struct *input)
> > +{
> > + struct goldfish_sync_state *sync_state =
> > + container_of(input, struct goldfish_sync_state, work_item);
> > + struct goldfish_sync_hostcmd to_run[GOLDFISH_SYNC_MAX_CMDS];
> > + u32 to_do_end;
> > +
> > + mutex_lock(&sync_state->mutex_lock);
> > + to_do_end = get_commands_todo_locked(sync_state, to_run);
> > + run_commands_locked(sync_state, to_run, to_do_end);
> > + mutex_unlock(&sync_state->mutex_lock);
> > +}
> > +
> > +/* Guest-side interface: file operations */
> > +
> > +/* Goldfish sync context and ioctl info.
> > + *
> > + * When a sync context is created by open()-ing the goldfish sync device, we
> > + * create a sync context (|goldfish_sync_context|).
> > + *
> > + * Currently, the only data required to track is the sync timeline itself
> > + * along with the current time, which are all packed up in the
> > + * |goldfish_sync_timeline_obj| field. We use a |goldfish_sync_context|
> > + * as the filp->private_data.
> > + *
> > + * Next, when a sync context user requests that work be queued and a fence
> > + * fd provided, we use the |goldfish_sync_ioctl_info| struct, which holds
> > + * information about which host handles to touch for this particular
> > + * queue-work operation. We need to know about the host-side sync thread
> > + * and the particular host-side GLsync object. We also possibly write out
> > + * a file descriptor.
> > + */
> > +struct goldfish_sync_context {
> > + struct goldfish_sync_timeline_obj *timeline;
> > +};
> > +
> > +static int goldfish_sync_open(struct inode *inode, struct file *file)
> > +{
> > + struct goldfish_sync_context *sync_context;
> > +
> > + dev_dbg(global_sync_state.dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + mutex_lock(&global_sync_state.mutex_lock);
> > +
> > + sync_context = kzalloc(sizeof(*sync_context), GFP_ATOMIC);
> > + if (!sync_context) {
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > + return -ENOMEM;
> > + }
> > +
> > + sync_context->timeline = NULL;
> > +
> > + file->private_data = sync_context;
> > +
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int goldfish_sync_release(struct inode *inode, struct file *file)
> > +{
> > + struct goldfish_sync_context *sync_context;
> > +
> > + dev_dbg(global_sync_state.dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + mutex_lock(&global_sync_state.mutex_lock);
> > +
> > + sync_context = file->private_data;
> > +
> > + if (sync_context->timeline)
> > + goldfish_sync_timeline_destroy(&global_sync_state,
> > + sync_context->timeline);
> > +
> > + kfree(sync_context);
> > +
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > +
> > + return 0;
> > +}
> > +
> > +/* |goldfish_sync_ioctl| is the guest-facing interface of goldfish sync
> > + * and is used in conjunction with eglCreateSyncKHR to queue up the
> > + * actual work of waiting for the EGL sync command to complete,
> > + * possibly returning a fence fd to the guest.
> > + */
> > +static long goldfish_sync_ioctl(struct file *file,
> > + unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct device *dev = global_sync_state.dev;
> > + struct goldfish_sync_context *sync_context_data;
> > + struct goldfish_sync_timeline_obj *timeline;
> > + struct goldfish_sync_ioctl_info ioctl_data;
> > + int fd_out;
> > + u32 current_time;
> > +
> > + sync_context_data = file->private_data;
> > + fd_out = -1;
>
> No need to initialize this to a bogus value.
>
> > +
> > + switch (cmd) {
> > + case GOLDFISH_SYNC_IOC_QUEUE_WORK:
> > + dev_info(dev, "exec GOLDFISH_SYNC_IOC_QUEUE_WORK\n");
> > +
> > + mutex_lock(&global_sync_state.mutex_lock);
> > +
> > + if (copy_from_user(&ioctl_data,
> > + (void __user *)arg,
> > + sizeof(ioctl_data))) {
> > + dev_err(dev,
> > + "Failed to copy memory for ioctl_data from user\n");
>
> Copy from user shouldn't be able to trigger a dev_err() or the user
> can DoS /var/log/messages. Please check all the dev_info() output as
> well.
>
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > + return -EFAULT;
> > + }
> > +
> > + if (ioctl_data.host_syncthread_handle_in == 0) {
> > + dev_err(dev, "Error: zero host syncthread handle\n");
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > + return -EFAULT;
>
> -EINVAL;
>
> > + }
> > +
> > + timeline = sync_context_data->timeline;
> > + if (!timeline) {
> > + dev_info(dev, "No timeline yet, create one\n");
> > + timeline = goldfish_sync_timeline_create(&global_sync_state);
> > + sync_context_data->timeline = timeline;
> > + }
> > +
> > + current_time = timeline->current_time + 1;
> > +
> > + fd_out = goldfish_sync_fence_create(&global_sync_state,
> > + timeline,
> > + current_time);
> > + dev_info(dev, "Created fence with fd %d and current time %u\n",
> > + fd_out, current_time);
> > +
> > + ioctl_data.fence_fd_out = fd_out;
> > +
> > + if (copy_to_user((void __user *)arg,
> > + &ioctl_data,
> > + sizeof(ioctl_data))) {
> > + dev_err(dev, "copy_to_user failed\n");
> > +
> > + ksys_close(fd_out);
> > + /* We won't be doing an increment,
> > + * kref_put immediately.
> > + */
> > + kref_put(&timeline->kref, delete_timeline_obj);
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > + return -EFAULT;
> > + }
> > +
> > + /* We are now about to trigger a host-side wait;
> > + * accumulate on |pending_waits|.
> > + */
> > + goldfish_sync_send_guestcmd(&global_sync_state,
> > + CMD_TRIGGER_HOST_WAIT,
> > + ioctl_data.host_glsync_handle_in,
> > + ioctl_data.host_syncthread_handle_in,
> > + (u64)timeline);
> > +
> > + mutex_unlock(&global_sync_state.mutex_lock);
> > + return 0;
> > +
> > + default:
> > + dev_err(dev, "Unexpected ioctl command: %u\n", cmd);
> > + return -ENOTTY;
> > + }
> > +}
> > +
> > +static const struct file_operations goldfish_sync_fops = {
> > + .owner = THIS_MODULE,
> > + .open = goldfish_sync_open,
> > + .release = goldfish_sync_release,
> > + .unlocked_ioctl = goldfish_sync_ioctl,
> > + .compat_ioctl = goldfish_sync_ioctl,
> > +};
> > +
> > +static struct miscdevice goldfish_sync_device = {
> > + .name = "goldfish_sync",
> > + .fops = &goldfish_sync_fops,
> > +};
> > +
> > +static bool setup_verify_batch_cmd_addr(struct goldfish_sync_state *sync_state,
> > + void *batch_addr,
> > + u32 addr_offset,
> > + u32 addr_offset_high)
> > +{
> > + u64 batch_addr_phys;
> > + u64 batch_addr_phys_test;
>
> These variables are too long so you end up hitting the 80 char limit.
>
> > +
> > + if (!batch_addr) {
> > + dev_err(sync_state->dev,
> > + "Could not use batch command address\n");
> > + return false;
> > + }
> > +
> > + batch_addr_phys = virt_to_phys(batch_addr);
> > + gf_write_u64(batch_addr_phys,
> > + sync_state->reg_base + addr_offset,
> > + sync_state->reg_base + addr_offset_high);
>
>
> Get rid of the batch_addr_phys variable.
>
> gf_write_u64(virt_to_phys(batch_addr),
> sync_state->reg_base + addr_offset,
> sync_state->reg_base + addr_offset_high);
>
>
> > +
> > + batch_addr_phys_test =
> > + gf_read_u64(sync_state->reg_base + addr_offset,
> > + sync_state->reg_base + addr_offset_high);
> > +
> > + if (virt_to_phys(batch_addr) != batch_addr_phys_test) {
> > + dev_err(sync_state->dev, "Invalid batch command address\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +int goldfish_sync_probe(struct platform_device *pdev)
> > +{
> > + struct goldfish_sync_state *sync_state = &global_sync_state;
> > + struct device *dev = &pdev->dev;
> > + struct resource *ioresource;
> > + int status;
> > +
> > + dev_dbg(sync_state->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > + sync_state->dev = dev;
> > + sync_state->to_do_end = 0;
> > +
> > + spin_lock_init(&sync_state->lock);
> > + mutex_init(&sync_state->mutex_lock);
> > +
> > + platform_set_drvdata(pdev, sync_state);
> > +
> > + ioresource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!ioresource) {
> > + dev_err(dev, "platform_get_resource failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + sync_state->reg_base =
> > + devm_ioremap(dev, ioresource->start, PAGE_SIZE);
>
> This can fit on one line. It's exactly 80 char.
>
> > + if (!sync_state->reg_base) {
> > + dev_err(dev, "devm_ioremap failed\n");
> > + return -ENOMEM;
> > + }
> > +
> > + sync_state->irq = platform_get_irq(pdev, 0);
> > + if (sync_state->irq < 0) {
> > + dev_err(dev, "platform_get_irq failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + status = devm_request_irq(dev,
> > + sync_state->irq,
> > + goldfish_sync_interrupt,
> > + IRQF_SHARED,
> > + pdev->name,
> > + sync_state);
> > + if (status) {
> > + dev_err(dev, "devm_request_irq failed\n");
> > + return -ENODEV;
>
> Preserve the error code "return status;".
>
> > + }
> > +
> > + INIT_WORK(&sync_state->work_item,
> > + goldfish_sync_work_item_fn);
> > +
> > + misc_register(&goldfish_sync_device);
> > +
> > + /* Obtain addresses for batch send/recv of commands. */
> > + {
> > + struct goldfish_sync_hostcmd *batch_addr_hostcmd;
> > + struct goldfish_sync_guestcmd *batch_addr_guestcmd;
>
> Put these at the start of the function and pull everything back a tab.
>
> > +
> > + batch_addr_hostcmd =
> > + devm_kzalloc(dev, sizeof(*batch_addr_hostcmd),
> > + GFP_KERNEL);
> > + batch_addr_guestcmd =
> > + devm_kzalloc(dev, sizeof(*batch_addr_guestcmd),
> > + GFP_KERNEL);
> > +
> > + if (!setup_verify_batch_cmd_addr(sync_state,
> > + batch_addr_hostcmd,
> > + SYNC_REG_BATCH_COMMAND_ADDR,
> > + SYNC_REG_BATCH_COMMAND_ADDR_HIGH)) {
> > + dev_err(dev, "Could not setup batch command address\n");
>
> We need to misc_deregister(&goldfish_sync_device); on these error paths.
>
> > + return -ENODEV;
> > + }
> > +
> > + if (!setup_verify_batch_cmd_addr(sync_state,
> > + batch_addr_guestcmd,
> > + SYNC_REG_BATCH_GUESTCOMMAND_ADDR,
> > + SYNC_REG_BATCH_GUESTCOMMAND_ADDR_HIGH)) {
> > + dev_err(dev, "Could not setup batch guest command address\n");
> > + return -ENODEV;
> > + }
> > +
> > + sync_state->batch_hostcmd = batch_addr_hostcmd;
> > + sync_state->batch_guestcmd = batch_addr_guestcmd;
> > + }
> > +
> > + dev_info(dev, "goldfish_sync: Initialized goldfish sync device\n");
> > +
> > + writel(0, sync_state->reg_base + SYNC_REG_INIT);
> > +
> > + return 0;
> > +}
> > +
>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
