Re: [Freedreno] [PATCH][next] drm: Replace zero-length array with flexible-array member

[Chris Wilson]

Quoting Gustavo A. R. Silva (2020-02-25 14:03:47)
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:

I remember when gcc didn't support []. For the record, it appears
support for flexible arrays landed in gcc-3.0. So passes the minimum
compiler spec. That would be useful to mention for old farts with
forgetful memories.
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v2 2/3] drm: plumb attaching dev thru to prime_pin/unpin

[Chris Wilson]

Quoting Rob Clark (2019-07-16 18:43:22)
> From: Rob Clark 
> 
> Needed in the following patch for cache operations.

What's the base for this patch? (I'm missing the ancestor for drm_gem.c)
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v1] drm/msm: Move fence put to where failure occurs

[Chris Wilson]

Quoting Robert Foss (2018-11-01 16:12:28)
> If dma_fence_wait fails to wait for a supplied in-fence in
> msm_ioctl_gem_submit, make sure we release that in-fence.
> 
> Also remove this dma_fence_put() from the 'out' label.
> 
> Signed-off-by: Robert Foss 
> ---
>  drivers/gpu/drm/msm/msm_gem_submit.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index a90aedd6883a..3e7704af5b24 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -411,7 +411,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void 
> *data,
> struct msm_file_private *ctx = file->driver_priv;
> struct msm_gem_submit *submit;
> struct msm_gpu *gpu = priv->gpu;
> -   struct dma_fence *in_fence = NULL;
> struct sync_file *sync_file = NULL;
> struct msm_gpu_submitqueue *queue;
> struct msm_ringbuffer *ring;
> @@ -444,7 +443,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void 
> *data,
> ring = gpu->rb[queue->prio];
>  
> if (args->flags & MSM_SUBMIT_FENCE_FD_IN) {
> -   in_fence = sync_file_get_fence(args->fence_fd);
> +   struct dma_fence *in_fence = sync_file_get_fence(
> +   args->fence_fd);
>  
> if (!in_fence)
> return -EINVAL;
> @@ -455,8 +455,10 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void 
> *data,
>  */
> if (!dma_fence_match_context(in_fence, ring->fctx->context)) {
> ret = dma_fence_wait(in_fence, true);
> -   if (ret)
> +   if (ret) {
> +   dma_fence_put(in_fence);
> return ret;
> +   }
> }

Careful, we need to keep the put for the normal path. Maybe,

if (args->flags & MSM_SUBMIT_FENCE_FD_IN) {
struct dma_fence *in_fence;

in_fence = sync_file_get_fence(args->fence_fd); // keep line breaks 
natural
if (!in_fence)
return -EINVAL;

ret = 0;
if (!dma_fence_match_match_context(in_fence, ring->fctx->context)
ret = dma_fence_wait(in_fence, true);
dma_fence_put(in_fence);
if (ret)
return ret;
}
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 08/13] drm/msm/gpu: Rearrange the code that collects the task during a hang

[Chris Wilson]

Quoting Jordan Crouse (2018-07-12 19:59:25)
> Do a bit of cleanup to prepare for upcoming changes to pass the
> hanging task comm and cmdline to the crash dump function.
> 
> Signed-off-by: Jordan Crouse 
> ---
>  drivers/gpu/drm/msm/msm_gpu.c | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 1c09acfb4028..2ca354047250 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -314,6 +314,7 @@ static void recover_worker(struct work_struct *work)
> struct msm_drm_private *priv = dev->dev_private;
> struct msm_gem_submit *submit;
> struct msm_ringbuffer *cur_ring = gpu->funcs->active_ring(gpu);
> +   char *comm = NULL, *cmd = NULL;
> int i;
>  
> mutex_lock(>struct_mutex);
> @@ -327,7 +328,7 @@ static void recover_worker(struct work_struct *work)
> rcu_read_lock();
> task = pid_task(submit->pid, PIDTYPE_PID);
> if (task) {
> -   char *cmd;
> +   comm = kstrdup(task->comm, GFP_KERNEL);

Under rcu_read_lock(), GFP_KERNEL is not allowed, you need GFP_NOWAIT or
some such (or grab a reference to the pid and drop rcu then GFP_KERNEL).
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 07/13] drm/msm/gpu: Convert the GPU show function to use the GPU state

[Chris Wilson]

Quoting Jordan Crouse (2018-07-12 19:59:24)
> Convert the existing GPU show function to use the GPU state to
> dump the information rather than reading it directly from the hardware.
> This will require an additional step to capture the state before
> dumping it for the existing nodes but it will greatly facilitate reusing
> the same code for dumping a previously captured state from a GPU hang.
> 
> Signed-off-by: Jordan Crouse 
> ---
>  drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 11 +--
>  drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 12 +---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 18 +
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 30 
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h |  3 +-
>  drivers/gpu/drm/msm/msm_debugfs.c   | 92 ++---
>  drivers/gpu/drm/msm/msm_gpu.h   |  3 +-
>  7 files changed, 104 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> index b707b5bca9ab..4cffec2b6adc 100644
> --- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
> @@ -411,15 +411,6 @@ static const unsigned int a3xx_registers[] = {
> ~0   /* sentinel */
>  };
>  
> -#ifdef CONFIG_DEBUG_FS
> -static void a3xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -   seq_printf(m, "status:   %08x\n",
> -   gpu_read(gpu, REG_A3XX_RBBM_STATUS));
> -   adreno_show(gpu, m);
> -}
> -#endif
> -
>  /* would be nice to not have to duplicate the _show() stuff with printk(): */
>  static void a3xx_dump(struct msm_gpu *gpu)
>  {
> @@ -464,7 +455,7 @@ static const struct adreno_gpu_funcs funcs = {
> .irq = a3xx_irq,
> .destroy = a3xx_destroy,
>  #ifdef CONFIG_DEBUG_FS
> -   .show = a3xx_show,
> +   .show = adreno_show,
>  #endif
> .gpu_state_get = a3xx_gpu_state_get,
> .gpu_state_put = adreno_gpu_state_put,
> diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> index 17e97ebc1077..95f08c22e8d7 100644
> --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> @@ -455,16 +455,6 @@ static const unsigned int a4xx_registers[] = {
> ~0 /* sentinel */
>  };
>  
> -#ifdef CONFIG_DEBUG_FS
> -static void a4xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -   seq_printf(m, "status:   %08x\n",
> -   gpu_read(gpu, REG_A4XX_RBBM_STATUS));
> -   adreno_show(gpu, m);
> -
> -}
> -#endif
> -
>  static struct msm_gpu_state *a4xx_gpu_state_get(struct msm_gpu *gpu)
>  {
> struct msm_gpu_state *state = adreno_gpu_state_get(gpu);
> @@ -551,7 +541,7 @@ static const struct adreno_gpu_funcs funcs = {
> .irq = a4xx_irq,
> .destroy = a4xx_destroy,
>  #ifdef CONFIG_DEBUG_FS
> -   .show = a4xx_show,
> +   .show = adreno_show,
>  #endif
> .gpu_state_get = a4xx_gpu_state_get,
> .gpu_state_put = adreno_gpu_state_put,
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 9e85e4f7016d..5f1aab3c1cb1 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1215,22 +1215,6 @@ static struct msm_gpu_state *a5xx_gpu_state_get(struct 
> msm_gpu *gpu)
> return state;
>  }
>  
> -#ifdef CONFIG_DEBUG_FS
> -static void a5xx_show(struct msm_gpu *gpu, struct seq_file *m)
> -{
> -   seq_printf(m, "status:   %08x\n",
> -   gpu_read(gpu, REG_A5XX_RBBM_STATUS));
> -
> -   /*
> -* Temporarily disable hardware clock gating before going into
> -* adreno_show to avoid issues while reading the registers
> -*/
> -   a5xx_set_hwcg(gpu, false);
> -   adreno_show(gpu, m);
> -   a5xx_set_hwcg(gpu, true);
> -}
> -#endif
> -
>  static struct msm_ringbuffer *a5xx_active_ring(struct msm_gpu *gpu)
>  {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -1260,7 +1244,7 @@ static const struct adreno_gpu_funcs funcs = {
> .irq = a5xx_irq,
> .destroy = a5xx_destroy,
>  #ifdef CONFIG_DEBUG_FS
> -   .show = a5xx_show,
> +   .show = adreno_show,
> .debugfs_init = a5xx_debugfs_init,
>  #endif
> .gpu_busy = a5xx_gpu_busy,
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c7a0d278c59e..0e937eedcec5 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -423,38 +423,34 @@ void adreno_gpu_state_put(struct msm_gpu_state *state)
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> -void adreno_show(struct msm_gpu *gpu, struct seq_file *m)
> +void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> +   struct seq_file *m)
>  {
> 

Re: [Freedreno] [PATCH 05/13] drm: Add put callback for the coredump printer

[Chris Wilson]

Quoting Jordan Crouse (2018-07-12 19:59:22)
> Add a put function for the coredump printer to bypass printf()
> for constant strings for a speed boost.
> 
> v2: Add EXPORT_SYMBOL for _drm_puts_coredump
> Signed-off-by: Jordan Crouse 
> ---
>  drivers/gpu/drm/drm_print.c | 43 +
>  include/drm/drm_print.h |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index bef8f0ec5d73..ff20f4a764c8 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -30,6 +30,49 @@
>  #include 
>  #include 
>  
> +void __drm_puts_coredump(struct drm_printer *p, const char *str)
> +{
> +   struct drm_print_iterator *iterator = p->arg;
> +
> +   ssize_t len;
> +
> +   if (!iterator->remain)
> +   return;
> +
> +   if (iterator->offset < iterator->start) {
> +   ssize_t copy;
> +
> +   len = strlen(str);
> +

printfn_coredump looks like it would then wrap puts_coredump?
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 02/13] drm: drm_printer: Add printer for devcoredump

[Chris Wilson]

Quoting Jordan Crouse (2018-07-12 19:59:19)
> Add a drm printer suitable for use with the read callback for
> devcoredump or other suitable buffer based output format that
> isn't otherwise covered by seq_file.
> 
> Signed-off-by: Jordan Crouse 
> ---
>  drivers/gpu/drm/drm_print.c | 74 +
>  include/drm/drm_print.h | 27 ++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index b25f98f33f6c..03d1f98e5ac7 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -30,6 +30,80 @@
>  #include 
>  #include 
>  
> +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
> +{
> +   struct drm_print_iterator *iterator = p->arg;
> +   ssize_t len;
> +
> +   if (!iterator->remain)
> +   return;
> +
> +   /* Figure out how big the string will be */
> +   len = snprintf(NULL, 0, "%pV", vaf);

I was thinking there's some duplication here (kmalloc + snprintf) that
could be reduced to kasprintf here. Is avoiding that allocation
important or frequent enough to merit open coding?

It's pity the kernel's printk doesn't support %n, so that leaves with

buf = kasprintf(GFP_... , "%pV", vaf);
if (!buf)
return;

len = strlen(buf);

and even the copy + increment looks like it can then be factored to share
more code.
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 01/13] include: Move ascii85 functions from i915 to linux/ascii85.h

[Chris Wilson]

Quoting Jordan Crouse (2018-07-12 19:59:18)
> The i915 DRM driver very cleverly used ascii85 encoding for their
> GPU state file. Move the encode functions to a general header file to
> support other drivers that might be interested in the same
> functionality.
> 
> v3: Fix error_puts -> err_puts pointed out by the 01.org bot
> v2: Update API to be cleaner for the caller as suggested by Chris Wilson
> 
> Signed-off-by: Jordan Crouse 
> ---
> +static inline long
> +ascii85_encode_len(long len)
> +{
> +   return DIV_ROUND_UP(len, 4);
> +}
> +
> +static inline char *

const char * to avoid a compiler warning with return "z".

Looks like that will be ok with the callers.

With that,
Reviewed-by: Chris Wilson 
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 04/10] drm/msm/gpu: Convert the GPU show function to use the GPU state

[Chris Wilson]

Quoting Jordan Crouse (2018-04-05 23:00:50)
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c 
> b/drivers/gpu/drm/msm/msm_debugfs.c
> index ba74cb4f94df..fd535dab3d5b 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -25,13 +25,22 @@ static int msm_gpu_show(struct drm_device *dev, struct 
> seq_file *m)
>  {
> struct msm_drm_private *priv = dev->dev_private;
> struct msm_gpu *gpu = priv->gpu;
> +   struct msm_gpu_state *state;
>  
> -   if (gpu) {
> -   seq_printf(m, "%s Status:\n", gpu->name);
> -   pm_runtime_get_sync(>pdev->dev);
> -   gpu->funcs->show(gpu, m);
> -   pm_runtime_put_sync(>pdev->dev);
> -   }
> +   if (!gpu)
> +   return 0;
> +
> +   pm_runtime_get_sync(>pdev->dev);
> +   state = gpu->funcs->gpu_state_get(gpu);
> +   pm_runtime_put_sync(>pdev->dev);
> +
> +   if (IS_ERR(state))
> +   return PTR_ERR(state);
> +
> +   seq_printf(m, "%s Status:\n", gpu->name);
> +   gpu->funcs->show(gpu, state, m);
> +
> +   gpu->funcs->gpu_state_put(state);

Ah. This be trickier than it appears thanks to how seq_file tries to
keep the interface simple :)

For a large buffer, seq_file will call the show multiple times to
convert it into a single string, which it then iterates over. (iirc)

Ideally, you grab the error state on open, and then use the
drm_printer_iterator you have to feed the chunks to seqfs. At a minimum,
I do recommend you stick the get into the seq_open callback, as my
memory says the show will be called multiple times.
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 02/10] drm: drm_printer: Add printer for devcoredump

[Chris Wilson]

Quoting Chris Wilson (2018-04-06 11:42:25)
> Quoting Jordan Crouse (2018-04-05 23:00:48)
> > +struct drm_print_iterator {
> > +   void *data;
> > +
> > +   ssize_t start;
> > +   ssize_t offset;
> > +   ssize_t remain;
> > +};
> 
> Neat, we should be able to use to replace struct
> drm_i915_error_state_buf (or at least the seq_file side).

Though comparing against which, we have loff_t for that interface.
But that's immaterial for an in-memory file. (So nothing to see here,
please move along.)
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [Intel-gfx] [PATCH 01/10] include: Move ascii85 functions from i915 to linux/ascii85.h

[Chris Wilson]

Quoting Jordan Crouse (2018-04-05 23:06:53)
> On Thu, Apr 05, 2018 at 04:00:47PM -0600, Jordan Crouse wrote:
> > The i915 DRM driver very cleverly used ascii85 encoding for their
> > GPU state file. Move the encode functions to a general header file to
> > support other drivers that might be interested in the same
> > functionality.
> 
> In a previous version of this patch, Chris asked what tree I wanted this 
> applied
> to, and the answer is: I'm not sure?  I'm hoping that Rob will be cool with
> picking the rest up for msm-next for 4.18 but I'm okay with putting this
> particular patch wherever it is easiest for the maintainers.

We are a bit late to sneak it into the 4.17 drm base via i915. I don't
anticipate any problems (for i915) with this patch going in through
msm-next, so happy to have it land there and percolate back to i915 3
months later.
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] include: Move ascii85 functions from i915 to linux/ascii85.h

[Chris Wilson]

Quoting Jordan Crouse (2018-01-26 20:59:22)
> The i915 DRM driver very cleverly used ascii85 encoding for their

All gfx drivers must eventually become PostScript.

> GPU state file. Move the encode functions to a general header file to
> support other drivers that might be interested in the same
> functionality.
> 
> Signed-off-by: Jordan Crouse 
> ---
> diff --git a/include/linux/ascii85.h b/include/linux/ascii85.h
> new file mode 100644
> index 000..7ee39f9
> --- /dev/null
> +++ b/include/linux/ascii85.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2008 Intel Corporation

Just cut this down to
/*
 * SPDX-License-Identifier: GPL-2.0
 *
 * Copyright (c) 2008 Intel Corporation
 * Copyright (c) 2018 My Name Here
 */

Fortunately ideas themselves are not copyrightable, otherwise Adobe has
a strong claim to ownership.
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 0/4] drm/msm: GPU crash state

[Chris Wilson]

Quoting Jordan Crouse (2018-01-05 18:00:17)
> This is a request for comment on code to store and dump a GPU state
> a hang with inspiration from the very good i915 GPU error state and
> the binary GPU snapshot in the downstream kernel.
> 
> The goal is to store and provide enough information to debug software
> and hardware issues on the Adreno hardware in a semi human-readable
> format that can also be parsed by scripts.
> 
> The goal for this request for comment is to get some consensus
> about the format and work through some of the technical issues.

My biggest regret for i915/error is that we didn't adopt a sensible file
format and organically grew it from dmesg-style logging. This is quite a
hindrance when it comes to trying to improve the capture whilst
maintaining compatibility with the existing tools. Switching to json/yaml
at this point won't be too difficult to spot the change in format, just a
large chunk of technical debt to pay off. So I would recommend you pick a
an adaptable, human readable, file format for ease of tool development.

The second important feature for capturing error state is to include as
much user information as possible. You want to be able to identify which
library generated the hang in a post-mortem dump from a user in 6-12
months time, and just as importantly, why the library did what it did. I
like the idea of userspace being able to attach buffers that are
included in the error state (supplied as auxiliary information to the
guilty command stream) to provide a flight-data-recorder from the user's
pov. So design your interface with a view to extending to include blobs.

It would be interesting to have a common file format... While
interpreting the data is going to highly specific to a gpu/driver, the
data itself will be similar between drivers. If we had a common file
format, we could extend something like mesa/intel/aubinator_error_decode
and throw in a bunch of xml descriptors for the different gpus. Just a
thought...
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm/mdp5: Fix compilation warnings

[Chris Wilson]

Quoting Viresh Kumar (2017-06-29 10:19:59)
> Following compilation warnings were observed for these files:
> 
>   CC [M]  drivers/gpu/drm/msm/mdp/mdp5/mdp5_mdss.o
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c: In function 'blend_setup':
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c:223:7: warning: missing braces 
> around initializer [-Wmissing-braces]
>   enum mdp5_pipe stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { SSPP_NONE };
>^
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c:223:7: warning: (near initialization 
> for 'stage[0]') [-Wmissing-braces]
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c:224:7: warning: missing braces 
> around initializer [-Wmissing-braces]
>   enum mdp5_pipe r_stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { SSPP_NONE };
>^
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c:224:7: warning: (near initialization 
> for 'r_stage[0]') [-Wmissing-braces]
> 
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c: In function 'mdp5_plane_mode_set':
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c:892:9: warning: missing braces 
> around initializer [-Wmissing-braces]
>   struct phase_step step = { 0 };
>  ^
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c:892:9: warning: (near 
> initialization for 'step.x') [-Wmissing-braces]
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c:893:9: warning: missing braces 
> around initializer [-Wmissing-braces]
>   struct pixel_ext pe = { 0 };
>  ^
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c:893:9: warning: (near 
> initialization for 'pe.left') [-Wmissing-braces]
> 
> This happens because in the first case we were initializing a two
> dimensional array with {0} and in the second case we were initializing a
> struct containing two arrays with {0}.
> 
> Fix them by adding another pair of {}.
> 
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c  | 4 ++--
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c 
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index 9217e0d6e93e..b2c68072a805 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -220,8 +220,8 @@ static void blend_setup(struct drm_crtc *crtc)
> struct mdp5_ctl *ctl = mdp5_cstate->ctl;
> uint32_t blend_op, fg_alpha, bg_alpha, ctl_blend_flags = 0;
> unsigned long flags;
> -   enum mdp5_pipe stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { SSPP_NONE };
> -   enum mdp5_pipe r_stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { SSPP_NONE };
> +   enum mdp5_pipe stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { { SSPP_NONE } 
> };
> +   enum mdp5_pipe r_stage[STAGE_MAX + 1][MAX_PIPE_STAGE] = { { SSPP_NONE 
> } };
> int i, plane_cnt = 0;
> bool bg_alpha_enabled = false;
> u32 mixer_op_mode = 0;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c 
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> index 7d3741215387..0ee9bd0041cd 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> @@ -889,8 +889,8 @@ static int mdp5_plane_mode_set(struct drm_plane *plane,
> struct mdp5_hw_pipe *right_hwpipe;
> const struct mdp_format *format;
> uint32_t nplanes, config = 0;
> -   struct phase_step step = { 0 };
> -   struct pixel_ext pe = { 0 };
> +   struct phase_step step = { { 0 } };
> +   struct pixel_ext pe = { { 0 } };
> uint32_t hdecm = 0, vdecm = 0;
> uint32_t pix_format;
> unsigned int rotation;

Or just use {} to initialise to zero whatever the struct layout?
-Chris
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH] drm/msm: return fence_fd = -1 if gem_submit fails

[Chris Wilson]

On Mon, Dec 12, 2016 at 05:41:08PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
> 
> Previously we were returning garbage here, fix it by setting it to -1
> before the first possible point of failure.

The convention is that on error paths you do not modify user inputs. In
particular, consider EINTR where the usual pattern (e.g. drmIoctl) is

do {
err = ioctl(fd, SUBMIT, arg);
} while (err == -EINTR);

If you modify the in fence before you consume it, you can't recreate it
after handling the signal.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno