Re: Plumbing explicit synchronization through the Linux ecosystem

[Jason Ekstrand]

On Mon, Mar 16, 2020 at 6:39 PM Roman Gilg  wrote:
>
> On Wed, Mar 11, 2020 at 8:21 PM Jason Ekstrand  wrote:
> >
> > On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  
> > wrote:
> > >
> > > All,
> > >
> > > Sorry for casting such a broad net with this one. I'm sure most people
> > > who reply will get at least one mailing list rejection.  However, this
> > > is an issue that affects a LOT of components and that's why it's
> > > thorny to begin with.  Please pardon the length of this e-mail as
> > > well; I promise there's a concrete point/proposal at the end.
> > >
> > >
> > > Explicit synchronization is the future of graphics and media.  At
> > > least, that seems to be the consensus among all the graphics people
> > > I've talked to.  I had a chat with one of the lead Android graphics
> > > engineers recently who told me that doing explicit sync from the start
> > > was one of the best engineering decisions Android ever made.  It's
> > > also the direction being taken by more modern APIs such as Vulkan.
> > >
> > >
> > > ## What are implicit and explicit synchronization?
> > >
> > > For those that aren't familiar with this space, GPUs, media encoders,
> > > etc. are massively parallel and synchronization of some form is
> > > required to ensure that everything happens in the right order and
> > > avoid data races.  Implicit synchronization is when bits of work (3D,
> > > compute, video encode, etc.) are implicitly based on the absolute
> > > CPU-time order in which API calls occur.  Explicit synchronization is
> > > when the client (whatever that means in any given context) provides
> > > the dependency graph explicitly via some sort of synchronization
> > > primitives.  If you're still confused, consider the following
> > > examples:
> > >
> > > With OpenGL and EGL, almost everything is implicit sync.  Say you have
> > > two OpenGL contexts sharing an image where one writes to it and the
> > > other textures from it.  The way the OpenGL spec works, the client has
> > > to make the API calls to render to the image before (in CPU time) it
> > > makes the API calls which texture from the image.  As long as it does
> > > this (and maybe inserts a glFlush?), the driver will ensure that the
> > > rendering completes before the texturing happens and you get correct
> > > contents.
> > >
> > > Implicit synchronization can also happen across processes.  Wayland,
> > > for instance, is currently built on implicit sync where the client
> > > does their rendering and then does a hand-off (via wl_surface::commit)
> > > to tell the compositor it's done at which point the compositor can now
> > > texture from the surface.  The hand-off ensures that the client's
> > > OpenGL API calls happen before the server's OpenGL API calls.
> > >
> > > A good example of explicit synchronization is the Vulkan API.  There,
> > > a client (or multiple clients) can simultaneously build command
> > > buffers in different threads where one of those command buffers
> > > renders to an image and the other textures from it and then submit
> > > both of them at the same time with instructions to the driver for
> > > which order to execute them in.  The execution order is described via
> > > the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
> > > extension, you can even submit the work which does the texturing
> > > BEFORE the work which does the rendering and the driver will sort it
> > > out.
> > >
> > > The #1 problem with implicit synchronization (which explicit solves)
> > > is that it leads to a lot of over-synchronization both in client space
> > > and in driver/device space.  The client has to synchronize a lot more
> > > because it has to ensure that the API calls happen in a particular
> > > order.  The driver/device have to synchronize a lot more because they
> > > never know what is going to end up being a synchronization point as an
> > > API call on another thread/process may occur at any time.  As we move
> > > to more and more multi-threaded programming this synchronization (on
> > > the client-side especially) becomes more and more painful.
> > >
> > >
> > > ## Current status in Linux
> > >
> > > Implicit synchronization in Linux works via a the kernel's internal
> > > dma_buf and dma_fence data structures.  A dma_fence is a tiny object
> > > which represents the "done" status for some bit of work.  Typically,
> > > dma_fences are created as a by-product of someone submitting some bit
> > > of work (say, 3D rendering) to the kernel.  The dma_buf object has a
> > > set of dma_fences on it representing shared (read) and exclusive
> > > (write) access to the object.  When work is submitted which, for
> > > instance renders to the dma_buf, it's queued waiting on all the fences
> > > on the dma_buf and and a dma_fence is created representing the end of
> > > said rendering work and it's installed as the dma_buf's exclusive
> > > fence.  This way, the kernel can manage all its internal queues (3D
> > > 

Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper

[james qian wang (Arm Technology China)]

On Wed, Mar 11, 2020 at 10:55:37PM +0800, Andrzej Pietrasiewicz wrote:
> The new struct contains afbc-specific data.
> 
> The new function can be used by drivers which support afbc to complete
> the preparation of struct drm_afbc_framebuffer. It must be called after
> allocating the said struct and calling drm_gem_fb_init_with_funcs().
> 
> Signed-off-by: Andrzej Pietrasiewicz 

Looks good to me.

Reviewed-by: James Qian Wang 

Thanks
James
> ---
>  Documentation/gpu/todo.rst   |  15 +++
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 108 +++
>  include/drm/drm_framebuffer.h|  45 
>  include/drm/drm_gem_framebuffer_helper.h |  10 ++
>  4 files changed, 178 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 439656f55c5d..37a3a023c114 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers
>  
>  Level: Intermediate
>  
> +Encode cpp properly in malidp
> +-
> +
> +cpp (chars per pixel) is not encoded properly in malidp, zero is
> +used instead. afbc implementation needs bpp or cpp, but if it is
> +zero it needs to be provided elsewhere, and so the bpp field exists
> +in struct drm_afbc_framebuffer.
> +
> +Properly encode cpp in malidp and remove the bpp field in struct
> +drm_afbc_framebuffer.
> +
> +Contact: malidp maintainers
> +
> +Level: Intermediate
> +
>  Core refactorings
>  =
>  
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c 
> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 86c1907c579a..7e3982c36baa 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -21,6 +21,13 @@
>  #include 
>  #include 
>  
> +#define AFBC_HEADER_SIZE 16
> +#define AFBC_TH_LAYOUT_ALIGNMENT 8
> +#define AFBC_HDR_ALIGN   64
> +#define AFBC_SUPERBLOCK_PIXELS   256
> +#define AFBC_SUPERBLOCK_ALIGNMENT128
> +#define AFBC_TH_BODY_START_ALIGNMENT 4096
> +
>  /**
>   * DOC: overview
>   *
> @@ -302,6 +309,107 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, 
> struct drm_file *file,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
>  
> +static int drm_gem_afbc_min_size(struct drm_device *dev,
> +  const struct drm_mode_fb_cmd2 *mode_cmd,
> +  struct drm_afbc_framebuffer *afbc_fb)
> +{
> + const struct drm_format_info *info;
> + __u32 n_blocks, w_alignment, h_alignment, hdr_alignment;
> + /* remove bpp when all users properly encode cpp in drm_format_info */
> + __u32 bpp;
> +
> + switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> + afbc_fb->block_width = 16;
> + afbc_fb->block_height = 16;
> + break;
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> + afbc_fb->block_width = 32;
> + afbc_fb->block_height = 8;
> + break;
> + /* no user exists yet - fall through */
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> + default:
> + DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> +   mode_cmd->modifier[0]
> +   & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> + return -EINVAL;
> + }
> +
> + /* tiled header afbc */
> + w_alignment = afbc_fb->block_width;
> + h_alignment = afbc_fb->block_height;
> + hdr_alignment = AFBC_HDR_ALIGN;
> + if (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_TILED) {
> + w_alignment *= AFBC_TH_LAYOUT_ALIGNMENT;
> + h_alignment *= AFBC_TH_LAYOUT_ALIGNMENT;
> + hdr_alignment = AFBC_TH_BODY_START_ALIGNMENT;
> + }
> +
> + afbc_fb->aligned_width = ALIGN(mode_cmd->width, w_alignment);
> + afbc_fb->aligned_height = ALIGN(mode_cmd->height, h_alignment);
> + afbc_fb->offset = mode_cmd->offsets[0];
> +
> + info = drm_get_format_info(dev, mode_cmd);
> + /*
> +  * Change to always using info->cpp[0]
> +  * when all users properly encode it
> +  */
> + bpp = info->cpp[0] ? info->cpp[0] * 8 : afbc_fb->bpp;
> +
> + n_blocks = (afbc_fb->aligned_width * afbc_fb->aligned_height)
> +/ AFBC_SUPERBLOCK_PIXELS;
> + afbc_fb->afbc_size = ALIGN(n_blocks * AFBC_HEADER_SIZE, hdr_alignment);
> + afbc_fb->afbc_size += n_blocks * ALIGN(bpp * AFBC_SUPERBLOCK_PIXELS / 8,
> +AFBC_SUPERBLOCK_ALIGNMENT);
> +
> + return 0;
> +}
> +
> +/**
> + * drm_gem_fb_afbc_init() - Helper function for drivers using afbc to
> + *   fill and validate all the afbc-specific
> + *   struct drm_afbc_framebuffer members
> + *
> + 

Re: [PATCHv7 1/6] drm/core: Allow drivers allocate a subclass of struct drm_framebuffer

[james qian wang (Arm Technology China)]

On Wed, Mar 11, 2020 at 10:55:36PM +0800, Andrzej Pietrasiewicz wrote:
> Allow allocating a specialized version of struct drm_framebuffer
> by moving the actual fb allocation out of drm_gem_fb_create_with_funcs();
> the respective functions names are adjusted to reflect that fact.
> Please note, though, that standard size checks are performed on buffers,
> so the drm_gem_fb_init_with_funcs() is useful for cases where those
> standard size checks are appropriate or at least don't conflict the
> checks to be performed in the specialized case.
> 
> Thanks to this change the drivers can call drm_gem_fb_init_with_funcs()
> having allocated their special version of struct drm_framebuffer, exactly
> the way the new version of drm_gem_fb_create_with_funcs() does.
> 
> Signed-off-by: Andrzej Pietrasiewicz 

Looks good to me. :)

Reviewed-by: James Qian Wang 

Thanks
James

> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 87 ++--
>  include/drm/drm_gem_framebuffer_helper.h |  5 ++
>  2 files changed, 67 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c 
> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 3a7ace19a902..86c1907c579a 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -54,19 +54,15 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct 
> drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_get_obj);
>  
> -static struct drm_framebuffer *
> -drm_gem_fb_alloc(struct drm_device *dev,
> +static int
> +drm_gem_fb_init(struct drm_device *dev,
> +  struct drm_framebuffer *fb,
>const struct drm_mode_fb_cmd2 *mode_cmd,
>struct drm_gem_object **obj, unsigned int num_planes,
>const struct drm_framebuffer_funcs *funcs)
>  {
> - struct drm_framebuffer *fb;
>   int ret, i;
>  
> - fb = kzalloc(sizeof(*fb), GFP_KERNEL);
> - if (!fb)
> - return ERR_PTR(-ENOMEM);
> -
>   drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>  
>   for (i = 0; i < num_planes; i++)
> @@ -76,10 +72,9 @@ drm_gem_fb_alloc(struct drm_device *dev,
>   if (ret) {
>   drm_err(dev, "Failed to init framebuffer: %d\n", ret);
>   kfree(fb);
> - return ERR_PTR(ret);
>   }
>  
> - return fb;
> + return ret;
>  }
>  
>  /**
> @@ -123,10 +118,13 @@ int drm_gem_fb_create_handle(struct drm_framebuffer 
> *fb, struct drm_file *file,
>  EXPORT_SYMBOL(drm_gem_fb_create_handle);
>  
>  /**
> - * drm_gem_fb_create_with_funcs() - Helper function for the
> - *  _mode_config_funcs.fb_create
> - *  callback
> + * drm_gem_fb_init_with_funcs() - Helper function for implementing
> + * _mode_config_funcs.fb_create
> + * callback in cases when the driver
> + * allocates a subclass of
> + * struct drm_framebuffer
>   * @dev: DRM device
> + * @fb: framebuffer object
>   * @file: DRM file that holds the GEM handle(s) backing the framebuffer
>   * @mode_cmd: Metadata from the userspace framebuffer creation request
>   * @funcs: vtable to be used for the new framebuffer object
> @@ -134,23 +132,26 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle);
>   * This function can be used to set _framebuffer_funcs for drivers that 
> need
>   * custom framebuffer callbacks. Use drm_gem_fb_create() if you don't need to
>   * change _framebuffer_funcs. The function does buffer size validation.
> + * The buffer size validation is for a general case, though, so users should
> + * pay attention to the checks being appropriate for them or, at least,
> + * non-conflicting.
>   *
>   * Returns:
> - * Pointer to a _framebuffer on success or an error pointer on failure.
> + * Zero or a negative error code.
>   */
> -struct drm_framebuffer *
> -drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
> -  const struct drm_mode_fb_cmd2 *mode_cmd,
> -  const struct drm_framebuffer_funcs *funcs)
> +int drm_gem_fb_init_with_funcs(struct drm_device *dev,
> +struct drm_framebuffer *fb,
> +struct drm_file *file,
> +const struct drm_mode_fb_cmd2 *mode_cmd,
> +const struct drm_framebuffer_funcs *funcs)
>  {
>   const struct drm_format_info *info;
>   struct drm_gem_object *objs[4];
> - struct drm_framebuffer *fb;
>   int ret, i;
>  
>   info = drm_get_format_info(dev, mode_cmd);
>   if (!info)
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
>  
>   for (i = 0; i < info->num_planes; i++) {
>   unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
> @@ -175,19 +176,55 @@ drm_gem_fb_create_with_funcs(struct 

Re: [PATCH] drm/lima: add trace point for tasks

[Qiang Yu]

On Sun, Mar 15, 2020 at 12:36 PM Vasily Khoruzhick  wrote:
>
> On Sat, Mar 7, 2020 at 5:55 AM Qiang Yu  wrote:
> >
> > track lima task start which can be combined with
> > dma_fence_signal to identify task execution time.
> >
> > example command to record:
> >
> > trace-cmd record -i \
> >   -e "lima:lima_task_submit" -e "lima:lima_task_run" \
> >   -e "*fence:*fence_signaled" -e "drm:drm_vblank_event" \
> >   -e "drm:drm_vblank_event_queued" sleep 4
>
> LGTM. Out of curiosity, is there any reason for not adding one more
> event for task completion?

Not concrete reason, as the comment, trace point when
dma_fence_signal act as the task completion event, so not add duplicate
one.

Regards,
Qiang

>
> > Signed-off-by: Qiang Yu 
> > ---
> >  drivers/gpu/drm/lima/Makefile |  3 +-
> >  drivers/gpu/drm/lima/lima_sched.c |  5 +++-
> >  drivers/gpu/drm/lima/lima_sched.h |  1 +
> >  drivers/gpu/drm/lima/lima_trace.c |  7 +
> >  drivers/gpu/drm/lima/lima_trace.h | 50 +++
> >  5 files changed, 64 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/gpu/drm/lima/lima_trace.c
> >  create mode 100644 drivers/gpu/drm/lima/lima_trace.h
> >
> > diff --git a/drivers/gpu/drm/lima/Makefile b/drivers/gpu/drm/lima/Makefile
> > index a85444b0a1d4..6e7b788408e8 100644
> > --- a/drivers/gpu/drm/lima/Makefile
> > +++ b/drivers/gpu/drm/lima/Makefile
> > @@ -14,6 +14,7 @@ lima-y := \
> > lima_sched.o \
> > lima_ctx.o \
> > lima_dlbu.o \
> > -   lima_bcast.o
> > +   lima_bcast.o \
> > +   lima_trace.o
> >
> >  obj-$(CONFIG_DRM_LIMA) += lima.o
> > diff --git a/drivers/gpu/drm/lima/lima_sched.c 
> > b/drivers/gpu/drm/lima/lima_sched.c
> > index f295479e3733..98d0d410b7d7 100644
> > --- a/drivers/gpu/drm/lima/lima_sched.c
> > +++ b/drivers/gpu/drm/lima/lima_sched.c
> > @@ -3,7 +3,6 @@
> >
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >
> >  #include "lima_drv.h"
> > @@ -12,6 +11,7 @@
> >  #include "lima_mmu.h"
> >  #include "lima_l2_cache.h"
> >  #include "lima_gem.h"
> > +#include "lima_trace.h"
> >
> >  struct lima_fence {
> > struct dma_fence base;
> > @@ -177,6 +177,7 @@ struct dma_fence *lima_sched_context_queue_task(struct 
> > lima_sched_context *conte
> >  {
> > struct dma_fence *fence = 
> > dma_fence_get(>base.s_fence->finished);
> >
> > +   trace_lima_task_submit(task);
> > drm_sched_entity_push_job(>base, >base);
> > return fence;
> >  }
> > @@ -251,6 +252,8 @@ static struct dma_fence *lima_sched_run_job(struct 
> > drm_sched_job *job)
> > if (last_vm)
> > lima_vm_put(last_vm);
> >
> > +   trace_lima_task_run(task);
> > +
> > pipe->error = false;
> > pipe->task_run(pipe, task);
> >
> > diff --git a/drivers/gpu/drm/lima/lima_sched.h 
> > b/drivers/gpu/drm/lima/lima_sched.h
> > index e29f5e3b675b..e5db1919f446 100644
> > --- a/drivers/gpu/drm/lima/lima_sched.h
> > +++ b/drivers/gpu/drm/lima/lima_sched.h
> > @@ -6,6 +6,7 @@
> >
> >  #include 
> >  #include 
> > +#include 
> >
> >  struct lima_vm;
> >
> > diff --git a/drivers/gpu/drm/lima/lima_trace.c 
> > b/drivers/gpu/drm/lima/lima_trace.c
> > new file mode 100644
> > index ..ea1c7289bebc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/lima/lima_trace.c
> > @@ -0,0 +1,7 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright 2020 Qiang Yu  */
> > +
> > +#include "lima_sched.h"
> > +
> > +#define CREATE_TRACE_POINTS
> > +#include "lima_trace.h"
> > diff --git a/drivers/gpu/drm/lima/lima_trace.h 
> > b/drivers/gpu/drm/lima/lima_trace.h
> > new file mode 100644
> > index ..9308b948b69d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/lima/lima_trace.h
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright 2020 Qiang Yu  */
> > +
> > +#if !defined(_LIMA_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _LIMA_TRACE_H_
> > +
> > +#include 
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM lima
> > +#define TRACE_INCLUDE_FILE lima_trace
> > +
> > +DECLARE_EVENT_CLASS(lima_task,
> > +   TP_PROTO(struct lima_sched_task *task),
> > +   TP_ARGS(task),
> > +   TP_STRUCT__entry(
> > +   __field(uint64_t, task_id)
> > +   __field(unsigned int, context)
> > +   __field(unsigned int, seqno)
> > +   __string(pipe, task->base.sched->name)
> > +   ),
> > +
> > +   TP_fast_assign(
> > +   __entry->task_id = task->base.id;
> > +   __entry->context = task->base.s_fence->finished.context;
> > +   __entry->seqno = task->base.s_fence->finished.seqno;
> > +   __assign_str(pipe, task->base.sched->name)
> > +   ),
> > +
> > +   TP_printk("task=%llu, context=%u seqno=%u pipe=%s",
> > + __entry->task_id, __entry->context, __entry->seqno,
> > + __get_str(pipe))
> > +);
> > +
> > 

Re: Plumbing explicit synchronization through the Linux ecosystem

[Roman Gilg]

On Wed, Mar 11, 2020 at 8:21 PM Jason Ekstrand  wrote:
>
> On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  wrote:
> >
> > All,
> >
> > Sorry for casting such a broad net with this one. I'm sure most people
> > who reply will get at least one mailing list rejection.  However, this
> > is an issue that affects a LOT of components and that's why it's
> > thorny to begin with.  Please pardon the length of this e-mail as
> > well; I promise there's a concrete point/proposal at the end.
> >
> >
> > Explicit synchronization is the future of graphics and media.  At
> > least, that seems to be the consensus among all the graphics people
> > I've talked to.  I had a chat with one of the lead Android graphics
> > engineers recently who told me that doing explicit sync from the start
> > was one of the best engineering decisions Android ever made.  It's
> > also the direction being taken by more modern APIs such as Vulkan.
> >
> >
> > ## What are implicit and explicit synchronization?
> >
> > For those that aren't familiar with this space, GPUs, media encoders,
> > etc. are massively parallel and synchronization of some form is
> > required to ensure that everything happens in the right order and
> > avoid data races.  Implicit synchronization is when bits of work (3D,
> > compute, video encode, etc.) are implicitly based on the absolute
> > CPU-time order in which API calls occur.  Explicit synchronization is
> > when the client (whatever that means in any given context) provides
> > the dependency graph explicitly via some sort of synchronization
> > primitives.  If you're still confused, consider the following
> > examples:
> >
> > With OpenGL and EGL, almost everything is implicit sync.  Say you have
> > two OpenGL contexts sharing an image where one writes to it and the
> > other textures from it.  The way the OpenGL spec works, the client has
> > to make the API calls to render to the image before (in CPU time) it
> > makes the API calls which texture from the image.  As long as it does
> > this (and maybe inserts a glFlush?), the driver will ensure that the
> > rendering completes before the texturing happens and you get correct
> > contents.
> >
> > Implicit synchronization can also happen across processes.  Wayland,
> > for instance, is currently built on implicit sync where the client
> > does their rendering and then does a hand-off (via wl_surface::commit)
> > to tell the compositor it's done at which point the compositor can now
> > texture from the surface.  The hand-off ensures that the client's
> > OpenGL API calls happen before the server's OpenGL API calls.
> >
> > A good example of explicit synchronization is the Vulkan API.  There,
> > a client (or multiple clients) can simultaneously build command
> > buffers in different threads where one of those command buffers
> > renders to an image and the other textures from it and then submit
> > both of them at the same time with instructions to the driver for
> > which order to execute them in.  The execution order is described via
> > the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
> > extension, you can even submit the work which does the texturing
> > BEFORE the work which does the rendering and the driver will sort it
> > out.
> >
> > The #1 problem with implicit synchronization (which explicit solves)
> > is that it leads to a lot of over-synchronization both in client space
> > and in driver/device space.  The client has to synchronize a lot more
> > because it has to ensure that the API calls happen in a particular
> > order.  The driver/device have to synchronize a lot more because they
> > never know what is going to end up being a synchronization point as an
> > API call on another thread/process may occur at any time.  As we move
> > to more and more multi-threaded programming this synchronization (on
> > the client-side especially) becomes more and more painful.
> >
> >
> > ## Current status in Linux
> >
> > Implicit synchronization in Linux works via a the kernel's internal
> > dma_buf and dma_fence data structures.  A dma_fence is a tiny object
> > which represents the "done" status for some bit of work.  Typically,
> > dma_fences are created as a by-product of someone submitting some bit
> > of work (say, 3D rendering) to the kernel.  The dma_buf object has a
> > set of dma_fences on it representing shared (read) and exclusive
> > (write) access to the object.  When work is submitted which, for
> > instance renders to the dma_buf, it's queued waiting on all the fences
> > on the dma_buf and and a dma_fence is created representing the end of
> > said rendering work and it's installed as the dma_buf's exclusive
> > fence.  This way, the kernel can manage all its internal queues (3D
> > rendering, display, video encode, etc.) and know which things to
> > submit in what order.
> >
> > For the last few years, we've had sync_file in the kernel and it's
> > plumbed into some drivers.  A sync_file is just a wrapper around a
> 

Re: [PATCH 4/4] mm: check the device private page owner in hmm_range_fault

[Ralph Campbell]

On 3/16/20 12:32 PM, Christoph Hellwig wrote:

Hmm range fault will succeed for any kind of device private memory,
even if it doesn't belong to the calling entity.  While nouveau
has some crude checks for that, they are broken because they assume
nouveau is the only user of device private memory.  Fix this by
passing in an expected pgmap owner in the hmm_range_fault structure.

Signed-off-by: Christoph Hellwig 
Fixes: 4ef589dc9b10 ("mm/hmm/devmem: device memory hotplug using ZONE_DEVICE")


Looks good.
Reviewed-by: Ralph Campbell 


---
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 12 
  include/linux/hmm.h|  2 ++
  mm/hmm.c   | 10 +-
  3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c 
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index edfd0805fba4..ad89e09a0be3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -672,12 +672,6 @@ nouveau_dmem_migrate_vma(struct nouveau_drm *drm,
return ret;
  }
  
-static inline bool

-nouveau_dmem_page(struct nouveau_drm *drm, struct page *page)
-{
-   return is_device_private_page(page) && drm->dmem == page_to_dmem(page);
-}
-
  void
  nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
 struct hmm_range *range)
@@ -696,12 +690,6 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
if (!is_device_private_page(page))
continue;
  
-		if (!nouveau_dmem_page(drm, page)) {

-   WARN(1, "Some unknown device memory !\n");
-   range->pfns[i] = 0;
-   continue;
-   }
-
addr = nouveau_dmem_page_addr(page);
range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 5e6034f105c3..bb6be4428633 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -132,6 +132,7 @@ enum hmm_pfn_value_e {
   * @pfn_flags_mask: allows to mask pfn flags so that only default_flags matter
   * @pfn_shifts: pfn shift value (should be <= PAGE_SHIFT)
   * @valid: pfns array did not change since it has been fill by an HMM function
+ * @dev_private_owner: owner of device private pages
   */
  struct hmm_range {
struct mmu_interval_notifier *notifier;
@@ -144,6 +145,7 @@ struct hmm_range {
uint64_tdefault_flags;
uint64_tpfn_flags_mask;
uint8_t pfn_shift;
+   void*dev_private_owner;
  };
  
  /*

diff --git a/mm/hmm.c b/mm/hmm.c
index cfad65f6a67b..b75b3750e03d 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -216,6 +216,14 @@ int hmm_vma_handle_pmd(struct mm_walk *walk, unsigned long 
addr,
unsigned long end, uint64_t *pfns, pmd_t pmd);
  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
  
+static inline bool hmm_is_device_private_entry(struct hmm_range *range,

+   swp_entry_t entry)
+{
+   return is_device_private_entry(entry) &&
+   device_private_entry_to_page(entry)->pgmap->owner ==
+   range->dev_private_owner;
+}
+
  static inline uint64_t pte_to_hmm_pfn_flags(struct hmm_range *range, pte_t 
pte)
  {
if (pte_none(pte) || !pte_present(pte) || pte_protnone(pte))
@@ -254,7 +262,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, 
unsigned long addr,
 * Never fault in device private pages pages, but just report
 * the PFN even if not present.
 */
-   if (is_device_private_entry(entry)) {
+   if (hmm_is_device_private_entry(range, entry)) {
*pfn = hmm_device_entry_from_pfn(range,
swp_offset(entry));
*pfn |= range->flags[HMM_PFN_VALID];


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault

[Ralph Campbell]

On 3/16/20 12:32 PM, Christoph Hellwig wrote:

Remove the code to fault device private pages back into system memory
that has never been used by any driver.  Also replace the usage of the
HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple
is_device_private_page check in nouveau.

Signed-off-by: Christoph Hellwig 


Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver can
look at the struct page but what if a driver needs to fault in a page from
another device's private memory? Should it call handle_mm_fault()?



---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  1 -
  drivers/gpu/drm/nouveau/nouveau_dmem.c  |  5 +++--
  drivers/gpu/drm/nouveau/nouveau_svm.c   |  1 -
  include/linux/hmm.h |  2 --
  mm/hmm.c| 25 +
  5 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dee446278417..90821ce5e6ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -776,7 +776,6 @@ struct amdgpu_ttm_tt {
  static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {
(1 << 0), /* HMM_PFN_VALID */
(1 << 1), /* HMM_PFN_WRITE */
-   0 /* HMM_PFN_DEVICE_PRIVATE */
  };
  
  static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c 
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 0e36345d395c..edfd0805fba4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -28,6 +28,7 @@
  
  #include 

  #include 
+#include 
  #include 
  #include 
  
@@ -692,9 +693,8 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,

if (page == NULL)
continue;
  
-		if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {

+   if (!is_device_private_page(page))
continue;
-   }
  
  		if (!nouveau_dmem_page(drm, page)) {

WARN(1, "Some unknown device memory !\n");
@@ -705,5 +705,6 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
addr = nouveau_dmem_page_addr(page);
range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
+   range->pfns[i] |= NVIF_VMM_PFNMAP_V0_VRAM;
}
  }
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c 
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index df9bf1fd1bc0..39c731a99937 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -367,7 +367,6 @@ static const u64
  nouveau_svm_pfn_flags[HMM_PFN_FLAG_MAX] = {
[HMM_PFN_VALID ] = NVIF_VMM_PFNMAP_V0_V,
[HMM_PFN_WRITE ] = NVIF_VMM_PFNMAP_V0_W,
-   [HMM_PFN_DEVICE_PRIVATE] = NVIF_VMM_PFNMAP_V0_VRAM,
  };
  
  static const u64

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 4bf8d6997b12..5e6034f105c3 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -74,7 +74,6 @@
   * Flags:
   * HMM_PFN_VALID: pfn is valid. It has, at least, read permission.
   * HMM_PFN_WRITE: CPU page table has write permission set
- * HMM_PFN_DEVICE_PRIVATE: private device memory (ZONE_DEVICE)
   *
   * The driver provides a flags array for mapping page protections to device
   * PTE bits. If the driver valid bit for an entry is bit 3,
@@ -86,7 +85,6 @@
  enum hmm_pfn_flag_e {
HMM_PFN_VALID = 0,
HMM_PFN_WRITE,
-   HMM_PFN_DEVICE_PRIVATE,
HMM_PFN_FLAG_MAX
  };
  
diff --git a/mm/hmm.c b/mm/hmm.c

index 180e398170b0..cfad65f6a67b 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -118,15 +118,6 @@ static inline void hmm_pte_need_fault(const struct 
hmm_vma_walk *hmm_vma_walk,
/* We aren't ask to do anything ... */
if (!(pfns & range->flags[HMM_PFN_VALID]))
return;
-   /* If this is device memory then only fault if explicitly requested */
-   if ((cpu_flags & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
-   /* Do we fault on device memory ? */
-   if (pfns & range->flags[HMM_PFN_DEVICE_PRIVATE]) {
-   *write_fault = pfns & range->flags[HMM_PFN_WRITE];
-   *fault = true;
-   }
-   return;
-   }
  
  	/* If CPU page table is not valid then we need to fault */

*fault = !(cpu_flags & range->flags[HMM_PFN_VALID]);
@@ -260,21 +251,15 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, 
unsigned long addr,
swp_entry_t entry = pte_to_swp_entry(pte);
  
  		/*

-* This is a special swap entry, ignore migration, use
-* device and report anything else as error.
+* Never fault in device private pages pages, but just report
+* the PFN even if not present.
 */

Re: Plumbing explicit synchronization through the Linux ecosystem

[Jason Ekstrand]

On Mon, Mar 16, 2020 at 4:15 PM Laurent Pinchart
 wrote:
>
> Hi Jason,
>
> On Mon, Mar 16, 2020 at 10:06:07AM -0500, Jason Ekstrand wrote:
> > On Mon, Mar 16, 2020 at 5:20 AM Laurent Pinchart wrote:
> > > On Wed, Mar 11, 2020 at 04:18:55PM -0400, Nicolas Dufresne wrote:
> > >> (I know I'm going to be spammed by so many mailing list ...)
> > >>
> > >> Le mercredi 11 mars 2020 à 14:21 -0500, Jason Ekstrand a écrit :
> > >>> On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  
> > >>> wrote:
> >  All,
> > 
> >  Sorry for casting such a broad net with this one. I'm sure most people
> >  who reply will get at least one mailing list rejection.  However, this
> >  is an issue that affects a LOT of components and that's why it's
> >  thorny to begin with.  Please pardon the length of this e-mail as
> >  well; I promise there's a concrete point/proposal at the end.
> > 
> > 
> >  Explicit synchronization is the future of graphics and media.  At
> >  least, that seems to be the consensus among all the graphics people
> >  I've talked to.  I had a chat with one of the lead Android graphics
> >  engineers recently who told me that doing explicit sync from the start
> >  was one of the best engineering decisions Android ever made.  It's
> >  also the direction being taken by more modern APIs such as Vulkan.
> > 
> > 
> >  ## What are implicit and explicit synchronization?
> > 
> >  For those that aren't familiar with this space, GPUs, media encoders,
> >  etc. are massively parallel and synchronization of some form is
> >  required to ensure that everything happens in the right order and
> >  avoid data races.  Implicit synchronization is when bits of work (3D,
> >  compute, video encode, etc.) are implicitly based on the absolute
> >  CPU-time order in which API calls occur.  Explicit synchronization is
> >  when the client (whatever that means in any given context) provides
> >  the dependency graph explicitly via some sort of synchronization
> >  primitives.  If you're still confused, consider the following
> >  examples:
> > 
> >  With OpenGL and EGL, almost everything is implicit sync.  Say you have
> >  two OpenGL contexts sharing an image where one writes to it and the
> >  other textures from it.  The way the OpenGL spec works, the client has
> >  to make the API calls to render to the image before (in CPU time) it
> >  makes the API calls which texture from the image.  As long as it does
> >  this (and maybe inserts a glFlush?), the driver will ensure that the
> >  rendering completes before the texturing happens and you get correct
> >  contents.
> > 
> >  Implicit synchronization can also happen across processes.  Wayland,
> >  for instance, is currently built on implicit sync where the client
> >  does their rendering and then does a hand-off (via wl_surface::commit)
> >  to tell the compositor it's done at which point the compositor can now
> >  texture from the surface.  The hand-off ensures that the client's
> >  OpenGL API calls happen before the server's OpenGL API calls.
> > 
> >  A good example of explicit synchronization is the Vulkan API.  There,
> >  a client (or multiple clients) can simultaneously build command
> >  buffers in different threads where one of those command buffers
> >  renders to an image and the other textures from it and then submit
> >  both of them at the same time with instructions to the driver for
> >  which order to execute them in.  The execution order is described via
> >  the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
> >  extension, you can even submit the work which does the texturing
> >  BEFORE the work which does the rendering and the driver will sort it
> >  out.
> > 
> >  The #1 problem with implicit synchronization (which explicit solves)
> >  is that it leads to a lot of over-synchronization both in client space
> >  and in driver/device space.  The client has to synchronize a lot more
> >  because it has to ensure that the API calls happen in a particular
> >  order.  The driver/device have to synchronize a lot more because they
> >  never know what is going to end up being a synchronization point as an
> >  API call on another thread/process may occur at any time.  As we move
> >  to more and more multi-threaded programming this synchronization (on
> >  the client-side especially) becomes more and more painful.
> > 
> > 
> >  ## Current status in Linux
> > 
> >  Implicit synchronization in Linux works via a the kernel's internal
> >  dma_buf and dma_fence data structures.  A dma_fence is a tiny object
> >  which represents the "done" status for some bit of work.  Typically,
> >  dma_fences are created as a by-product of someone submitting some bit
> > 

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Sam Ravnborg]

Hi Maxime.

On Mon, Mar 16, 2020 at 09:48:50PM +0100, Maxime Ripard wrote:
> Hi Sam,
> 
> On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:
> > Independent bindings can be SPI slaves which for example is
> > the case for several panel bindings.
> >
> > Move SPI slave properties to spi-slave.yaml so the independent
> > SPI slave bindings can include spi-slave.yaml rather than
> > duplicating the properties.
> >
> > Signed-off-by: Sam Ravnborg 
> > Cc: Maxime Ripard 
> > Cc: Rob Herring 
> > Cc: Mark Brown 
> > Cc: linux-...@vger.kernel.org
> > ---
> >  .../bindings/spi/spi-controller.yaml  | 63 +-
> >  .../devicetree/bindings/spi/spi-slave.yaml| 83 +++
> >  2 files changed, 86 insertions(+), 60 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-slave.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml 
> > b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > index 1e0ca6ccf64b..99531c8d10dd 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > @@ -67,71 +67,14 @@ patternProperties:
> >"^.*@[0-9a-f]+$":
> >  type: object
> >
> > +allOf:
> > +  - $ref: spi-slave.yaml#
> > +
> >  properties:
> >compatible:
> >  description:
> >Compatible of the SPI device.
> >
> > -  reg:
> > -minimum: 0
> > -maximum: 256
> > -description:
> > -  Chip select used by the device.
> > -
> > -  spi-3wire:
> > -$ref: /schemas/types.yaml#/definitions/flag
> > -description:
> > -  The device requires 3-wire mode.
> > -
> > -  spi-cpha:
> > -$ref: /schemas/types.yaml#/definitions/flag
> > -description:
> > -  The device requires shifted clock phase (CPHA) mode.
> > -
> > -  spi-cpol:
> > -$ref: /schemas/types.yaml#/definitions/flag
> > -description:
> > -  The device requires inverse clock polarity (CPOL) mode.
> > -
> > -  spi-cs-high:
> > -$ref: /schemas/types.yaml#/definitions/flag
> > -description:
> > -  The device requires the chip select active high.
> > -
> > -  spi-lsb-first:
> > -$ref: /schemas/types.yaml#/definitions/flag
> > -description:
> > -  The device requires the LSB first mode.
> > -
> > -  spi-max-frequency:
> > -$ref: /schemas/types.yaml#/definitions/uint32
> > -description:
> > -  Maximum SPI clocking speed of the device in Hz.
> > -
> > -  spi-rx-bus-width:
> > -allOf:
> > -  - $ref: /schemas/types.yaml#/definitions/uint32
> > -  - enum: [ 1, 2, 4, 8 ]
> > -  - default: 1
> > -description:
> > -  Bus width to the SPI bus used for MISO.
> > -
> > -  spi-rx-delay-us:
> > -description:
> > -  Delay, in microseconds, after a read transfer.
> > -
> > -  spi-tx-bus-width:
> > -allOf:
> > -  - $ref: /schemas/types.yaml#/definitions/uint32
> > -  - enum: [ 1, 2, 4, 8 ]
> > -  - default: 1
> > -description:
> > -  Bus width to the SPI bus used for MOSI.
> > -
> > -  spi-tx-delay-us:
> > -description:
> > -  Delay, in microseconds, after a write transfer.
> > -
> 
> I can see what you're trying to do, but you don't really need to.
> 
> All the SPI devices will be declared under a spi controller node that
> will validate its child nodes (and thus the devices) already.

This was the missing piece - thanks.
And as Mark put it "why is this suddenly an issue"?
Turns out this is already properly handled and I made up an issue.
Maybe Mark tried to explian it to me already...

> 
> Doing it this way would actually make all the checks happen twice,
> once as part of the SPI controller, once as part of the SPI device
> binding, without any good reason.

I had focus on validating the example in the binding
file and not the full picture.

One thing I do not see properly addressed, but maybe I just miss it.
What triggers that we catch properties that are not supposed to be
present?

If we see a unsupported property "foobar":

spi {
...
panel {
   
   foobar = <1>;
};
};

somewhere in a SPI slave binding we should catch this.
If for no other reasons that it could be a simple spelling mistake
that otherwise could go undetected for a long time.
But maybe this is really not feasible to do?

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] mm: handle multiple owners of device private pages in migrate_vma

[Ralph Campbell]

On 3/16/20 12:32 PM, Christoph Hellwig wrote:

Add a new src_owner field to struct migrate_vma.  If the field is set,
only device private pages with page->pgmap->owner equal to that field
are migrated.  If the field is not set only "normal" pages are migrated.

Signed-off-by: Christoph Hellwig 
Fixes: df6ad69838fc ("mm/device-public-memory: device memory cache coherent with 
CPU")


When migrating to device private memory, setting the src_owner lets the caller
know about source pages that are already migrated and skips pages migrated to a
different device similar to pages swapped out an actual swap device.
But, it prevents normal pages from being migrated to device private memory.
It can still be useful for the driver to know that a page is owned by a
different device if it has a device to device way of migrating data.
nouveau_dmem_migrate_vma() isn't setting args.src_owner so only normal pages
will be migrated which I guess is OK for now since nouveau doesn't handle
direct GPU to GPU migration currently.

When migrating device private memory to system memory due to a CPU fault,
the source page should be the device's device private struct page so if it
isn't, then it does make sense to not migrate whatever normal page is there.
nouveau_dmem_migrate_to_ram() sets src_owner so this case looks OK.

Just had to think this through.
Reviewed-by: Ralph Campbell 


---
  arch/powerpc/kvm/book3s_hv_uvmem.c | 1 +
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
  include/linux/migrate.h| 8 
  mm/migrate.c   | 9 ++---
  4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c 
b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 67fefb03b9b7..f44f6b27950f 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -563,6 +563,7 @@ kvmppc_svm_page_out(struct vm_area_struct *vma, unsigned 
long start,
mig.end = end;
mig.src = _pfn;
mig.dst = _pfn;
+   mig.src_owner = _uvmem_pgmap;
  
  	mutex_lock(>arch.uvmem_lock);

/* The requested page is already paged-out, nothing to do */
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c 
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index a4682272586e..0e36345d395c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -176,6 +176,7 @@ static vm_fault_t nouveau_dmem_migrate_to_ram(struct 
vm_fault *vmf)
.end= vmf->address + PAGE_SIZE,
.src= ,
.dst= ,
+   .src_owner  = drm->dev,
};
  
  	/*

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 72120061b7d4..3e546cbf03dd 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -196,6 +196,14 @@ struct migrate_vma {
unsigned long   npages;
unsigned long   start;
unsigned long   end;
+
+   /*
+* Set to the owner value also stored in page->pgmap->owner for
+* migrating out of device private memory.  If set only device
+* private pages with this owner are migrated.  If not set
+* device private pages are not migrated at all.
+*/
+   void*src_owner;
  };
  
  int migrate_vma_setup(struct migrate_vma *args);

diff --git a/mm/migrate.c b/mm/migrate.c
index b1092876e537..7605d2c23433 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2241,7 +2241,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
arch_enter_lazy_mmu_mode();
  
  	for (; addr < end; addr += PAGE_SIZE, ptep++) {

-   unsigned long mpfn, pfn;
+   unsigned long mpfn = 0, pfn;
struct page *page;
swp_entry_t entry;
pte_t pte;
@@ -2255,8 +2255,6 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
}
  
  		if (!pte_present(pte)) {

-   mpfn = 0;
-
/*
 * Only care about unaddressable device page special
 * page table entry. Other special swap entries are not
@@ -2267,11 +2265,16 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
goto next;
  
  			page = device_private_entry_to_page(entry);

+   if (page->pgmap->owner != migrate->src_owner)
+   goto next;
+
mpfn = migrate_pfn(page_to_pfn(page)) |
MIGRATE_PFN_MIGRATE;
if (is_write_device_private_entry(entry))
mpfn |= MIGRATE_PFN_WRITE;
} else {
+   if (migrate->src_owner)
+   goto next;
pfn = pte_pfn(pte);
if (is_zero_pfn(pfn)) {
mpfn = MIGRATE_PFN_MIGRATE;

Re: Plumbing explicit synchronization through the Linux ecosystem

[Laurent Pinchart]

Hi Jason,

On Mon, Mar 16, 2020 at 10:06:07AM -0500, Jason Ekstrand wrote:
> On Mon, Mar 16, 2020 at 5:20 AM Laurent Pinchart wrote:
> > On Wed, Mar 11, 2020 at 04:18:55PM -0400, Nicolas Dufresne wrote:
> >> (I know I'm going to be spammed by so many mailing list ...)
> >>
> >> Le mercredi 11 mars 2020 à 14:21 -0500, Jason Ekstrand a écrit :
> >>> On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  
> >>> wrote:
>  All,
> 
>  Sorry for casting such a broad net with this one. I'm sure most people
>  who reply will get at least one mailing list rejection.  However, this
>  is an issue that affects a LOT of components and that's why it's
>  thorny to begin with.  Please pardon the length of this e-mail as
>  well; I promise there's a concrete point/proposal at the end.
> 
> 
>  Explicit synchronization is the future of graphics and media.  At
>  least, that seems to be the consensus among all the graphics people
>  I've talked to.  I had a chat with one of the lead Android graphics
>  engineers recently who told me that doing explicit sync from the start
>  was one of the best engineering decisions Android ever made.  It's
>  also the direction being taken by more modern APIs such as Vulkan.
> 
> 
>  ## What are implicit and explicit synchronization?
> 
>  For those that aren't familiar with this space, GPUs, media encoders,
>  etc. are massively parallel and synchronization of some form is
>  required to ensure that everything happens in the right order and
>  avoid data races.  Implicit synchronization is when bits of work (3D,
>  compute, video encode, etc.) are implicitly based on the absolute
>  CPU-time order in which API calls occur.  Explicit synchronization is
>  when the client (whatever that means in any given context) provides
>  the dependency graph explicitly via some sort of synchronization
>  primitives.  If you're still confused, consider the following
>  examples:
> 
>  With OpenGL and EGL, almost everything is implicit sync.  Say you have
>  two OpenGL contexts sharing an image where one writes to it and the
>  other textures from it.  The way the OpenGL spec works, the client has
>  to make the API calls to render to the image before (in CPU time) it
>  makes the API calls which texture from the image.  As long as it does
>  this (and maybe inserts a glFlush?), the driver will ensure that the
>  rendering completes before the texturing happens and you get correct
>  contents.
> 
>  Implicit synchronization can also happen across processes.  Wayland,
>  for instance, is currently built on implicit sync where the client
>  does their rendering and then does a hand-off (via wl_surface::commit)
>  to tell the compositor it's done at which point the compositor can now
>  texture from the surface.  The hand-off ensures that the client's
>  OpenGL API calls happen before the server's OpenGL API calls.
> 
>  A good example of explicit synchronization is the Vulkan API.  There,
>  a client (or multiple clients) can simultaneously build command
>  buffers in different threads where one of those command buffers
>  renders to an image and the other textures from it and then submit
>  both of them at the same time with instructions to the driver for
>  which order to execute them in.  The execution order is described via
>  the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
>  extension, you can even submit the work which does the texturing
>  BEFORE the work which does the rendering and the driver will sort it
>  out.
> 
>  The #1 problem with implicit synchronization (which explicit solves)
>  is that it leads to a lot of over-synchronization both in client space
>  and in driver/device space.  The client has to synchronize a lot more
>  because it has to ensure that the API calls happen in a particular
>  order.  The driver/device have to synchronize a lot more because they
>  never know what is going to end up being a synchronization point as an
>  API call on another thread/process may occur at any time.  As we move
>  to more and more multi-threaded programming this synchronization (on
>  the client-side especially) becomes more and more painful.
> 
> 
>  ## Current status in Linux
> 
>  Implicit synchronization in Linux works via a the kernel's internal
>  dma_buf and dma_fence data structures.  A dma_fence is a tiny object
>  which represents the "done" status for some bit of work.  Typically,
>  dma_fences are created as a by-product of someone submitting some bit
>  of work (say, 3D rendering) to the kernel.  The dma_buf object has a
>  set of dma_fences on it representing shared (read) and exclusive
>  (write) access to the object.  When work is submitted which, for
>  

Re: [PATCH 1/4] memremap: add an owner field to struct dev_pagemap

[Ralph Campbell]

On 3/16/20 12:32 PM, Christoph Hellwig wrote:

Add a new opaque owner field to struct dev_pagemap, which will allow
the hmm and migrate_vma code to identify who owns ZONE_DEVICE memory,
and refuse to work on mappings not owned by the calling entity.

Signed-off-by: Christoph Hellwig 


This looks like a reasonable approach to take.
Reviewed-by: Ralph Campbell 


---
  arch/powerpc/kvm/book3s_hv_uvmem.c | 2 ++
  drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 +
  include/linux/memremap.h   | 4 
  mm/memremap.c  | 4 
  4 files changed, 11 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c 
b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 79b1202b1c62..67fefb03b9b7 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -779,6 +779,8 @@ int kvmppc_uvmem_init(void)
kvmppc_uvmem_pgmap.type = MEMORY_DEVICE_PRIVATE;
kvmppc_uvmem_pgmap.res = *res;
kvmppc_uvmem_pgmap.ops = _uvmem_ops;
+   /* just one global instance: */
+   kvmppc_uvmem_pgmap.owner = _uvmem_pgmap;
addr = memremap_pages(_uvmem_pgmap, NUMA_NO_NODE);
if (IS_ERR(addr)) {
ret = PTR_ERR(addr);
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c 
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 0ad5d87b5a8e..a4682272586e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -526,6 +526,7 @@ nouveau_dmem_init(struct nouveau_drm *drm)
drm->dmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
drm->dmem->pagemap.res = *res;
drm->dmem->pagemap.ops = _dmem_pagemap_ops;
+   drm->dmem->pagemap.owner = drm->dev;
if (IS_ERR(devm_memremap_pages(device, >dmem->pagemap)))
goto out_free;
  
diff --git a/include/linux/memremap.h b/include/linux/memremap.h

index 6fefb09af7c3..60d97e8fd3c0 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -103,6 +103,9 @@ struct dev_pagemap_ops {
   * @type: memory type: see MEMORY_* in memory_hotplug.h
   * @flags: PGMAP_* flags to specify defailed behavior
   * @ops: method table
+ * @owner: an opaque pointer identifying the entity that manages this
+ * instance.  Used by various helpers to make sure that no
+ * foreign ZONE_DEVICE memory is accessed.
   */
  struct dev_pagemap {
struct vmem_altmap altmap;
@@ -113,6 +116,7 @@ struct dev_pagemap {
enum memory_type type;
unsigned int flags;
const struct dev_pagemap_ops *ops;
+   void *owner;
  };
  
  static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)

diff --git a/mm/memremap.c b/mm/memremap.c
index 09b5b7adc773..9b2c97ceb775 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -181,6 +181,10 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
WARN(1, "Missing migrate_to_ram method\n");
return ERR_PTR(-EINVAL);
}
+   if (!pgmap->owner) {
+   WARN(1, "Missing owner\n");
+   return ERR_PTR(-EINVAL);
+   }
break;
case MEMORY_DEVICE_FS_DAX:
if (!IS_ENABLED(CONFIG_ZONE_DEVICE) ||


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 11/36] dt-bindings: display: convert innolux,p120zdg-bf1 to DT Schema

[Doug Anderson]

Hi,

On Sun, Mar 15, 2020 at 6:44 AM Sam Ravnborg  wrote:
>
> Signed-off-by: Sam Ravnborg 
> Cc: Sandeep Panda 
> Cc: Douglas Anderson 
> Cc: Thierry Reding 
> Cc: Sam Ravnborg 
> ---
>  .../display/panel/innolux,p120zdg-bf1.txt | 22 --
>  .../display/panel/innolux,p120zdg-bf1.yaml| 43 +++
>  2 files changed, 43 insertions(+), 22 deletions(-)

Reviewed-by: Douglas Anderson 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] mm: remove device private page support from hmm_range_fault

[Ralph Campbell]

On 3/16/20 1:09 PM, Jason Gunthorpe wrote:

On Mon, Mar 16, 2020 at 07:49:35PM +0100, Christoph Hellwig wrote:

On Mon, Mar 16, 2020 at 11:42:19AM -0700, Ralph Campbell wrote:


On 3/16/20 10:52 AM, Christoph Hellwig wrote:

No driver has actually used properly wire up and support this feature.
There is various code related to it in nouveau, but as far as I can tell
it never actually got turned on, and the only changes since the initial
commit are global cleanups.


This is not actually true. OpenCL 2.x does support SVM with nouveau and
device private memory via clEnqueueSVMMigrateMem().
Also, Ben Skeggs has accepted a set of patches to map GPU memory after being
migrated and this change would conflict with that.


Can you explain me how we actually invoke this code?

For that we'd need HMM_PFN_DEVICE_PRIVATE NVIF_VMM_PFNMAP_V0_VRAM
set in ->pfns before calling hmm_range_fault, which isn't happening.


Oh, I got tripped on this too

The logic is backwards from what you'd think.. If you *set*
HMM_PFN_DEVICE_PRIVATE then this triggers:

hmm_pte_need_fault():
if ((cpu_flags & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
/* Do we fault on device memory ? */
if (pfns & range->flags[HMM_PFN_DEVICE_PRIVATE]) {
*write_fault = pfns & range->flags[HMM_PFN_WRITE];
*fault = true;
}
return;
}

Ie if the cpu page is a DEVICE_PRIVATE and the caller sets
HMM_PFN_DEVICE_PRIVATE in the input flags (pfns) then it always faults
it and never sets HMM_PFN_DEVICE_PRIVATE in the output flags.

So setting 0 enabled device_private support, and nouveau is Ok.

AMDGPU is broken because it can't handle device private and can't set
the flag to supress it.

I was going to try and sort this out as part of getting rid of range->flags

Jason



The reason for it being backwards is that "normally" a device doesn't want
the device private page to be faulted back to system memory, it wants to
get the device private struct page so it can update its page table to point
to the memory already in the device.
Also, a device like Nvidia's GPUs may have an alternate path for copying
one GPU's memory to another (nvlink) without going through system memory
so getting a device private struct page/PFN from hmm_range_fault() that isn't
"owned" by the faulting GPU is useful.
I agree that the current code isn't well tested or thought out for multiple 
devices
(rdma, NVMe drives, GPUs, etc.) but it also ties in with peer-to-peer access 
via PCIe.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Mark Brown]

On Mon, Mar 16, 2020 at 07:57:33PM +0100, Sam Ravnborg wrote:

> It was the best I could come up with - and this patch was called out
> for review in the hope there is a better way than this patch.

It definitely seems like a useful thing, just a bit surprised it's not
already there and if this is the best way to do it.

> We have similar examples like:
>   - pincfg-node.yaml
>   - regulatro.yaml

I'm curious what properties your consumers have for regulators - I'd
expect them to just have simple pointers to the regualtors with no
configuration.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] mm: remove device private page support from hmm_range_fault

[Ralph Campbell]

On 3/16/20 11:49 AM, Christoph Hellwig wrote:

On Mon, Mar 16, 2020 at 11:42:19AM -0700, Ralph Campbell wrote:


On 3/16/20 10:52 AM, Christoph Hellwig wrote:

No driver has actually used properly wire up and support this feature.
There is various code related to it in nouveau, but as far as I can tell
it never actually got turned on, and the only changes since the initial
commit are global cleanups.


This is not actually true. OpenCL 2.x does support SVM with nouveau and
device private memory via clEnqueueSVMMigrateMem().
Also, Ben Skeggs has accepted a set of patches to map GPU memory after being
migrated and this change would conflict with that.


Can you explain me how we actually invoke this code?


GPU memory is allocated when the device private memory "struct page" is
allocated. See where nouveau_dmem_chunk_alloc() calls nouveau_bo_new().
Then when a page is migrated to the GPU, the GPU memory physical address
is just the offset into the "fake" PFN range allocated by
devm_request_free_mem_region().

I'm looking into allocating GPU memory at the time of migration instead of when
the device private memory struct pages are allocated but that is a future
improvement.

System memory is migrated to GPU memory:
# mesa
clEnqueueSVMMigrateMem()
  svm_migrate_op()
q.svm_migrate()
  pipe->svm_migrate() // really nvc0_svm_migrate()
drmCommandWrite() // in libdrm
  drmIoctl()
ioctl()
  nouveau_drm_ioctl() // nouveau_drm.c
drm_ioctl()
  nouveau_svmm_bind()
nouveau_dmem_migrate_vma()
  migrate_vma_setup()
  nouveau_dmem_migrate_chunk()
nouveau_dmem_migrate_copy_one()
  // allocate device private struct page
  dpage = nouveau_dmem_page_alloc_locked()
dpage = nouveau_dmem_pages_alloc()
// Get GPU VRAM physical address
nouveau_dmem_page_addr(dpage)
// This does the DMA to GPU memory
drm->dmem->migrate.copy_func()
  migrate_vma_pages()
  migrate_vma_finalize()

Without my recent patch set, there is no GPU page table entry created for
this migrated memory so there will be a GPU fault which is handled in a
worker thread:
nouveau_svm_fault()
  // examine fault buffer entries and compute range of pages
  nouveau_range_fault()
// This will fill in the pfns array with a device private entry PFN
hmm_range_fault()
// This sees the range->flags[HMM_PFN_DEVICE_PRIVATE] flag
// and converts the HMM PFN to a GPU physical address
nouveau_dmem_convert_pfn()
// This sets up the GPU page tables
nvif_object_ioctl()


For that we'd need HMM_PFN_DEVICE_PRIVATE NVIF_VMM_PFNMAP_V0_VRAM
set in ->pfns before calling hmm_range_fault, which isn't happening.



It is set by hmm_range_fault() via the range->flags[HMM_PFN_DEVICE_PRIVATE] 
entry
when hmm_range_fault() sees a device private struct page. The call to
nouveau_dmem_convert_pfn() is just replacing the "fake" PFN with the real PFN
but not clearing/changing the read/write or VRAM/system memory PTE bits.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Sam Ravnborg]

Hi Mark.

On Mon, Mar 16, 2020 at 04:35:38PM +, Mark Brown wrote:
> On Mon, Mar 16, 2020 at 02:28:44PM +0100, Sam Ravnborg wrote:
> > On Mon, Mar 16, 2020 at 12:02:41PM +, Mark Brown wrote:
> > > On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:
> 
> > > > Independent bindings can be SPI slaves which for example is
> > > > the case for several panel bindings.
> 
> > > What is an "independent binding"?
> 
> > For several panels we have device trees that looks like this:
> 
> So what you're trying to do is define a generic class for SPI slaves
> which are just normal children of SPI nodes?  I really can't get to
> there from your changelog so we need some work there - in particular
> "non-spi bindings" is *very* confusing as as far as I can see these are
> bindings for SPI devices.
> 
> > The bindings are child of the spi controller node, but not specified
> > in the same binding file as the spi controller node.
> 
> Of course not, this how all buses work isn't it?
> 
> > So SPI slaves can now reference spi-slave.yaml to get access to
> > the SPI slave properties - and the copies can be avoided.
> > Likewise spi-controller.yml now references spi-slave.yaml.
> 
> > This was the best way I saw it could be done.
> 
> Rob didn't do the binding conversion but he did review it - I'm a bit
> surprised that there's issues here?

For panels we have panel-common.yaml that list all the
typical properties used by a panel - so the individual
panel bindings shall not repeat them.
This is also aligned with the principle of re-using properties rather
than inventing new properties all over.

And with a number of bindings describing HW that is SPI slaves
the idea is to do something like we do for panels.

I look forward for Rob's feedback - but as he is on vacation this week
we may have to wait a week for that.

The simple way forward had been to do like we do in many other places
and include a few SPI properties and be done with it.
This is an attempt to do something better.
If there is push-back or a nack, then we can always do like we do in
other places and just duplicate the properties.

> Also shouldn't there be some constraint that these devices have to be
> the child of a SPI controller or something?  Just including a file
> doesn't look right for something like class definition.

It was the best I could come up with - and this patch was called out
for review in the hope there is a better way than this patch.

We have similar examples like:
  - pincfg-node.yaml
  - regulatro.yaml
  - dma-common.yaml

They are not exactly 1:1 to what we do with spi-slave.yaml, but they
served as inspiration.

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] mm: remove device private page support from hmm_range_fault

[Ralph Campbell]

On 3/16/20 10:52 AM, Christoph Hellwig wrote:

No driver has actually used properly wire up and support this feature.
There is various code related to it in nouveau, but as far as I can tell
it never actually got turned on, and the only changes since the initial
commit are global cleanups.


This is not actually true. OpenCL 2.x does support SVM with nouveau and
device private memory via clEnqueueSVMMigrateMem().
Also, Ben Skeggs has accepted a set of patches to map GPU memory after being
migrated and this change would conflict with that.



Signed-off-by: Christoph Hellwig 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  1 -
  drivers/gpu/drm/nouveau/nouveau_dmem.c  | 37 -
  drivers/gpu/drm/nouveau/nouveau_dmem.h  |  2 --
  drivers/gpu/drm/nouveau/nouveau_svm.c   |  3 --
  include/linux/hmm.h |  2 --
  mm/hmm.c| 28 ---
  6 files changed, 73 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dee446278417..90821ce5e6ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -776,7 +776,6 @@ struct amdgpu_ttm_tt {
  static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {
(1 << 0), /* HMM_PFN_VALID */
(1 << 1), /* HMM_PFN_WRITE */
-   0 /* HMM_PFN_DEVICE_PRIVATE */
  };
  
  static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c 
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 7605c4c48985..42808efceaf2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -671,40 +671,3 @@ nouveau_dmem_migrate_vma(struct nouveau_drm *drm,
  out:
return ret;
  }
-
-static inline bool
-nouveau_dmem_page(struct nouveau_drm *drm, struct page *page)
-{
-   return is_device_private_page(page) && drm->dmem == page_to_dmem(page);
-}
-
-void
-nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
-struct hmm_range *range)
-{
-   unsigned long i, npages;
-
-   npages = (range->end - range->start) >> PAGE_SHIFT;
-   for (i = 0; i < npages; ++i) {
-   struct page *page;
-   uint64_t addr;
-
-   page = hmm_device_entry_to_page(range, range->pfns[i]);
-   if (page == NULL)
-   continue;
-
-   if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
-   continue;
-   }
-
-   if (!nouveau_dmem_page(drm, page)) {
-   WARN(1, "Some unknown device memory !\n");
-   range->pfns[i] = 0;
-   continue;
-   }
-
-   addr = nouveau_dmem_page_addr(page);
-   range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
-   range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
-   }
-}
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.h 
b/drivers/gpu/drm/nouveau/nouveau_dmem.h
index 92394be5d649..1ac620b3d4fb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.h
@@ -38,8 +38,6 @@ int nouveau_dmem_migrate_vma(struct nouveau_drm *drm,
 unsigned long start,
 unsigned long end);
  
-void nouveau_dmem_convert_pfn(struct nouveau_drm *drm,

- struct hmm_range *range);
  #else /* IS_ENABLED(CONFIG_DRM_NOUVEAU_SVM) */
  static inline void nouveau_dmem_init(struct nouveau_drm *drm) {}
  static inline void nouveau_dmem_fini(struct nouveau_drm *drm) {}
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c 
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index df9bf1fd1bc0..7e0376dca137 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -367,7 +367,6 @@ static const u64
  nouveau_svm_pfn_flags[HMM_PFN_FLAG_MAX] = {
[HMM_PFN_VALID ] = NVIF_VMM_PFNMAP_V0_V,
[HMM_PFN_WRITE ] = NVIF_VMM_PFNMAP_V0_W,
-   [HMM_PFN_DEVICE_PRIVATE] = NVIF_VMM_PFNMAP_V0_VRAM,
  };
  
  static const u64

@@ -558,8 +557,6 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm,
break;
}
  
-	nouveau_dmem_convert_pfn(drm, );

-
svmm->vmm->vmm.object.client->super = true;
ret = nvif_object_ioctl(>vmm->vmm.object, data, size, NULL);
svmm->vmm->vmm.object.client->super = false;
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 4bf8d6997b12..5e6034f105c3 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -74,7 +74,6 @@
   * Flags:
   * HMM_PFN_VALID: pfn is valid. It has, at least, read permission.
   * HMM_PFN_WRITE: CPU page table has write permission set
- * HMM_PFN_DEVICE_PRIVATE: private device memory (ZONE_DEVICE)
   *
   * The driver provides a flags array for mapping page protections to device
   * PTE bits. If the 

Re: [Mesa-dev] Plumbing explicit synchronization through the Linux ecosystem

[Marek Olšák]

On Mon, Mar 16, 2020 at 5:57 AM Michel Dänzer  wrote:

> On 2020-03-16 4:50 a.m., Marek Olšák wrote:
> > The synchronization works because the Mesa driver waits for idle (drains
> > the GFX pipeline) at the end of command buffers and there is only 1
> > graphics queue, so everything is ordered.
> >
> > The GFX pipeline runs asynchronously to the command buffer, meaning the
> > command buffer only starts draws and doesn't wait for completion. If the
> > Mesa driver didn't wait at the end of the command buffer, the command
> > buffer would finish and a different process could start execution of its
> > own command buffer while shaders of the previous process are still
> running.
> >
> > If the Mesa driver submits a command buffer internally (because it's
> full),
> > it doesn't wait, so the GFX pipeline doesn't notice that a command buffer
> > ended and a new one started.
> >
> > The waiting at the end of command buffers happens only when the flush is
> > external (Swap buffers, glFlush).
> >
> > It's a performance problem, because the GFX queue is blocked until the
> GFX
> > pipeline is drained at the end of every frame at least.
> >
> > So explicit fences for SwapBuffers would help.
>
> Not sure what difference it would make, since the same thing needs to be
> done for explicit fences as well, doesn't it?
>

No. Explicit fences don't require userspace to wait for idle in the command
buffer. Fences are signalled when the last draw is complete and caches are
flushed. Before that happens, any command buffer that is not dependent on
the fence can start execution. There is never a need for the GPU to be idle
if there is enough independent work to do.

Marek
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 2/2] drm/panel: add support for rm69299 visionox panel driver

[Matthias Kaehlcke]

On Mon, Mar 16, 2020 at 09:46:47AM +0530, Harigovindan P wrote:
> Add support for Visionox panel driver.
> 
> Signed-off-by: Harigovindan P 

Reviewed-by: Matthias Kaehlcke 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/mipi-dbi: Make mipi_dbi_command_stackbuf() data parameter const

[Geert Uytterhoeven]

mipi_dbi_command_stackbuf() copies the passed buffer data, so it can be
const.

Signed-off-by: Geert Uytterhoeven 
---
 drivers/gpu/drm/drm_mipi_dbi.c | 3 ++-
 include/drm/drm_mipi_dbi.h | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 558baf989f5a8432..89e911ccea06cf99 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -169,7 +169,8 @@ int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 
*data, size_t len)
 EXPORT_SYMBOL(mipi_dbi_command_buf);
 
 /* This should only be used by mipi_dbi_command() */
-int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t 
len)
+int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
+ size_t len)
 {
u8 *buf;
int ret;
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 33f325f5af2b921f..9333b9086c30bd3c 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -170,7 +170,8 @@ int mipi_dbi_spi_transfer(struct spi_device *spi, u32 
speed_hz,
 
 int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
 int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
-int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t 
len);
+int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
+ size_t len);
 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
  struct drm_rect *clip, bool swap);
 /**
@@ -187,7 +188,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
  */
 #define mipi_dbi_command(dbi, cmd, seq...) \
 ({ \
-   u8 d[] = { seq }; \
+   const u8 d[] = { seq }; \
mipi_dbi_command_stackbuf(dbi, cmd, d, ARRAY_SIZE(d)); \
 })
 
-- 
2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dpu: ensure device suspend happens during PM sleep

[Rob Clark]

On Mon, Mar 16, 2020 at 4:05 AM Kalyan Thota  wrote:
>
> "The PM core always increments the runtime usage counter
> before calling the ->suspend() callback and decrements it
> after calling the ->resume() callback"
>
> DPU and DSI are managed as runtime devices. When
> suspend is triggered, PM core adds a refcount on all the
> devices and calls device suspend, since usage count is
> already incremented, runtime suspend was not getting called
> and it kept the clocks on which resulted in target not
> entering into XO shutdown.
>
> Add changes to manage runtime devices during pm sleep.
>
> Signed-off-by: Kalyan Thota 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 41 
> +
>  drivers/gpu/drm/msm/dsi/dsi.c   |  7 ++
>  drivers/gpu/drm/msm/msm_drv.c   | 14 +++
>  drivers/gpu/drm/msm/msm_kms.h   |  2 ++
>  4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index cb08faf..6e103d5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -26,6 +26,7 @@
>  #include "dpu_encoder.h"
>  #include "dpu_plane.h"
>  #include "dpu_crtc.h"
> +#include "dsi.h"
>
>  #define CREATE_TRACE_POINTS
>  #include "dpu_trace.h"
> @@ -250,6 +251,37 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
> pm_runtime_put_sync(_kms->pdev->dev);
>  }
>
> +static void _dpu_kms_disable_dpu(struct msm_kms *kms)
> +{
> +   struct drm_device *dev;
> +   struct msm_drm_private *priv;
> +   struct dpu_kms *dpu_kms;
> +   int i = 0;
> +   struct msm_dsi *dsi;
> +
> +   dpu_kms = to_dpu_kms(kms);
> +   dev = dpu_kms->dev;
> +   if (!dev) {
> +   DPU_ERROR("invalid device\n");
> +   return;
> +   }
> +
> +   priv = dev->dev_private;
> +   if (!priv) {
> +   DPU_ERROR("invalid private data\n");
> +   return;
> +   }

the !dev and !priv checks can be dropped.. these aren't things a user
should hit, and if I screw somethign up in development and hit that
case, I'd rather see a stack trace

otherwise, I think it looks reasonable

BR,
-R

> +
> +   dpu_kms_disable_commit(kms);
> +
> +   for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> +   if (!priv->dsi[i])
> +   continue;
> +   dsi = priv->dsi[i];
> +   pm_runtime_put_sync(>pdev->dev);
> +   }
> +}
> +
>  static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
>  {
> struct drm_encoder *encoder;
> @@ -683,6 +715,7 @@ static void dpu_irq_uninstall(struct msm_kms *kms)
>  #ifdef CONFIG_DEBUG_FS
> .debugfs_init= dpu_kms_debugfs_init,
>  #endif
> +   .disable_dpu = _dpu_kms_disable_dpu,
>  };
>
>  static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
> @@ -1053,7 +1086,15 @@ static int __maybe_unused dpu_runtime_resume(struct 
> device *dev)
> return rc;
>  }
>
> +
> +static int __maybe_unused dpu_pm_suspend_late(struct device *dev)
> +{
> +   pm_runtime_get_noresume(dev);
> +   return 0;
> +}
> +
>  static const struct dev_pm_ops dpu_pm_ops = {
> +   SET_LATE_SYSTEM_SLEEP_PM_OPS(dpu_pm_suspend_late, NULL)
> SET_RUNTIME_PM_OPS(dpu_runtime_suspend, dpu_runtime_resume, NULL)
>  };
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 55ea4bc2..3d3740e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -154,12 +154,19 @@ static int dsi_dev_remove(struct platform_device *pdev)
> return 0;
>  }
>
> +static int __maybe_unused dsi_pm_suspend_late(struct device *dev)
> +{
> +   pm_runtime_get_noresume(dev);
> +   return 0;
> +}
> +
>  static const struct of_device_id dt_match[] = {
> { .compatible = "qcom,mdss-dsi-ctrl" },
> {}
>  };
>
>  static const struct dev_pm_ops dsi_pm_ops = {
> +   SET_LATE_SYSTEM_SLEEP_PM_OPS(dsi_pm_suspend_late, NULL)
> SET_RUNTIME_PM_OPS(msm_dsi_runtime_suspend, msm_dsi_runtime_resume, 
> NULL)
>  };
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index e4b750b..12ec1c6 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1038,6 +1038,7 @@ static int msm_pm_suspend(struct device *dev)
>  {
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct msm_drm_private *priv = ddev->dev_private;
> +   struct msm_kms *kms = priv->kms;
>
> if (WARN_ON(priv->pm_state))
> drm_atomic_state_put(priv->pm_state);
> @@ -1049,6 +1050,11 @@ static int msm_pm_suspend(struct device *dev)
> return ret;
> }
>
> +   if (kms->funcs->disable_dpu)
> +   kms->funcs->disable_dpu(kms);
> +
> +   pm_runtime_put_sync(dev);
> +
> return 0;
>  }
>
> @@ -1067,6 +1073,13 @@ static int 

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Mark Brown]

On Mon, Mar 16, 2020 at 02:28:44PM +0100, Sam Ravnborg wrote:
> On Mon, Mar 16, 2020 at 12:02:41PM +, Mark Brown wrote:
> > On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:

> > > Independent bindings can be SPI slaves which for example is
> > > the case for several panel bindings.

> > What is an "independent binding"?

> For several panels we have device trees that looks like this:

So what you're trying to do is define a generic class for SPI slaves
which are just normal children of SPI nodes?  I really can't get to
there from your changelog so we need some work there - in particular
"non-spi bindings" is *very* confusing as as far as I can see these are
bindings for SPI devices.

> The bindings are child of the spi controller node, but not specified
> in the same binding file as the spi controller node.

Of course not, this how all buses work isn't it?

> So SPI slaves can now reference spi-slave.yaml to get access to
> the SPI slave properties - and the copies can be avoided.
> Likewise spi-controller.yml now references spi-slave.yaml.

> This was the best way I saw it could be done.

Rob didn't do the binding conversion but he did review it - I'm a bit
surprised that there's issues here?

Also shouldn't there be some constraint that these devices have to be
the child of a SPI controller or something?  Just including a file
doesn't look right for something like class definition.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Daniel Stone]

Hi,

On Mon, 16 Mar 2020 at 15:33, Tomek Bury  wrote:
> > GL and GLES are not relevant. What is relevant is EGL, which defines
> > interfaces to make things work on the native platform.
> Yes and no. This is what EGL spec says about sharing a texture between 
> contexts:

Contexts are different though ...

> There are similar statements with regards to the lack of
> synchronisation guarantees for EGL images or between GL and native
> rendering, etc.

This also isn't about native rendering.

> But the main thing here is that EGL and Vulkan differ
> significantly.

Sure, I totally agree.

> The eglSwapBuffers() is expected to post an unspecified
> "back buffer" to the display system using some internal driver magic.
> EGL driver is then expected to obtain another back buffer at some
> unspecified point in the future.

Yes, this is rather the point: EGL doesn't specify platform-related
'black magic' to make things just work, because that's part of the
platform implementation details. And, as things stand, on Linux one of
those things is implicit synchronisation, unless the desired end state
of your driver is no synchronisation.

This thread is a discussion about changing that.

> > If you are using EGL_WL_bind_wayland_display, then one of the things
> > it is explicitly allowed/expected to do is to create a Wayland
> > protocol interface between client and compositor, which can be used to
> > pass buffer handles and metadata in a platform-specific way. Adding
> > synchronisation is also possible.
> Only one-way synchronisation is possible with this mechanism. There's
> a standard protocol for recycling buffers - wl_buffer_release() so
> buffer hand-over from the compositor to client remains unsynchronised
> - see below.

That's not true; you can post back a sync token every time the client
buffer is used by the compositor.

> > > The most troublesome part was Wayland buffer release mechanism, as it 
> > > only involves a CPU signalling over Wayland IPC, without any 3D driver 
> > > involvement. The choices were: explicit synchronisation extension or a 
> > > buffer copy in the compositor (i.e. compositor textures from the copy, so 
> > > the client can re-write the original), or some implicit synchronisation 
> > > in kernel space (but that wasn't an option in Broadcom driver).
> >
> > You can add your own explicit synchronisation extension.
> I could but that requires implementing in in the driver and in a
> number of compositors, therefore a standard extension
> zwp_linux_explicit_synchronization_v1 is much better choice here than
> a custom one.

EGL_WL_bind_wayland_display is explicitly designed to allow each
driver to implement its own private extensions without modifying
compositors. For instance, Mesa adds the `wl_drm` extension, which is
used for bidirectional communication between the EGL implementations
in the client and compositor address spaces, without modifying either.

> > In every cross-process and cross-subsystem usecase, synchronisation is
> > obviously required. The two options for this are to implement kernel
> > support for implicit synchronisation (as everyone else has done),
> That would require major changes in driver architecture or a 2nd
> mechanisms doing the same thing but in kernel space - both are
> non-starters.

OK. As it stands, everyone else has the kernel mechanism (e.g. via
dmabuf resv), so in this case if you are reinventing the underlying
platform in a proprietary stack, you get to solve the same problems
yourselves.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Jason Ekstrand]

On Mon, Mar 16, 2020 at 10:33 AM Tomek Bury  wrote:
>
> > GL and GLES are not relevant. What is relevant is EGL, which defines
> > interfaces to make things work on the native platform.
> Yes and no. This is what EGL spec says about sharing a texture between 
> contexts:
>
> "OpenGL and OpenGL ES makes no attempt to synchronize access to
> texture objects. If a texture object is bound to more than one
> context, then it is up to the programmer to ensure that the contents
> of the object are not being changed via one context while another
> context is using the texture object for rendering. The results of
> changing a texture object while another context is using it are
> undefined."
>
> There are similar statements with regards to the lack of
> synchronisation guarantees for EGL images or between GL and native
> rendering, etc. But the main thing here is that EGL and Vulkan differ
> significantly. The eglSwapBuffers() is expected to post an unspecified
> "back buffer" to the display system using some internal driver magic.
> EGL driver is then expected to obtain another back buffer at some
> unspecified point in the future. Vulkan on the other hand is very
> specific and explicit. The vkQueuePresentKHR() is expected to post a
> specific vkImage with an explicit set of set of semaphores. Another
> image is obtained through vkAcquireNextImageKHR() and it's the
> application's decision whether it wants a fence, a semaphore, both or
> none with the acquired buffer. The implicit synchronisation doesn't
> mix well with Vulkan drivers and requires a lot of extra plumbing  in
> the WSI code.

Yes, and that (the Vulkan issues in particular) is what I'm trying to
fix. :-)  (among other things...)  Assuming the kernel patch I linked
to, your usermode driver could stuff fences in the dma-buf without
having that be part of your kernel driver.  This assumes, of course,
that your kernel driver supports sync_file.

> > If you are using EGL_WL_bind_wayland_display, then one of the things
> > it is explicitly allowed/expected to do is to create a Wayland
> > protocol interface between client and compositor, which can be used to
> > pass buffer handles and metadata in a platform-specific way. Adding
> > synchronisation is also possible.
> Only one-way synchronisation is possible with this mechanism. There's
> a standard protocol for recycling buffers - wl_buffer_release() so
> buffer hand-over from the compositor to client remains unsynchronised
>
> - see below.
>
> > > The most troublesome part was Wayland buffer release mechanism, as it 
> > > only involves a CPU signalling over Wayland IPC, without any 3D driver 
> > > involvement. The choices were: explicit synchronisation extension or a 
> > > buffer copy in the compositor (i.e. compositor textures from the copy, so 
> > > the client can re-write the original), or some implicit synchronisation 
> > > in kernel space (but that wasn't an option in Broadcom driver).
> >
> > You can add your own explicit synchronisation extension.
> I could but that requires implementing in in the driver and in a
> number of compositors, therefore a standard extension
> zwp_linux_explicit_synchronization_v1 is much better choice here than
> a custom one.

I think you may be missing what Daniel is saying.  Wayland allows you
to do basically anything you want within your client and server-side
EGL implementations.  That could include the server-side EGL sending
an event with a fence every single time a flush operation happens in
the server-side GL/GLES implementation. (Could be glFlush, glFinish,
eglSwapBuffers, or other things).  Since wayland protocol events are
ordered, the client-side EGL implementation would get the most recent
flush event before it got the wl_buffer::release.  I fully agree that
it's rather cumbersome though.

> > In every cross-process and cross-subsystem usecase, synchronisation is
> > obviously required. The two options for this are to implement kernel
> > support for implicit synchronisation (as everyone else has done),
> That would require major changes in driver architecture or a 2nd
> mechanisms doing the same thing but in kernel space - both are
> non-starters.
>
> > or implement generic support for explicit synchronisation (as we have
> > been working on with implementations inside Weston and Exosphere at
> > least),
> The zwp_linux_explicit_synchronization_v1 is a good step forward. I'm
> using this extension as a main synchronisation mechanism in EGL and
> Vulkan driver whenever available. I remember that Gustavo Padovan was
> working on explicit sync support in the display system some time ago.
> I hope it got merged into kernel by now, but I don't know to what
> extend it's actually being used.

It is supported by KMS/atomic.  Legacy KMS, however, does not support it.

> > or implement private support for explicit synchronisation,
> If everything else fails, that would be the last resort scenario, but
> far from ideal and very costly in terms of 

DMA, TTM and memory encryption

[VMware]

Hi, Christoph,

It would be good to revisit this to see if we could set a direction for 
supporting user-space mapping of dma-coherent memory with TTM.


I think in the end, what TTM needs is a DMA interface similar to the one 
outlined here:


https://lore.kernel.org/lkml/b811f66d-2353-23c6-c9fa-e279cdb0f...@shipmail.org/

That could without too much effort be implemented for all architectures 
TTM supports and also for those weird architectures you've described 
that need special treatment, it would probably suffice with a more 
elaborate definition of the dma_pfn_t together with the dma coherent 
fault work you were suggesting earlier.


Regardless, once this patchset is in,

https://lore.kernel.org/lkml/20200304114527.3636-1-thomas...@shipmail.org/

a short term solution would be to export the dma_pgprot() function and 
have TTM use it to set the pgprot for dma-coherent memory.


What do you think about this?

Thanks,

Thomas


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/5] drm: Introduce scaling filter property

[Ville Syrjälä]

On Mon, Mar 16, 2020 at 09:31:32AM +0100, Daniel Vetter wrote:
> On Tue, Mar 10, 2020 at 06:01:06PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 25, 2020 at 12:35:41PM +0530, Pankaj Bharadiya wrote:
> > > Introduce new scaling filter property to allow userspace to select
> > > the driver's default scaling filter or Nearest-neighbor(NN) filter
> > > for upscaling operations on crtc/plane.
> > > 
> > > Drivers can set up this property for a plane by calling
> > > drm_plane_enable_scaling_filter() and for a CRTC by calling
> > > drm_crtc_enable_scaling_filter().
> > > 
> > > NN filter works by filling in the missing color values in the upscaled
> > > image with that of the coordinate-mapped nearest source pixel value.
> > > 
> > > NN filter for integer multiple scaling can be particularly useful for
> > > for pixel art games that rely on sharp, blocky images to deliver their
> > > distinctive look.
> > > 
> > > Signed-off-by: Pankaj Bharadiya 
> > > Signed-off-by: Shashank Sharma 
> > > Signed-off-by: Ankit Nautiyal 
> > > ---
> > >  drivers/gpu/drm/drm_atomic_uapi.c |  8 +++
> > >  drivers/gpu/drm/drm_crtc.c| 16 ++
> > >  drivers/gpu/drm/drm_mode_config.c | 13 
> > >  drivers/gpu/drm/drm_plane.c   | 35 +++
> > >  include/drm/drm_crtc.h| 10 +
> > >  include/drm/drm_mode_config.h |  6 ++
> > >  include/drm/drm_plane.h   | 14 +
> > >  7 files changed, 102 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
> > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index a1e5e262bae2..4e3c1f3176e4 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct 
> > > drm_crtc *crtc,
> > >   return ret;
> > >   } else if (property == config->prop_vrr_enabled) {
> > >   state->vrr_enabled = val;
> > > + } else if (property == config->scaling_filter_property) {
> > > + state->scaling_filter = val;
> > 
> > I think we want a per-plane/per-crtc prop for this. If we start adding
> > more filters we are surely going to need different sets for different hw
> > blocks.
> 
> In the past we've only done that once we have a demonstrated need. Usually
> the patch to move the property to a per-object location isn't a lot of
> churn.

Seems silly to not do it from the start when we already know there is
hardware out there that has different capabilities per hw block.

-- 
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Jason Ekstrand]

On Mon, Mar 16, 2020 at 5:20 AM Laurent Pinchart
 wrote:
>
> On Wed, Mar 11, 2020 at 04:18:55PM -0400, Nicolas Dufresne wrote:
> > (I know I'm going to be spammed by so many mailing list ...)
> >
> > Le mercredi 11 mars 2020 à 14:21 -0500, Jason Ekstrand a écrit :
> > > On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  
> > > wrote:
> > > > All,
> > > >
> > > > Sorry for casting such a broad net with this one. I'm sure most people
> > > > who reply will get at least one mailing list rejection.  However, this
> > > > is an issue that affects a LOT of components and that's why it's
> > > > thorny to begin with.  Please pardon the length of this e-mail as
> > > > well; I promise there's a concrete point/proposal at the end.
> > > >
> > > >
> > > > Explicit synchronization is the future of graphics and media.  At
> > > > least, that seems to be the consensus among all the graphics people
> > > > I've talked to.  I had a chat with one of the lead Android graphics
> > > > engineers recently who told me that doing explicit sync from the start
> > > > was one of the best engineering decisions Android ever made.  It's
> > > > also the direction being taken by more modern APIs such as Vulkan.
> > > >
> > > >
> > > > ## What are implicit and explicit synchronization?
> > > >
> > > > For those that aren't familiar with this space, GPUs, media encoders,
> > > > etc. are massively parallel and synchronization of some form is
> > > > required to ensure that everything happens in the right order and
> > > > avoid data races.  Implicit synchronization is when bits of work (3D,
> > > > compute, video encode, etc.) are implicitly based on the absolute
> > > > CPU-time order in which API calls occur.  Explicit synchronization is
> > > > when the client (whatever that means in any given context) provides
> > > > the dependency graph explicitly via some sort of synchronization
> > > > primitives.  If you're still confused, consider the following
> > > > examples:
> > > >
> > > > With OpenGL and EGL, almost everything is implicit sync.  Say you have
> > > > two OpenGL contexts sharing an image where one writes to it and the
> > > > other textures from it.  The way the OpenGL spec works, the client has
> > > > to make the API calls to render to the image before (in CPU time) it
> > > > makes the API calls which texture from the image.  As long as it does
> > > > this (and maybe inserts a glFlush?), the driver will ensure that the
> > > > rendering completes before the texturing happens and you get correct
> > > > contents.
> > > >
> > > > Implicit synchronization can also happen across processes.  Wayland,
> > > > for instance, is currently built on implicit sync where the client
> > > > does their rendering and then does a hand-off (via wl_surface::commit)
> > > > to tell the compositor it's done at which point the compositor can now
> > > > texture from the surface.  The hand-off ensures that the client's
> > > > OpenGL API calls happen before the server's OpenGL API calls.
> > > >
> > > > A good example of explicit synchronization is the Vulkan API.  There,
> > > > a client (or multiple clients) can simultaneously build command
> > > > buffers in different threads where one of those command buffers
> > > > renders to an image and the other textures from it and then submit
> > > > both of them at the same time with instructions to the driver for
> > > > which order to execute them in.  The execution order is described via
> > > > the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
> > > > extension, you can even submit the work which does the texturing
> > > > BEFORE the work which does the rendering and the driver will sort it
> > > > out.
> > > >
> > > > The #1 problem with implicit synchronization (which explicit solves)
> > > > is that it leads to a lot of over-synchronization both in client space
> > > > and in driver/device space.  The client has to synchronize a lot more
> > > > because it has to ensure that the API calls happen in a particular
> > > > order.  The driver/device have to synchronize a lot more because they
> > > > never know what is going to end up being a synchronization point as an
> > > > API call on another thread/process may occur at any time.  As we move
> > > > to more and more multi-threaded programming this synchronization (on
> > > > the client-side especially) becomes more and more painful.
> > > >
> > > >
> > > > ## Current status in Linux
> > > >
> > > > Implicit synchronization in Linux works via a the kernel's internal
> > > > dma_buf and dma_fence data structures.  A dma_fence is a tiny object
> > > > which represents the "done" status for some bit of work.  Typically,
> > > > dma_fences are created as a by-product of someone submitting some bit
> > > > of work (say, 3D rendering) to the kernel.  The dma_buf object has a
> > > > set of dma_fences on it representing shared (read) and exclusive
> > > > (write) access to the object.  When work is submitted which, for
> > > > 

Re: [PATCH 2/2] dt-bindings: Document the Synopsys ARC HDMI TX bindings

[Sam Ravnborg]

Hi Eugeniy.

On Mon, Mar 16, 2020 at 05:46:47PM +0300, Eugeniy Paltsev wrote:
> This patch adds documentation of device tree bindings for the Synopsys
> HDMI 2.0 TX encoder driver for ARC SoCs.
> 
> Signed-off-by: Eugeniy Paltsev 
> ---
>  .../display/bridge/snps,arc-dw-hdmi.txt   | 73 +++

New bindings in DT Schema format please (.yaml files).
We are working on migrating all bindings to DT Schema format.

Sam

>  1 file changed, 73 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt 
> b/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
> new file mode 100644
> index ..d5e006b392cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
> @@ -0,0 +1,73 @@
> +Synopsys DesignWare HDMI 2.0 TX encoder driver for ARC SoCs
> +
> +
> +The HDMI transmitter is a Synopsys DesignWare HDMI 2.0 TX controller IP
> +with a companion of Synopsys DesignWare HDMI 2.0 TX PHY IP.
> +
> +These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> +Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
> +with the following device-specific properties.
> +
> +
> +Required properties:
> +
> +- compatible : Shall contain
> +  - "snps,dw-hdmi-hsdk" for HSDK4xD compatible HDMI TX
> +
> +- reg: See dw_hdmi.txt.
> +- interrupts: HDMI interrupt number.
> +- clocks: See dw_hdmi.txt.
> +- clock-names: Must contain "iahb" and "isfr" as defined in dw_hdmi.txt.
> +- ports: See dw_hdmi.txt. The DWC HDMI shall have one port numbered 0
> +  corresponding to the video input of the controller and one port numbered 1
> +  corresponding to its HDMI output.
> +
> +Example:
> +
> +hdmi: hdmi@0x1 {
> + compatible = "snps,dw-hdmi-hsdk";
> + reg = <0x1 0x1>;
> + reg-io-width = <4>;
> + interrupts = <14>;
> + clocks = <>, <_pix_clk>;
> + clock-names = "iahb", "isfr";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + hdmi_enc_input: endpoint {
> + remote-endpoint = <_output>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + hdmi_enc_out: endpoint {
> + remote-endpoint = <_con>;
> + };
> + };
> + };
> +};
> +
> +hdmi-out {
> + ...
> +
> + port {
> + hdmi_con: endpoint {
> + remote-endpoint = <_enc_out>;
> + };
> + };
> +};
> +
> +pgu {
> + ...
> +
> + port_o: port {
> + pgu_output: endpoint {
> + remote-endpoint = <_enc_input>;
> + };
> + };
> +};
> -- 
> 2.21.1
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] DRM: ARC: add HDMI 2.0 TX encoder support

[Eugeniy Paltsev]

The Synopsys ARC SoCs (like HSDK4xD) include on-chip DesignWare HDMI
encoders. Support them with a platform driver to provide platform glue
data to the dw-hdmi driver.

Signed-off-by: Eugeniy Paltsev 
---
 MAINTAINERS   |   6 ++
 drivers/gpu/drm/Makefile  |   2 +-
 drivers/gpu/drm/arc/Kconfig   |   7 ++
 drivers/gpu/drm/arc/Makefile  |   1 +
 drivers/gpu/drm/arc/arc-dw-hdmi.c | 126 ++
 5 files changed, 141 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/arc/arc-dw-hdmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a6fbdf354d34..a6dd992c5f95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1258,6 +1258,12 @@ S:   Supported
 F: drivers/gpu/drm/arc/
 F: Documentation/devicetree/bindings/display/snps,arcpgu.txt
 
+ARC DW HDMI DRIVER
+M: Eugeniy Paltsev 
+S: Supported
+F: drivers/gpu/drm/arc/arc-dw-hdmi.c
+F: Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
+
 ARCNET NETWORK LAYER
 M: Michael Grzeschik 
 L: net...@vger.kernel.org
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6493088a0fdd..5b0bcf7f45cd 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -109,7 +109,7 @@ obj-y   += panel/
 obj-y  += bridge/
 obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
 obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
-obj-$(CONFIG_DRM_ARCPGU)+= arc/
+obj-y  += arc/
 obj-y  += hisilicon/
 obj-$(CONFIG_DRM_ZTE)  += zte/
 obj-$(CONFIG_DRM_MXSFB)+= mxsfb/
diff --git a/drivers/gpu/drm/arc/Kconfig b/drivers/gpu/drm/arc/Kconfig
index e8f3d63e0b91..baec9d2a4fba 100644
--- a/drivers/gpu/drm/arc/Kconfig
+++ b/drivers/gpu/drm/arc/Kconfig
@@ -8,3 +8,10 @@ config DRM_ARCPGU
  Choose this option if you have an ARC PGU controller.
 
  If M is selected the module will be called arcpgu.
+
+config DRM_ARC_DW_HDMI
+   tristate "ARC DW HDMI"
+   depends on DRM && OF
+   select DRM_DW_HDMI
+   help
+ Synopsys DW HDMI driver for various ARC development boards
diff --git a/drivers/gpu/drm/arc/Makefile b/drivers/gpu/drm/arc/Makefile
index c7028b7427b3..7a156d8c2c3c 100644
--- a/drivers/gpu/drm/arc/Makefile
+++ b/drivers/gpu/drm/arc/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 arcpgu-y := arcpgu_crtc.o arcpgu_hdmi.o arcpgu_sim.o arcpgu_drv.o
 obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
+obj-$(CONFIG_DRM_ARC_DW_HDMI) += arc-dw-hdmi.o
diff --git a/drivers/gpu/drm/arc/arc-dw-hdmi.c 
b/drivers/gpu/drm/arc/arc-dw-hdmi.c
new file mode 100644
index ..4869dd668a51
--- /dev/null
+++ b/drivers/gpu/drm/arc/arc-dw-hdmi.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Synopsys DW HDMI driver for various ARC development boards
+//
+// Copyright (C) 2020 Synopsys
+// Author: Eugeniy Paltsev 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static const struct dw_hdmi_mpll_config snps_hdmi_mpll_cfg[] = {
+   {
+   2700, {
+   { 0x00B3, 0x },
+   { 0x00B3, 0x },
+   { 0x00B3, 0x }
+   },
+   }, {
+   7425, {
+   { 0x0072, 0x0001},
+   { 0x0072, 0x0001},
+   { 0x0072, 0x0001}
+   },
+   }, {
+   14850, {
+   { 0x0051, 0x0002},
+   { 0x0051, 0x0002},
+   { 0x0051, 0x0002}
+   },
+   }, {
+   ~0UL, {
+   { 0x00B3, 0x },
+   { 0x00B3, 0x },
+   { 0x00B3, 0x },
+   },
+   }
+};
+
+static const struct dw_hdmi_curr_ctrl snps_hdmi_cur_ctr[] = {
+   /* pixelclkbpp8bpp10   bpp12 */
+   { 2700,  { 0x, 0x, 0x }, },
+   { 7425,  { 0x0008, 0x0008, 0x0008 }, },
+   { 14850, { 0x001b, 0x001b, 0x001b }, },
+   { ~0UL,  { 0x, 0x, 0x }, }
+};
+
+
+static const struct dw_hdmi_phy_config snps_hdmi_phy_config[] = {
+   /* pixelclk   symbol  termvlev */
+   { 2700,   0x8009, 0x0004, 0x0232},
+   { 7425,   0x8009, 0x0004, 0x0232},
+   { 14850,  0x8009, 0x0004, 0x0232},
+   { ~0UL,   0x8009, 0x0004, 0x0232}
+};
+
+static enum drm_mode_status snps_dw_hdmi_mode_valid(struct drm_connector *con,
+   const struct 
drm_display_mode *mode)
+{
+   return MODE_OK;
+}
+
+static struct dw_hdmi_plat_data snps_dw_hdmi_drv_data = {
+   .mpll_cfg   = snps_hdmi_mpll_cfg,
+   .cur_ctr= snps_hdmi_cur_ctr,
+   .phy_config = snps_hdmi_phy_config,
+   .mode_valid = snps_dw_hdmi_mode_valid,
+};
+
+static const struct of_device_id snps_dw_hdmi_dt_ids[] = {
+   {
+   

[PATCH 0/2] DRM: ARC: add HDMI 2.0 TX encoder support

[Eugeniy Paltsev]

Eugeniy Paltsev (2):
  DRM: ARC: add HDMI 2.0 TX encoder support
  dt-bindings: Document the Synopsys ARC HDMI TX bindings

 .../display/bridge/snps,arc-dw-hdmi.txt   |  73 ++
 MAINTAINERS   |   6 +
 drivers/gpu/drm/Makefile  |   2 +-
 drivers/gpu/drm/arc/Kconfig   |   7 +
 drivers/gpu/drm/arc/Makefile  |   1 +
 drivers/gpu/drm/arc/arc-dw-hdmi.c | 126 ++
 6 files changed, 214 insertions(+), 1 deletion(-)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
 create mode 100644 drivers/gpu/drm/arc/arc-dw-hdmi.c

-- 
2.21.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] dt-bindings: Document the Synopsys ARC HDMI TX bindings

[Eugeniy Paltsev]

This patch adds documentation of device tree bindings for the Synopsys
HDMI 2.0 TX encoder driver for ARC SoCs.

Signed-off-by: Eugeniy Paltsev 
---
 .../display/bridge/snps,arc-dw-hdmi.txt   | 73 +++
 1 file changed, 73 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt

diff --git 
a/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt 
b/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
new file mode 100644
index ..d5e006b392cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/snps,arc-dw-hdmi.txt
@@ -0,0 +1,73 @@
+Synopsys DesignWare HDMI 2.0 TX encoder driver for ARC SoCs
+
+
+The HDMI transmitter is a Synopsys DesignWare HDMI 2.0 TX controller IP
+with a companion of Synopsys DesignWare HDMI 2.0 TX PHY IP.
+
+These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
+Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
+with the following device-specific properties.
+
+
+Required properties:
+
+- compatible : Shall contain
+  - "snps,dw-hdmi-hsdk" for HSDK4xD compatible HDMI TX
+
+- reg: See dw_hdmi.txt.
+- interrupts: HDMI interrupt number.
+- clocks: See dw_hdmi.txt.
+- clock-names: Must contain "iahb" and "isfr" as defined in dw_hdmi.txt.
+- ports: See dw_hdmi.txt. The DWC HDMI shall have one port numbered 0
+  corresponding to the video input of the controller and one port numbered 1
+  corresponding to its HDMI output.
+
+Example:
+
+hdmi: hdmi@0x1 {
+   compatible = "snps,dw-hdmi-hsdk";
+   reg = <0x1 0x1>;
+   reg-io-width = <4>;
+   interrupts = <14>;
+   clocks = <>, <_pix_clk>;
+   clock-names = "iahb", "isfr";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   hdmi_enc_input: endpoint {
+   remote-endpoint = <_output>;
+   };
+   };
+
+   port@1 {
+   reg = <1>;
+   hdmi_enc_out: endpoint {
+   remote-endpoint = <_con>;
+   };
+   };
+   };
+};
+
+hdmi-out {
+   ...
+
+   port {
+   hdmi_con: endpoint {
+   remote-endpoint = <_enc_out>;
+   };
+   };
+};
+
+pgu {
+   ...
+
+   port_o: port {
+   pgu_output: endpoint {
+   remote-endpoint = <_enc_input>;
+   };
+   };
+};
-- 
2.21.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Daniel Stone]

Hi Tomek,

On Mon, 16 Mar 2020 at 12:55, Tomek Bury  wrote:
> I've been wrestling with the sync problems in Wayland some time ago, but only 
> with regards to 3D drivers.
>
> The guarantee given by the GL/GLES spec is limited to a single graphics 
> context. If the same buffer is accessed by 2 contexts the outcome is 
> unspecified. The cross-context and cross-process synchronisation is not 
> guaranteed. It happens to work on Mesa, because the read/write locking is 
> implemented in the kernel space, but it didn't work on Broadcom driver, which 
> has read-write interlocks in user space.

GL and GLES are not relevant. What is relevant is EGL, which defines
interfaces to make things work on the native platform. EGL doesn't
define any kind of synchronisation model for the Wayland, X11, or
GBM/KMS platforms - but it's one of the things which has to work. It
doesn't say that the implementation must make sure that the requested
format is displayable, but you sort of take it for granted that if you
ask EGL to display something it will do so.

Synchronisation is one of those mechanisms which is left to the
platform to implement under the hood. In the absence of platform
support for explicit synchronisation, the synchronisation must be
implicit.

>  A Vulkan client makes it even worse because of conflicting requirements: 
> Vulkan's vkQueuePresentKHR() passes in a number of semaphores but disallows 
> waiting. Wayland WSI requires wl_surface_commit() to be called from 
> vkQueuePresentKHR() which does require a wait, unless a synchronisation 
> primitive representing Vulkan samaphores is passed between Vulkan client and 
> the compositor.

If you are using EGL_WL_bind_wayland_display, then one of the things
it is explicitly allowed/expected to do is to create a Wayland
protocol interface between client and compositor, which can be used to
pass buffer handles and metadata in a platform-specific way. Adding
synchronisation is also possible.

> The most troublesome part was Wayland buffer release mechanism, as it only 
> involves a CPU signalling over Wayland IPC, without any 3D driver 
> involvement. The choices were: explicit synchronisation extension or a buffer 
> copy in the compositor (i.e. compositor textures from the copy, so the client 
> can re-write the original), or some implicit synchronisation in kernel space 
> (but that wasn't an option in Broadcom driver).

You can add your own explicit synchronisation extension.

In every cross-process and cross-subsystem usecase, synchronisation is
obviously required. The two options for this are to implement kernel
support for implicit synchronisation (as everyone else has done), or
implement generic support for explicit synchronisation (as we have
been working on with implementations inside Weston and Exosphere at
least), or implement private support for explicit synchronisation, or
do nothing and then be surprised at the lack of synchronisation.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv7 6/6] drm/rockchip: Add support for afbc

[Emil Velikov]

On Wed, 11 Mar 2020 at 14:56, Andrzej Pietrasiewicz
 wrote:
>
> This patch adds support for afbc handling. afbc is a compressed format
> which reduces the necessary memory bandwidth.
>
> Co-developed-by: Mark Yao 
> Signed-off-by: Mark Yao 
> Signed-off-by: Andrzej Pietrasiewicz 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  43 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 137 +++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  17 +++
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  83 +++-
>  5 files changed, 276 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h 
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index c5b06048124e..e33c2dcd0d4b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -30,6 +30,7 @@ struct rockchip_crtc_state {
> int output_mode;
> int output_bpc;
> int output_flags;
> +   bool enable_afbc;
>  };
>  #define to_rockchip_crtc_state(s) \
> container_of(s, struct rockchip_crtc_state, base)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 221e72e71432..9b13c784b347 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -57,8 +57,49 @@ static const struct drm_mode_config_helper_funcs 
> rockchip_mode_config_helpers =
> .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>  };
>
> +static struct drm_framebuffer *
> +rockchip_fb_create(struct drm_device *dev, struct drm_file *file,
> +  const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +   struct drm_afbc_framebuffer *afbc_fb;
> +   const struct drm_format_info *info;
> +   int ret;
> +
> +   info = drm_get_format_info(dev, mode_cmd);
> +   if (!info)
> +   return ERR_PTR(-ENOMEM);
> +
> +   afbc_fb = kzalloc(sizeof(*afbc_fb), GFP_KERNEL);
> +   if (!afbc_fb)
> +   return ERR_PTR(-ENOMEM);
> +
> +   ret = drm_gem_fb_init_with_funcs(dev, _fb->base, file, mode_cmd,
> +_drm_fb_funcs);
> +   if (ret) {
> +   kfree(afbc_fb);
> +   return ERR_PTR(ret);
> +   }
> +
> +   if (drm_is_afbc(mode_cmd->modifier[0])) {
> +   int ret, i;
> +
> +   ret = drm_gem_fb_afbc_init(dev, mode_cmd, afbc_fb);
> +   if (ret) {
> +   struct drm_gem_object **obj = afbc_fb->base.obj;
> +
> +   for (i = 0; i < info->num_planes; ++i)
> +   drm_gem_object_put_unlocked(obj[i]);
> +
> +   kfree(afbc_fb);
> +   return ERR_PTR(ret);
> +   }
> +   }
> +
> +   return _fb->base;
> +}
> +
>  static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> -   .fb_create = drm_gem_fb_create_with_dirty,
> +   .fb_create = rockchip_fb_create,
> .output_poll_changed = drm_fb_helper_output_poll_changed,
> .atomic_check = drm_atomic_helper_check,
> .atomic_commit = drm_atomic_helper_commit,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index cecb2cc781f5..b87d22eb6ae1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -91,9 +91,22 @@
>  #define VOP_WIN_TO_INDEX(vop_win) \
> ((vop_win) - (vop_win)->vop->win)
>
> +#define VOP_AFBC_SET(vop, name, v) \
> +   do { \
> +   if ((vop)->data->afbc) \
> +   vop_reg_set((vop), &(vop)->data->afbc->name, \
> +   0, ~0, v, #name); \
> +   } while (0)
> +
>  #define to_vop(x) container_of(x, struct vop, crtc)
>  #define to_vop_win(x) container_of(x, struct vop_win, base)
>
> +#define AFBC_FMT_RGB5650x0
> +#define AFBC_FMT_U8U8U8U8  0x5
> +#define AFBC_FMT_U8U8U80x4
> +
> +#define AFBC_TILE_16x16BIT(4)
> +
>  /*
>   * The coefficients of the following matrix are all fixed points.
>   * The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the 
> offsets.
> @@ -274,6 +287,29 @@ static enum vop_data_format vop_convert_format(uint32_t 
> format)
> }
>  }
>
> +static int vop_convert_afbc_format(uint32_t format)
> +{
> +   switch (format) {
> +   case DRM_FORMAT_XRGB:
> +   case DRM_FORMAT_ARGB:
> +   case DRM_FORMAT_XBGR:
> +   case DRM_FORMAT_ABGR:
> +   return AFBC_FMT_U8U8U8U8;
> +   case DRM_FORMAT_RGB888:
> +   case DRM_FORMAT_BGR888:
> +   return AFBC_FMT_U8U8U8;
> +   case DRM_FORMAT_RGB565:
> +   case DRM_FORMAT_BGR565:
> +   return AFBC_FMT_RGB565;

Re: [PATCHv7 4/6] drm/arm/malidp: Allocate an afbc-specific drm_framebuffer

[Emil Velikov]

On Wed, 11 Mar 2020 at 14:56, Andrzej Pietrasiewicz
 wrote:
>
> Prepare for using generic afbc helpers.
>
> Use an existing helper which allows allocating a struct drm_framebuffer
> in the driver.
>
> afbc-specific checks should go after drm_gem_fb_init_with_funcs().
>
> Signed-off-by: Andrzej Pietrasiewicz 
> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 50 +---
>  1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c 
> b/drivers/gpu/drm/arm/malidp_drv.c
> index cafbd81e4c1e..b9715b19af94 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -269,13 +269,36 @@ static const struct drm_mode_config_helper_funcs 
> malidp_mode_config_helpers = {
> .atomic_commit_tail = malidp_atomic_commit_tail,
>  };
>
> +static const struct drm_framebuffer_funcs malidp_fb_funcs = {
> +   .destroy= drm_gem_fb_destroy,
> +   .create_handle  = drm_gem_fb_create_handle,
> +};
> +
>  static struct drm_framebuffer *
>  malidp_fb_create(struct drm_device *dev, struct drm_file *file,
>  const struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> +   struct drm_afbc_framebuffer *afbc_fb;
> +   const struct drm_format_info *info;
> +   int ret, i;
> +
> +   info = drm_get_format_info(dev, mode_cmd);
> +   if (!info)
> +   return ERR_PTR(-ENOMEM);
> +
The underlying implementation in drm_get_format_info() will throw a
WARN_ON() if we return NULL.
Returning ENOMEM here is misleading and EINVAL sounds better IMHO.

Regardless, of the above 1-5 are:
Reviewed-by: Emil Velikov 

I don't know much about Rockchip HW to review 6/6.

-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Sam Ravnborg]

Hi Mark.

On Mon, Mar 16, 2020 at 12:02:41PM +, Mark Brown wrote:
> On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:
> 
> > Independent bindings can be SPI slaves which for example is
> > the case for several panel bindings.
> 
> What is an "independent binding"?

For several panels we have device trees that looks like this:

spi {
#address-cells = <1>;
#size-cells = <0>;

panel@0 {
compatible = "kingdisplay,kd035g6-54nt";
reg = <0>;

spi-max-frequency = <3125000>;
spi-3wire;
spi-cs-high;
...


The bindings are child of the spi controller node, but not specified
in the same binding file as the spi controller node.

A lot of bindings repeats the descriptions of (some of) the
pi-slave properties.
To avoid introducing yet another set of redundant and maybe incomplete
SPI slave property descriptions I moved the relevant properties
from spi-controller.yaml to spi-slave.yaml.

So SPI slaves can now reference spi-slave.yaml to get access to
the SPI slave properties - and the copies can be avoided.
Likewise spi-controller.yml now references spi-slave.yaml.

This was the best way I saw it could be done.

This approach is used in several bindings in this patch set.

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Laurent Pinchart]

Hi Tomek,

On Mon, Mar 16, 2020 at 12:55:27PM +, Tomek Bury wrote:
> Hi Jason,
> 
> I've been wrestling with the sync problems in Wayland some time ago, but only
> with regards to 3D drivers.
> 
> The guarantee given by the GL/GLES spec is limited to a single graphics
> context. If the same buffer is accessed by 2 contexts the outcome is
> unspecified. The cross-context and cross-process synchronisation is not
> guaranteed. It happens to work on Mesa, because the read/write locking is
> implemented in the kernel space, but it didn't work on Broadcom driver, which
> has read-write interlocks in user space.
> 
>  A Vulkan client makes it even worse because of conflicting requirements:
> Vulkan's vkQueuePresentKHR() passes in a number of semaphores but disallows
> waiting. Wayland WSI requires wl_surface_commit() to be called from
> vkQueuePresentKHR() which does require a wait, unless a synchronisation
> primitive representing Vulkan samaphores is passed between Vulkan client and
> the compositor.
> 
> The most troublesome part was Wayland buffer release mechanism, as it only
> involves a CPU signalling over Wayland IPC, without any 3D driver involvement.
> The choices were: explicit synchronisation extension or a buffer copy in the
> compositor (i.e. compositor textures from the copy, so the client can re-write
> the original), or some implicit synchronisation in kernel space (but that
> wasn't an option in Broadcom driver).
> 
> With regards to V4L2, I believe it could easily work the same way as 3D
> drivers, i.e. pass a buffer+fence pair to the next stage. The encode always
> succeeds, but for capture or decode, the main problem is the uncertain 
> outcome,
> I believe? If we're fine with rendering or displaying an occasional broken
> frame, then buffer+fence pair would work too. The broken frame will go into 
> the
> pipeline, but application can drain the pipeline and start over once the
> capture works again.
> 
> To answer some points raised by Laurent (although I'm unfamiliar with the
> camera drivers):
> 
> > you don't know until capture complete in which buffer the frame has
> > been captured
>
> Surely you do, you only don't know in advance if the capture will be 
> successful

You do in kernelspace, but not in userspace at the moment, due to buffer
recycling.

> > but if an error occurs during capture, they can be recycled internally and
> > put to the back of the queue.
>
> That would have to change in order to use explicit synchronisation. Every
> started capture becomes immediately available as a buffer+fence pair. Fence is
> signalled once the capture is finished (successfully or otherwise). The buffer
> must not be reused until it's released, possibly with another fence - in that
> case the buffer must not be reused until the release fence is signalled.

We could certainly change this at least in some cases, but it would
break existing userspace that doesn't expect incorrect frames.

I'm however not sure we could change this behaviour in every case, there
may be hardware that can't provide a guarantee on the order in which
buffers will be used. I'm aware this wouldn't be compatible with
explicit synchronization, and that's my point: camera hardware may not
always support explicit synchronization. As long as we can fall back to
not using fences then we should be fine.

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ack to merge through DRM? WAS [PATCH v6 0/9] Huge page-table entries for TTM

[VMware]

On 3/4/20 11:28 AM, Thomas Hellström (VMware) wrote:

In order to reduce CPU usage [1] and in theory TLB misses this patchset enables
huge- and giant page-table entries for TTM and TTM-enabled graphics drivers.

Patch 1 and 2 introduce a vma_is_special_huge() function to make the mm code
take the same path as DAX when splitting huge- and giant page table entries,
(which currently means zapping the page-table entry and rely on re-faulting).

Patch 3 makes the mm code split existing huge page-table entries
on huge_fault fallbacks. Typically on COW or on buffer-objects that want
write-notify. COW and write-notification is always done on the lowest
page-table level. See the patch log message for additional considerations.

Patch 4 introduces functions to allow the graphics drivers to manipulate
the caching- and encryption flags of huge page-table entries without ugly
hacks.

Patch 5 implements the huge_fault handler in TTM.
This enables huge page-table entries, provided that the kernel is configured
to support transhuge pages, either by default or using madvise().
However, they are unlikely to be inserted unless the kernel buffer object
pfns and user-space addresses align perfectly. There are various options
here, but since buffer objects that reside in system pages typically start
at huge page boundaries if they are backed by huge pages, we try to enforce
buffer object starting pfns and user-space addresses to be huge page-size
aligned if their size exceeds a huge page-size. If pud-size transhuge
("giant") pages are enabled by the arch, the same holds for those.

Patch 6 implements a specialized huge_fault handler for vmwgfx.
The vmwgfx driver may perform dirty-tracking and needs some special code
to handle that correctly.

Patch 7 implements a drm helper to align user-space addresses according
to the above scheme, if possible.

Patch 8 implements a TTM range manager for vmwgfx that does the same for
graphics IO memory. This may later be reused by other graphics drivers
if necessary.

Patch 9 finally hooks up the helpers of patch 7 and 8 to the vmwgfx driver.
A similar change is needed for graphics drivers that want a reasonable
likelyhood of actually using huge page-table entries.

If a buffer object size is not huge-page or giant-page aligned,
its size will NOT be inflated by this patchset. This means that the buffer
object tail will use smaller size page-table entries and thus no memory
overhead occurs. Drivers that want to pay the memory overhead price need to
implement their own scheme to inflate buffer-object sizes.

PMD size huge page-table-entries have been tested with vmwgfx and found to
work well both with system memory backed and IO memory backed buffer objects.

PUD size giant page-table-entries have seen limited (fault and COW) testing
using a modified kernel (to support 1GB page allocations) and a fake vmwgfx
TTM memory type. The vmwgfx driver does otherwise not support 1GB-size IO
memory resources.

Comments and suggestions welcome.
Thomas

Changes since RFC:
* Check for buffer objects present in contigous IO Memory (Christian König)
* Rebased on the vmwgfx emulated coherent memory functionality. That rebase
   adds patch 5.
Changes since v1:
* Make the new TTM range manager vmwgfx-specific. (Christian König)
* Minor fixes for configs that don't support or only partially support
   transhuge pages.
Changes since v2:
* Minor coding style and doc fixes in patch 5/9 (Christian König)
* Patch 5/9 doesn't touch mm. Remove from the patch title.
Changes since v3:
* Added reviews and acks
* Implemented ugly but generic ttm_pgprot_is_wrprotecting() instead of arch
   specific code.
Changes since v4:
* Added timings (Andrew Morton)
* Updated function documentation (Andrew Morton)
Changes since v6:
* Fix drm build error with !CONFIG_MMU

[1]
The below test program generates the following gnu time output when run on a
vmwgfx-enabled kernel without the patch series:

4.78user 6.02system 0:10.91elapsed 99%CPU (0avgtext+0avgdata 1624maxresident)k
0inputs+0outputs (0major+640077minor)pagefaults 0swaps

and with the patch series:

1.71user 3.60system 0:05.40elapsed 98%CPU (0avgtext+0avgdata 1656maxresident)k
0inputs+0outputs (0major+20079minor)pagefaults 0swaps

A consistent number of reduced graphics page-faults can be seen with normal
graphics applications, but due to the aggressive buffer object caching in
vmwgfx user-space drivers the CPU time reduction is within the error marginal.

#include 
#include 
#include 
#include 

static void checkerr(int ret, const char *name)
{
   if (ret < 0) {
 perror(name);
 exit(-1);
   }
}

int main(int agc, const char *argv[])
{
 struct drm_mode_create_dumb c_arg = {0};
 struct drm_mode_map_dumb m_arg = {0};
 struct drm_mode_destroy_dumb d_arg = {0};
 int ret, i, fd;
 void *map;

 fd = open("/dev/dri/card0", O_RDWR);
 checkerr(fd, argv[0]);

 for (i = 0; i < 1; ++i) {
   c_arg.bpp = 32;
   c_arg.width = 1024;
   

Re: [PATCH v1 02/36] dt-bindings: spi: support non-spi bindings as SPI slaves

[Mark Brown]

On Sun, Mar 15, 2020 at 02:43:42PM +0100, Sam Ravnborg wrote:

> Independent bindings can be SPI slaves which for example is
> the case for several panel bindings.

What is an "independent binding"?

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[git-pull] vmwgfx-next

[Thomas Hellstrom (VMware)]

Dave, Daniel,

The first vmwgfx-next pull for 5.7. Roland Scheidegger will follow up with
a larger pull request for functionality needed for GL4 support.

- Disable DMA when using SEV encryption
- An -RT fix
- Code cleanups

Thanks,
Thomas

The following changes since commit d3bd37f587b4438d47751d0f1d5aaae3d39bd416:

  Merge v5.6-rc5 into drm-next (2020-03-11 07:27:21 +1000)

are available in the Git repository at:

  git://people.freedesktop.org/~thomash/linux vmwgfx-next

for you to fetch changes up to 6b656755428dc0c96d21d7af697dc2a10c7ff175:

  drm/vmwgfx: Replace zero-length array with flexible-array member (2020-03-16 
10:42:01 +0100)


Gustavo A. R. Silva (1):
  drm/vmwgfx: Replace zero-length array with flexible-array member

Sebastian Andrzej Siewior (2):
  drm/vmwgfx: Drop preempt_disable() in vmw_fifo_ping_host()
  drm/vmwgfx: Remove a few unused functions

Thomas Hellstrom (2):
  drm/vmwgfx: Fix the refuse_dma mode when using guest-backed objects
  drm/vmwgfx: Refuse DMA operation when SEV encryption is active

 drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf.c |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 11 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 28 ---
 drivers/gpu/drm/vmwgfx/vmwgfx_fifo.c   |  2 -
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c| 81 --
 drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c| 31 
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c   |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c|  2 +-
 9 files changed, 14 insertions(+), 148 deletions(-)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

[Laurent Pinchart]

On Wed, Mar 11, 2020 at 04:18:55PM -0400, Nicolas Dufresne wrote:
> (I know I'm going to be spammed by so many mailing list ...)
> 
> Le mercredi 11 mars 2020 à 14:21 -0500, Jason Ekstrand a écrit :
> > On Wed, Mar 11, 2020 at 12:31 PM Jason Ekstrand  
> > wrote:
> > > All,
> > > 
> > > Sorry for casting such a broad net with this one. I'm sure most people
> > > who reply will get at least one mailing list rejection.  However, this
> > > is an issue that affects a LOT of components and that's why it's
> > > thorny to begin with.  Please pardon the length of this e-mail as
> > > well; I promise there's a concrete point/proposal at the end.
> > > 
> > > 
> > > Explicit synchronization is the future of graphics and media.  At
> > > least, that seems to be the consensus among all the graphics people
> > > I've talked to.  I had a chat with one of the lead Android graphics
> > > engineers recently who told me that doing explicit sync from the start
> > > was one of the best engineering decisions Android ever made.  It's
> > > also the direction being taken by more modern APIs such as Vulkan.
> > > 
> > > 
> > > ## What are implicit and explicit synchronization?
> > > 
> > > For those that aren't familiar with this space, GPUs, media encoders,
> > > etc. are massively parallel and synchronization of some form is
> > > required to ensure that everything happens in the right order and
> > > avoid data races.  Implicit synchronization is when bits of work (3D,
> > > compute, video encode, etc.) are implicitly based on the absolute
> > > CPU-time order in which API calls occur.  Explicit synchronization is
> > > when the client (whatever that means in any given context) provides
> > > the dependency graph explicitly via some sort of synchronization
> > > primitives.  If you're still confused, consider the following
> > > examples:
> > > 
> > > With OpenGL and EGL, almost everything is implicit sync.  Say you have
> > > two OpenGL contexts sharing an image where one writes to it and the
> > > other textures from it.  The way the OpenGL spec works, the client has
> > > to make the API calls to render to the image before (in CPU time) it
> > > makes the API calls which texture from the image.  As long as it does
> > > this (and maybe inserts a glFlush?), the driver will ensure that the
> > > rendering completes before the texturing happens and you get correct
> > > contents.
> > > 
> > > Implicit synchronization can also happen across processes.  Wayland,
> > > for instance, is currently built on implicit sync where the client
> > > does their rendering and then does a hand-off (via wl_surface::commit)
> > > to tell the compositor it's done at which point the compositor can now
> > > texture from the surface.  The hand-off ensures that the client's
> > > OpenGL API calls happen before the server's OpenGL API calls.
> > > 
> > > A good example of explicit synchronization is the Vulkan API.  There,
> > > a client (or multiple clients) can simultaneously build command
> > > buffers in different threads where one of those command buffers
> > > renders to an image and the other textures from it and then submit
> > > both of them at the same time with instructions to the driver for
> > > which order to execute them in.  The execution order is described via
> > > the VkSemaphore primitive.  With the new VK_KHR_timeline_semaphore
> > > extension, you can even submit the work which does the texturing
> > > BEFORE the work which does the rendering and the driver will sort it
> > > out.
> > > 
> > > The #1 problem with implicit synchronization (which explicit solves)
> > > is that it leads to a lot of over-synchronization both in client space
> > > and in driver/device space.  The client has to synchronize a lot more
> > > because it has to ensure that the API calls happen in a particular
> > > order.  The driver/device have to synchronize a lot more because they
> > > never know what is going to end up being a synchronization point as an
> > > API call on another thread/process may occur at any time.  As we move
> > > to more and more multi-threaded programming this synchronization (on
> > > the client-side especially) becomes more and more painful.
> > > 
> > > 
> > > ## Current status in Linux
> > > 
> > > Implicit synchronization in Linux works via a the kernel's internal
> > > dma_buf and dma_fence data structures.  A dma_fence is a tiny object
> > > which represents the "done" status for some bit of work.  Typically,
> > > dma_fences are created as a by-product of someone submitting some bit
> > > of work (say, 3D rendering) to the kernel.  The dma_buf object has a
> > > set of dma_fences on it representing shared (read) and exclusive
> > > (write) access to the object.  When work is submitted which, for
> > > instance renders to the dma_buf, it's queued waiting on all the fences
> > > on the dma_buf and and a dma_fence is created representing the end of
> > > said rendering work and it's installed as the dma_buf's 

Re: [Intel-gfx] [PATCH] drm/mm: Allow drm_mm_initialized() to be used outside of the locks

[Daniel Vetter]

On Mon, Mar 09, 2020 at 12:15:29PM +, Chris Wilson wrote:
> Mark up the potential racy read in drm_mm_initialized(), as we want a
> cheap and cheerful check:
> 
> [  121.098731] BUG: KCSAN: data-race in _i915_gem_object_create_stolen [i915] 
> / rm_hole
> [  121.098766]
> [  121.098789] write (marked) to 0x8881f01ed330 of 8 bytes by task 3568 
> on cpu 3:
> [  121.098831]  rm_hole+0x64/0x140
> [  121.098860]  drm_mm_insert_node_in_range+0x3d3/0x6c0
> [  121.099254]  i915_gem_stolen_insert_node_in_range+0x91/0xe0 [i915]
> [  121.099646]  _i915_gem_object_create_stolen+0x9d/0x100 [i915]
> [  121.100047]  i915_gem_object_create_region+0x7a/0xa0 [i915]
> [  121.100451]  i915_gem_object_create_stolen+0x33/0x50 [i915]
> [  121.100849]  intel_engine_create_ring+0x1af/0x280 [i915]
> [  121.101242]  __execlists_context_alloc+0xce/0x3d0 [i915]
> [  121.101635]  execlists_context_alloc+0x25/0x40 [i915]
> [  121.102030]  intel_context_alloc_state+0xb6/0xf0 [i915]
> [  121.102420]  __intel_context_do_pin+0x1ff/0x220 [i915]
> [  121.102815]  i915_gem_do_execbuffer+0x46b4/0x4c20 [i915]
> [  121.103211]  i915_gem_execbuffer2_ioctl+0x2c3/0x580 [i915]
> [  121.103244]  drm_ioctl_kernel+0xe4/0x120
> [  121.103269]  drm_ioctl+0x297/0x4c7
> [  121.103296]  ksys_ioctl+0x89/0xb0
> [  121.103321]  __x64_sys_ioctl+0x42/0x60
> [  121.103349]  do_syscall_64+0x6e/0x2c0
> [  121.103377]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  121.103403]
> [  121.103426] read to 0x8881f01ed330 of 8 bytes by task 3109 on cpu 1:
> [  121.103819]  _i915_gem_object_create_stolen+0x30/0x100 [i915]
> [  121.104228]  i915_gem_object_create_region+0x7a/0xa0 [i915]
> [  121.104631]  i915_gem_object_create_stolen+0x33/0x50 [i915]
> [  121.105025]  intel_engine_create_ring+0x1af/0x280 [i915]
> [  121.105420]  __execlists_context_alloc+0xce/0x3d0 [i915]
> [  121.105818]  execlists_context_alloc+0x25/0x40 [i915]
> [  121.106202]  intel_context_alloc_state+0xb6/0xf0 [i915]
> [  121.106595]  __intel_context_do_pin+0x1ff/0x220 [i915]
> [  121.106985]  i915_gem_do_execbuffer+0x46b4/0x4c20 [i915]
> [  121.107375]  i915_gem_execbuffer2_ioctl+0x2c3/0x580 [i915]
> [  121.107409]  drm_ioctl_kernel+0xe4/0x120
> [  121.107437]  drm_ioctl+0x297/0x4c7
> [  121.107464]  ksys_ioctl+0x89/0xb0
> [  121.107489]  __x64_sys_ioctl+0x42/0x60
> [  121.107511]  do_syscall_64+0x6e/0x2c0
> [  121.107535]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Signed-off-by: Chris Wilson 

A bit on the fence on this, but also kinda the only option.

Reviewed-by: Daniel Vetter 

> ---
>  include/drm/drm_mm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index d7939c054259..ee8b0e80ca90 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -272,7 +272,7 @@ static inline bool drm_mm_node_allocated(const struct 
> drm_mm_node *node)
>   */
>  static inline bool drm_mm_initialized(const struct drm_mm *mm)
>  {
> - return mm->hole_stack.next;
> + return READ_ONCE(mm->hole_stack.next);
>  }
>  
>  /**
> -- 
> 2.20.1
> 
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Mark up racy check of drm_gem_object.handle_count

[Daniel Vetter]

On Mon, Mar 09, 2020 at 12:01:51PM +, Chris Wilson wrote:
> [ 1715.899800] BUG: KCSAN: data-race in drm_gem_handle_create_tail / 
> drm_gem_object_handle_put_unlocked
> [ 1715.899838]
> [ 1715.899861] write to 0x8881830f3604 of 4 bytes by task 7834 on cpu 1:
> [ 1715.899896]  drm_gem_handle_create_tail+0x62/0x250
> [ 1715.899927]  drm_gem_open_ioctl+0xc1/0x160
> [ 1715.899956]  drm_ioctl_kernel+0xe4/0x120
> [ 1715.899981]  drm_ioctl+0x297/0x4c7
> [ 1715.93]  ksys_ioctl+0x89/0xb0
> [ 1715.900027]  __x64_sys_ioctl+0x42/0x60
> [ 1715.900052]  do_syscall_64+0x6e/0x2c0
> [ 1715.900079]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1715.900100]
> [ 1715.900119] read to 0x8881830f3604 of 4 bytes by task 8137 on cpu 0:
> [ 1715.900149]  drm_gem_object_handle_put_unlocked+0x31/0x130
> [ 1715.900180]  drm_gem_object_release_handle+0x93/0xe0
> [ 1715.900208]  drm_gem_handle_delete+0x7b/0xe0
> [ 1715.900235]  drm_gem_close_ioctl+0x61/0x80
> [ 1715.900264]  drm_ioctl_kernel+0xe4/0x120
> [ 1715.900291]  drm_ioctl+0x297/0x4c7
> [ 1715.900316]  ksys_ioctl+0x89/0xb0
> [ 1715.900340]  __x64_sys_ioctl+0x42/0x60
> [ 1715.900363]  do_syscall_64+0x6e/0x2c0
> [ 1715.900388]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

I'm impressed. The dream that we all get replaced by some scripts might be
real :-)

> Signed-off-by: Chris Wilson 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/drm_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index a9e4a610445a..37627d06fb06 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -218,7 +218,7 @@ drm_gem_object_handle_put_unlocked(struct drm_gem_object 
> *obj)
>   struct drm_device *dev = obj->dev;
>   bool final = false;
>  
> - if (WARN_ON(obj->handle_count == 0))
> + if (WARN_ON(READ_ONCE(obj->handle_count) == 0))
>   return;
>  
>   /*
> -- 
> 2.20.1
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Mesa-dev] Plumbing explicit synchronization through the Linux ecosystem

[Michel Dänzer]

On 2020-03-16 4:50 a.m., Marek Olšák wrote:
> The synchronization works because the Mesa driver waits for idle (drains
> the GFX pipeline) at the end of command buffers and there is only 1
> graphics queue, so everything is ordered.
> 
> The GFX pipeline runs asynchronously to the command buffer, meaning the
> command buffer only starts draws and doesn't wait for completion. If the
> Mesa driver didn't wait at the end of the command buffer, the command
> buffer would finish and a different process could start execution of its
> own command buffer while shaders of the previous process are still running.
> 
> If the Mesa driver submits a command buffer internally (because it's full),
> it doesn't wait, so the GFX pipeline doesn't notice that a command buffer
> ended and a new one started.
> 
> The waiting at the end of command buffers happens only when the flush is
> external (Swap buffers, glFlush).
> 
> It's a performance problem, because the GFX queue is blocked until the GFX
> pipeline is drained at the end of every frame at least.
> 
> So explicit fences for SwapBuffers would help.

Not sure what difference it would make, since the same thing needs to be
done for explicit fences as well, doesn't it?


-- 
Earthling Michel Dänzer   |   https://redhat.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] MAINTAINERS: adjust to reservation.h renaming

[Daniel Vetter]

On Mon, Mar 16, 2020 at 10:50:07AM +0100, Daniel Vetter wrote:
> On Fri, Mar 06, 2020 at 02:56:06AM -0800, Joe Perches wrote:
> > On Fri, 2020-03-06 at 11:39 +0100, Daniel Vetter wrote:
> > > On Wed, Mar 04, 2020 at 01:08:32PM +0100, Christian König wrote:
> > > > Am 04.03.20 um 13:07 schrieb Lukas Bulwahn:
> > > > > Commit 52791eeec1d9 ("dma-buf: rename reservation_object to dma_resv")
> > > > > renamed include/linux/reservation.h to include/linux/dma-resv.h, but
> > > > > missed the reference in the MAINTAINERS entry.
> > > > > 
> > > > > Since then, ./scripts/get_maintainer.pl --self-test complains:
> > > > > 
> > > > >warning: no file matches F: include/linux/reservation.h
> > > > > 
> > > > > Adjust the DMA BUFFER SHARING FRAMEWORK entry in MAINTAINERS.
> > > > > 
> > > > > Co-developed-by: Sebastian Duda 
> > > > > Signed-off-by: Sebastian Duda 
> > > > > Signed-off-by: Lukas Bulwahn 
> > > > 
> > > > Reviewed-by: Christian König 
> > > 
> > > You'll push this too?
> > > -Daniel
> > > 
> > > > > ---
> > > > > Christian, please pick this patch.
> > > > > applies cleanly on current master and next-20200303
> > > > > 
> > > > >   MAINTAINERS | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index 6158a143a13e..3d6cb2789c9e 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -5022,7 +5022,7 @@ L:  dri-devel@lists.freedesktop.org
> > > > >   L:  linaro-mm-...@lists.linaro.org (moderated for non-subscribers)
> > > > >   F:  drivers/dma-buf/
> > > > >   F:  include/linux/dma-buf*
> > > > > -F:   include/linux/reservation.h
> > > > > +F:   include/linux/dma-resv.h
> > > > >   F:  include/linux/*fence.h
> > > > >   F:  Documentation/driver-api/dma-buf.rst
> > > > >   K:  dma_(buf|fence|resv)
> > 
> > Slightly unrelated:
> > 
> > The K: entry matches a lot of other things
> > and may have a lot of false positive matches
> > like any variable named dma_buffer
> > 
> > This should also use (?:...) to avoid a perl
> > capture group.
> > 
> > Perhaps:
> > 
> > K:  '\bdma_(?:buf|fence|resv)\b'
> 
> Hm either people aren't using get_maintainers.pl consistently, or it
> doesn't seem to be a real world problem. I'm not seeing any unrelated
> patches on dri-devel at least.
> 
> But happy to merge such a patch if it shows up ofc, it's definitely the
> more correct thing :-)

Ofc as usual if you lean out the window you immediately get to eat your
hat, right after sending this I got a mail from syzbot about some random
stuff because of this :-)

I'm gonna do the patch now ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/edid: Distribute switch variables for initialization

[Daniel Vetter]

On Fri, Mar 06, 2020 at 09:32:13AM -0800, Kees Cook wrote:
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
> 
> To avoid these problems, lift such variables up into the next code
> block.
> 
> drivers/gpu/drm/drm_edid.c: In function ‘drm_edid_to_eld’:
> drivers/gpu/drm/drm_edid.c:4395:9: warning: statement will never be
> executed [-Wswitch-unreachable]
>  4395 | int sad_count;
>   | ^
> 
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
> 
> Signed-off-by: Kees Cook 

Thanks for your patch, applied to drm-misc-next.
-Daniel

> ---
> v2: move into function block instead being switch-local (Ville Syrjälä)
> ---
>  drivers/gpu/drm/drm_edid.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 805fb004c8eb..46cee78bc175 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4381,6 +4381,7 @@ static void drm_edid_to_eld(struct drm_connector 
> *connector, struct edid *edid)
>  
>   if (cea_revision(cea) >= 3) {
>   int i, start, end;
> + int sad_count;
>  
>   if (cea_db_offsets(cea, , )) {
>   start = 0;
> @@ -4392,8 +4393,6 @@ static void drm_edid_to_eld(struct drm_connector 
> *connector, struct edid *edid)
>   dbl = cea_db_payload_len(db);
>  
>   switch (cea_db_tag(db)) {
> - int sad_count;
> -
>   case AUDIO_BLOCK:
>   /* Audio Data Block, contains SADs */
>   sad_count = min(dbl / 3, 15 - total_sad_count);
> -- 
> 2.20.1
> 
> 
> -- 
> Kees Cook

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] MAINTAINERS: adjust to reservation.h renaming

[Daniel Vetter]

On Fri, Mar 06, 2020 at 02:56:06AM -0800, Joe Perches wrote:
> On Fri, 2020-03-06 at 11:39 +0100, Daniel Vetter wrote:
> > On Wed, Mar 04, 2020 at 01:08:32PM +0100, Christian König wrote:
> > > Am 04.03.20 um 13:07 schrieb Lukas Bulwahn:
> > > > Commit 52791eeec1d9 ("dma-buf: rename reservation_object to dma_resv")
> > > > renamed include/linux/reservation.h to include/linux/dma-resv.h, but
> > > > missed the reference in the MAINTAINERS entry.
> > > > 
> > > > Since then, ./scripts/get_maintainer.pl --self-test complains:
> > > > 
> > > >warning: no file matches F: include/linux/reservation.h
> > > > 
> > > > Adjust the DMA BUFFER SHARING FRAMEWORK entry in MAINTAINERS.
> > > > 
> > > > Co-developed-by: Sebastian Duda 
> > > > Signed-off-by: Sebastian Duda 
> > > > Signed-off-by: Lukas Bulwahn 
> > > 
> > > Reviewed-by: Christian König 
> > 
> > You'll push this too?
> > -Daniel
> > 
> > > > ---
> > > > Christian, please pick this patch.
> > > > applies cleanly on current master and next-20200303
> > > > 
> > > >   MAINTAINERS | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 6158a143a13e..3d6cb2789c9e 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -5022,7 +5022,7 @@ L:dri-devel@lists.freedesktop.org
> > > >   L:linaro-mm-...@lists.linaro.org (moderated for non-subscribers)
> > > >   F:drivers/dma-buf/
> > > >   F:include/linux/dma-buf*
> > > > -F: include/linux/reservation.h
> > > > +F: include/linux/dma-resv.h
> > > >   F:include/linux/*fence.h
> > > >   F:Documentation/driver-api/dma-buf.rst
> > > >   K:dma_(buf|fence|resv)
> 
> Slightly unrelated:
> 
> The K: entry matches a lot of other things
> and may have a lot of false positive matches
> like any variable named dma_buffer
> 
> This should also use (?:...) to avoid a perl
> capture group.
> 
> Perhaps:
> 
> K:'\bdma_(?:buf|fence|resv)\b'

Hm either people aren't using get_maintainers.pl consistently, or it
doesn't seem to be a real world problem. I'm not seeing any unrelated
patches on dri-devel at least.

But happy to merge such a patch if it shows up ofc, it's definitely the
more correct thing :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib/scatterlist: add sg_set_dma_addr() function

[Christian König]

Am 16.03.20 um 09:56 schrieb Christoph Hellwig:

On Fri, Mar 13, 2020 at 09:17:42AM -0300, Jason Gunthorpe wrote:

On Fri, Mar 13, 2020 at 04:21:39AM -0700, Christoph Hellwig wrote:

On Thu, Mar 12, 2020 at 11:19:28AM -0300, Jason Gunthorpe wrote:

The non-page scatterlist is also a big concern for RDMA as we have
drivers that want the page list, so even if we did as this series
contemplates I'd have still have to split the drivers and create the
notion of a dma-only SGL.

The drivers I looked at want a list of IOVA address, aligned to the
device "page size".  What other data do drivers want?  Execept for the
software protocol stack drivers, which of couse need pages for the
stack futher down.

In principle it is possible to have just an aligned page list -
however the page size is variable, following certain rules, and today
the drivers still determine the correct page size largely on their
own.

Some progress was made recently to consolidate this, but more is
needed.

If the common code doesn't know the device page size in advance then
today's approach of sending largest possible dma mapped SGLs into the
device driver is best. The driver only has to do splitting.

The point was that drivers don't need pages, drivers need IOVAs.  In
what form they are stuffed into the hardware is the drivers problem.


Well I would prefer if the drivers can somehow express their 
requirements and get IOVA structures already in the form they need.


Converting the IOVA data from one form to another is sometimes quite 
costly. Especially when it is only temporarily needed.


Regards,
Christian.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 206575] [amdgpu] [drm] No video signal on resume from suspend, R9 380

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=206575

Duncan (dickvandr...@gmail.com) changed:

   What|Removed |Added

 CC||dickvandr...@gmail.com

--- Comment #13 from Duncan (dickvandr...@gmail.com) ---
I have the same problem. My graphic card is an AMD R9 285 and since some kernel
updates ago the resume from suspend dont work with a black screen output but
system and keyboard respond well.
My distro is Anarchy Linux with KDE desktop and SDDM like display manager.
Regards.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 35/36] dt-bindings: display: convert lgphilips, lb035q02 to DT Schema

[Tomi Valkeinen]

On 16/03/2020 10:53, Sam Ravnborg wrote:


We have panel-simple-dsi for DSI simple based panels.
This binding includes the reg property.

If we have included DSI panels in panel-simple.yaml, and we likely have
by accident, then they should be moved to panel-simple-dsi.yaml.

If they requires anything else then they shall have their
own binding.

panel-simple.yaml and panel-simple.dsi.yaml are on purpose
only for the simple panels and they have:
"additionalProperties: false" to avoid that a lot
of extra sneaks in.

I actually considered shortly a panel-simple-spi.yaml,
but the few panels I looked at had different names
for the power-supply so that did not fly.
I did not check them all - we have today (with this patch-set)
9 bindings that references spi-slave.yaml.


Okay, I understand now. Makes sense.

panel-simple.c has dsi_of_match, which lists DSI panels. I was looking at that when I said 
panel-simple binding has DSI panels. At least auo,b080uan01 and osddisplays,osd101t2045-53ts are 
there, and earlier in this series you moved osddisplays,osd101t2587-53ts to panel-simple bindings.


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 22/51] drm: manage drm_minor cleanup with drmm_

[Daniel Vetter]

On Wed, Mar 11, 2020 at 10:59:10AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.03.20 um 23:26 schrieb Daniel Vetter:
> > The cleanup here is somewhat tricky, since we can't tell apart the
> > allocated minor index from 0. So register a cleanup action first, and
> > if the index allocation fails, unregister that cleanup action again to
> > avoid bad mistakes.
> > 
> > The kdev for the minor already handles NULL, so no problem there.
> > 
> > Hence add drmm_remove_action() to the drm_managed library.
> > 
> > v2: Make pointer math around void ** consistent with what Laurent
> > suggested.
> > 
> > Cc: Laurent Pinchart 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_drv.c | 74 +--
> >  drivers/gpu/drm/drm_managed.c | 28 +
> >  include/drm/drm_managed.h |  4 ++
> >  3 files changed, 59 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 23e5b0e7e041..29d106195ab3 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -93,19 +93,35 @@ static struct drm_minor **drm_minor_get_slot(struct 
> > drm_device *dev,
> > }
> >  }
> >  
> > +static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > +{
> > +   struct drm_minor *minor = data;
> > +   unsigned long flags;
> > +
> > +   put_device(minor->kdev);
> > +
> > +   spin_lock_irqsave(_minor_lock, flags);
> > +   idr_remove(_minors_idr, minor->index);
> > +   spin_unlock_irqrestore(_minor_lock, flags);
> > +}
> > +
> >  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> >  {
> > struct drm_minor *minor;
> > unsigned long flags;
> > int r;
> >  
> > -   minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> > +   minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > if (!minor)
> > return -ENOMEM;
> >  
> > minor->type = type;
> > minor->dev = dev;
> >  
> > +   r = drmm_add_action(dev, drm_minor_alloc_release, minor);
> > +   if (r)
> > +   return r;
> > +
> > idr_preload(GFP_KERNEL);
> > spin_lock_irqsave(_minor_lock, flags);
> > r = idr_alloc(_minors_idr,
> > @@ -116,47 +132,18 @@ static int drm_minor_alloc(struct drm_device *dev, 
> > unsigned int type)
> > spin_unlock_irqrestore(_minor_lock, flags);
> > idr_preload_end();
> >  
> > -   if (r < 0)
> > -   goto err_free;
> > +   if (r < 0) {
> > +   drmm_remove_action(dev, drm_minor_alloc_release, minor);
> > +   return r;
> > +   }
> >  
> > minor->index = r;
> > -
> > minor->kdev = drm_sysfs_minor_alloc(minor);
> > -   if (IS_ERR(minor->kdev)) {
> > -   r = PTR_ERR(minor->kdev);
> > -   goto err_index;
> > -   }
> > +   if (IS_ERR(minor->kdev))
> > +   return PTR_ERR(minor->kdev);
> >  
> > *drm_minor_get_slot(dev, type) = minor;
> > return 0;
> > -
> > -err_index:
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_remove(_minors_idr, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > -err_free:
> > -   kfree(minor);
> > -   return r;
> > -}
> 
> TBH, I think you're reducing code quality by removing the rollback code
> from init functions, just for the sake of it.
> 
> Specifically in this case here, you saved a few lines of code, but the
> overall flow is way more complicated to follow. That's typically a
> reliably source of bugs. This call to drmm_remove_action() just makes it
> worse.
> 
> Rather, see my remark on OOP destruction in patch 21. For now, I'd focus
> on the device cleanup and leave init functions as they are.

Ah, I can simplify this with add_action_or_reset. This removes the only
user of drmm_remove_action, which I think is actually a good thing :-)
-Daniel

> 
> Best regards
> Thomas
> 
> > -
> > -static void drm_minor_free(struct drm_device *dev, unsigned int type)
> > -{
> > -   struct drm_minor **slot, *minor;
> > -   unsigned long flags;
> > -
> > -   slot = drm_minor_get_slot(dev, type);
> > -   minor = *slot;
> > -   if (!minor)
> > -   return;
> > -
> > -   put_device(minor->kdev);
> > -
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_remove(_minors_idr, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > -
> > -   kfree(minor);
> > -   *slot = NULL;
> >  }
> >  
> >  static int drm_minor_register(struct drm_device *dev, unsigned int type)
> > @@ -678,16 +665,16 @@ int drm_dev_init(struct drm_device *dev,
> > if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> > ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
> > if (ret)
> > -   goto err_minors;
> > +   goto err;
> > }
> >  
> > ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> > if (ret)
> > -   goto err_minors;
> > +   goto err;
> >  
> > ret = drm_legacy_create_map_hash(dev);
> > if (ret)
> > -   goto err_minors;
> > +   goto 

Re: [PATCH] backlight: lp855x: Ensure regulators are disabled on probe failure

[Daniel Thompson]

On Fri, Mar 13, 2020 at 02:16:16PM +, Jon Hunter wrote:
> Hi Lee, Daniel,
> 
> On 24/02/2020 14:37, Daniel Thompson wrote:
> > On Mon, Feb 24, 2020 at 02:07:48PM +, Jon Hunter wrote:
> >> If probing the LP885x backlight fails after the regulators have been
> >> enabled, then the following warning is seen when releasing the
> >> regulators ...
> >>
> >>  WARNING: CPU: 1 PID: 289 at drivers/regulator/core.c:2051 
> >> _regulator_put.part.28+0x158/0x160
> >>  Modules linked in: tegra_xudc lp855x_bl(+) host1x pwm_tegra ip_tables 
> >> x_tables ipv6 nf_defrag_ipv6
> >>  CPU: 1 PID: 289 Comm: systemd-udevd Not tainted 5.6.0-rc2-next-20200224 #1
> >>  Hardware name: NVIDIA Jetson TX1 Developer Kit (DT)
> >>
> >>  ...
> >>
> >> Fix this by ensuring that the regulators are disabled, if enabled, on
> >> probe failure.
> >>
> >> Finally, ensure that the vddio regulator is disabled in the driver
> >> remove handler.
> >>
> >> Signed-off-by: Jon Hunter 
> > 
> > Reviewed-by: Daniel Thompson 
> I received a bounce from Milo's email and so I am not sure that his
> email address is still valid.
> 
> Can either of you pick this up?

Lee generally starts to hoover up patches about this stage in the dev
cycle so I'd expect this to move fairly soon.


> Not sure if we should update the MAINTAINERS as well?

Sounds like a good plan, yes.


Daniel.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 21/51] drm: Use drmm_ for drm_dev_init cleanup

[Daniel Vetter]

On Wed, Mar 11, 2020 at 10:39:13AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.03.20 um 23:26 schrieb Daniel Vetter:
> > Well for the simple stuff at least, vblank, gem and minor cleanup I
> > want to further split up as a demonstration.
> > 
> > v2: We need to clear drm_device->dev otherwise the debug drm printing
> > after our cleanup hook (e.g. in drm_manged_release) will chase
> > released memory and result in a use-after-free. Not really pretty, but
> > oh well.
> > 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_drv.c | 48 ---
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index ef79c03e311c..23e5b0e7e041 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -580,6 +580,23 @@ static void drm_fs_inode_free(struct inode *inode)
> >   *used.
> >   */
> >  
> > +static void drm_dev_init_release(struct drm_device *dev, void *res)
> > +{
> > +   drm_legacy_ctxbitmap_cleanup(dev);
> > +   drm_legacy_remove_map_hash(dev);
> > +   drm_fs_inode_free(dev->anon_inode);
> > +
> > +   put_device(dev->dev);
> > +   /* Prevent use-after-free in drm_managed_release when debugging is
> > +* enabled. Slightly awkward, but can't really be helped. */
> > +   dev->dev = NULL;
> > +   mutex_destroy(>master_mutex);
> > +   mutex_destroy(>clientlist_mutex);
> > +   mutex_destroy(>filelist_mutex);
> > +   mutex_destroy(>struct_mutex);
> > +   drm_legacy_destroy_members(dev);
> > +}
> > +
> >  /**
> >   * drm_dev_init - Initialise new DRM device
> >   * @dev: DRM device
> > @@ -647,11 +664,15 @@ int drm_dev_init(struct drm_device *dev,
> > mutex_init(>clientlist_mutex);
> > mutex_init(>master_mutex);
> >  
> > +   ret = drmm_add_action(dev, drm_dev_init_release, NULL);
> > +   if (ret)
> > +   return ret;
> > +
> 
> Is this code supposed to stay for the long term? As devices are
> allocated dynamically, I can imagine that there will be a call that
> allocates the memory and, at the same time, sets drm_dev_init_release()
> as the release callback.

There's a chicken-egg situation here. The plan is to fix this with a
devm_drm_dev_alloc() macro, which we discussed quite a bit in earlier
versions. It's just that the patch series is already big as-is, hence this
is postponed to the next round.

> The question is also released to patch 3, where I proposed to rename
> __drm_add_action() to __drmm_kzalloc().
> 
> > dev->anon_inode = drm_fs_inode_new();
> > if (IS_ERR(dev->anon_inode)) {
> > ret = PTR_ERR(dev->anon_inode);
> > DRM_ERROR("Cannot allocate anonymous inode: %d\n", ret);
> > -   goto err_free;
> > +   goto err;
> > }
> >  
> > if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> > @@ -688,19 +709,12 @@ int drm_dev_init(struct drm_device *dev,
> > if (drm_core_check_feature(dev, DRIVER_GEM))
> > drm_gem_destroy(dev);
> >  err_ctxbitmap:
> > -   drm_legacy_ctxbitmap_cleanup(dev);
> > -   drm_legacy_remove_map_hash(dev);
> >  err_minors:
> > drm_minor_free(dev, DRM_MINOR_PRIMARY);
> > drm_minor_free(dev, DRM_MINOR_RENDER);
> > -   drm_fs_inode_free(dev->anon_inode);
> > -err_free:
> > -   put_device(dev->dev);
> > -   mutex_destroy(>master_mutex);
> > -   mutex_destroy(>clientlist_mutex);
> > -   mutex_destroy(>filelist_mutex);
> > -   mutex_destroy(>struct_mutex);
> > -   drm_legacy_destroy_members(dev);
> > +err:
> > +   drm_managed_release(dev);
> > +
> 
> Here's more of a general observation than a comment on the actual patch:
> 
> One odd thing about the overall interface is that there's no way of
> updating the release callback afterwards. In an OOP language, such as
> C++, an error within the constructor would rollback the performed
> actions and return without calling the destructor. Destructors only run
> for fully constructed objects.
> 
> In our case, the equivalent is to run the init function and set
> drm_dev_init_release() as the final step. The init's rollback-code would
> have to stay, obviously.

See the various drivers later on in the series, the init rollback
completely disappears once we're done here. If this wouldn't Just Work for
both final destruction and init rollback there's really no point. There's
a few ugly corner-cases with getting this boot-strapped though.
-Daniel

> 
> Best regards
> Thomas
> 
> > return ret;
> >  }
> >  EXPORT_SYMBOL(drm_dev_init);
> > @@ -763,20 +777,8 @@ void drm_dev_fini(struct drm_device *dev)
> > if (drm_core_check_feature(dev, DRIVER_GEM))
> > drm_gem_destroy(dev);
> >  
> > -   drm_legacy_ctxbitmap_cleanup(dev);
> > -   drm_legacy_remove_map_hash(dev);
> > -   drm_fs_inode_free(dev->anon_inode);
> > -
> > drm_minor_free(dev, DRM_MINOR_PRIMARY);
> > drm_minor_free(dev, DRM_MINOR_RENDER);
> > -
> > -   put_device(dev->dev);
> > -
> > -   mutex_destroy(>master_mutex);

Re: [PATCH v1 35/36] dt-bindings: display: convert lgphilips, lb035q02 to DT Schema

[Tomi Valkeinen]

On 16/03/2020 10:26, Sam Ravnborg wrote:


Isn't this also compatible with panel-simple bindings? 'label' is the only
one not in panel-simple, but that's optional and has never been used by the
panel driver.

The panel is a SPI slave - which is not too obvious from the old
binding.

The new DT Schema includes spi/spi-slave.yaml to give the binding
proper access to the spi slave properties.

That would not be possible with panel-simple binding as no further
properties are allowed with the panel-simple binding.

I hope this explains why there is a dedicated binding for this panel.


Hmm, but how is this different than, say, DSI panels? There are DSI panels in panel-simple bindings, 
and those might require DSI bus parameters ('reg' in the minimum).


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 35/36] dt-bindings: display: convert lgphilips,lb035q02 to DT Schema

[Sam Ravnborg]

Hi Tomi.

On Mon, Mar 16, 2020 at 10:42:45AM +0200, Tomi Valkeinen wrote:
> On 16/03/2020 10:26, Sam Ravnborg wrote:
> 
> > > Isn't this also compatible with panel-simple bindings? 'label' is the only
> > > one not in panel-simple, but that's optional and has never been used by 
> > > the
> > > panel driver.
> > The panel is a SPI slave - which is not too obvious from the old
> > binding.
> > 
> > The new DT Schema includes spi/spi-slave.yaml to give the binding
> > proper access to the spi slave properties.
> > 
> > That would not be possible with panel-simple binding as no further
> > properties are allowed with the panel-simple binding.
> > 
> > I hope this explains why there is a dedicated binding for this panel.
> 
> Hmm, but how is this different than, say, DSI panels? There are DSI panels
> in panel-simple bindings, and those might require DSI bus parameters ('reg'
> in the minimum).


We have panel-simple-dsi for DSI simple based panels.
This binding includes the reg property.

If we have included DSI panels in panel-simple.yaml, and we likely have
by accident, then they should be moved to panel-simple-dsi.yaml.

If they requires anything else then they shall have their
own binding.

panel-simple.yaml and panel-simple.dsi.yaml are on purpose
only for the simple panels and they have:
"additionalProperties: false" to avoid that a lot
of extra sneaks in.

I actually considered shortly a panel-simple-spi.yaml,
but the few panels I looked at had different names
for the power-supply so that did not fly.
I did not check them all - we have today (with this patch-set)
9 bindings that references spi-slave.yaml.

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/51] drm: add managed resources tied to drm_device

[Daniel Vetter]

On Wed, Mar 11, 2020 at 10:14:03AM +0100, Thomas Zimmermann wrote:
> 
> 
> Am 02.03.20 um 23:25 schrieb Daniel Vetter:
> <...>
> > +
> > +int __drmm_add_action(struct drm_device *dev,
> > + drmres_release_t action,
> > + void *data, const char *name)
> > +{
> > +   struct drmres *dr;
> > +   void **void_ptr;
> > +
> > +   dr = alloc_dr(action, data ? sizeof(void*) : 0,
> > + GFP_KERNEL | __GFP_ZERO,
> > + dev_to_node(dev->dev));
> > +   if (!dr) {
> > +   drm_dbg_drmres(dev, "failed to add action %s for %p\n",
> > +  name, data);
> > +   return -ENOMEM;
> > +   }
> > +
> > +   dr->node.name = name;
> 
> Maybe do a kstrdup_const() on name and later a kfree_const() during
> release. Just in case someone decides to allocate 'name' dynamically.

Makes sense, but a bit of churn since I need a free_dr() helper now :-)
-Daniel

> 
> > +   if (data) {
> > +   void_ptr = (void **)>data;
> > +   *void_ptr = data;
> > +   }
> > +
> > +   add_dr(dev, dr);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(__drmm_add_action);
> > +
> > +void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp)
> > +{
> > +   struct drmres *dr;
> > +
> > +   dr = alloc_dr(NULL, size, gfp, dev_to_node(dev->dev));
> > +   if (!dr) {
> > +   drm_dbg_drmres(dev, "failed to allocate %zu bytes, %u flags\n",
> > +  size, gfp);
> > +   return NULL;
> > +   }
> > +   dr->node.name = "kmalloc";
> > +
> > +   add_dr(dev, dr);
> > +
> > +   return dr->data;
> > +}
> > +EXPORT_SYMBOL(drmm_kmalloc);
> > +
> > +void drmm_kfree(struct drm_device *dev, void *data)
> > +{
> > +   struct drmres *dr_match = NULL, *dr;
> > +   unsigned long flags;
> > +
> > +   if (!data)
> > +   return;
> > +
> > +   spin_lock_irqsave(>managed.lock, flags);
> > +   list_for_each_entry(dr, >managed.resources, node.entry) {
> > +   if (dr->data == data) {
> > +   dr_match = dr;
> > +   del_dr(dev, dr_match);
> > +   break;
> > +   }
> > +   }
> > +   spin_unlock_irqrestore(>managed.lock, flags);
> > +
> > +   if (WARN_ON(!dr_match))
> > +   return;
> > +
> > +   kfree(dr_match);
> > +}
> > +EXPORT_SYMBOL(drmm_kfree);
> > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > index bb60a949f416..d39132b477dd 100644
> > --- a/include/drm/drm_device.h
> > +++ b/include/drm/drm_device.h
> > @@ -67,6 +67,21 @@ struct drm_device {
> > /** @dev: Device structure of bus-device */
> > struct device *dev;
> >  
> > +   /**
> > +* @managed:
> > +*
> > +* Managed resources linked to the lifetime of this _device as
> > +* tracked by @ref.
> > +*/
> > +   struct {
> > +   /** @managed.resources: managed resources list */
> > +   struct list_head resources;
> > +   /** @managed.final_kfree: pointer for final kfree() call */
> > +   void *final_kfree;
> > +   /** @managed.lock: protects @managed.resources */
> > +   spinlock_t lock;
> > +   } managed;
> > +
> > /** @driver: DRM driver managing the device */
> > struct drm_driver *driver;
> >  
> > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> > new file mode 100644
> > index ..7b5df7d09b19
> > --- /dev/null
> > +++ b/include/drm/drm_managed.h
> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#ifndef _DRM_MANAGED_H_
> > +#define _DRM_MANAGED_H_
> > +
> > +#include 
> > +#include 
> > +
> > +struct drm_device;
> > +
> > +typedef void (*drmres_release_t)(struct drm_device *dev, void *res);
> > +
> > +#define drmm_add_action(dev, action, data) \
> > +   __drmm_add_action(dev, action, data, #action)
> > +
> > +int __must_check __drmm_add_action(struct drm_device *dev,
> > +  drmres_release_t action,
> > +  void *data, const char *name);
> > +
> > +void drmm_add_final_kfree(struct drm_device *dev, void *parent);
> > +
> > +void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp) 
> > __malloc;
> > +static inline void *drmm_kzalloc(struct drm_device *dev, size_t size, 
> > gfp_t gfp)
> > +{
> > +   return drmm_kmalloc(dev, size, gfp | __GFP_ZERO);
> > +}
> > +
> > +void drmm_kfree(struct drm_device *dev, void *data);
> > +
> > +#endif
> > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> > index ca7cee8e728a..1c9417430d08 100644
> > --- a/include/drm/drm_print.h
> > +++ b/include/drm/drm_print.h
> > @@ -313,6 +313,10 @@ enum drm_debug_category {
> >  * @DRM_UT_DP: Used in the DP code.
> >  */
> > DRM_UT_DP   = 0x100,
> > +   /**
> > +* @DRM_UT_DRMRES: Used in the drm managed resources code.
> > +*/
> > +   DRM_UT_DRMRES   = 0x200,
> >  };
> >  
> >  static inline bool drm_debug_enabled(enum drm_debug_category category)
> > @@ 

Re: [PATCH 03/51] drm: add managed resources tied to drm_device

[Daniel Vetter]

On Wed, Mar 11, 2020 at 10:07:13AM +0100, Thomas Zimmermann wrote:
> Hi Daniel
> 
> Am 02.03.20 um 23:25 schrieb Daniel Vetter:
> > We have lots of these. And the cleanup code tends to be of dubious
> > quality. The biggest wrong pattern is that developers use devm_, which
> > ties the release action to the underlying struct device, whereas
> > all the userspace visible stuff attached to a drm_device can long
> > outlive that one (e.g. after a hotunplug while userspace has open
> > files and mmap'ed buffers). Give people what they want, but with more
> > correctness.
> > 
> > Mostly copied from devres.c, with types adjusted to fit drm_device and
> > a few simplifications - I didn't (yet) copy over everything. Since
> > the types don't match code sharing looked like a hopeless endeavour.
> > 
> > For now it's only super simplified, no groups, you can't remove
> > actions (but kfree exists, we'll need that soon). Plus all specific to
> > drm_device ofc, including the logging. Which I didn't bother to make
> > compile-time optional, since none of the other drm logging is compile
> > time optional either.
> > 
> > One tricky bit here is the chicken between allocating your
> > drm_device structure and initiliazing it with drm_dev_init. For
> > perfect onion unwinding we'd need to have the action to kfree the
> > allocation registered before drm_dev_init registers any of its own
> > release handlers. But drm_dev_init doesn't know where exactly the
> > drm_device is emebedded into the overall structure, and by the time it
> > returns it'll all be too late. And forcing drivers to be able clean up
> > everything except the one kzalloc is silly.
> > 
> > Work around this by having a very special final_kfree pointer. This
> > also avoids troubles with the list head possibly disappearing from
> > underneath us when we release all resources attached to the
> > drm_device.
> > 
> > v2: Do all the kerneldoc at the end, to avoid lots of fairly pointless
> > shuffling while getting everything into shape.
> > 
> > v3: Add static to add/del_dr (Neil)
> > Move typo fix to the right patch (Neil)
> > 
> > v4: Enforce contract for drmm_add_final_kfree:
> > 
> > Use ksize() to check that the drm_device is indeed contained somewhere
> > in the final kfree(). Because we need that or the entire managed
> > release logic blows up in a pile of use-after-frees. Motivated by a
> > discussion with Laurent.
> > 
> > v5: Review from Laurent:
> > - %zu instead of casting size_t
> > - header guards
> > - sorting of includes
> > - guarding of data assignment if we didn't allocate it for a NULL
> >   pointer
> > - delete spurious newline
> > - cast void* data parameter correctly in ->release call, no idea how
> >   this even worked before
> > 
> > v3: Review from Sam
> > - Add the kerneldoc for the managed sub-struct back in, even if it
> >   doesn't show up in the generated html somehow.
> > - Explain why __always_inline.
> > - Fix bisectability around the final kfree() in drm_dev_relase(). This
> >   is just interim code which will disappear again.
> > - Some whitespace polish.
> > - Add debug output when drmm_add_action or drmm_kmalloc fail.
> > 
> > Cc: Sam Ravnborg 
> > Cc: Laurent Pinchart 
> > Cc: Neil Armstrong  > Cc: Greg Kroah-Hartman 
> > Cc: "Rafael J. Wysocki" 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  Documentation/gpu/drm-internals.rst |   6 +
> >  drivers/gpu/drm/Makefile|   3 +-
> >  drivers/gpu/drm/drm_drv.c   |  12 ++
> >  drivers/gpu/drm/drm_internal.h  |   3 +
> >  drivers/gpu/drm/drm_managed.c   | 186 
> >  include/drm/drm_device.h|  15 +++
> >  include/drm/drm_managed.h   |  30 +
> >  include/drm/drm_print.h |   6 +
> >  8 files changed, 260 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/drm_managed.c
> >  create mode 100644 include/drm/drm_managed.h
> > 
> > diff --git a/Documentation/gpu/drm-internals.rst 
> > b/Documentation/gpu/drm-internals.rst
> > index a73320576ca9..a6b6145fda78 100644
> > --- a/Documentation/gpu/drm-internals.rst
> > +++ b/Documentation/gpu/drm-internals.rst
> > @@ -132,6 +132,12 @@ be unmapped; on many devices, the ROM address decoder 
> > is shared with
> >  other BARs, so leaving it mapped could cause undesired behaviour like
> >  hangs or memory corruption.
> >  
> > +Managed Resources
> > +-
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_managed.c
> > +   :doc: managed resources
> > +
> >  Bus-specific Device Registration and PCI Support
> >  
> >  
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 7f72ef5e7811..183c60048307 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,8 @@ drm-y   :=drm_auth.o drm_cache.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o 

Re: [PATCH 10/21] drm/tegra: remove checks for debugfs functions return value

[Daniel Vetter]

On Thu, Mar 12, 2020 at 12:24:46AM +0100, Thierry Reding wrote:
> On Wed, Mar 11, 2020 at 05:54:46PM +0300, Wambui Karuga wrote:
> > Hey Thierry,
> > 
> > On Wed, 11 Mar 2020, Thierry Reding wrote:
> > 
> > > On Thu, Feb 27, 2020 at 03:02:21PM +0300, Wambui Karuga wrote:
> > > > Since 987d65d01356 (drm: debugfs: make
> > > > drm_debugfs_create_files() never fail) there is no need to check the
> > > > return value of drm_debugfs_create_files(). Therefore, remove the
> > > > return checks and error handling of the drm_debugfs_create_files()
> > > > function from various debugfs init functions in drm/tegra and have
> > > > them return 0 directly.
> > > > 
> > > > This change also includes removing the use of drm_debugfs_create_files
> > > > as a return value in tegra_debugfs_init() and have the function declared
> > > > as void.
> > > > 
> > > > Signed-off-by: Wambui Karuga 
> > > > ---
> > > >  drivers/gpu/drm/tegra/dc.c   | 11 +--
> > > >  drivers/gpu/drm/tegra/drm.c  |  8 
> > > >  drivers/gpu/drm/tegra/dsi.c  | 11 +--
> > > >  drivers/gpu/drm/tegra/hdmi.c | 11 +--
> > > >  drivers/gpu/drm/tegra/sor.c  | 11 +--
> > > >  5 files changed, 8 insertions(+), 44 deletions(-)
> > > 
> > > Applied, thanks.
> > > 
> > There's a newer version[1] of this patch series as this specific patch
> > depends on other work in drm.
> 
> Oh yeah, I just noticed that this patch causes a build failure, so I
> backed it out again.
> 
> If there's dependencies on other work, it's probably best to take this
> through drm-misc, in which case:
> 
> Acked-by: Thierry Reding 
> 
> Let me know if you'd prefer me to apply this to drm/tegra instead.

Yeah I'm going to pull in the entire series through -misc rsn.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/5] drm: Introduce scaling filter property

[Daniel Vetter]

On Tue, Mar 10, 2020 at 06:01:06PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 12:35:41PM +0530, Pankaj Bharadiya wrote:
> > Introduce new scaling filter property to allow userspace to select
> > the driver's default scaling filter or Nearest-neighbor(NN) filter
> > for upscaling operations on crtc/plane.
> > 
> > Drivers can set up this property for a plane by calling
> > drm_plane_enable_scaling_filter() and for a CRTC by calling
> > drm_crtc_enable_scaling_filter().
> > 
> > NN filter works by filling in the missing color values in the upscaled
> > image with that of the coordinate-mapped nearest source pixel value.
> > 
> > NN filter for integer multiple scaling can be particularly useful for
> > for pixel art games that rely on sharp, blocky images to deliver their
> > distinctive look.
> > 
> > Signed-off-by: Pankaj Bharadiya 
> > Signed-off-by: Shashank Sharma 
> > Signed-off-by: Ankit Nautiyal 
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  8 +++
> >  drivers/gpu/drm/drm_crtc.c| 16 ++
> >  drivers/gpu/drm/drm_mode_config.c | 13 
> >  drivers/gpu/drm/drm_plane.c   | 35 +++
> >  include/drm/drm_crtc.h| 10 +
> >  include/drm/drm_mode_config.h |  6 ++
> >  include/drm/drm_plane.h   | 14 +
> >  7 files changed, 102 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index a1e5e262bae2..4e3c1f3176e4 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc 
> > *crtc,
> > return ret;
> > } else if (property == config->prop_vrr_enabled) {
> > state->vrr_enabled = val;
> > +   } else if (property == config->scaling_filter_property) {
> > +   state->scaling_filter = val;
> 
> I think we want a per-plane/per-crtc prop for this. If we start adding
> more filters we are surely going to need different sets for different hw
> blocks.

In the past we've only done that once we have a demonstrated need. Usually
the patch to move the property to a per-object location isn't a lot of
churn.
-Daniel

> 
> > } else if (property == config->degamma_lut_property) {
> > ret = drm_atomic_replace_property_blob_from_id(dev,
> > >degamma_lut,
> > @@ -503,6 +505,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > else if (property == config->prop_out_fence_ptr)
> > *val = 0;
> > +   else if (property == config->scaling_filter_property)
> > +   *val = state->scaling_filter;
> > else if (crtc->funcs->atomic_get_property)
> > return crtc->funcs->atomic_get_property(crtc, state, property, 
> > val);
> > else
> > @@ -583,6 +587,8 @@ static int drm_atomic_plane_set_property(struct 
> > drm_plane *plane,
> > sizeof(struct drm_rect),
> > );
> > return ret;
> > +   } else if (property == config->scaling_filter_property) {
> > +   state->scaling_filter = val;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -641,6 +647,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > } else if (property == config->prop_fb_damage_clips) {
> > *val = (state->fb_damage_clips) ?
> > state->fb_damage_clips->base.id : 0;
> > +   } else if (property == config->scaling_filter_property) {
> > +   *val = state->scaling_filter;
> > } else if (plane->funcs->atomic_get_property) {
> > return plane->funcs->atomic_get_property(plane, state, 
> > property, val);
> > } else {
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 4936e1080e41..1ce7b2ac9eb5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -748,3 +748,19 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object 
> > *obj,
> >  
> > return ret;
> >  }
> > +
> > +/**
> > + * drm_crtc_enable_scaling_filter - Enables crtc scaling filter property.
> > + * @crtc: CRTC on which to enable scaling filter property.
> > + *
> > + * This function lets driver to enable the scaling filter property on a 
> > crtc.
> > + */
> > +void drm_crtc_enable_scaling_filter(struct drm_crtc *crtc)
> > +{
> > +   struct drm_device *dev = crtc->dev;
> > +
> > +   drm_object_attach_property(>base,
> > +  dev->mode_config.scaling_filter_property,
> > +  0);
> > +}
> > +EXPORT_SYMBOL(drm_crtc_enable_scaling_filter);
> > diff --git a/drivers/gpu/drm/drm_mode_config.c 
> > 

Re: [PATCH v1 35/36] dt-bindings: display: convert lgphilips,lb035q02 to DT Schema

[Sam Ravnborg]

Hi Tomi.

Thanks for your feedback.

On Mon, Mar 16, 2020 at 09:57:57AM +0200, Tomi Valkeinen wrote:
> Hi Sam,
> 
> On 15/03/2020 15:44, Sam Ravnborg wrote:
> > Signed-off-by: Sam Ravnborg 
> > Cc: Tomi Valkeinen 
> > Cc: Thierry Reding 
> > Cc: Sam Ravnborg 
> > ---
> >   .../display/panel/lgphilips,lb035q02.txt  | 33 
> >   .../display/panel/lgphilips,lb035q02.yaml | 54 +++
> >   2 files changed, 54 insertions(+), 33 deletions(-)
> >   delete mode 100644 
> > Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
> >   create mode 100644 
> > Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.yaml
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt 
> > b/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
> > deleted file mode 100644
> > index 1a1e653e5407..
> > --- a/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
> > +++ /dev/null
> > @@ -1,33 +0,0 @@
> > -LG.Philips LB035Q02 Panel
> > -=
> > -
> > -Required properties:
> > -- compatible: "lgphilips,lb035q02"
> > -- enable-gpios: panel enable gpio
> > -
> > -Optional properties:
> > -- label: a symbolic name for the panel
> > -
> > -Required nodes:
> > -- Video port for DPI input
> 
> Isn't this also compatible with panel-simple bindings? 'label' is the only
> one not in panel-simple, but that's optional and has never been used by the
> panel driver.
The panel is a SPI slave - which is not too obvious from the old
binding.

The new DT Schema includes spi/spi-slave.yaml to give the binding
proper access to the spi slave properties.

That would not be possible with panel-simple binding as no further
properties are allowed with the panel-simple binding.

I hope this explains why there is a dedicated binding for this panel.

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RESEND] drm/lease: fix potential race in fill_object_idr

[Qiujun Huang]

We should hold idr_mutex for idr_alloc.

Signed-off-by: Qiujun Huang 
---
 drivers/gpu/drm/drm_lease.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index b481caf..427ee21 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -418,6 +418,7 @@ static int fill_object_idr(struct drm_device *dev,
goto out_free_objects;
}
 
+   mutex_lock(>mode_config.idr_mutex);
/* add their IDs to the lease request - taking into account
   universal planes */
for (o = 0; o < object_count; o++) {
@@ -437,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
if (ret < 0) {
DRM_DEBUG_LEASE("Object %d cannot be inserted into 
leases (%d)\n",
object_id, ret);
-   goto out_free_objects;
+   goto out_unlock;
}
if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
struct drm_crtc *crtc = obj_to_crtc(obj);
@@ -445,20 +446,22 @@ static int fill_object_idr(struct drm_device *dev,
if (ret < 0) {
DRM_DEBUG_LEASE("Object primary plane %d cannot 
be inserted into leases (%d)\n",
object_id, ret);
-   goto out_free_objects;
+   goto out_unlock;
}
if (crtc->cursor) {
ret = idr_alloc(leases, _lease_idr_object, 
crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL);
if (ret < 0) {
DRM_DEBUG_LEASE("Object cursor plane %d 
cannot be inserted into leases (%d)\n",
object_id, ret);
-   goto out_free_objects;
+   goto out_unlock;
}
}
}
}
 
ret = 0;
+out_unlock:
+   mutex_unlock(>mode_config.idr_mutex);
 out_free_objects:
for (o = 0; o < object_count; o++) {
if (objects[o])
-- 
1.8.3.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 2/2] drm/panel: add support for rm69299 visionox panel driver

[Harigovindan P]

Add support for Visionox panel driver.

Signed-off-by: Harigovindan P 
---

Changes in v2:
- Dropping redundant space in Kconfig(Sam Ravnborg).
- Changing structure for include files(Sam Ravnborg).
- Removing backlight related code and functions(Sam Ravnborg).
- Removing repeated printing of error message(Sam Ravnborg).
- Adding drm_connector as an argument for get_modes function.
Changes in v3:
- Adding arguments for drm_panel_init to support against mainline.
Changes in v4:
- Removing error messages from regulator_set_load.
- Removing dev struct entry.
- Removing checks.
- Dropping empty comment lines.
Changes in v5:
- Removing unused struct member variables.
- Removing blank lines.
- Fixed indentation.
- Invoking dsi_detach and panel_remove while early exiting from probe.
Changes in v6:
- Changed "35597" to "rm69299" for power_on function.
- Removing rm69299_config since it supports single type of panel for 
now.
- Fixed alignment.
- Using goto statements when regulator_set_load fails.
Changes in v7:
- Added new goto statement when regulator_set_load fails.

 drivers/gpu/drm/panel/Kconfig |   8 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-visionox-rm69299.c| 295 ++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index ae44ac2ec106..7b696f304a99 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -389,6 +389,14 @@ config DRM_PANEL_TRULY_NT35597_WQXGA
  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual 
DSI
  Video Mode panel
 
+config DRM_PANEL_VISIONOX_RM69299
+   tristate "Visionox RM69299"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   help
+ Say Y here if you want to enable support for Visionox
+ RM69299  DSI Video Mode panel.
+
 config DRM_PANEL_XINPENG_XPP055C272
tristate "Xinpeng XPP055C272 panel driver"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 7c4d3c581fd4..9f11d067a6b2 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -41,4 +41,5 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += 
panel-tpo-td028ttec1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
+obj-$(CONFIG_DRM_PANEL_VISIONOX_RM69299) += panel-visionox-rm69299.o
 obj-$(CONFIG_DRM_PANEL_XINPENG_XPP055C272) += panel-xinpeng-xpp055c272.o
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c 
b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
new file mode 100644
index ..0f877d21fdf2
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+struct visionox_rm69299 {
+   struct drm_panel panel;
+   struct regulator_bulk_data supplies[2];
+   struct gpio_desc *reset_gpio;
+   struct mipi_dsi_device *dsi;
+   bool prepared;
+   bool enabled;
+};
+
+static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
+{
+   return container_of(panel, struct visionox_rm69299, panel);
+}
+
+static int visionox_rm69299_power_on(struct visionox_rm69299 *ctx)
+{
+   int ret;
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+   if (ret < 0)
+   return ret;
+
+   /*
+* Reset sequence of visionox panel requires the panel to be
+* out of reset for 10ms, followed by being held in reset
+* for 10ms and then out again
+*/
+   gpiod_set_value(ctx->reset_gpio, 1);
+   usleep_range(1, 2);
+   gpiod_set_value(ctx->reset_gpio, 0);
+   usleep_range(1, 2);
+   gpiod_set_value(ctx->reset_gpio, 1);
+   usleep_range(1, 2);
+
+   return 0;
+}
+
+static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
+{
+   gpiod_set_value(ctx->reset_gpio, 0);
+
+   return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int visionox_rm69299_unprepare(struct drm_panel *panel)
+{
+   struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+   int ret;
+
+   ctx->dsi->mode_flags = 0;
+
+   ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
+   if (ret < 0)
+   DRM_DEV_ERROR(ctx->panel.dev,
+   "set_display_off cmd failed ret = %d\n", ret);

Re: [PATCH] MAINTAINERS: adjust to VIVANTE GPU schema conversion

[Benjamin GAIGNARD]

On 3/15/20 8:21 AM, Lukas Bulwahn wrote:
> Commit 90aeca875f8a ("dt-bindings: display: Convert etnaviv to
> json-schema") missed to adjust the DRM DRIVERS FOR VIVANTE GPU IP entry
> in MAINTAINERS.
>
> Since then, ./scripts/get_maintainer.pl --self-test complains:
>
>warning: no file matches \
>F: Documentation/devicetree/bindings/display/etnaviv/
>
> Update MAINTAINERS entry to location of converted schema.
>
> Signed-off-by: Lukas Bulwahn 
Reviewed-by: Benjamin Gaignard 

Thanks
> ---
> applies cleanly on next-20200313
>
> Benjamin, please ack.
> Rob, please pick this patch (it is not urgent, though)
>
>
>   MAINTAINERS | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77eede976d0f..50a7a6d62e06 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5766,7 +5766,7 @@ L:  dri-devel@lists.freedesktop.org
>   S:  Maintained
>   F:  drivers/gpu/drm/etnaviv/
>   F:  include/uapi/drm/etnaviv_drm.h
> -F:   Documentation/devicetree/bindings/display/etnaviv/
> +F:   Documentation/devicetree/bindings/gpu/vivante,gc.yaml
>   
>   DRM DRIVERS FOR ZTE ZX
>   M:  Shawn Guo 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 0/2] Add support for rm69299 Visionox panel driver and add devicetree bindings for visionox panel

[Harigovindan P]

Adding support for visionox rm69299 panel driver and adding bindings for the 
same panel.

Harigovindan P (2):
  dt-bindings: display: add visionox rm69299 panel variant
  drm/panel: add support for rm69299 visionox panel driver

 .../display/panel/visionox,rm69299.yaml   |  81 +
 drivers/gpu/drm/panel/Kconfig |   8 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-visionox-rm69299.c| 295 ++
 4 files changed, 385 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 15/36] dt-bindings: display: convert simple lg panels to DT Schema

[Brian Masney]

On Sun, Mar 15, 2020 at 02:43:55PM +0100, Sam Ravnborg wrote:
> Add the lg panels that matches the panel-simple binding to
> panel-simple.yaml
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Alexandre Courbot 
> Cc: Brian Masney 
> Cc: Thierry Reding 
> Cc: Sam Ravnborg 

Reviewed-by: Brian Masney 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/lease: fix WARNING in idr_destroy

[Qiujun Huang]

leases has been destroyed:
drm_master_put
->drm_master_destroy
->idr_destroy

so the "out_lessee" needn't to call idr_destroy again.

Reported-and-tested-by: syzbot+05835159fe322770f...@syzkaller.appspotmail.com
Signed-off-by: Qiujun Huang 
---
 drivers/gpu/drm/drm_lease.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index b481caf..54506c2 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -577,11 +577,14 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 
 out_lessee:
drm_master_put();
+   goto err_exit;
 
 out_leases:
-   put_unused_fd(fd);
idr_destroy();
 
+err_exit:
+   put_unused_fd(fd);
+
DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret);
return ret;
 }
-- 
1.8.3.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 0/2] Add support for rm69299 Visionox panel driver and add devicetree bindings for visionox panel

[Harigovindan P]

Adding support for visionox rm69299 panel driver and adding bindings for the 
same panel.

Harigovindan P (2):
  dt-bindings: display: add visionox rm69299 panel variant
  drm/panel: add support for rm69299 visionox panel driver

 .../display/panel/visionox,rm69299.yaml   |  81 +
 drivers/gpu/drm/panel/Kconfig |   8 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-visionox-rm69299.c| 295 ++
 4 files changed, 385 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 1/2] dt-bindings: display: add visionox rm69299 panel variant

[Harigovindan P]

Add bindings for visionox rm69299 panel.

Signed-off-by: Harigovindan P 
---

Changes in v2:
- Removed unwanted properties from description.
- Creating source files without execute permissions(Rob Herring).
Changes in v3:
- Changing txt file into yaml
Changes in v4:
- Updating license identifier.
- Moving yaml file inside panel directory.
- Removing pinctrl entries.
- Adding documentation for reset-gpios.
Changes in v5:
- No changes. Updated 2/2 Patch.
Changes in v6:
- Removing patternProperties.
- Added " |" after description.
- Setting port and reset-gpios to true.
- Removing @ae94000 for dsi node.
Changes in v7:
- Added reg property.

 .../display/panel/visionox,rm69299.yaml   | 81 +++
 1 file changed, 81 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml

diff --git 
a/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml 
b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
new file mode 100644
index ..6ea1a7be3787
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/visionox,rm69299.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Visionox model RM69299 Panels Device Tree Bindings.
+
+maintainers:
+ - Harigovindan P 
+
+description: |
+ This binding is for display panels using a Visionox RM692999 panel.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+const: visionox,rm69299-1080p-display
+
+  reg:
+maxItems: 1
+
+  vdda-supply:
+description: |
+  Phandle of the regulator that provides the vdda supply voltage.
+
+  vdd3p3-supply:
+description: |
+  Phandle of the regulator that provides the vdd3p3 supply voltage.
+
+  ports:
+type: object
+description: |
+  A node containing DSI input & output port nodes with endpoint
+  definitions as documented in
+  Documentation/devicetree/bindings/media/video-interfaces.txt
+  Documentation/devicetree/bindings/graph.txt
+  properties:
+port: true
+
+  reset-gpios: true
+
+required:
+  - compatible
+  - reg
+  - vdda-supply
+  - vdd3p3-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+dsi {
+reg = <0x0ae94000 0x400>;
+#address-cells = <1>;
+#size-cells = <0>;
+panel@0 {
+compatible = "visionox,rm69299-1080p-display";
+reg = <0x0ae94000 0x400>;
+
+vdda-supply = <_pp1800_l8c>;
+vdd3p3-supply = <_pp2800_l18a>;
+
+reset-gpios = <_gpio 3 0>;
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+panel0_in: endpoint {
+remote-endpoint = <_out>;
+};
+};
+};
+};
+};
+
+...
-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 1/2] dt-bindings: display: add visionox rm69299 panel variant

[Harigovindan P]

Add bindings for visionox rm69299 panel.

Signed-off-by: Harigovindan P 
---
 .../display/panel/visionox,rm69299.yaml   | 81 +++
 1 file changed, 81 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml

diff --git 
a/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml 
b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
new file mode 100644
index ..6ea1a7be3787
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/visionox,rm69299.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/visionox,rm69299.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Visionox model RM69299 Panels Device Tree Bindings.
+
+maintainers:
+ - Harigovindan P 
+
+description: |
+ This binding is for display panels using a Visionox RM692999 panel.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+const: visionox,rm69299-1080p-display
+
+  reg:
+maxItems: 1
+
+  vdda-supply:
+description: |
+  Phandle of the regulator that provides the vdda supply voltage.
+
+  vdd3p3-supply:
+description: |
+  Phandle of the regulator that provides the vdd3p3 supply voltage.
+
+  ports:
+type: object
+description: |
+  A node containing DSI input & output port nodes with endpoint
+  definitions as documented in
+  Documentation/devicetree/bindings/media/video-interfaces.txt
+  Documentation/devicetree/bindings/graph.txt
+  properties:
+port: true
+
+  reset-gpios: true
+
+required:
+  - compatible
+  - reg
+  - vdda-supply
+  - vdd3p3-supply
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+dsi {
+reg = <0x0ae94000 0x400>;
+#address-cells = <1>;
+#size-cells = <0>;
+panel@0 {
+compatible = "visionox,rm69299-1080p-display";
+reg = <0x0ae94000 0x400>;
+
+vdda-supply = <_pp1800_l8c>;
+vdd3p3-supply = <_pp2800_l18a>;
+
+reset-gpios = <_gpio 3 0>;
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+panel0_in: endpoint {
+remote-endpoint = <_out>;
+};
+};
+};
+};
+};
+
+...
-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v7 2/2] drm/panel: add support for rm69299 visionox panel driver

[Harigovindan P]

Add support for Visionox panel driver.

Signed-off-by: Harigovindan P 
---
 drivers/gpu/drm/panel/Kconfig |   8 +
 drivers/gpu/drm/panel/Makefile|   1 +
 .../gpu/drm/panel/panel-visionox-rm69299.c| 295 ++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-visionox-rm69299.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index ae44ac2ec106..7b696f304a99 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -389,6 +389,14 @@ config DRM_PANEL_TRULY_NT35597_WQXGA
  Say Y here if you want to enable support for Truly NT35597 WQXGA Dual 
DSI
  Video Mode panel
 
+config DRM_PANEL_VISIONOX_RM69299
+   tristate "Visionox RM69299"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   help
+ Say Y here if you want to enable support for Visionox
+ RM69299  DSI Video Mode panel.
+
 config DRM_PANEL_XINPENG_XPP055C272
tristate "Xinpeng XPP055C272 panel driver"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 7c4d3c581fd4..9f11d067a6b2 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -41,4 +41,5 @@ obj-$(CONFIG_DRM_PANEL_TPO_TD028TTEC1) += 
panel-tpo-td028ttec1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TD043MTEA1) += panel-tpo-td043mtea1.o
 obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
+obj-$(CONFIG_DRM_PANEL_VISIONOX_RM69299) += panel-visionox-rm69299.o
 obj-$(CONFIG_DRM_PANEL_XINPENG_XPP055C272) += panel-xinpeng-xpp055c272.o
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c 
b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
new file mode 100644
index ..0f877d21fdf2
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+struct visionox_rm69299 {
+   struct drm_panel panel;
+   struct regulator_bulk_data supplies[2];
+   struct gpio_desc *reset_gpio;
+   struct mipi_dsi_device *dsi;
+   bool prepared;
+   bool enabled;
+};
+
+static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
+{
+   return container_of(panel, struct visionox_rm69299, panel);
+}
+
+static int visionox_rm69299_power_on(struct visionox_rm69299 *ctx)
+{
+   int ret;
+
+   ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+   if (ret < 0)
+   return ret;
+
+   /*
+* Reset sequence of visionox panel requires the panel to be
+* out of reset for 10ms, followed by being held in reset
+* for 10ms and then out again
+*/
+   gpiod_set_value(ctx->reset_gpio, 1);
+   usleep_range(1, 2);
+   gpiod_set_value(ctx->reset_gpio, 0);
+   usleep_range(1, 2);
+   gpiod_set_value(ctx->reset_gpio, 1);
+   usleep_range(1, 2);
+
+   return 0;
+}
+
+static int visionox_rm69299_power_off(struct visionox_rm69299 *ctx)
+{
+   gpiod_set_value(ctx->reset_gpio, 0);
+
+   return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int visionox_rm69299_unprepare(struct drm_panel *panel)
+{
+   struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+   int ret;
+
+   ctx->dsi->mode_flags = 0;
+
+   ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_SET_DISPLAY_OFF, NULL, 0);
+   if (ret < 0)
+   DRM_DEV_ERROR(ctx->panel.dev,
+   "set_display_off cmd failed ret = %d\n", ret);
+
+   /* 120ms delay required here as per DCS spec */
+   msleep(120);
+
+   ret = mipi_dsi_dcs_write(ctx->dsi, MIPI_DCS_ENTER_SLEEP_MODE, NULL, 0);
+   if (ret < 0) {
+   DRM_DEV_ERROR(ctx->panel.dev,
+   "enter_sleep cmd failed ret = %d\n", ret);
+   }
+
+   ret = visionox_rm69299_power_off(ctx);
+
+   ctx->prepared = false;
+   return ret;
+}
+
+static int visionox_rm69299_prepare(struct drm_panel *panel)
+{
+   struct visionox_rm69299 *ctx = panel_to_ctx(panel);
+   int ret;
+
+   if (ctx->prepared)
+   return 0;
+
+   ret = visionox_rm69299_power_on(ctx);
+   if (ret < 0)
+   return ret;
+
+   ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+   ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xfe, 0x00 }, 2);
+   if (ret < 0) {
+   DRM_DEV_ERROR(ctx->panel.dev,
+   "cmd set tx 0 failed, ret = %d\n", ret);
+   goto power_off;
+   }
+
+   ret = mipi_dsi_dcs_write_buffer(ctx->dsi, (u8[]){ 0xc2, 0x08 }, 2);
+   if (ret < 0) {
+   

Re: [PATCH v1 35/36] dt-bindings: display: convert lgphilips, lb035q02 to DT Schema

[Tomi Valkeinen]

Hi Sam,

On 15/03/2020 15:44, Sam Ravnborg wrote:

Signed-off-by: Sam Ravnborg 
Cc: Tomi Valkeinen 
Cc: Thierry Reding 
Cc: Sam Ravnborg 
---
  .../display/panel/lgphilips,lb035q02.txt  | 33 
  .../display/panel/lgphilips,lb035q02.yaml | 54 +++
  2 files changed, 54 insertions(+), 33 deletions(-)
  delete mode 100644 
Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
  create mode 100644 
Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.yaml

diff --git 
a/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt 
b/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
deleted file mode 100644
index 1a1e653e5407..
--- a/Documentation/devicetree/bindings/display/panel/lgphilips,lb035q02.txt
+++ /dev/null
@@ -1,33 +0,0 @@
-LG.Philips LB035Q02 Panel
-=
-
-Required properties:
-- compatible: "lgphilips,lb035q02"
-- enable-gpios: panel enable gpio
-
-Optional properties:
-- label: a symbolic name for the panel
-
-Required nodes:
-- Video port for DPI input


Isn't this also compatible with panel-simple bindings? 'label' is the only one not in panel-simple, 
but that's optional and has never been used by the panel driver.


 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 17/36] dt-bindings: display: convert osddisplays,osd101t2587-53ts to DT Schema

[Tomi Valkeinen]

On 15/03/2020 15:43, Sam Ravnborg wrote:

osddisplays,osd101t2587-53ts is compatible with panel-simple binding,
so list the compatible in the panel-simple binding file.

Signed-off-by: Sam Ravnborg 
Cc: Peter Ujfalusi 
Cc: Thierry Reding 
Cc: Sam Ravnborg 
---
  .../display/panel/osddisplays,osd101t2587-53ts.txt | 14 --
  .../bindings/display/panel/panel-simple.yaml   |  2 ++
  2 files changed, 2 insertions(+), 14 deletions(-)
  delete mode 100644 
Documentation/devicetree/bindings/display/panel/osddisplays,osd101t2587-53ts.txt


Reviewed-by: Tomi Valkeinen 

 Tomi

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> >> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
> >> virtio command when it finished rendering, period.
> >>
> >>
> >> On the guest side we don't need the fence_id.  The completion callback
> >> gets passed the virtio_gpu_vbuffer, so it can figure which command did
> >> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
> >>
> >> On the host side we depend on the fence_id right now, but only because
> >> that is the way the virgl_renderer_callbacks->write_fence interface is
> >> designed.  We have to change that anyway for per-context (or whatever)
> >> fences, so it should not be a problem to drop the fence_id dependency
> >> too and just pass around an opaque pointer instead.
> 
> I am still catching up, but IIUC, indeed I don't think the host needs
> to depend on fence_id.  We should be able to repurpose fence_id.

I'd rather ignore it altogether for FENCE_V2 (or whatever we call the
feature flag).

> On the other hand, the VIRTIO_GPU_FLAG_FENCE flag is interesting, and
> it indicates that the vbuf is on the host GPU timeline instead of the
> host CPU timeline.

Yep, we have to keep that (unless we do command completion on GPU
timeline unconditionally with FENCE_V2).

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel