Re: [Nouveau] [PATCH 5/6] drm: Delete "mandatory" stereographic modes

[Damien Lespiau]

On Wed, Jan 18, 2017 at 11:27:16AM -0500, Ilia Mirkin wrote:
> Damien - did you ever test these mandatory modes on an actual
> commercial 3D TV or similar device?

My main testing device was a Samsung TV with this 3D_present bit set and
all the advertised modes were working. Can't quite remember if that
included the interleaved mode.

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

Re: [Nouveau] [PATCH 5/6] drm: Delete "mandatory" stereographic modes

[Damien Lespiau]

On Wed, Jan 18, 2017 at 01:48:04PM -0500, Ilia Mirkin wrote:
> After some more conversations with Alastair, it sounds like what's
> actually going on is that it's just the frame-packing modes that
> aren't working, but all the side-by-side and top-and-bottom modes from
> the "mandatory" list work. At this point, I'm more inclined to believe
> that there's an issue in the nouveau implementation for frame-packed
> modes. But it could still be the TVs themselves that don't support
> that at all.

If Alastair has an intel GPU as well, an "easy" way to check if the
frame packing modes of those TVs work would be to use the testdisplay[1]
tool of intel-gpu-tools.

Shameless plug: http://damien.lespiau.name/2013/10/hdmi-stereo-3d-kms.html

Towards the end of the post, there are test display usage examples to go
and test FP modes. Mind you, people have been trying to make
intel-gpu-tools run on any DRM driver when possible, not sure how far we
are with that though.

-- 
Damien

[1] 
http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/testdisplay.c
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Nouveau] [PATCH 5/6] drm: Delete "mandatory" stereographic modes

[Damien Lespiau]

On Wed, Jan 18, 2017 at 04:33:43PM +, Damien Lespiau wrote:
> On Wed, Jan 18, 2017 at 11:27:16AM -0500, Ilia Mirkin wrote:
> > Damien - did you ever test these mandatory modes on an actual
> > commercial 3D TV or similar device?
> 
> My main testing device was a Samsung TV with this 3D_present bit set and
> all the advertised modes were working. Can't quite remember if that
> included the interleaved mode.

I even pushed the EDID of that TV to edid-decode [1] if someone needs to
check that the EDID parsing is correct. It'd be interesting to see what
the tool has to say about the edid of the sink causing problems, in
particular compare the mandatory modes to the other modes advertised by
that TV. Maybe we could see some kind of pattern emerge, like the 3D
modes supported being the ones with the timings in table 8-15.

-- 
Damien

[1] https://cgit.freedesktop.org/xorg/app/edid-decode/
data/samsung-UE40D8000YU-hmdi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Nouveau] [PATCH 5/6] drm: Delete "mandatory" stereographic modes

[Damien Lespiau]

On Wed, Jan 18, 2017 at 12:10:56AM -0500, Ilia Mirkin wrote:
> On Tue, Jan 17, 2017 at 5:42 PM, Alastair Bridgewater
>  wrote:
> > HDMI specification 1.4a, table 8-15 is very explicitly a "must
> > support at least one of" table, not a "must support all of" table.
> > It is not hard to find hardware that does not support some of the
> > so-called "mandatory" modes.
> >
> > More seriously, this code generates invalid display modes for both
> > of the 3D-capable panels that I have (a 42-inch LG TV and a Sony
> > PlayStation 3D Display).
> >
> > If we want to be persnickety, one option would be to check the
> > final list of modes against the table and give some message if
> > none of them are valid, but it's a whole lot easier just to delete
> > the code in question.
> 
> Damien added this in commit c858cfcae6d some 3 years ago.
> 
> Damien, do you remember why you added these "required" modes? Did you
> have a monitor that only advertised 3D support without the actual
> modes?

Another quick glance at the specs leads me to believe c858cfcae6d is
correct.

8-15 does say that a sink supporting 3D must at lesst support one of the
modes listed in that table. But that's just the very start of the story
and we are really talking about the 3D_present bit in the HMDI VSDB and
the associated "mandatory" modes (the term comes from the spec).

HDMI 1.4a Page 155:

"3D_present [1bit] This bit indicates 3D support by the HDMI Sink,
including the mandatory formats. If set (=1), an HDMI Sink supports the
3D video formats that are mandatory formats," 

Continuing page 157:

"If 3D_present is set (=1), an HDMI Sink shall support 3D video formats
per the following requirements.
・An HDMI Sink which supports at least one 59.94 / 60Hz 2D video format
shall support *all* of "
・An HDMI Sink which supports at least one 50Hz 2D video format shall
support *all* of " 

The 3D pdf extraction from the spec is available would one want to
triple check the above:

   http://www.hdmi.org/manufacturer/specification.aspx

I have even dug up a direct link that someone made available:

  https://etmriwi.home.xs4all.nl/forum/hdmi_spec1.4a_3dextraction.pdf

Look for 3D_present, p. 15 and 17.

If the above is indeed correct, there may still be an issue in the way
we derive the mandatory modes - that part isn't really clear. Or, it
could be that people don't follow standards!

HTH,

-- 
Damien


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

[Intel-gfx] [PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[Damien Lespiau]

On Fri, Jan 22, 2016 at 04:48:05PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 22, 2016 at 12:51:23PM +0000, Damien Lespiau wrote:
> > We are reading at most sizeof(data) bytes, but then data may not contain
> > a terminating '\0', at least in theory, so strstr() may overflow the
> > stack allocated array.
> > 
> > Make sure that data always contains at least one '\0'.
> > 
> > Signed-off-by: Damien Lespiau 
> > ---
> >  xf86drm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index 7e28b4f..5f587d9 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, 
> > drmPciBusInfoPtr info)
> >  {
> >  #ifdef __linux__
> >  char path[PATH_MAX + 1];
> > -char data[128];
> > +char data[128 + 1];
> >  char *str;
> >  int domain, bus, dev, func;
> >  int fd, ret;
> > @@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, 
> > drmPciBusInfoPtr info)
> >  return -errno;
> >  
> >  ret = read(fd, data, sizeof(data));
> > +data[128] = '\0';
> 
> Slightly more paranoid would be something along the lines of
> if (ret >= 0)
>   data[ret] = '\0';
> 
> But this should be good enough I think so
> Reviewed-by: Ville Syrjälä 

Thanks for the review, pushed!

> The other thing I spotted while looking at the code is the fact that it
> doesn't check the snprint() return value. But I guess PATH_MAX is big
> enough that even if you somehow make maj and min INT_MIN it'll still
> fit.

Right, doesn't seem we can overflow path[].

-- 
Damien

[PATCH libdrm] xf86drm: Bound strstr() to the allocated data

[Damien Lespiau]

We are reading at most sizeof(data) bytes, but then data may not contain
a terminating '\0', at least in theory, so strstr() may overflow the
stack allocated array.

Make sure that data always contains at least one '\0'.

Signed-off-by: Damien Lespiau 
---
 xf86drm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index 7e28b4f..5f587d9 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2863,7 +2863,7 @@ static int drmParsePciBusInfo(int maj, int min, 
drmPciBusInfoPtr info)
 {
 #ifdef __linux__
 char path[PATH_MAX + 1];
-char data[128];
+char data[128 + 1];
 char *str;
 int domain, bus, dev, func;
 int fd, ret;
@@ -2874,6 +2874,7 @@ static int drmParsePciBusInfo(int maj, int min, 
drmPciBusInfoPtr info)
 return -errno;

 ret = read(fd, data, sizeof(data));
+data[128] = '\0';
 close(fd);
 if (ret < 0)
 return -errno;
-- 
2.4.3

[Intel-gfx] [PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Fri, Jul 03, 2015 at 08:41:31AM +0200, Daniel Vetter wrote:
> > Yeah. My first idea for the gamma stuff was that we'd simply have the
> > blob property for the data, and then a separate enum property which
> > decides how we interpret the blob contents. The default for the enum
> > would be the 8bpc/256 entries thing always, but the other values could
> > be potentially hardware specific. Obviously this would limit the use of
> > the fancier modes to the atomic API since you'd need to configure both
> > the blob and enum at the same time. But I don't see why we shouldn't
> > be allowed to rely on atomic from now on.
> 
> Yeah, enum+blob is what I like too. We can do one gamma table format enum
> in drm core. And drivers can then create it with just the enum values they
> actually support, similar to how we have rotation/mirror defines for
> everything, but the driver doesn't necessarily support it all. And the
> enum would fully encode everything there is to know about a layout, i.e.
> including excat precision and stuff like the 1025th/513th additional entry
> we have in some intel gamma tables.

I'm assuming you're still talking about 3 separate properties though?
pre-csc lut, csc, post-csc lut?

-- 
Damien

[Intel-gfx] [PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Thu, Jul 02, 2015 at 05:20:45PM +0100, Damien Lespiau wrote:
> On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> > From: Kausal Malladi 
> > 
> > The DRM color management framework is targeting various hardware
> > platforms and drivers. Different platforms can have different color
> > correction and enhancement capabilities.
> > 
> > A commom user space application can query these capabilities using the
> > DRM property interface. Each driver can fill this property with its
> > platform's color capabilities.
> > 
> > This patch adds new structures in DRM layer for querying color
> > capabilities. These structures will be used by all user space
> > agents to configure appropriate color configurations.
> 
> As I indicated before, I don't think we should go for a full fledged 
> query API, because, I don't believe we can ever make it good enough to 
> cover future hardware (and probably not today's hardware across 
> vendors). 
>  
> These kinds of query APIs have been frown upon in the past for that 
> exact reason. 
>  
> - Accept configurations that are mostly likely to be working across vendors 
> (256 enties for 8 bits) That should be enough for basic functionality. 
>  
> - To support things that are really hw specific: make sure the kernel API can 
> accept those, put the hw specific knowledge into a user-space HAL where APIs 
> can evolve. What you're trying to do here with queries about per-platform 
> details can go into userspace and still have a generic compositor code using 
> those limits. Let's just not put the limits into the kernel API, we won't be 
> able to get it right. 
>  
> Now maybe there's a middle ground and we can expose basic limits. In this 
> case, 
> maybe a list of supported LUT sizes, but the sampling details don't belong to 
> a 
> kernel interface IMHO. I'm even hesitant putting the hw precision at that 
> level. 

To re-iterate the point with actual examples, the proposed query API
doesn't seem to handle things we'd want to know today:

- What are the extra values are coding for. eg. for 8 bits, the values
  after index 255 are special and we have no description for those. (and
  it can get fiddly to describe them, you may want to add the type of
  interpolation for instance).
- How to you represent capabilities across hardware unit. Eg. split
  gamma lowering the number of LUT entries.

That somewhat demonstrates my point I think. We won't be able to get the
query API right.

-- 
Damien

[PATCH 03/12] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:13PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch does the following:
> 1. Adds new files intel_color_manager(.c/.h)
> 2. Attaches color properties to CRTC while initialization

We usually order the series so that "it always works". In this case, you
are attaching the properties to the CRTC before having any code to
handle them, so if we are bisecting the tree and end up in the middle of
the series we get a broken behaviour.

If you put the attach() at the end of the series, we'll only expose the
properties when we do have to handle them.

-- 
Damien

[PATCH 08/12] drm: Export drm_property_replace_global_blob function

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:18PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> drm_property_replace_global_blob() is getting used by many wrapper
> functions to replace an existing blob with new values. Because this
> function was static, modules are forced to create wrapper functions in
> same file. Exporting this function will remove need for such wrapper
> functions.
> 
> This patch makes the function drm_property_replace_global_blob() be
> accessible globally.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/drm_crtc.c | 2 +-
>  include/drm/drm_crtc.h | 6 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5d12ea9..15263a1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4458,7 +4458,7 @@ EXPORT_SYMBOL(drm_property_lookup_blob);
>   * a completely atomic update. The access to path_blob_ptr is protected by 
> the
>   * caller holding a lock on the connector.
>   */
> -static int drm_property_replace_global_blob(struct drm_device *dev,
> +int drm_property_replace_global_blob(struct drm_device *dev,
>  struct drm_property_blob 
> **replace,
>  size_t length,
>  const void *data,

Whether this is a good idea or not, you probably want an EXPORT_SYMBOL().

-- 
Damien

> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 408d39a..821424e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1541,6 +1541,12 @@ extern struct drm_property 
> *drm_mode_create_rotation_property(struct drm_device
> unsigned int 
> supported_rotations);
>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
> unsigned int supported_rotations);
> +extern int drm_property_replace_global_blob(struct drm_device *dev,
> + struct drm_property_blob **replace,
> + size_t length,
> + const void *data,
> + struct drm_mode_object *obj_holds_id,
> + struct drm_property *prop_holds_id);
>  
>  /* Helpers */
>  
> -- 
> 2.4.5
> 

[PATCH 07/12] drm: Add structures to set/get a palette color property

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:17PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch adds new structures in DRM layer for Palette color correction.
> These structures will be used by user space agents to configure
> appropriate number of samples and Palette LUT for a platform.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  include/uapi/drm/drm.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index d9562a2..04a8f2a 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -863,6 +863,18 @@ struct drm_color_caps {
>   struct drm_cge_caps cge_caps;
>  };
>  
> +struct drm_r32g32b32 {
> + __u32 r32;
> + __u32 g32;
> + __u32 b32;
> +};

r32 means red is an u32 to me. Is that the right encoding? I thought we
need values > 1.0 for our hw?

> +
> +struct drm_palette {
> + __u32 version;
> + __u32 palette_num_samples;

No need to prefix the fields with the structure name.

> + struct drm_r32g32b32 palette_lut[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.5
> 

[PATCH 03/12] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:13PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch does the following:
> 1. Adds new files intel_color_manager(.c/.h)
> 2. Attaches color properties to CRTC while initialization

A small note that we could remove manager from the whole API name,
intel_color_ looks enough of a prefix to me and will save some typing.

> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/Makefile  |  3 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 61 
> ++
>  drivers/gpu/drm/i915/intel_color_manager.h | 28 ++
>  drivers/gpu/drm/i915/intel_display.c   |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h   |  4 ++
>  5 files changed, 98 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index de21965..ad928d8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -56,7 +56,8 @@ i915-y += intel_audio.o \
> intel_overlay.o \
> intel_psr.o \
> intel_sideband.o \
> -   intel_sprite.o
> +   intel_sprite.o \
> +   intel_color_manager.o
>  i915-$(CONFIG_ACPI)  += intel_acpi.o intel_opregion.o
>  i915-$(CONFIG_DRM_I915_FBDEV)+= intel_fbdev.o
>  
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 000..9280ea6
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Shashank Sharma 
> + * Kausal Malladi 
> + */
> +
> +#include "intel_color_manager.h"
> +
> +void intel_color_manager_attach(struct drm_device *dev,
> + struct drm_mode_object *mode_obj)
> +{
> + struct drm_mode_config *config = >mode_config;
> +
> + if (mode_obj->type == DRM_MODE_OBJECT_CRTC) {
> + if (config->prop_color_capabilities) {
> + drm_object_attach_property(mode_obj,
> + config->prop_color_capabilities, 0);
> + DRM_DEBUG_DRIVER("Attached Color Caps property to 
> CRTC\n");
> + } else
> + DRM_ERROR("Error attaching Color Capabilities property 
> to CRTC\n");
> + if (config->prop_palette_before_ctm) {
> + drm_object_attach_property(mode_obj,
> + config->prop_palette_before_ctm, 0);
> + DRM_DEBUG_DRIVER("Attached Palette (before CTM) 
> property to CRTC\n");
> + } else
> + DRM_ERROR("Error attaching Palette (before CTM) 
> property to CRTC\n");
> + if (config->prop_palette_after_ctm) {
> + drm_object_attach_property(mode_obj,
> + config->prop_palette_after_ctm, 0);
> + DRM_DEBUG_DRIVER("Attached Palette (after CTM) property 
> to CRTC\n");
> + } else
> + DRM_ERROR("Error attaching Palette (after CTM) property 
> to CRTC\n");
> + if (config->prop_ctm) {
> + drm_object_attach_property(mode_obj,
> + config->prop_ctm, 0);
> + DRM_DEBUG_DRIVER("Attached CTM property to CRTC\n");
> + } else
> + DRM_ERROR("Error attaching CTM property to CRTC\n");
> + }

Way too verbose, don't add any of those log message to the driver, I can
see how they may be useful for the initial effort, but it's enough to
inspect the properties 

[PATCH 04/12] drm: Add structures for querying color capabilities

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:14PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> The DRM color management framework is targeting various hardware
> platforms and drivers. Different platforms can have different color
> correction and enhancement capabilities.
> 
> A commom user space application can query these capabilities using the
> DRM property interface. Each driver can fill this property with its
> platform's color capabilities.
> 
> This patch adds new structures in DRM layer for querying color
> capabilities. These structures will be used by all user space
> agents to configure appropriate color configurations.

As I indicated before, I don't think we should go for a full fledged 
query API, because, I don't believe we can ever make it good enough to 
cover future hardware (and probably not today's hardware across 
vendors). 

These kinds of query APIs have been frown upon in the past for that 
exact reason. 

- Accept configurations that are mostly likely to be working across vendors 
(256 enties for 8 bits) That should be enough for basic functionality. 

- To support things that are really hw specific: make sure the kernel API can 
accept those, put the hw specific knowledge into a user-space HAL where APIs 
can evolve. What you're trying to do here with queries about per-platform 
details can go into userspace and still have a generic compositor code using 
those limits. Let's just not put the limits into the kernel API, we won't be 
able to get it right. 

Now maybe there's a middle ground and we can expose basic limits. In this case, 
maybe a list of supported LUT sizes, but the sampling details don't belong to a 
kernel interface IMHO. I'm even hesitant putting the hw precision at that 
level. 

> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  include/uapi/drm/drm.h | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..d9562a2 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,40 @@ struct drm_event_vblank {
>   __u32 reserved;
>  };
>  
> +struct drm_palette_sampling_details {
> + __u32 sample_fract_precision;
> + __u32 last_sample_int_max;
> + __u32 remaining_sample_int_max;
> + __u32 num_samples;
> +};

If we're are indeed going with this, it will need documentation in the
code. Why do we need the precision for instance? It's not clear to me.

Also, we don't describe what the values beyond 1.0 represent, how the
interpolation is done, etc... (to support my first point to no try
describing too much).

> +struct drm_palette_caps {
> + __u32 version;
> + __u32 num_supported_types;
> + struct drm_palette_sampling_details palette_sampling_types[4];
> +};
> +
> +struct drm_ctm_caps {
> + __u32 version;
> + __u32 ctm_coeff_fract_precision;
> + __u32 ctm_coeff_int_max;
> + __s32 ctm_coeff_int_min;
> +};

Same story for those precision, int_min and int_max fields. Why do we need
those? if we get 256 enties, we can take the values in the interface form
(whatever it is, say 15.16 + sign bit) and clip the values to what the hardware
supports.

> +struct drm_cge_caps {
> + __u32 version;
> + __u32 cge_max_weight;
> +};

This doesn't beling to DRM
> +struct drm_color_caps {
> + __u32 version;
> + __u32 reserved;

Why do we need reserved here? isn't version enough to make sure we can grow the
structure?

> + struct drm_palette_caps palette_caps_after_ctm;
> + struct drm_palette_caps palette_caps_before_ctm;
> + struct drm_ctm_caps ctm_caps;
> + struct drm_cge_caps cge_caps;

No CGE in the DRM core, other hw won't have it.

> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.5
> 

[PATCH 09/12] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

On Wed, Jul 01, 2015 at 09:18:19PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> CHV/BSW platform supports various Gamma correction modes, which are:
> 1. Legacy 8-bit mode
> 2. 10-bit CGM (Color Gamut Mapping) mode
> 
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
>platform
> 2. Adds Gamma correction macros/defines
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|  10 ++
>  drivers/gpu/drm/i915/intel_atomic.c|   6 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 154 
> +
>  drivers/gpu/drm/i915/intel_color_manager.h |  12 +++
>  drivers/gpu/drm/i915/intel_drv.h   |   2 +
>  5 files changed, 184 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 313b1f9..36672e7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7900,4 +7900,14 @@ enum skl_disp_power_wells {
>  #define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
>  #define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
>  
> +/* Color Management */
> +#define PIPEA_CGM_CONTROL(VLV_DISPLAY_BASE + 0x67A00)
> +#define PIPEA_CGM_GAMMA_MIN  (VLV_DISPLAY_BASE + 0x67000)
> +#define CGM_OFFSET   0x2000
> +#define GAMMA_OFFSET 0x2000
> +#define _PIPE_CGM_CONTROL(pipe) \
> + (PIPEA_CGM_CONTROL + (pipe * CGM_OFFSET))
> +#define _PIPE_GAMMA_BASE(pipe) \
> + (PIPEA_CGM_GAMMA_MIN + (pipe * GAMMA_OFFSET))

We use the _PIPE() macro with tha pipe A and B registers for those ,
instead of having to defies the offsets.

>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
> b/drivers/gpu/drm/i915/intel_atomic.c
> index d2674a6..21f0ac2 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -473,6 +473,12 @@ int intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  struct drm_property *property,
>  uint64_t val)
>  {
> + struct drm_device *dev = crtc->dev;
> + struct drm_mode_config *config = >mode_config;
> +
> + if (property == config->prop_palette_after_ctm)
> + return intel_color_manager_set_gamma(dev, >base, val);
> +
>   DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>   return -EINVAL;
>  }

You are touching the hardware instead of staging the configuration?

> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index 71b4c05..84cc3e47 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,160 @@
>  
>  #include "intel_color_manager.h"
>  
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> +   struct drm_crtc *crtc)
> +{
> + if (num_samples == 0) {

[...]

> + } else if (num_samples == CHV_8BIT_GAMMA_MAX_VALS) {

[...]
> + } else if (num_samples == CHV_10BIT_GAMMA_MAX_VALS) {

[...]

> + } else {
> + DRM_ERROR("Invalid number of samples for Gamma LUT\n");
> + return -EINVAL;
> + }

This means you're not accepting 256 values, something we do today with
the legacy ioctl() and something we probably want for generic userspace.

> + ret = drm_property_replace_global_blob(dev, , length,
> + (void *) gamma_data, >base,
> + config->prop_palette_after_ctm);
> +
> + if (ret) {
> + DRM_ERROR("Error updating Gamma blob\n");
> + return -EFAULT;
> + }
> +
> + return ret;
> +}
> +
> +int intel_color_manager_set_gamma(struct drm_device *dev,
> + struct drm_mode_object *obj, uint32_t blob_id)
> +{
> + struct drm_crtc *crtc = obj_to_crtc(obj);
> +
> + if (IS_CHERRYVIEW(dev))
> + return chv_set_gamma(dev, blob_id, crtc);
> +
> + return -EINVAL;
> +}
> +
>  int get_chv_pipe_capabilities(struct drm_device *dev,
>   struct drm_color_caps *color_caps, struct drm_crtc *crtc)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h 
> b/drivers/gpu/drm/i915/intel_color_manager.h
> index 32262ac..d83567a 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -31,6 +31,8 @@
>  #define CHV_PALETTE_STRUCT_VERSION   1
>  #define CHV_CTM_STRUCT_VERSION   1
>  #define CHV_PLATFORM_STRUCT_VERSION  1
> +#define CHV_GAMMA_DATA_STRUCT_VERSION1
> +
>  #define CHV_MAX_PALETTE_CAPS_BEFORE_CTM  1
>  #define CHV_MAX_PALETTE_CAPS_AFTER_CTM   2
>  #define CHV_DEGAMMA_PRECISION14
> @@ -43,6 +45,16 @@
>  #define CHV_10BIT_GAMMA_MAX_VALS 

[PATCH v2 08/10] drm: Add CSC correction structure

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:39PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch adds a new structure in DRM layer for CSC color correction.
> This structure will be used by all user space agents to configure
> appropriate CSC format and CSC level.

And another little one:

> /* Color Management structure for CSC */
> struct drm_intel_csc {
>   __u32 csc_level;
>   (The csc_level indicates whether the CSC to be applied at
>   pipe/plane level)
>   __u32 csc_format;
>   (The csc_format indicates the supported correction value \
>   formats by underlying hardware)
>   __u32 reserved;
>   __s32 csc_matrix[9];
>   (Raw CSC correction matrix)

You haven't defined if that data represents a row-major or column-major
matrix.

> };

-- 
Damien

[PATCH v2 01/10] drm/i915: Initialize Color Manager

[Damien Lespiau]

On Tue, Jun 09, 2015 at 11:34:44AM +0100, Damien Lespiau wrote:
> "CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix
> transform on colors, maybe we could be more generic there?
> "color-matrix"? This also depends if we want to try to create somewhat
> generic properties or not (if not, maybe we should prefix the properties
> with "i915-")

Actually, given the list of current properties "color-matrix" would look
out of place (but seems like a good property name if you're doing a bit
of CSS). People seem to go with either:

  - "COLOR_MATRIX", please don't :(
  - "color_matrix"
  - "color matrix"
  - "Color matrix"

Oh well, naming rat hole...

-- 
DAmien

[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch does the following:
> 1. Adds the core function to program Gamma correction values for CHV/BSW
>platform

A couple of things I forgot (they are probably others):
  - This patch doesn't actually add anything for CHV despite the claim
  - The properties are exposed everywhere, but return -EINVAL on
!IS_CHERRYVIEW. We only expose the properties on a platform when we
do have code that can make this property work for that platform.
This way userspace can easily know if the feature is supported or
not by querying if the property exists.

-- 
Damien

[PATCH v2 00/10] Color Manager Implementation

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color 
> correction/enhancement. Various Intel platforms support several
> color correction capabilities. Color Manager provides abstraction
> of these properties and allows a user space UI agent to 
> correct/enhance the display.

So I did a first rough pass on the API itself. The big question that
isn't solved at the moment is: do we want to try to do generic KMS
properties for pre-LUT + matrix + post-LUT or not. "Generic" has 3 levels:

  1/ Generic for all KMS drivers
  2/ Generic for i915 supported platfoms
  3/ Specific to each platform

At this point, I'm quite tempted to say we should give 1/ a shot. We
should be able to have pre-LUT + matrix + post-LUT on CRTC objects and
guarantee that, when the drivers expose such properties, user space can
at least give 8 bits LUT + 3x3 matrix + 8 bits LUT.

It may be possible to use the "try" version of the atomic ioctl to
explore the space of possibilities from a generic user space to use
bigger LUTs as well. A HAL layer (which is already there in some but not
all OSes) would still be able to use those generic properties to load
"precision optimized" LUTs with some knowledge of the hardware.

Option 3/ is, IMHO, a no-go, we should really try hard to limit the work
we need to do per-platform, which means defining a common format for the
values we give to the kernel. As stated in various places, 16.16 seems
the format of choice, even for the LUTs as we have wide gamut support in
some of the LUTs where we can map values > 1.0 to other values > 1.0.

Another thing, the documentation of the interface needs to be a bit more
crisp. For instance, we don't currently define the order in which the
CSC and LUT transforms of this patch set are applied: is this a de-gamma
LUT to do the CSC in linear space? but then that means the display is
linear, oops. So it must be a post-CSC lut, but then we don't de-gamma
sRGB (not technically a single gamma power curve for sRGB, but details,
details) before applying a linear transform. So with this interface, we
have to enforce the fbs are linear, losing dynamic range. I'm sure later
patches would expose more properties, but as a stand-alone patch set, it
would seem we can't do anything useful?

-- 
Damien

[PATCH v2 10/10] drm/i915: Add CSC support for CHV/BSW

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:41PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch does the following:
> 1. Adds the core function to program CSC correction values for
>CHV/BSW platform
> 2. Adds CSC correction macros/defines
> 3. Adds a pointer to hold blob for CSC property in drm_crtc
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/intel_atomic.c|   3 +-
>  drivers/gpu/drm/i915/intel_color_manager.c | 115 
> +
>  drivers/gpu/drm/i915/intel_color_manager.h |  26 +++
>  3 files changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 4726847..dcf4694 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -433,7 +433,8 @@ intel_crtc_atomic_set_property(struct drm_crtc *crtc,
>  
>   if (property == config->gamma_property)
>   return intel_color_manager_set_gamma(dev, >base, val);
> + if (property == config->csc_property)
> + return intel_color_manager_set_csc(dev, >base, val);
>  
> - DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
>   return -EINVAL;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index 421c267..d904050 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,108 @@
>  
>  #include "intel_color_manager.h"
>  
> +int chv_set_csc(struct drm_device *dev, uint64_t blob_id,
> + struct drm_crtc *crtc)
> +{
> + struct drm_csc *csc_data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_property_blob *blob;
> + struct drm_mode_config *config = >mode_config;
> + u32 reg;
> + enum pipe pipe;
> + s16 csc_value;
> + s32 word, temp;
> + int ret, count = 0;
> +
> + blob = drm_property_lookup_blob(dev, blob_id);
> + if (!blob) {
> + DRM_ERROR("Invalid Blob ID\n");
> + return -EINVAL;
> + }
> +
> + csc_data = (struct drm_csc *)blob->data;
> + pipe = to_intel_crtc(crtc)->pipe;
> +
> + if (csc_data->csc_format == I915_CSC_COEFF_FORMAT_UNKNOWN) {
> +
> + /* Disable CSC functionality */
> + reg = _PIPE_CGM_CONTROL(pipe);
> + I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
> +
> + DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
> + pipe_name(pipe));
> + ret = 0;

Same remarks here I did for the gamma property. i915 specific defines
for an interface that looks like it want to be generic, UNKNOWN Vs
DISABLED, DRM_ERROR, ...

And I haven't actually looked at the CHV details in this pass.

> + } else if (csc_data->csc_format == I915_CSC_COEFF_FORMAT_S2_29) {
> +
> + /* Disable CSC functionality in case it was set earlier */
> + reg = _PIPE_CGM_CONTROL(pipe);
> + I915_WRITE(reg, I915_READ(reg) & (~CGM_CSC_EN));
> +
> + DRM_DEBUG_DRIVER("Disabled CSC Functionality on Pipe %c\n",
> + pipe_name(pipe));
> +
> + reg = _PIPE_CSC_BASE(pipe);
> + while (count < CSC_MAX_VALS) {
> +
> + /* Rounding off, to decrease loss of precision */
> + if (csc_data->csc_matrix[count] < 0) {
> + temp = csc_data->csc_matrix[count];
> + temp -= CHV_CSC_ROUNDOFF;
> + if (temp < CHV_CSC_MIN)
> + temp = CHV_CSC_MIN;
> + } else {
> + temp = csc_data->csc_matrix[count];
> + temp += CHV_CSC_ROUNDOFF;
> + if (temp > CHV_CSC_MAX)
> + temp = CHV_CSC_MIN;
> + }
> + csc_value = temp >> S2_29_CSC_COEFF_SHIFT;
> + word = csc_value;
> +
> + /*
> +  * Last value to be written in 1 register.
> +  * Otherwise, each pair of CSC values go
> +  * into 1 register
> +  */
> + if (count != (CSC_MAX_VALS - 1)) {
> + count++;
> + csc_value = temp >> S2_29_CSC_COEFF_SHIFT;
> + temp = csc_value;
> + temp <<= CHV_CSC_SHIFT;
> + word |= temp;
> + }
> + I915_WRITE(reg, word);
> + reg += 4;
> + count++;
> + }
> +
> + DRM_DEBUG_DRIVER("All CSC values written to registers\n");
> +
> + /* Enable CSC 

[PATCH v2 08/10] drm: Add CSC correction structure

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:39PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch adds a new structure in DRM layer for CSC color correction.
> This structure will be used by all user space agents to configure
> appropriate CSC format and CSC level.
> 
> /* Color Management structure for CSC */
> struct drm_intel_csc {
>   __u32 csc_level;
>   (The csc_level indicates whether the CSC to be applied at
>   pipe/plane level)

Same as before, I don't believe we need this.

>   __u32 csc_format;
>   (The csc_format indicates the supported correction value \
>   formats by underlying hardware)

Well, as with this whole series, leaking hw specific details in generic
interfaces.

>   __u32 reserved;
>   __s32 csc_matrix[9];
>   (Raw CSC correction matrix)

Well, here again, hw specific, how about going for 16.16 fixed point?

> };

> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  include/drm/drm_crtc.h | 1 +
>  include/uapi/drm/drm.h | 8 
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 31b52cb..bcba1e7 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -486,6 +486,7 @@ struct drm_crtc {
>  
>   /* Color Management Blob IDs */
>   u32 gamma_blob_id;
> + u32 csc_blob_id;
>  };
>  
>  /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index fc2661c..2458b6c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -839,6 +839,14 @@ struct drm_gamma {
>   __u16 values[0];
>  };
>  
> +/* Color Management structure for CSC */
> +struct drm_csc {
> + __u32 csc_level;
> + __u32 csc_format;
> + __u32 reserved;
> + __s32 csc_matrix[9];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.2
> 

[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

(Note I haven't actually looked at the CHV specific details just yet,
that'll be for another pass).

On Thu, Jun 04, 2015 at 07:12:38PM +0530, Kausal Malladi wrote:
> +int chv_set_gamma(struct drm_device *dev, uint32_t blob_id,
> + struct drm_crtc *crtc)
> +{
> + struct drm_gamma *gamma_data;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_property_blob *blob;
> + struct drm_mode_config *config = >mode_config;
> + u32 cgm_control_reg = 0;
> + u32 cgm_gamma_reg = 0;
> + u32 reg, val;
> + enum pipe pipe;
> + u16 red, green, blue;
> + struct rgb_pixel correct_rgb;
> + u32 count = 0;
> + struct rgb_pixel *correction_values = NULL;
> + u32 num_samples;
> + u32 word;
> + u32 palette;
> + int ret = 0, length;
> +
> + blob = drm_property_lookup_blob(dev, blob_id);
> + if (!blob) {
> + DRM_ERROR("Invalid Blob ID\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> + return -EINVAL;
> + }
> +
> + gamma_data = (struct drm_gamma *)blob->data;
> + pipe = to_intel_crtc(crtc)->pipe;
> + num_samples = gamma_data->num_samples;
> + length = num_samples * sizeof(struct rgb_pixel);
> +
> + if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> +
> + /* Disable Gamma functionality on Pipe - CGM Block */
> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> + cgm_control_reg &= ~CGM_GAMMA_EN;
> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> + DRM_DEBUG_DRIVER("Gamma disabled on Pipe %c\n",
> + pipe_name(pipe));
> + ret = 0;
> + } else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_LEGACY) {
> +
> + if (num_samples != CHV_8BIT_GAMMA_MAX_VALS) {
> + DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> + return -EINVAL;
> + }

This means the current code doesn't allow us to load LUTs with 256
values (like today). That's not what we want.

Have you looked at the interactions between this property and the legacy
gamma ioctl()?

> +
> + /* First, disable CGM Gamma, if already set */
> + cgm_control_reg = I915_READ(_PIPE_CGM_CONTROL(pipe));
> + cgm_control_reg &= ~CGM_GAMMA_EN;
> + I915_WRITE(_PIPE_CGM_CONTROL(pipe), cgm_control_reg);
> +
> + /* Enable (Legacy) Gamma on Pipe */
> + palette = _PIPE_GAMMA_BASE(pipe);
> +
> + count = 0;
> + correction_values = (struct rgb_pixel *)_data->values;
> + while (count < num_samples) {
> + correct_rgb = correction_values[count];
> + blue = correction_values[count].blue;
> + green = correction_values[count].green;
> + red = correction_values[count].red;
> +
> + blue = blue >> CHV_8BIT_GAMMA_MSB_SHIFT;
> + green = green >> CHV_8BIT_GAMMA_MSB_SHIFT;
> + red = red >> CHV_8BIT_GAMMA_MSB_SHIFT;
> +
> + /* Red (23:16), Green (15:8), Blue (7:0) */
> + word = (red << CHV_8BIT_GAMMA_SHIFT_RED_REG) |
> + (green <<
> +  CHV_8BIT_GAMMA_SHIFT_GREEN_REG) |
> + blue;
> + I915_WRITE(palette, word);
> +
> + palette += 4;
> + count++;
> + }
> + reg = PIPECONF(pipe);
> + val = I915_READ(reg) | PIPECONF_GAMMA;
> + I915_WRITE(reg, val);
> +
> + DRM_DEBUG_DRIVER("Gamma LUT loaded successfully for Pipe %c\n",
> + pipe_name(pipe));
> + ret = 0;
> + } else if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_10BIT) {
> +
> + if (num_samples != CHV_10BIT_GAMMA_MAX_VALS) {
> + DRM_ERROR("Incorrect number of samples received\n");

Not DRM_ERROR. We do give -EINVAL hint for people developping user
space, but with DRM_DEBUG_KMS().

> + return -EINVAL;
> + }
> +
> + /* Enable (CGM) Gamma on Pipe */
> + cgm_gamma_reg = _PIPE_GAMMA_BASE(pipe);
> +
> + count = 0;
> + correction_values = (struct rgb_pixel *)_data->values;
> + while (count < num_samples) {
> + correct_rgb = correction_values[count];
> + blue = correction_values[count].blue;
> + green = correction_values[count].green;
> + red = correction_values[count].red;
> +
> + blue = blue >> CHV_10BIT_GAMMA_MSB_SHIFT;

[PATCH v2 07/10] drm/i915: Add pipe level Gamma correction for CHV/BSW

[Damien Lespiau]

On Sat, Jun 06, 2015 at 05:39:23PM +0530, Sharma, Shashank wrote:
> >>+if (gamma_data->gamma_precision == I915_GAMMA_PRECISION_UNKNOWN) {
> >Switch/case instead?
> Yep, got it.
> >
> >Also, is UNKNOWN for disabling? Why not rename it to DISABLE then?
> Actually unknown is valid in case of get_property() when we want to query
> about the capabilities, just want to reuse the same, to avoid need for
> another one. Else we have to handle one extra case in each get_prop
> (disable) and set_prop(unknown)

Is it? the code isn't telling that story, at least in the current form.

get_property() should primarily be for retrieving the current value of
that property, so I'd suggest having a precision field of 0 means
what is CURRENT today (that'd be the default expected behaviour of
get_property()).

Then, 2 other "needs":

 - Query interface: how useful is the query interface in this present
   form? a query for the precision only isn't that useful as we need
   per-platform knowledge beyond that anyway (say we can't encode things
   like split gamma configurations sharing the same LUT storage)

 - Some other value to disable the function (set_property() with
   precision = 0 looks fine to me.

The get_property() path is missing from this patch set AFAICT.

-- 
Damien

[PATCH v2 04/10] drm: Add Gamma correction structure

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:35PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch adds a new structure in DRM layer for Gamma color correction.
> This structure will be used by all user space agents to configure
> appropriate Gamma precision and Gamma level.
> 
> struct drm_intel_gamma {
>__u32 gamma_level;
>   (The gamma_level variable indicates if the Gamma correction is to be

Why do we need this? The properties are installed on pipe and plane
objects and the set_property() interface already tells us which object
we are talking about.

>   applied on Pipe/plane)
>__u32 gamma_precision;
>   (The Gamma precision indicates the Gamma mode to be applied)
> 
>   Supported precisions are -
>   #define I915_GAMMA_PRECISION_UNKNOWN0

UNKNOWN is used to disable the function, why not call it GAMMA_DISABLE?

>   #define I915_GAMMA_PRECISION_CURRENT0x

Current isn't used in this patch set.

>   #define I915_GAMMA_PRECISION_LEGACY (1 << 0)

That looks like a name leaking hw specific knowledge to an interface
that want to be generic. _8BITS?

>   #define I915_GAMMA_PRECISION_10BIT  (1 << 1)
>   #define I915_GAMMA_PRECISION_12BIT  (1 << 2)
>   #define I915_GAMMA_PRECISION_14BIT  (1 << 3)
>   #define I915_GAMMA_PRECISION_16BIT  (1 << 4)

I915_ prefix but the structures are in drm.h. Here again, there's an
impedance mismatch between generic properties and driver (or worse
platform) specific interface.

> 
>   __u32 num_samples;
>   (The num_samples indicates the number of Gamma correction
>   coefficients)
>   __u32 reserved;
>   __u16 values[0];
>   (An array of size 0, to accommodate the "num_samples" number of
>   R16G16B16 pixels, dynamically)

That doesn't seem enough. We can have values > 1.0 for wide gamut
pipelines, I'd suggest 16.16 fixed point values.

> };

The defines and documentation of that fields should be in the code, not
in the commit message. It'd also be handy to have all the corresponding
defines in this patch so we have a good view of the API.

> 
> v2: Addressing Daniel Stone's comment, added a variable sized array to
> carry Gamma correction values as blob property.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  include/drm/drm_crtc.h |  3 +++
>  include/uapi/drm/drm.h | 10 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2a75d7d..bc44f27 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -483,6 +483,9 @@ struct drm_crtc {
>* acquire context.
>*/
>   struct drm_modeset_acquire_ctx *acquire_ctx;
> +
> + /* Color Management Blob IDs */
> + u32 gamma_blob_id;
>  };
>  
>  /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3801584..fc2661c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -829,6 +829,16 @@ struct drm_event_vblank {
>   __u32 reserved;
>  };
>  
> +/* Color Management structure for Gamma */
> +struct drm_gamma {
> + __u32 flags;
> + __u32 gamma_level;
> + __u32 gamma_precision;
> + __u32 num_samples;
> + __u32 reserved;
> + __u16 values[0];
> +};
> +
>  /* typedef area */
>  #ifndef __KERNEL__
>  typedef struct drm_clip_rect drm_clip_rect_t;
> -- 
> 2.4.2
> 

[PATCH v2 02/10] drm/i915: Attach color properties to CRTC

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:33PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> Every CRTC would be carrying its own instances of CSC and Gamma color
> correction values. This patch adds a new function to attach color
> properties to respective CRTCs while initialization.
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/intel_color_manager.c | 24 
>  drivers/gpu/drm/i915/intel_color_manager.h |  2 ++
>  drivers/gpu/drm/i915/intel_display.c   |  3 +++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> index f7e2380..8d4ee8f 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.c
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -27,6 +27,30 @@
>  
>  #include "intel_color_manager.h"
>  
> +void intel_color_manager_attach(struct drm_device *dev,
> + struct drm_mode_object *mode_obj)
> +{
> + struct drm_mode_config *config = >mode_config;
> +
> + /* Attach all properties generic to crtc and plane */
> + if (config->gamma_property) {
> + drm_object_attach_property(mode_obj,
> + config->gamma_property, 0);

No platform check? I doubt all of our platforms have LUTs on all planes.
Ah but we don't attach this property to any plane just yet, so the
comment is misleading.

> +
> + DRM_DEBUG_DRIVER("Initialized gamma property\n");

That looks too verbose to me.

> + }
> +
> + /* Attach properties specific to crtc only */
> + if (mode_obj->type == DRM_MODE_OBJECT_CRTC) {
> + if (config->csc_property) {
> + drm_object_attach_property(mode_obj,
> + config->csc_property, 0);
> +
> + DRM_DEBUG_DRIVER("Initialized CSC property\n");
> + }
> + }
> +}
> +
>  void intel_color_manager_init(struct drm_device *dev)
>  {
>   struct drm_mode_config *config = >mode_config;
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h 
> b/drivers/gpu/drm/i915/intel_color_manager.h
> index 154bf16..a55ce23 100644
> --- a/drivers/gpu/drm/i915/intel_color_manager.h
> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
> @@ -30,3 +30,5 @@
>  
>  /* Generic Function prototypes */
>  void intel_color_manager_init(struct drm_device *dev);
> +void intel_color_manager_attach(struct drm_device *dev,
> + struct drm_mode_object *mode_obj);
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 2322dee..d4e9aa3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13794,6 +13794,9 @@ static void intel_crtc_init(struct drm_device *dev, 
> int pipe)
>   intel_crtc->cursor_cntl = ~0;
>   intel_crtc->cursor_size = ~0;
>  
> + /* Attaching color properties to the CRTC */
> + intel_color_manager_attach(dev, _crtc->base.base);
> +
>   BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>   dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = _crtc->base;
> -- 
> 2.4.2
> 

[PATCH v2 01/10] drm/i915: Initialize Color Manager

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:32PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> Color Manager is an extension in i915 driver to handle color correction
> and enhancements across various Intel platforms.
> 
> This patch initializes color manager framework by :
> 1. Adding two new files, intel_color_manager(.c/.h)
> 2. Introducing new pointers in DRM mode_config structure to
>carry CSC and Gamma color correction properties.
> 3. Creating these DRM properties in Color Manager initialization
>sequence.
> 
> v2: Addressing Sonika's review comment.
> 1. Made intel_color_manager_init void
> 2. Moved init after NUM_PIPES check
> 
> Signed-off-by: Shashank Sharma 
> Signed-off-by: Kausal Malladi 
> ---
>  drivers/gpu/drm/i915/Makefile  |  3 ++
>  drivers/gpu/drm/i915/intel_color_manager.c | 48 
> ++
>  drivers/gpu/drm/i915/intel_color_manager.h | 32 
>  drivers/gpu/drm/i915/intel_display.c   |  3 ++
>  include/drm/drm_crtc.h |  4 +++
>  5 files changed, 90 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.c
>  create mode 100644 drivers/gpu/drm/i915/intel_color_manager.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b7ddf48..c62d048 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -89,6 +89,9 @@ i915-y += i915_vgpu.o
>  # legacy horrors
>  i915-y += i915_dma.o
>  
> +# Color Management
> +i915-y += intel_color_manager.o
> +

FWIW, I'd put this in the "# modesetting core code" section. The objects
are somewhat sorted by big categories, having a category saying that
"intel_color_manager.c" is for "Color Management" is, well, not that
useful :)

>  obj-$(CONFIG_DRM_I915)  += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c 
> b/drivers/gpu/drm/i915/intel_color_manager.c
> new file mode 100644
> index 000..f7e2380
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Shashank Sharma 
> + * Kausal Malladi 
> + */
> +
> +#include "intel_color_manager.h"
> +
> +void intel_color_manager_init(struct drm_device *dev)

This function create "generic" properties (more on that in the cover
letter) in drm_mode_config. It looks like it should be part of the DRM
core, not i915?

> +{
> + struct drm_mode_config *config = >mode_config;
> +
> + /* Create Gamma and CSC properties */
> + config->gamma_property = drm_property_create(dev,
> + DRM_MODE_PROP_BLOB, "gamma_property", 0);

You are creating properties with the names "gamma_property",
"csc_property". While we do have (and that's quite unfortunate) a
variety of naming schemes for properties none of them include the
_property suffix. Just "gamma" and "csc" would be more appropriate. See
the current list of properties:

https://www.kernel.org/doc/htmldocs/drm/drm-kms-properties.html#idp13803344

Speaking of which, you also need to add the property to the
documentation (Documentation/DocBook/drm.xml):

> + if (!config->gamma_property)
> + DRM_ERROR("Gamma property creation failed\n");
> + else
> + DRM_DEBUG_DRIVER("Created Gamma property\n");

That's really too verbose on the logs, I don't think you need any of
them. It's easy enough to check if the property was created, it's part
of the API.
> +
> + config->csc_property = drm_property_create(dev,
> + DRM_MODE_PROP_BLOB, "csc_property", 0);

"CSC" is quite specific to how Intel calls the unit doing the 3x3 matrix
transform on colors, maybe we could be more generic there?
"color-matrix"? This also depends if we want to try to 

[PATCH v2 00/10] Color Manager Implementation

[Damien Lespiau]

On Thu, Jun 04, 2015 at 07:12:31PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch set adds color manager implementation in drm/i915 layer.
> Color Manager is an extension in i915 driver to support color 
> correction/enhancement. Various Intel platforms support several
> color correction capabilities. Color Manager provides abstraction
> of these properties and allows a user space UI agent to 
> correct/enhance the display.

For some reason, this patch set doesn't appear to have reached the
mailing lists: people are saying they haven't received the emails and the
mailing list archives[1][2] don't show them. Out of curiosity, how did
you send those emails?

Thanks,

-- 
Damien

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-June/thread.html
[2] http://lists.freedesktop.org/archives/dri-devel/2015-June/thread.html

[PATCH 0/7] Color Manager Implementation

[Damien Lespiau]

On Tue, Jun 02, 2015 at 01:22:42AM +0530, Kausal Malladi wrote:
> From: Kausal Malladi 
> 
> This patch set adds color manager implementation in drm/i915 layer.

Is anyone working on tests/test plan?

Thanks,

-- 
Damien

[Intel-gfx] drivers/gpu/drm/i915/i915_gem_gtt.c

[Damien Lespiau]

On Fri, May 22, 2015 at 02:17:32PM -0700, Andrew Morton wrote:
> I'm not sure what's happened to the drm code in linux-next - it's
> exploding all over the place.  Did someone turn on -Werror without
> doing anywhere near enough testing?
> 
> Anyway, I don't know how to fix this i386 build error:

Seems like you have CONFIG_DRM_I915_WERROR set?

We explicitely made sure to not enable -Werror by default, because
different versions of gcc will generate different set of warnings making
it roughly impossible to use -Werror by default (say, newer versions of
gcc with more clever warnings trying to compile old commits while
bisecting) 

-- 
Damien

[PATCH 1/3] RELEASING: Fix releasing instructions to match the latest release.sh

[Damien Lespiau]

On Thu, Mar 19, 2015 at 05:39:06PM +, Emil Velikov wrote:
> On 19/03/15 16:35, Damien Lespiau wrote:
> > - Running "make distcheck" should result in no warnings or errors
> > - and end with a message of the form:
> > + Verify that the code passes "make distcheck".  Running "make
> > + distcheck" should result in no warnings or errors and end with a
> > + message of the form:
> >  
> Side note: Pretty sure that current make distcheck produces a handful of
> warnings ;-)

Yup, it does, that'll have to be for another time though.

> Although we'll try to have them sorted by next release.

That'd be excellent.

> With my comment in patch 2 and Ilia's in 3 the series is
> Reviewed-by: Emil Velikov 

Done and pushed, thanks for the review!

-- 
Damien

[PATCH 2/3] RELEASING: Fix the step numbering

[Damien Lespiau]

On Thu, Mar 19, 2015 at 05:32:28PM +, Emil Velikov wrote:
> On 19/03/15 16:35, Damien Lespiau wrote:
> > Signed-off-by: Damien Lespiau 
> > ---
> >  RELEASING | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/RELEASING b/RELEASING
> > index e17dbea..8ed7490 100644
> > --- a/RELEASING
> > +++ b/RELEASING
> > @@ -13,14 +13,14 @@ Follow these steps to release a new version of libdrm:
> ...
> > -  7) Push the commit and tag by saying
> > +  6) Push the commit and tag by saying
> >  
> > git push --tags origin master
> >  
> >   assuming the remote for the upstream libdrm repo is called origin.
> >  
> > -  6) Use the release.sh script from the xorg/util/modular repo to
> > +  8) Use the release.sh script from the xorg/util/modular repo to
> This should be 7 :-)

I'm crying a little. Can't count and typoed a patch that fixed a typo.

-- 
Damien

[PATCH v2] intel: Merge latest i915_drm.h

[Damien Lespiau]

On Thu, Mar 19, 2015 at 05:11:08PM +, Neil Roberts wrote:
> The main incentive to do this is to get I915_PARAM_REVISION.
> 
> v2: Rebase on top of some changes that were made to the header without
> copying the whole file from the kernel source.
> 
> Signed-off-by: Neil Roberts 

Reviewed-by: Damien Lespiau 

And merged into master. Thanks!

-- 
Damien

> ---
> 
> Here is a v2 of the patch just to rebase it on the changes that were
> added manually in commit d556e068a7e4e9d.
> 
>  include/drm/i915_drm.h | 48 ++--
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index b037e56..ded43b1 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -224,6 +224,8 @@ typedef struct _drm_i915_sarea {
>  #define DRM_I915_REG_READ0x31
>  #define DRM_I915_GET_RESET_STATS 0x32
>  #define DRM_I915_GEM_USERPTR 0x33
> +#define DRM_I915_GEM_CONTEXT_GETPARAM0x34
> +#define DRM_I915_GEM_CONTEXT_SETPARAM0x35
>  
>  #define DRM_IOCTL_I915_INIT  DRM_IOW( DRM_COMMAND_BASE + 
> DRM_I915_INIT, drm_i915_init_t)
>  #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + 
> DRM_I915_FLUSH)
> @@ -274,7 +276,9 @@ typedef struct _drm_i915_sarea {
>  #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY   DRM_IOW (DRM_COMMAND_BASE + 
> DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
>  #define DRM_IOCTL_I915_REG_READ  DRM_IOWR 
> (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
>  #define DRM_IOCTL_I915_GET_RESET_STATS   DRM_IOWR 
> (DRM_COMMAND_BASE + DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
> -#define DRM_IOCTL_I915_GEM_USERPTR   DRM_IOWR(DRM_COMMAND_BASE + 
> DRM_I915_GEM_USERPTR,  struct drm_i915_gem_userptr)
> +#define DRM_IOCTL_I915_GEM_USERPTR   DRM_IOWR 
> (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, struct drm_i915_gem_userptr)
> +#define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM  DRM_IOWR (DRM_COMMAND_BASE + 
> DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
> +#define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM  DRM_IOWR (DRM_COMMAND_BASE + 
> DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
>  
>  /* Allow drivers to submit batchbuffers directly to hardware, relying
>   * on the security mechanisms provided by hardware.
> @@ -340,6 +344,10 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
>  #define I915_PARAM_HAS_WT 27
>  #define I915_PARAM_CMD_PARSER_VERSION 28
> +#define I915_PARAM_HAS_COHERENT_PHYS_GTT 29
> +#define I915_PARAM_MMAP_VERSION  30
> +#define I915_PARAM_HAS_BSD2   31
> +#define I915_PARAM_REVISION  32
>  #define I915_PARAM_SUBSLICE_TOTAL 33
>  #define I915_PARAM_EU_TOTAL   34
>  
> @@ -489,6 +497,14 @@ struct drm_i915_gem_mmap {
>* This is a fixed-size type for 32/64 compatibility.
>*/
>   __u64 addr_ptr;
> +
> + /**
> +  * Flags for extended behaviour.
> +  *
> +  * Added in version 2.
> +  */
> + __u64 flags;
> +#define I915_MMAP_WC 0x1
>  };
>  
>  struct drm_i915_gem_mmap_gtt {
> @@ -738,7 +754,13 @@ struct drm_i915_gem_execbuffer2 {
>   */
>  #define I915_EXEC_HANDLE_LUT (1<<12)
>  
> -#define __I915_EXEC_UNKNOWN_FLAGS -(I915_EXEC_HANDLE_LUT<<1)
> +/** Used for switching BSD rings on the platforms with two BSD rings */
> +#define I915_EXEC_BSD_MASK   (3<<13)
> +#define I915_EXEC_BSD_DEFAULT(0<<13) /* default ping-pong 
> mode */
> +#define I915_EXEC_BSD_RING1  (1<<13)
> +#define I915_EXEC_BSD_RING2  (2<<13)
> +
> +#define __I915_EXEC_UNKNOWN_FLAGS -(1<<15)
>  
>  #define I915_EXEC_CONTEXT_ID_MASK(0x)
>  #define i915_execbuffer2_set_context_id(eb2, context) \
> @@ -878,6 +900,12 @@ struct drm_i915_gem_get_tiling {
>* mmap mapping.
>*/
>   __u32 swizzle_mode;
> +
> + /**
> +  * Returned address bit 6 swizzling required for CPU access through
> +  * mmap mapping whilst bound.
> +  */
> + __u32 phys_swizzle_mode;
>  };
>  
>  struct drm_i915_gem_get_aperture {
> @@ -1061,11 +1089,19 @@ struct drm_i915_gem_userptr {
>  #define I915_USERPTR_READ_ONLY 0x1
>  #define I915_USERPTR_UNSYNCHRONIZED 0x8000
>   /**
> - * Returned handle for the object.
> - *
> - * Object handles are nonzero.
> - */
> +  * Returned handle for the object.
> +  *
> +  * Object handles are nonzero.
> +  */
>   __u32 handle;
>  };
>  
> +struct drm_i915_gem_context_param {
> + __u32 ctx_id;
> + __u32 size;
> + __u64 param;
> +#define I915_CONTEXT_PARAM_BAN_PERIOD 0x1
> + __u64 value;
> +};
> +
>  #endif /* _I915_DRM_H_ */
> -- 
> 1.9.3
> 

[PATCH 3/3] RELEASING: Fix annouce typo

[Damien Lespiau]

That's the only type :set spell found.

Signed-off-by: Damien Lespiau 
---
 RELEASING | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/RELEASING b/RELEASING
index 8ed7490..78e90c0 100644
--- a/RELEASING
+++ b/RELEASING
@@ -51,7 +51,7 @@ Follow these steps to release a new version of libdrm:

   8) Use the release.sh script from the xorg/util/modular repo to
  upload the tarballs to the freedesktop.org download area and
- create an annouce email template.  The script takes one argument:
+ create an announce email template.  The script takes one argument:
  the path to the libdrm checkout. So, if a checkout of modular is
  at the same level than the libdrm repo:

-- 
1.8.3.1

[PATCH 2/3] RELEASING: Fix the step numbering

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 RELEASING | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/RELEASING b/RELEASING
index e17dbea..8ed7490 100644
--- a/RELEASING
+++ b/RELEASING
@@ -13,14 +13,14 @@ Follow these steps to release a new version of libdrm:
  modifications. You're probably in a good state if both "git diff
  HEAD" and "git log master..origin/master" give no output.

-  3) Bump the version number in configure.ac. We seem to have settled
+  2) Bump the version number in configure.ac. We seem to have settled
  for 2.4.x as the versioning scheme for libdrm, so just bump the
  micro version.

-  4) Run autoconf and then re-run ./configure so the build system
+  3) Run autoconf and then re-run ./configure so the build system
  picks up the new version number.

-  5) (optional step, release.sh will make distcheck for you, but it can be
+  4) (optional step, release.sh will make distcheck for you, but it can be
   heart warming to verify that make distcheck passes)

  Verify that the code passes "make distcheck".  Running "make
@@ -36,20 +36,20 @@ Follow these steps to release a new version of libdrm:
  Make sure that the version number reported by distcheck and in
  the tarball names matches the number you bumped to in configure.ac.

-  6) Commit the configure.ac change and make an annotated tag for that
+  5) Commit the configure.ac change and make an annotated tag for that
  commit with the version number of the release as the name and a
  message of "libdrm X.Y.Z".  For example, for the 2.4.16 release
  the command is:

git tag -a 2.4.16 -m "libdrm 2.4.16"

-  7) Push the commit and tag by saying
+  6) Push the commit and tag by saying

git push --tags origin master

  assuming the remote for the upstream libdrm repo is called origin.

-  6) Use the release.sh script from the xorg/util/modular repo to
+  8) Use the release.sh script from the xorg/util/modular repo to
  upload the tarballs to the freedesktop.org download area and
  create an annouce email template.  The script takes one argument:
  the path to the libdrm checkout. So, if a checkout of modular is
-- 
1.8.3.1

[PATCH 1/3] RELEASING: Fix releasing instructions to match the latest release.sh

[Damien Lespiau]

It seems that the tests don't need DRM master anymore? at least make
distcheck passes when X is running.

release.sh is also invoked with just the path to the libdrm git checkout
and we don't want to pass additional arguments that will be treated as
additional modules we want to release.

Also, make a note that release.sh will run make distcheck for you, so we
don't strickly need to run it beforehand.

Signed-off-by: Damien Lespiau 
---
 RELEASING | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/RELEASING b/RELEASING
index 3f07146..e17dbea 100644
--- a/RELEASING
+++ b/RELEASING
@@ -20,13 +20,12 @@ Follow these steps to release a new version of libdrm:
   4) Run autoconf and then re-run ./configure so the build system
  picks up the new version number.

-  5) Verify that the code passes "make distcheck".  libdrm is tricky
- to distcheck since the test suite will need to become drm master.
- This means that you need to run it outside X, that is, in text
- mode (KMS or no KMS doesn't matter).
+  5) (optional step, release.sh will make distcheck for you, but it can be
+  heart warming to verify that make distcheck passes)

- Running "make distcheck" should result in no warnings or errors
- and end with a message of the form:
+ Verify that the code passes "make distcheck".  Running "make
+ distcheck" should result in no warnings or errors and end with a
+ message of the form:

=
libdrm-X.Y.Z archives ready for distribution:
@@ -52,11 +51,11 @@ Follow these steps to release a new version of libdrm:

   6) Use the release.sh script from the xorg/util/modular repo to
  upload the tarballs to the freedesktop.org download area and
- create an annouce email template.  The script takes three
- arguments: a "section", the previous tag and the new tag we just
- created.  For 2.4.16 again, the command is:
+ create an annouce email template.  The script takes one argument:
+ the path to the libdrm checkout. So, if a checkout of modular is
+ at the same level than the libdrm repo:

-   ../modular/release.sh libdrm 2.4.15 2.4.16
+   ./modular/release.sh libdrm

  This copies the two tarballs to freedesktop.org and creates
  libdrm-2.4.16.announce which has a detailed summary of the
-- 
1.8.3.1

[PATCH 0/3] A few fixes in the libdrm release documentation

[Damien Lespiau]

Made a release with those instructions, they turned out to be slightly
outdated. Fix it for the next first-time person.

-- 
Damien

Damien Lespiau (3):
  RELEASING: Fix releasing instructions to match the latest release.sh
  RELEASING: Fix the step numbering
  RELEASING: Fix annouce typo

 RELEASING | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[ANNOUNCE] libdrm 2.4.60

[Damien Lespiau]

Alan Coopersmith (2):
  Stop undefining _ATOMIC_TYPE in Solaris/NetBSD section of xf86atomic.h
  On Solaris, #include  in xf86drm.c

Chih-Wei Huang (1):
  android: remove duplicate libdrm in LOCAL_SHARED_LIBRARIES

Damien Lespiau (1):
  build: Bump version number to 2.4.60 before release

Daniel Vetter (7):
  intel: Unconditionally clear ioctl structs
  xf86drmMode: Unconditionally clear ioctl structs
  drm: use drmIoctl everywhere
  xf86drm: Unconditionally clear ioctl structs
  tests: remove intel-specific tests
  xf86drm: Fix ioctl struct clearing in drmGetVersion
  Revert "intel: Fix documentation for drm_intel_gem_bo_wait()"

Emil Velikov (21):
  libdrm: fix the Android 64bit build
  autotools: add AM_DISTCHECK_CONFIGURE_FLAGS
  exynos_fimg2d_test: fix implicit funciton declaration errors
  tests: fix implicit funciton declaration errors
  autotools: add WARN_CFLAGS to all targets
  tests: remove unused variables
  exynos_fimg2d_test: remove unused variables
  tests/radeon: set the list* functions as inline
  automake: wrap an insanely long line
  configure: omap, freedreno and tegra require atomics
  configure: update help strings
  automake: drop the NULL variable from the makefile
  tests: move the SUBDIR at the top of the makefile
  tests: automake: keep the libs link at the final stage
  drm: add drmGet(Primary|Render)DeviceNameFromFd functions
  modetest: include into the build when libkms is not selected.
  configure: Stop using AM_MAINTAINER_MODE
  autogen.sh: handle out-of-tree invokation
  configure.ac: fix host_cpu/atomics detection
  configure.ac: fix help string copy/pasta
  configure.ac: error out if building freedreno_kgsl without freedreno

Frank Binns (3):
  Rename DRM_NODE_RENDER to DRM_NODE_PRIMARY
  Add new drmOpenRender function
  Add new drmGetNodeTypeFromFd function

Hyungwon Hwang (2):
  exynos: Don't use DRM_EXYNOS_GEM_{MAP_OFFSET/MMAP} ioctls
  exynos: remove DRM_EXYNOS_GEM_{MAP_OFFSET/MMAP} ioctls

Jammy Zhou (2):
  Add new drmOpenWithType function (v4)
  Add new drmOpenOnceWithType function (v2)

Jan Vesely (7):
  random: Use unsigned long for seed
  Fix gcc -Wextra warnings
  tests: String literals are const char *
  Fix type-limits, pointer-arith and sign-compare warnings
  dristat: Handle DRM_CONSISTENT
  Fix unused, and unused-but-set variables warnings
  Add static qualifier to local functions

Jeff McGee (1):
  intel: Export total subslice and EU counts

Jerome Glisse (1):
  nouveau: fix unlock nouveau_bo_name_ref()

Kristian Høgsberg (1):
  intel: Fix documentation for drm_intel_gem_bo_wait()

Maarten Lankhorst (4):
  Add atomic_inc_return to atomics.
  Use __sync_add_and_fetch instead of __sync_fetch_and_add for 
atomic_dec_and_test
  nouveau: make nouveau importing global buffers completely thread-safe, 
with tests
  nouveau: Do not add most bo's to the global bo list.

Philipp Zabel (1):
  tests: add support for imx-drm

Thomas Klausner (4):
  Fix libdrm's atomic_dec_and_test on Solaris.
  Add NetBSD atomic ops support.
  intel: Only define variable when it's used.
  nouveau: Remove unused static function.

Tobias Jakobi (18):
  exynos: replace G2D_DOUBLE_TO_FIXED macro with function
  tests/exynos: fix typos and change wording
  tests/exynos: disable the G2D userptr/blend test
  tests/exynos: introduce wait_for_user_input
  exynos: introduce g2d_add_base_addr helper function
  tests/exynos: improve error handling
  exynos: fimg2d: remove TRUE/FALSE from header
  exynos: fimg2d: fix comment for G2D_COEFF_MODE_GB_COLOR
  exynos: fimg2d: unify register style
  exynos: fimg2d: introduce G2D_OP_INTERPOLATE
  exynos: fimg2d: whitespace fix in g2d_flush
  tests/exynos: fimg2d: add a checkerboard test
  exynos: add g2d_scale_and_blend
  exynos: honor the repeat mode in g2d_copy_with_scale
  exynos: use structure initialization instead of memset
  exynos: add exynos prefix to fimg2d header
  exynos: add fimg2d header to common includes
  exynos: fimg2d: follow-up fix for G2D_COEFF_MODE_GB_COLOR

git tag: libdrm-2.4.60

http://dri.freedesktop.org/libdrm/libdrm-2.4.60.tar.bz2
MD5:  13e35a7a1cf38b4c9c0fa0f8c9be6b93  libdrm-2.4.60.tar.bz2
SHA1: 4e041a5ff22b2b9132b216eb0574638bf252b7a9  libdrm-2.4.60.tar.bz2
SHA256: 99575fc6c8e31f59193f5320fd4db7a5478e2641b5266147caab9aa875b59889  
libdrm-2.4.60.tar.bz2
PGP:  http://dri.freedesktop.org/libdrm/libdrm-2.4.60.tar.bz2.sig

http://dri.freedesktop.org/libdrm/libdrm-2.4.60.tar.gz
MD5:  3d162e5f4186e90c5582b1c376e4b579  libdrm-2.4.60.tar.gz
SHA1: 35eaa701e76ea0fbc3a256d72712008926b3fc84  libdrm-2.4.60.tar.gz
SHA256: b05e7f22b3b17852b66c9d656d70b2ffd7c8d9e99f63fb09980475843bedbd50  
libdrm-2.4.60.tar.gz
PGP:  http://dri.freed

[Intel-gfx] [PATCH 1/2 v2] intel: Export total subslice and EU counts

[Damien Lespiau]

On Mon, Mar 09, 2015 at 04:13:03PM -0700, jeff.mcgee at intel.com wrote:
> From: Jeff McGee 
> 
> Update kernel interface with new I915_GETPARAM ioctl entries for
> subslice total and EU total. Add a wrapping function for each
> parameter. Userspace drivers need these values when constructing
> GPGPU commands. This kernel query method is intended to replace
> the PCI ID-based tables that userspace drivers currently maintain.
> The kernel driver can employ fuse register reads as needed to
> ensure the most accurate determination of GT config attributes.
> This first became important with Cherryview in which the config
> could differ between devices with the same PCI ID.
> 
> The kernel detection of these values is device-specific. Userspace
> drivers should continue to maintain ID-based tables for older
> devices which return ENODEV when using this query.
> 
> v2: remove unnecessary include of  and increment the
> I915_GETPARAM indices to match updated kernel patch.
> 
> For: VIZ-4636
> Signed-off-by: Jeff McGee 

Pushed to libdrm.

-- 
Damien

[PATCH] intel: Export total subslice and EU counts

[Damien Lespiau]

On Mon, Mar 02, 2015 at 03:39:27PM -0800, jeff.mcgee at intel.com wrote:
> From: Jeff McGee 

2 small details, but otherwise:

Reviewed-by: Damien Lespiau 


> Update kernel interface with new I915_GETPARAM ioctl entries for
> subslice total and EU total. Add a wrapping function for each
> parameter. Userspace drivers need these values when constructing
> GPGPU commands. This kernel query method is intended to replace
> the PCI ID-based tables that userspace drivers currently maintain.
> The kernel driver can employ fuse register reads as needed to
> ensure the most accurate determination of GT config attributes.
> This first became important with Cherryview in which the config
> could differ between devices with the same PCI ID.
> 
> The kernel detection of these values is device-specific. Userspace
> drivers should continue to maintain ID-based tables for older
> devices which return ENODEV when using this query.

This should probably part of some comment near the API entry point.

> 
> For: VIZ-4636
> Signed-off-by: Jeff McGee 
> ---
>  include/drm/i915_drm.h   |  2 ++
>  intel/intel_bufmgr.h |  4 
>  intel/intel_bufmgr_gem.c | 31 +++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 15dd01d..e34f5b2 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -340,6 +340,8 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
>  #define I915_PARAM_HAS_WT 27
>  #define I915_PARAM_CMD_PARSER_VERSION 28
> +#define I915_PARAM_SUBSLICE_TOTAL 32
> +#define I915_PARAM_EU_TOTAL   33
>  
>  typedef struct drm_i915_getparam {
>   int param;
> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> index be83a56..4b2472e 100644
> --- a/intel/intel_bufmgr.h
> +++ b/intel/intel_bufmgr.h
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

But you don't seem to use bool or _Bool in the rest of the patch?

>  struct drm_clip_rect;
>  
> @@ -264,6 +265,9 @@ int drm_intel_get_reset_stats(drm_intel_context *ctx,
> uint32_t *active,
> uint32_t *pending);
>  
> +int drm_intel_get_subslice_total(int fd, unsigned int *subslice_total);
> +int drm_intel_get_eu_total(int fd, unsigned int *eu_total);
> +
>  /** @{ Compatibility defines to keep old code building despite the symbol 
> rename
>   * from dri_* to drm_intel_*
>   */
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 78875fd..2d77f32 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -3292,6 +3292,37 @@ drm_intel_reg_read(drm_intel_bufmgr *bufmgr,
>   return ret;
>  }
>  
> +drm_public int
> +drm_intel_get_subslice_total(int fd, unsigned int *subslice_total)
> +{
> + drm_i915_getparam_t gp;
> + int ret;
> +
> + memclear(gp);
> + gp.value = (int*)subslice_total;
> + gp.param = I915_PARAM_SUBSLICE_TOTAL;
> + ret = drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, );
> + if (ret)
> + return -errno;
> +
> + return 0;
> +}
> +
> +drm_public int
> +drm_intel_get_eu_total(int fd, unsigned int *eu_total)
> +{
> + drm_i915_getparam_t gp;
> + int ret;
> +
> + memclear(gp);
> + gp.value = (int*)eu_total;
> + gp.param = I915_PARAM_EU_TOTAL;
> + ret = drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, );
> + if (ret)
> + return -errno;
> +
> + return 0;
> +}
>  
>  /**
>   * Annotate the given bo for use in aub dumping.
> -- 
> 2.3.0
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 00/12] Introduce drm_intel_device and use i915_pciid.h

[Damien Lespiau]

On Fri, Mar 06, 2015 at 02:10:44PM +, Emil Velikov wrote:
> On 05/03/15 16:20, Damien Lespiau wrote:
> > A couple of things I wanted to do for the longest time:
> >   
> >   - Have (intel's) libdrm use the kernel i915_pciids.h so we can just copy 
> > the
> > file when updating
> >   - Start a new object, struct drm_intel_device where we could put common 
> > code
> > across several userspace projects. For instance it could be where we put
> > the "number of threads" logic we need to use in several 3d/gpgpu
> > states/instructions (that's a bit fiddly starting with CHV: we can't use
> > static tables anymore and need a runtime query to the kernel)
> > 
> Hi Damien,
> 
> So this paves the way to de-duplicate the device information stored in
> beignet, libva-intel-driver, mesa, xf86-video-intel, other ?

I don't expect xf86-video-intel to use this, but otherwise that's the
long term plan (will need a lot more work though).

-- 
Damien

[PATCH 09/12] intel: Provide IS_GENX() macros taking a drm_intel_device as argument

[Damien Lespiau]

On Thu, Mar 05, 2015 at 10:41:19AM -0800, Ian Romanick wrote:
> On 03/05/2015 08:20 AM, Damien Lespiau wrote:
> > Time to switch over all the IS_GENX() macros to the new device object.
> > Nothing more than a mechanical search & replace here.
> 
> Hmm... why not just do the comparisons directly?  The macros seem
> superfluous.

I asked myself the same question as well and then went for the solution
that wasn't going to change what was already there too much. I really
have no opinion either way.

-- 
Damien

[PATCH 12/12] intel: Remove intel_chipset.h

[Damien Lespiau]

Finally, we can remove this file now that everything is using
drm_intel_device.

Signed-off-by: Damien Lespiau 
---
 intel/Makefile.sources   |   1 -
 intel/intel_bufmgr_gem.c |   1 -
 intel/intel_chipset.h| 184 ---
 intel/intel_decode.c |   1 -
 4 files changed, 187 deletions(-)
 delete mode 100644 intel/intel_chipset.h

diff --git a/intel/Makefile.sources b/intel/Makefile.sources
index 2f8398b..b58ca4f 100644
--- a/intel/Makefile.sources
+++ b/intel/Makefile.sources
@@ -8,7 +8,6 @@ LIBDRM_INTEL_FILES := \
intel_device.c \
intel_device.h \
intel_device_priv.h \
-   intel_chipset.h \
mm.c \
mm.h

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 011fa5b..d0119fc 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -61,7 +61,6 @@
 #include "intel_bufmgr.h"
 #include "intel_bufmgr_priv.h"
 #include "intel_device_priv.h"
-#include "intel_chipset.h"
 #include "intel_aub.h"
 #include "string.h"

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
deleted file mode 100644
index 134c877..000
--- a/intel/intel_chipset.h
+++ /dev/null
@@ -1,184 +0,0 @@
-/*
- *
- * Copyright 2003 Tungsten Graphics, Inc., Cedar Park, Texas.
- * All Rights Reserved.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the
- * "Software"), to deal in the Software without restriction, including
- * without limitation the rights to use, copy, modify, merge, publish,
- * distribute, sub license, and/or sell copies of the Software, and to
- * permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
- * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
- * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
- * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
- * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
- * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
- *
- */
-
-#ifndef _INTEL_CHIPSET_H
-#define _INTEL_CHIPSET_H
-
-#define PCI_CHIP_I810  0x7121
-#define PCI_CHIP_I810_DC1000x7123
-#define PCI_CHIP_I810_E0x7125
-#define PCI_CHIP_I815  0x1132
-
-#define PCI_CHIP_I830_M0x3577
-#define PCI_CHIP_845_G 0x2562
-#define PCI_CHIP_I855_GM   0x3582
-#define PCI_CHIP_I865_G0x2572
-
-#define PCI_CHIP_I915_G0x2582
-#define PCI_CHIP_E7221_G   0x258A
-#define PCI_CHIP_I915_GM   0x2592
-#define PCI_CHIP_I945_G0x2772
-#define PCI_CHIP_I945_GM   0x27A2
-#define PCI_CHIP_I945_GME  0x27AE
-
-#define PCI_CHIP_Q35_G 0x29B2
-#define PCI_CHIP_G33_G 0x29C2
-#define PCI_CHIP_Q33_G 0x29D2
-
-#define PCI_CHIP_IGD_GM0xA011
-#define PCI_CHIP_IGD_G 0xA001
-
-#define IS_IGDGM(devid)((devid) == PCI_CHIP_IGD_GM)
-#define IS_IGDG(devid) ((devid) == PCI_CHIP_IGD_G)
-#define IS_IGD(devid)  (IS_IGDG(devid) || IS_IGDGM(devid))
-
-#define PCI_CHIP_I965_G0x29A2
-#define PCI_CHIP_I965_Q0x2992
-#define PCI_CHIP_I965_G_1  0x2982
-#define PCI_CHIP_I946_GZ   0x2972
-#define PCI_CHIP_I965_GM   0x2A02
-#define PCI_CHIP_I965_GME  0x2A12
-
-#define PCI_CHIP_GM45_GM   0x2A42
-
-#define PCI_CHIP_IGD_E_G   0x2E02
-#define PCI_CHIP_Q45_G 0x2E12
-#define PCI_CHIP_G45_G 0x2E22
-#define PCI_CHIP_G41_G 0x2E32
-
-#define PCI_CHIP_ILD_G 0x0042
-#define PCI_CHIP_ILM_G 0x0046
-
-#define PCI_CHIP_SANDYBRIDGE_GT1   0x0102 /* desktop */
-#define PCI_CHIP_SANDYBRIDGE_GT2   0x0112
-#define PCI_CHIP_SANDYBRIDGE_GT2_PLUS  0x0122
-#define PCI_CHIP_SANDYBRIDGE_M_GT1 0x0106 /* mobile */
-#define PCI_CHIP_SANDYBRIDGE_M_GT2 0x0116
-#define PCI_CHIP_SANDYBRIDGE_M_GT2_PLUS0x0126
-#define PCI_CHIP_SANDYBRIDGE_S 0x010A /* server */
-
-#define PCI_CHIP_IVYBRIDGE_GT1 0x0152 /* desktop */
-#define PCI_CHIP_IVYBRIDGE_GT2 0x0162
-#define PCI_CHIP_IVYBRIDGE_M_GT1   0x0156 /* mobile */
-#define PCI_CHIP_IVYBRIDGE_M_GT2   0x0166
-#define PCI_CHIP_IVYBRIDG

[PATCH 11/12] intel: Make test_decode not depend on intel_chipset.h

[Damien Lespiau]

We were pulling a few PCI ids, we can just hardcode them, it doesn't
change much.

Signed-off-by: Damien Lespiau 
---
 intel/test_decode.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/intel/test_decode.c b/intel/test_decode.c
index 1ffd829..b9897b3 100644
--- a/intel/test_decode.c
+++ b/intel/test_decode.c
@@ -36,7 +36,6 @@

 #include "libdrm.h"
 #include "intel_bufmgr.h"
-#include "intel_chipset.h"

 #define HW_OFFSET 0x1230

@@ -147,9 +146,9 @@ infer_devid(const char *batch_filename)
{ "945",  0x2772},
{ "gen4", 0x2a02 },
{ "gm45", 0x2a42 },
-   { "gen5", PCI_CHIP_ILD_G },
-   { "gen6", PCI_CHIP_SANDYBRIDGE_GT2 },
-   { "gen7", PCI_CHIP_IVYBRIDGE_GT2 },
+   { "gen5", 0x0042 },
+   { "gen6", 0x0112 },
+   { "gen7", 0x0162 },
{ "gen8", 0x1616 },
{ NULL, 0 },
};
-- 
1.8.3.1

[PATCH 10/12] intel: Make test_decode fail gracefully the decode context is NULL

[Damien Lespiau]

If, for some reason, we couldn't create the decode context, exit,
instead of segfaulting.

Signed-off-by: Damien Lespiau 
---
 intel/test_decode.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/intel/test_decode.c b/intel/test_decode.c
index 93f47ef..1ffd829 100644
--- a/intel/test_decode.c
+++ b/intel/test_decode.c
@@ -182,6 +182,11 @@ main(int argc, char **argv)
devid = infer_devid(argv[1]);

ctx = drm_intel_decode_context_alloc(devid);
+   if (!ctx) {
+   fprintf(stderr, "Couldn't create decode context for 0x%04x\n",
+   devid);
+   exit(1);
+   }

if (argc == 3) {
if (strcmp(argv[2], "-dump") == 0)
-- 
1.8.3.1

[PATCH 09/12] intel: Provide IS_GENX() macros taking a drm_intel_device as argument

[Damien Lespiau]

Time to switch over all the IS_GENX() macros to the new device object.
Nothing more than a mechanical search & replace here.

Signed-off-by: Damien Lespiau 
---
 intel/intel_bufmgr_gem.c  |   7 +-
 intel/intel_chipset.h | 158 --
 intel/intel_decode.c  |  41 ++--
 intel/intel_device_priv.h |   8 +++
 4 files changed, 31 insertions(+), 183 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 58543a2..011fa5b 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3451,8 +3451,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
bufmgr_gem->pci_device = drm_intel_device_get_devid(bufmgr_gem->dev);
bufmgr_gem->gen = bufmgr_gem->dev->gen;

-   if (IS_GEN3(bufmgr_gem->pci_device) &&
-   bufmgr_gem->gtt_size > 256*1024*1024) {
+   if (IS_GEN3(bufmgr_gem->dev) && bufmgr_gem->gtt_size > 256*1024*1024) {
/* The unmappable part of gtt on gen 3 (i.e. above 256MB) can't
 * be used for tiled blits. To simplify the accounting, just
 * substract the unmappable part (fixed to 256MB on all known
@@ -3494,8 +3493,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
/* Kernel does not supports HAS_LLC query, fallback to GPU
 * generation detection and assume that we have LLC on GEN6/7
 */
-   bufmgr_gem->has_llc = IS_GEN6(bufmgr_gem->pci_device) ||
- IS_GEN7(bufmgr_gem->pci_device);
+   bufmgr_gem->has_llc = IS_GEN6(bufmgr_gem->dev) ||
+ IS_GEN7(bufmgr_gem->dev);
} else
bufmgr_gem->has_llc = *gp.value;

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
index 241d700..134c877 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -181,162 +181,4 @@
 #define PCI_CHIP_SKYLAKE_SRV_GT1   0x190A
 #define PCI_CHIP_SKYLAKE_WKS_GT2   0x191D

-#define IS_ILD(devid)  ((devid) == PCI_CHIP_ILD_G)
-#define IS_ILM(devid)  ((devid) == PCI_CHIP_ILM_G)
-
-#define IS_915(devid)  ((devid) == PCI_CHIP_I915_G || \
-(devid) == PCI_CHIP_E7221_G || \
-(devid) == PCI_CHIP_I915_GM)
-
-#define IS_945GM(devid)((devid) == PCI_CHIP_I945_GM || \
-(devid) == PCI_CHIP_I945_GME)
-
-#define IS_945(devid)  ((devid) == PCI_CHIP_I945_G || \
-(devid) == PCI_CHIP_I945_GM || \
-(devid) == PCI_CHIP_I945_GME || \
-IS_G33(devid))
-
-#define IS_G33(devid)  ((devid) == PCI_CHIP_G33_G || \
-(devid) == PCI_CHIP_Q33_G || \
-(devid) == PCI_CHIP_Q35_G || IS_IGD(devid))
-
-#define IS_GEN2(devid) ((devid) == PCI_CHIP_I830_M || \
-(devid) == PCI_CHIP_845_G || \
-(devid) == PCI_CHIP_I855_GM || \
-(devid) == PCI_CHIP_I865_G)
-
-#define IS_GEN3(devid) (IS_945(devid) || IS_915(devid))
-
-#define IS_GEN5(devid) (IS_ILD(devid) || IS_ILM(devid))
-
-#define IS_GEN6(devid) ((devid) == PCI_CHIP_SANDYBRIDGE_GT1 || \
-(devid) == PCI_CHIP_SANDYBRIDGE_GT2 || \
-(devid) == PCI_CHIP_SANDYBRIDGE_GT2_PLUS || \
-(devid) == PCI_CHIP_SANDYBRIDGE_M_GT1 || \
-(devid) == PCI_CHIP_SANDYBRIDGE_M_GT2 || \
-(devid) == PCI_CHIP_SANDYBRIDGE_M_GT2_PLUS || \
-(devid) == PCI_CHIP_SANDYBRIDGE_S)
-
-#define IS_GEN7(devid) (IS_IVYBRIDGE(devid) || \
-IS_HASWELL(devid) || \
-IS_VALLEYVIEW(devid))
-
-#define IS_IVYBRIDGE(devid)((devid) == PCI_CHIP_IVYBRIDGE_GT1 || \
-(devid) == PCI_CHIP_IVYBRIDGE_GT2 || \
-(devid) == PCI_CHIP_IVYBRIDGE_M_GT1 || \
-(devid) == PCI_CHIP_IVYBRIDGE_M_GT2 || \
-(devid) == PCI_CHIP_IVYBRIDGE_S || \
-(devid) == PCI_CHIP_IVYBRIDGE_S_GT2)
-
-#define IS_VALLEYVIEW(devid)   ((devid) == PCI_CHIP_VALLEYVIEW_PO || \
-(devid) == PCI_CHIP_VALLEYVIEW_1 || \
-(devid) == PCI_CHIP_VALLEYVIEW_2 || \
-(devid) == PCI_CHIP_VALLEYVIEW_3)
-
-#define IS_HSW_GT1(devid)  ((devid) == PCI_CHIP_HASWELL_GT1 || \
-(devid) == PCI_CHIP_HASWELL_M_G

[PATCH 08/12] intel: Remove direct usage of IS_915()

[Damien Lespiau]

One more step towards getting rid of intel_chipset.h. The slightly
tricky bit here is that I don't want to leave defines like IS_CHIP() or
IS_GEN4() is a file that can potentially become a public header.
intel_device_priv.h was introduced then.

Signed-off-by: Damien Lespiau 
---
 intel/Makefile.sources|  1 +
 intel/intel_bufmgr_gem.c  |  6 +++---
 intel/intel_device_priv.h | 35 +++
 3 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 intel/intel_device_priv.h

diff --git a/intel/Makefile.sources b/intel/Makefile.sources
index 0077a17..2f8398b 100644
--- a/intel/Makefile.sources
+++ b/intel/Makefile.sources
@@ -7,6 +7,7 @@ LIBDRM_INTEL_FILES := \
intel_decode.c \
intel_device.c \
intel_device.h \
+   intel_device_priv.h \
intel_chipset.h \
mm.c \
mm.h
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 8570a30..58543a2 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -60,7 +60,7 @@
 #include "libdrm_lists.h"
 #include "intel_bufmgr.h"
 #include "intel_bufmgr_priv.h"
-#include "intel_device.h"
+#include "intel_device_priv.h"
 #include "intel_chipset.h"
 #include "intel_aub.h"
 #include "string.h"
@@ -338,7 +338,7 @@ drm_intel_gem_bo_tile_pitch(drm_intel_bufmgr_gem 
*bufmgr_gem,
return ALIGN(pitch, 64);

if (*tiling_mode == I915_TILING_X
-   || (IS_915(bufmgr_gem->pci_device)
+   || (IS_CHIP(bufmgr_gem->dev, I915)
&& *tiling_mode == I915_TILING_Y))
tile_width = 512;
else
@@ -843,7 +843,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, 
const char *name,
if ((bufmgr_gem->gen == 2) && tiling != I915_TILING_NONE)
height_alignment = 16;
else if (tiling == I915_TILING_X
-   || (IS_915(bufmgr_gem->pci_device)
+   || (IS_CHIP(bufmgr_gem->dev, I915)
&& tiling == I915_TILING_Y))
height_alignment = 8;
else if (tiling == I915_TILING_Y)
diff --git a/intel/intel_device_priv.h b/intel/intel_device_priv.h
new file mode 100644
index 000..87dc1dc
--- /dev/null
+++ b/intel/intel_device_priv.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef __INTEL_DEVICE_PRIV_H__
+#define __INTEL_DEVICE_PRIV_H__
+
+#include "intel_device.h"
+
+/*
+ * Shorthand defines. These are not namespaced and shouldn't be in a public
+ * header, hence a _priv.h one for internal use.
+ */
+#define IS_CHIP(dev, id)   ((dev)->chip == DRM_INTEL_CHIP_ ## id)
+
+#endif /* __INTEL_DEVICE_PRIV_H__ */
-- 
1.8.3.1

[PATCH 07/12] intel: Kill the IS_GEN4() macro

[Damien Lespiau]

Turns out nobody was using it, nor the underlying defines.

Signed-off-by: Damien Lespiau 
---
 intel/intel_chipset.h | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
index a8a2b0e..241d700 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -181,13 +181,6 @@
 #define PCI_CHIP_SKYLAKE_SRV_GT1   0x190A
 #define PCI_CHIP_SKYLAKE_WKS_GT2   0x191D

-#define IS_G45(devid)  ((devid) == PCI_CHIP_IGD_E_G || \
-(devid) == PCI_CHIP_Q45_G || \
-(devid) == PCI_CHIP_G45_G || \
-(devid) == PCI_CHIP_G41_G)
-#define IS_GM45(devid) ((devid) == PCI_CHIP_GM45_GM)
-#define IS_G4X(devid)  (IS_G45(devid) || IS_GM45(devid))
-
 #define IS_ILD(devid)  ((devid) == PCI_CHIP_ILD_G)
 #define IS_ILM(devid)  ((devid) == PCI_CHIP_ILM_G)

@@ -214,14 +207,6 @@

 #define IS_GEN3(devid) (IS_945(devid) || IS_915(devid))

-#define IS_GEN4(devid) ((devid) == PCI_CHIP_I965_G || \
-(devid) == PCI_CHIP_I965_Q || \
-(devid) == PCI_CHIP_I965_G_1 || \
-(devid) == PCI_CHIP_I965_GM || \
-(devid) == PCI_CHIP_I965_GME || \
-(devid) == PCI_CHIP_I946_GZ || \
-IS_G4X(devid))
-
 #define IS_GEN5(devid) (IS_ILD(devid) || IS_ILM(devid))

 #define IS_GEN6(devid) ((devid) == PCI_CHIP_SANDYBRIDGE_GT1 || \
-- 
1.8.3.1

[PATCH 06/12] intel: Kill the IS_9XX() macro

[Damien Lespiau]

IS_9XX() has grown to mean gen >= 3. It was only used in a single test:

  (IS_9XX && !IS_GEN3)

Which has then be replaced with gen >= 4.

The code in that area was idented a bit weirdly, so do a pass on that as
well.

Signed-off-by: Damien Lespiau 
---
 intel/intel_chipset.h |  9 -
 intel/intel_decode.c  | 13 +
 2 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
index 9a8df6a..a8a2b0e 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -354,13 +354,4 @@

 #define IS_GEN9(devid) IS_SKYLAKE(devid)

-#define IS_9XX(dev)(IS_GEN3(dev) || \
-IS_GEN4(dev) || \
-IS_GEN5(dev) || \
-IS_GEN6(dev) || \
-IS_GEN7(dev) || \
-IS_GEN8(dev) || \
-IS_GEN9(dev))
-
-
 #endif /* _INTEL_CHIPSET_H */
diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index 9ada2fa..2fd2cc5 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -3949,15 +3949,12 @@ drm_intel_decode(struct drm_intel_decode *ctx)
index += decode_2d(ctx);
break;
case 0x3:
-   if (IS_9XX(devid) && !IS_GEN3(devid)) {
-   index +=
-   decode_3d_965(ctx);
-   } else if (IS_GEN3(devid)) {
+   if (ctx->dev->gen >= 4)
+   index += decode_3d_965(ctx);
+   else if (IS_GEN3(devid))
index += decode_3d(ctx);
-   } else {
-   index +=
-   decode_3d_i830(ctx);
-   }
+   else
+   index += decode_3d_i830(ctx);
break;
default:
instr_out(ctx, index, "UNKNOWN\n");
-- 
1.8.3.1

[PATCH 05/12] intel: Use '||' for the boolean or

[Damien Lespiau]

While the bitwise operator should do the right thing here, it's probably
better to use the logical or here, at least to not cause a 'wtf' when
reading the code.

At the same time, get rid of unnecessary '()'.

Signed-off-by: Damien Lespiau 
---
 intel/intel_bufmgr_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 72a6ab1..8570a30 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3494,8 +3494,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
/* Kernel does not supports HAS_LLC query, fallback to GPU
 * generation detection and assume that we have LLC on GEN6/7
 */
-   bufmgr_gem->has_llc = (IS_GEN6(bufmgr_gem->pci_device) |
-   IS_GEN7(bufmgr_gem->pci_device));
+   bufmgr_gem->has_llc = IS_GEN6(bufmgr_gem->pci_device) ||
+ IS_GEN7(bufmgr_gem->pci_device);
} else
bufmgr_gem->has_llc = *gp.value;

-- 
1.8.3.1

[PATCH 04/12] intel: Make drm_intel_decode use a drm_intel_device

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 intel/intel_decode.c | 31 ---
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index 7d5cbe5..9ada2fa 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -35,6 +35,7 @@

 #include "libdrm.h"
 #include "xf86drm.h"
+#include "intel_device.h"
 #include "intel_chipset.h"
 #include "intel_bufmgr.h"

@@ -43,6 +44,9 @@ struct drm_intel_decode {
/** stdio file where the output should land.  Defaults to stdout. */
FILE *out;

+   /** Description of the GPU */
+   struct drm_intel_device *dev;
+
/** PCI device ID. */
uint32_t devid;

@@ -3826,27 +3830,15 @@ drm_intel_decode_context_alloc(uint32_t devid)
if (!ctx)
return NULL;

+   ctx->dev = drm_intel_device_new_from_devid(devid);
+   if (!ctx->dev) {
+   free(ctx);
+   return NULL;
+   }
+
ctx->devid = devid;
ctx->out = stdout;
-
-   if (IS_GEN9(devid))
-   ctx->gen = 9;
-   else if (IS_GEN8(devid))
-   ctx->gen = 8;
-   else if (IS_GEN7(devid))
-   ctx->gen = 7;
-   else if (IS_GEN6(devid))
-   ctx->gen = 6;
-   else if (IS_GEN5(devid))
-   ctx->gen = 5;
-   else if (IS_GEN4(devid))
-   ctx->gen = 4;
-   else if (IS_9XX(devid))
-   ctx->gen = 3;
-   else {
-   assert(IS_GEN2(devid));
-   ctx->gen = 2;
-   }
+   ctx->gen = ctx->dev->gen;

return ctx;
 }
@@ -3854,6 +3846,7 @@ drm_intel_decode_context_alloc(uint32_t devid)
 drm_public void
 drm_intel_decode_context_free(struct drm_intel_decode *ctx)
 {
+   drm_intel_device_free(ctx->dev);
free(ctx);
 }

-- 
1.8.3.1

[PATCH 03/12] intel: Use drm_intel_device in the gem buffer manager

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 intel/intel_bufmgr_gem.c | 58 +++-
 1 file changed, 8 insertions(+), 50 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 33d8fbc..72a6ab1 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -60,6 +60,7 @@
 #include "libdrm_lists.h"
 #include "intel_bufmgr.h"
 #include "intel_bufmgr_priv.h"
+#include "intel_device.h"
 #include "intel_chipset.h"
 #include "intel_aub.h"
 #include "string.h"
@@ -120,6 +121,7 @@ typedef struct _drm_intel_bufmgr_gem {

uint64_t gtt_size;
int available_fences;
+   struct drm_intel_device *dev;
int pci_device;
int gen;
unsigned int has_bsd : 1;
@@ -1763,6 +1765,7 @@ drm_intel_bufmgr_gem_destroy(drm_intel_bufmgr *bufmgr)
}
}

+   drm_intel_device_free(bufmgr_gem->dev);
free(bufmgr);
 }

@@ -3071,37 +3074,6 @@ drm_intel_bufmgr_gem_set_vma_cache_size(drm_intel_bufmgr 
*bufmgr, int limit)
drm_intel_gem_bo_purge_vma_cache(bufmgr_gem);
 }

-/**
- * Get the PCI ID for the device.  This can be overridden by setting the
- * INTEL_DEVID_OVERRIDE environment variable to the desired ID.
- */
-static int
-get_pci_device_id(drm_intel_bufmgr_gem *bufmgr_gem)
-{
-   char *devid_override;
-   int devid = 0;
-   int ret;
-   drm_i915_getparam_t gp;
-
-   if (geteuid() == getuid()) {
-   devid_override = getenv("INTEL_DEVID_OVERRIDE");
-   if (devid_override) {
-   bufmgr_gem->no_exec = true;
-   return strtod(devid_override, NULL);
-   }
-   }
-
-   memclear(gp);
-   gp.param = I915_PARAM_CHIPSET_ID;
-   gp.value = 
-   ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, );
-   if (ret) {
-   fprintf(stderr, "get chip id failed: %d [%d]\n", ret, errno);
-   fprintf(stderr, "param: %d, val: %d\n", gp.param, *gp.value);
-   }
-   return devid;
-}
-
 drm_public int
 drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr)
 {
@@ -3469,30 +3441,16 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
(int)bufmgr_gem->gtt_size / 1024);
}

-   bufmgr_gem->pci_device = get_pci_device_id(bufmgr_gem);
-
-   if (IS_GEN2(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 2;
-   else if (IS_GEN3(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 3;
-   else if (IS_GEN4(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 4;
-   else if (IS_GEN5(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 5;
-   else if (IS_GEN6(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 6;
-   else if (IS_GEN7(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 7;
-   else if (IS_GEN8(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 8;
-   else if (IS_GEN9(bufmgr_gem->pci_device))
-   bufmgr_gem->gen = 9;
-   else {
+   bufmgr_gem->dev = drm_intel_device_new(fd);
+   if (bufmgr_gem->dev == NULL) {
free(bufmgr_gem);
bufmgr_gem = NULL;
goto exit;
}

+   bufmgr_gem->pci_device = drm_intel_device_get_devid(bufmgr_gem->dev);
+   bufmgr_gem->gen = bufmgr_gem->dev->gen;
+
if (IS_GEN3(bufmgr_gem->pci_device) &&
bufmgr_gem->gtt_size > 256*1024*1024) {
/* The unmappable part of gtt on gen 3 (i.e. above 256MB) can't
-- 
1.8.3.1

[PATCH 02/12] intel: Introduce an drm_intel_device object

[Damien Lespiau]

The intention here is to:

  - have a single object that represents a device

  - reuse the kernel i915_pciids.h file so we only one place to update
and copy the file over.

  - hide the various information about an intel device in that object
instead of having endless #define in intel_chipset.h. That can be
basic info like which gen are we talking about or, hopefully soon,
detailed information about the device (number of
slices/sub-slices/eus/...)

We'll start slowy by making this API an internal detail at the moment.
Maybe it can grow into something better.

Signed-off-by: Damien Lespiau 
---
 intel/Makefile.sources |   3 +
 intel/i915_pciids.h| 289 +++
 intel/intel_device.c   | 300 +
 intel/intel_device.h   |  99 
 4 files changed, 691 insertions(+)
 create mode 100644 intel/i915_pciids.h
 create mode 100644 intel/intel_device.c
 create mode 100644 intel/intel_device.h

diff --git a/intel/Makefile.sources b/intel/Makefile.sources
index 7b2272c..0077a17 100644
--- a/intel/Makefile.sources
+++ b/intel/Makefile.sources
@@ -1,9 +1,12 @@
 LIBDRM_INTEL_FILES := \
+   i915_pciids.h \
intel_bufmgr.c \
intel_bufmgr_priv.h \
intel_bufmgr_fake.c \
intel_bufmgr_gem.c \
intel_decode.c \
+   intel_device.c \
+   intel_device.h \
intel_chipset.h \
mm.c \
mm.h
diff --git a/intel/i915_pciids.h b/intel/i915_pciids.h
new file mode 100644
index 000..f2e47fd
--- /dev/null
+++ b/intel/i915_pciids.h
@@ -0,0 +1,289 @@
+/*
+ * Copyright 2013 Intel Corporation
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+#ifndef _I915_PCIIDS_H
+#define _I915_PCIIDS_H
+
+/*
+ * A pci_device_id struct {
+ * __u32 vendor, device;
+ *  __u32 subvendor, subdevice;
+ * __u32 class, class_mask;
+ * kernel_ulong_t driver_data;
+ * };
+ * Don't use C99 here because "class" is reserved and we want to
+ * give userspace flexibility.
+ */
+#define INTEL_VGA_DEVICE(id, info) {   \
+   0x8086, id, \
+   ~0, ~0, \
+   0x03, 0xff, \
+   (unsigned long) info }
+
+#define INTEL_QUANTA_VGA_DEVICE(info) {\
+   0x8086, 0x16a,  \
+   0x152d, 0x8990, \
+   0x03, 0xff, \
+   (unsigned long) info }
+
+#define INTEL_I830_IDS(info)   \
+   INTEL_VGA_DEVICE(0x3577, info)
+
+#define INTEL_I845G_IDS(info)  \
+   INTEL_VGA_DEVICE(0x2562, info)
+
+#define INTEL_I85X_IDS(info)   \
+   INTEL_VGA_DEVICE(0x3582, info), /* I855_GM */ \
+   INTEL_VGA_DEVICE(0x358e, info)
+
+#define INTEL_I865G_IDS(info)  \
+   INTEL_VGA_DEVICE(0x2572, info) /* I865_G */
+
+#define INTEL_I915G_IDS(info)  \
+   INTEL_VGA_DEVICE(0x2582, info), /* I915_G */ \
+   INTEL_VGA_DEVICE(0x258a, info)  /* E7221_G */
+
+#define INTEL_I915GM_IDS(info) \
+   INTEL_VGA_DEVICE(0x2592, info) /* I915_GM */
+
+#define INTEL_I945G_IDS(info)  \
+   INTEL_VGA_DEVICE(0x2772, info) /* I945_G */
+
+#define INTEL_I945GM_IDS(info) \
+   INTEL_VGA_DEVICE(0x27a2, info), /* I945_GM */ \
+   INTEL_VGA_DEVICE(0x27ae, info)  /* I945_GME */
+
+#define INTEL_I965G_IDS(info)  \
+   INTEL_VGA_DEVICE(0x2972, info), /* I946_GZ */   \
+   INTEL_VGA_DEVICE(0x2982, info), /* G35_G */ \
+   INTEL_VGA_DEVICE(0x2992, info), /* I965_Q */\
+   INTEL_VGA_DEVICE(0x29a2, info)  /* I965_G */

[PATCH 01/12] intel: Remove unused define IS_MOBILE()

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 intel/intel_chipset.h | 10 --
 1 file changed, 10 deletions(-)

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
index e22a867..9a8df6a 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -181,16 +181,6 @@
 #define PCI_CHIP_SKYLAKE_SRV_GT1   0x190A
 #define PCI_CHIP_SKYLAKE_WKS_GT2   0x191D

-#define IS_MOBILE(devid)   ((devid) == PCI_CHIP_I855_GM || \
-(devid) == PCI_CHIP_I915_GM || \
-(devid) == PCI_CHIP_I945_GM || \
-(devid) == PCI_CHIP_I945_GME || \
-(devid) == PCI_CHIP_I965_GM || \
-(devid) == PCI_CHIP_I965_GME || \
-(devid) == PCI_CHIP_GM45_GM || IS_IGD(devid) 
|| \
-(devid) == PCI_CHIP_IVYBRIDGE_M_GT1 || \
-(devid) == PCI_CHIP_IVYBRIDGE_M_GT2)
-
 #define IS_G45(devid)  ((devid) == PCI_CHIP_IGD_E_G || \
 (devid) == PCI_CHIP_Q45_G || \
 (devid) == PCI_CHIP_G45_G || \
-- 
1.8.3.1

[PATCH libdrm 00/12] Introduce drm_intel_device and use i915_pciid.h

[Damien Lespiau]

A couple of things I wanted to do for the longest time:

  - Have (intel's) libdrm use the kernel i915_pciids.h so we can just copy the
file when updating
  - Start a new object, struct drm_intel_device where we could put common code
across several userspace projects. For instance it could be where we put
the "number of threads" logic we need to use in several 3d/gpgpu
states/instructions (that's a bit fiddly starting with CHV: we can't use
static tables anymore and need a runtime query to the kernel)

I tested it a bit so it can't be totally wrong:

  - I ran with this series on a couple of machines with no noticeable problem
  - I check that the INTEL_DEVID_OVERRIDE env variable was still working (to
dump AUB files)
  - make check, which exercises changes in the decoder path, still passes

-- 
Damien

Damien Lespiau (12):
  intel: Remove unused define IS_MOBILE()
  intel: Introduce an drm_intel_device object
  intel: Use drm_intel_device in the gem buffer manager
  intel: Make drm_intel_decode use a drm_intel_device
  intel: Use '||' for the boolean or
  intel: Kill the IS_9XX() macro
  intel: Kill the IS_GEN4() macro
  intel: Remove direct usage of IS_915()
  intel: Provide IS_GENX() macros taking a drm_intel_device as argument
  intel: Make test_decode fail gracefully the decode context is NULL
  intel: Make test_decode not depend on intel_chipset.h
  intel: Remove intel_chipset.h

 intel/Makefile.sources|   5 +-
 intel/i915_pciids.h   | 289 +++
 intel/intel_bufmgr_gem.c  |  70 ++---
 intel/intel_chipset.h | 376 --
 intel/intel_decode.c  |  82 +-
 intel/intel_device.c  | 300 
 intel/intel_device.h  |  99 
 intel/intel_device_priv.h |  43 ++
 intel/test_decode.c   |  12 +-
 9 files changed, 791 insertions(+), 485 deletions(-)
 create mode 100644 intel/i915_pciids.h
 delete mode 100644 intel/intel_chipset.h
 create mode 100644 intel/intel_device.c
 create mode 100644 intel/intel_device.h
 create mode 100644 intel/intel_device_priv.h

-- 
1.8.3.1

[PATCH] drm: Fix the CRTC_STEREO_DOUBLE_ONLY define to include stero modes

[Damien Lespiau]

The CRTC_STEREO_DOUBLE_ONLY define was introduced in commit:

  commit ecb7e16bf187bc369cf6a5cd108582c01329980d
  Author: Gustavo Padovan 
  Date:   Mon Dec 1 15:40:09 2014 -0800

  drm: add helper to get crtc timings (v5)

but if we want the stereo h/v adjustments, we need to set the
CRTC_STEREO_DOUBLE flag. Otherwise, we'll get the wrong h/v for frame packing
stereo 3d modes.

Cc: Gustavo Padovan 
Cc: Matt Roper 
Cc: Ander Conselvan de Oliveira 
Signed-off-by: Damien Lespiau 
---
 include/drm/drm_modes.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index d92f6dd..0616188 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -92,7 +92,7 @@ enum drm_mode_status {
 #define CRTC_STEREO_DOUBLE (1 << 1) /* adjust timings for stereo modes */
 #define CRTC_NO_DBLSCAN(1 << 2) /* don't adjust doublescan */
 #define CRTC_NO_VSCAN  (1 << 3) /* don't adjust doublescan */
-#define CRTC_STEREO_DOUBLE_ONLY(CRTC_NO_DBLSCAN | CRTC_NO_VSCAN)
+#define CRTC_STEREO_DOUBLE_ONLY(CRTC_STEREO_DOUBLE | CRTC_NO_DBLSCAN | 
CRTC_NO_VSCAN)

 #define DRM_MODE_FLAG_3D_MAX   DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF

-- 
1.8.3.1

[PATCH] drm: Fix drm_crtc_vblank_get() documentation

[Damien Lespiau]

drm_crtc_vblank_get() is the new drm_vblank_get(), not the new
drm_vblank_off().

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 10574a0..c9f5453 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1052,7 +1052,7 @@ EXPORT_SYMBOL(drm_vblank_get);
  * Acquire a reference count on vblank events to avoid having them disabled
  * while in use.
  *
- * This is the native kms version of drm_vblank_off().
+ * This is the native kms version of drm_vblank_get().
  *
  * Returns:
  * Zero on success, nonzero on failure.
-- 
1.8.3.1

[Intel-gfx] [PATCH 3/5] drm/i915: Use fb format modifiers in skylake_update_primary_plane

[Damien Lespiau]

On Mon, Feb 09, 2015 at 07:03:26PM +0100, Daniel Vetter wrote:
> Just a little demo really. We probably need to introduce skl specific
> functions for a lot of the format validation stuff, or at least
> helpers. Specifically I think intel_framebuffer_init and
> intel_fb_align_height must be adjusted to have an i915_ and a skl_
> variant. And only shared code should be converted to fb modifiers,
> platform code (like the plane config readout can keep on using old
> tiling_mode defines to avoid some churn).
> 
> Cc: Tvrtko Ursulin 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 2d69cce03ab5..41b3ddc4068d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2773,11 +2773,11 @@ static void skylake_update_primary_plane(struct 
> drm_crtc *crtc,
>* The stride is either expressed as a multiple of 64 bytes chunks for
>* linear buffers or in number of tiles for tiled buffers.
>*/
> - switch (obj->tiling_mode) {
> - case I915_TILING_NONE:
> + switch (fb->modifier[0]) {
> + case DRM_FORMAT_MOD_NONE:
>   stride = fb->pitches[0] >> 6;
>   break;
> - case I915_TILING_X:
> + case I915_FORMAT_MOD_X_TILED:
>   plane_ctl |= PLANE_CTL_TILED_X;
>   stride = fb->pitches[0] >> 9;
>   break;

Just a remark across the board is that this code won't work as we add
new types of modifiers besides tiling. Might as wels reserve tiling bits
today and select on them.

-- 
Damien

[PATCH libdrm] drm: Avoid out of bound write in drmOpenByName()

[Damien Lespiau]

In the fallback code that looks for devices in /proc, the read() may
return with -1 in case of error (interruption from a signal for
instance). We'll then happily write '\0' to buf[-2].

As we didn't really care about the signal interruption before, I kept it
the same way, just making sure that retcode is > 0 to avoid that case.

This was found by static analysis.

Signed-off-by: Damien Lespiau 
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index d900b4b..106b8ab 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -579,7 +579,7 @@ static int drmOpenByName(const char *name)
if ((fd = open(proc_name, 0, 0)) >= 0) {
retcode = read(fd, buf, sizeof(buf)-1);
close(fd);
-   if (retcode) {
+   if (retcode > 0) {
buf[retcode-1] = '\0';
for (driver = pt = buf; *pt && *pt != ' '; ++pt)
;
-- 
1.8.3.1

[PATCH] Fix SIGSEGV in libdrm for heigth = 0 and width = 0

[Damien Lespiau]

On Fri, Nov 07, 2014 at 07:43:04PM +0100, Thomas Meyer wrote:
> 
> drm_intel_gem_bo_free() crashes because the list bo_gem->vma_list is not
> yet initialised, but the error path tries to free it.
> 
> See also https://bugs.freedesktop.org/show_bug.cgi?id=75844
> 
> Reviewed-by: Chris Wilson 
> Signed-off-by: Thomas Meyer 

Thanks for the patch and review tag. Sorry it took so long to push, it
wasn't clear who was going to do it.

-- 
Damien

> ---
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index f2f4fea..b3e9dba 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -759,15 +759,16 @@ retry:
>   bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
>   bo_gem->stride = 0;
>  
> + /* drm_intel_gem_bo_free calls DRMLISTDEL() for an uninitialized
> +list (vma_list), so better set the list head here */
> + DRMINITLISTHEAD(_gem->name_list);
> + DRMINITLISTHEAD(_gem->vma_list);
>   if (drm_intel_gem_bo_set_tiling_internal(_gem->bo,
>tiling_mode,
>stride)) {
>   drm_intel_gem_bo_free(_gem->bo);
>   return NULL;
>   }
> -
> - DRMINITLISTHEAD(_gem->name_list);
> - DRMINITLISTHEAD(_gem->vma_list);
>   }
>  
>   bo_gem->name = name;
> 
> 
> 

[Intel-gfx] [PATCH 05/17] drm: Add atomic driver interface definitions for objects

[Damien Lespiau]

On Wed, Nov 05, 2014 at 06:04:59PM +0100, Daniel Vetter wrote:
> On Wed, Nov 5, 2014 at 5:26 PM, Thierry Reding  
> wrote:
> >> +struct drm_plane_state {
> >> + struct drm_crtc *crtc;
> >> + struct drm_framebuffer *fb;
> >> +
> >> + /* Signed dest location allows it to be partially off screen */
> >> + int32_t crtc_x, crtc_y;
> >> + uint32_t crtc_w, crtc_h;
> >
> > Should these perhaps be unsized types? For the same reasons you argued
> > the other day.
> >
> >> +
> >> + /* Source values are 16.16 fixed point */
> >> + uint32_t src_x, src_y;
> >> + uint32_t src_h, src_w;
> >
> > Do we really use the 16.16 fixed point format for these? Maybe now would
> > be a good time to get rid of that if we don't need it. If they're not a
> > 16.16 fixed point format they could also be unsized.
> 
> The samantics and types intentionally and precisely match what we
> currently pass in through the set_plane ioctl. And yeah most drivers
> can't do subpixel precise upscaling, but some hardware can do that. So
> I don't see a point in changing the interface here.
> 
> I've implemented all the other stuff you've spotted.

Just a pass by comment, it'd be awesome to have a fixed point 16.16 type
at some point so we don't have to always specify that an uint32_t
variable happens to be 16.16.

-- 
Damien

[PATCH 3/3] drm/gma500: Don't destroy DRM properties in the driver

[Damien Lespiau]

When drm properties are created, they are added to mode_config.property_list
which is then used in drm_mode_config_cleanup() to destroy every single
property created by the driver.

Cc: Patrik Jakobsson 
Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/gma500/psb_intel_sdvo.c | 49 -
 1 file changed, 49 deletions(-)

diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c 
b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 0be96fd..58529ce 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -1631,57 +1631,8 @@ static int psb_intel_sdvo_get_modes(struct drm_connector 
*connector)
return !list_empty(>probed_modes);
 }

-static void
-psb_intel_sdvo_destroy_enhance_property(struct drm_connector *connector)
-{
-   struct psb_intel_sdvo_connector *psb_intel_sdvo_connector = 
to_psb_intel_sdvo_connector(connector);
-   struct drm_device *dev = connector->dev;
-
-   if (psb_intel_sdvo_connector->left)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->left);
-   if (psb_intel_sdvo_connector->right)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->right);
-   if (psb_intel_sdvo_connector->top)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->top);
-   if (psb_intel_sdvo_connector->bottom)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->bottom);
-   if (psb_intel_sdvo_connector->hpos)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->hpos);
-   if (psb_intel_sdvo_connector->vpos)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->vpos);
-   if (psb_intel_sdvo_connector->saturation)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->saturation);
-   if (psb_intel_sdvo_connector->contrast)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->contrast);
-   if (psb_intel_sdvo_connector->hue)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->hue);
-   if (psb_intel_sdvo_connector->sharpness)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->sharpness);
-   if (psb_intel_sdvo_connector->flicker_filter)
-   drm_property_destroy(dev, 
psb_intel_sdvo_connector->flicker_filter);
-   if (psb_intel_sdvo_connector->flicker_filter_2d)
-   drm_property_destroy(dev, 
psb_intel_sdvo_connector->flicker_filter_2d);
-   if (psb_intel_sdvo_connector->flicker_filter_adaptive)
-   drm_property_destroy(dev, 
psb_intel_sdvo_connector->flicker_filter_adaptive);
-   if (psb_intel_sdvo_connector->tv_luma_filter)
-   drm_property_destroy(dev, 
psb_intel_sdvo_connector->tv_luma_filter);
-   if (psb_intel_sdvo_connector->tv_chroma_filter)
-   drm_property_destroy(dev, 
psb_intel_sdvo_connector->tv_chroma_filter);
-   if (psb_intel_sdvo_connector->dot_crawl)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->dot_crawl);
-   if (psb_intel_sdvo_connector->brightness)
-   drm_property_destroy(dev, psb_intel_sdvo_connector->brightness);
-}
-
 static void psb_intel_sdvo_destroy(struct drm_connector *connector)
 {
-   struct psb_intel_sdvo_connector *psb_intel_sdvo_connector = 
to_psb_intel_sdvo_connector(connector);
-
-   if (psb_intel_sdvo_connector->tv_format)
-   drm_property_destroy(connector->dev,
-psb_intel_sdvo_connector->tv_format);
-
-   psb_intel_sdvo_destroy_enhance_property(connector);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
-- 
1.8.3.1

[PATCH 2/3] drm/i915: Don't destroy DRM properties in the driver

[Damien Lespiau]

When drm properties are created, they are added to mode_config.property_list,
which is then used in drm_mode_config_cleanup() to destroy every single
property created by the driver.

Cc: Chandra Konduru 
Cc: Daniel Vetter 
Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/i915/intel_sdvo.c | 47 ---
 1 file changed, 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c 
b/drivers/gpu/drm/i915/intel_sdvo.c
index 9350edd..6d7a277 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1991,57 +1991,10 @@ static int intel_sdvo_get_modes(struct drm_connector 
*connector)
return !list_empty(>probed_modes);
 }

-static void
-intel_sdvo_destroy_enhance_property(struct drm_connector *connector)
-{
-   struct intel_sdvo_connector *intel_sdvo_connector = 
to_intel_sdvo_connector(connector);
-   struct drm_device *dev = connector->dev;
-
-   if (intel_sdvo_connector->left)
-   drm_property_destroy(dev, intel_sdvo_connector->left);
-   if (intel_sdvo_connector->right)
-   drm_property_destroy(dev, intel_sdvo_connector->right);
-   if (intel_sdvo_connector->top)
-   drm_property_destroy(dev, intel_sdvo_connector->top);
-   if (intel_sdvo_connector->bottom)
-   drm_property_destroy(dev, intel_sdvo_connector->bottom);
-   if (intel_sdvo_connector->hpos)
-   drm_property_destroy(dev, intel_sdvo_connector->hpos);
-   if (intel_sdvo_connector->vpos)
-   drm_property_destroy(dev, intel_sdvo_connector->vpos);
-   if (intel_sdvo_connector->saturation)
-   drm_property_destroy(dev, intel_sdvo_connector->saturation);
-   if (intel_sdvo_connector->contrast)
-   drm_property_destroy(dev, intel_sdvo_connector->contrast);
-   if (intel_sdvo_connector->hue)
-   drm_property_destroy(dev, intel_sdvo_connector->hue);
-   if (intel_sdvo_connector->sharpness)
-   drm_property_destroy(dev, intel_sdvo_connector->sharpness);
-   if (intel_sdvo_connector->flicker_filter)
-   drm_property_destroy(dev, intel_sdvo_connector->flicker_filter);
-   if (intel_sdvo_connector->flicker_filter_2d)
-   drm_property_destroy(dev, 
intel_sdvo_connector->flicker_filter_2d);
-   if (intel_sdvo_connector->flicker_filter_adaptive)
-   drm_property_destroy(dev, 
intel_sdvo_connector->flicker_filter_adaptive);
-   if (intel_sdvo_connector->tv_luma_filter)
-   drm_property_destroy(dev, intel_sdvo_connector->tv_luma_filter);
-   if (intel_sdvo_connector->tv_chroma_filter)
-   drm_property_destroy(dev, 
intel_sdvo_connector->tv_chroma_filter);
-   if (intel_sdvo_connector->dot_crawl)
-   drm_property_destroy(dev, intel_sdvo_connector->dot_crawl);
-   if (intel_sdvo_connector->brightness)
-   drm_property_destroy(dev, intel_sdvo_connector->brightness);
-}
-
 static void intel_sdvo_destroy(struct drm_connector *connector)
 {
struct intel_sdvo_connector *intel_sdvo_connector = 
to_intel_sdvo_connector(connector);

-   if (intel_sdvo_connector->tv_format)
-   drm_property_destroy(connector->dev,
-intel_sdvo_connector->tv_format);
-
-   intel_sdvo_destroy_enhance_property(connector);
drm_connector_cleanup(connector);
kfree(intel_sdvo_connector);
 }
-- 
1.8.3.1

[PATCH 1/3] drm: Add a note to drm_property_create() about property lifetime

[Damien Lespiau]

Cc: Chandra Konduru 
Cc: Daniel Vetter 
Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_crtc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4081d7a..0f3c24c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3417,6 +3417,10 @@ void drm_fb_release(struct drm_file *priv)
  * object with drm_object_attach_property. The returned property object must be
  * freed with drm_property_destroy.
  *
+ * Note that the DRM core keeps a per-device list of properties and that, if
+ * drm_mode_config_cleanup() is called, it will destroy all properties created
+ * by the driver.
+ *
  * Returns:
  * A pointer to the newly created property on success, NULL on failure.
  */
-- 
1.8.3.1

[PATCH libdrm 1/3] intel/skl: Add SKL PCI ids

[Damien Lespiau]

On Tue, Sep 30, 2014 at 11:19:37AM +0100, Thomas Wood wrote:
> On 26 September 2014 14:19, Damien Lespiau  
> wrote:
> > v2: Add more PCI IDs (Michael H. Nguyen)
> > v3: Synchronize one more with the kernel PCI IDs (Damien)
> >
> > Signed-off-by: Damien Lespiau 
> > Signed-off-by: Ben Widawsky 
> > Signed-off-by: Michael H. Nguyen 
> 
> Reviewed-by: Thomas Wood 

Thanks for the review, pushed the whole series (the other two patches
where already reviewed).

-- 
Damien

[PATCH libdrm 3/3] intel/skl: add gen9 to the CS decoding init

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
Reviewed-by: Kenneth Graunke 
Signed-off-by: Ben Widawsky 
---
 intel/intel_decode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/intel/intel_decode.c b/intel/intel_decode.c
index a5d6e04..7d5cbe5 100644
--- a/intel/intel_decode.c
+++ b/intel/intel_decode.c
@@ -3829,7 +3829,9 @@ drm_intel_decode_context_alloc(uint32_t devid)
ctx->devid = devid;
ctx->out = stdout;

-   if (IS_GEN8(devid))
+   if (IS_GEN9(devid))
+   ctx->gen = 9;
+   else if (IS_GEN8(devid))
ctx->gen = 8;
else if (IS_GEN7(devid))
ctx->gen = 7;
-- 
1.8.3.1

[PATCH libdrm 2/3] intel/skl: Add gen9 to the buffer manager init

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
Reviewed-by: Kenneth Graunke 
Signed-off-by: Ben Widawsky 
---
 intel/intel_bufmgr_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index ba65527..a6fa224 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3479,6 +3479,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
bufmgr_gem->gen = 7;
else if (IS_GEN8(bufmgr_gem->pci_device))
bufmgr_gem->gen = 8;
+   else if (IS_GEN9(bufmgr_gem->pci_device))
+   bufmgr_gem->gen = 9;
else {
free(bufmgr_gem);
bufmgr_gem = NULL;
-- 
1.8.3.1

[PATCH libdrm 1/3] intel/skl: Add SKL PCI ids

[Damien Lespiau]

v2: Add more PCI IDs (Michael H. Nguyen)
v3: Synchronize one more with the kernel PCI IDs (Damien)

Signed-off-by: Damien Lespiau 
Signed-off-by: Ben Widawsky 
Signed-off-by: Michael H. Nguyen 
---
 intel/intel_chipset.h | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
index 6f9bfad..e22a867 100644
--- a/intel/intel_chipset.h
+++ b/intel/intel_chipset.h
@@ -165,6 +165,22 @@
 #define PCI_CHIP_CHERRYVIEW_2  0x22b2
 #define PCI_CHIP_CHERRYVIEW_3  0x22b3

+#define PCI_CHIP_SKYLAKE_ULT_GT2   0x1916
+#define PCI_CHIP_SKYLAKE_ULT_GT1   0x1906
+#define PCI_CHIP_SKYLAKE_ULT_GT3   0x1926
+#define PCI_CHIP_SKYLAKE_ULT_GT2F  0x1921
+#define PCI_CHIP_SKYLAKE_ULX_GT1   0x190E
+#define PCI_CHIP_SKYLAKE_ULX_GT2   0x191E
+#define PCI_CHIP_SKYLAKE_DT_GT20x1912
+#define PCI_CHIP_SKYLAKE_DT_GT10x1902
+#define PCI_CHIP_SKYLAKE_HALO_GT2  0x191B
+#define PCI_CHIP_SKYLAKE_HALO_GT3  0x192B
+#define PCI_CHIP_SKYLAKE_HALO_GT1  0x190B
+#define PCI_CHIP_SKYLAKE_SRV_GT2   0x191A
+#define PCI_CHIP_SKYLAKE_SRV_GT3   0x192A
+#define PCI_CHIP_SKYLAKE_SRV_GT1   0x190A
+#define PCI_CHIP_SKYLAKE_WKS_GT2   0x191D
+
 #define IS_MOBILE(devid)   ((devid) == PCI_CHIP_I855_GM || \
 (devid) == PCI_CHIP_I915_GM || \
 (devid) == PCI_CHIP_I945_GM || \
@@ -324,12 +340,37 @@
 #define IS_GEN8(devid) (IS_BROADWELL(devid) || \
 IS_CHERRYVIEW(devid))

+#define IS_SKL_GT1(devid)  ((devid) == PCI_CHIP_SKYLAKE_ULT_GT1|| \
+(devid) == PCI_CHIP_SKYLAKE_ULX_GT1|| \
+(devid) == PCI_CHIP_SKYLAKE_DT_GT1 || \
+(devid) == PCI_CHIP_SKYLAKE_HALO_GT1   || \
+(devid) == PCI_CHIP_SKYLAKE_SRV_GT1)
+
+#define IS_SKL_GT2(devid)  ((devid) == PCI_CHIP_SKYLAKE_ULT_GT2|| \
+(devid) == PCI_CHIP_SKYLAKE_ULT_GT2F   || \
+(devid) == PCI_CHIP_SKYLAKE_ULX_GT2|| \
+(devid) == PCI_CHIP_SKYLAKE_DT_GT2 || \
+(devid) == PCI_CHIP_SKYLAKE_HALO_GT2   || \
+(devid) == PCI_CHIP_SKYLAKE_SRV_GT2|| \
+(devid) == PCI_CHIP_SKYLAKE_WKS_GT2)
+
+#define IS_SKL_GT3(devid)  ((devid) == PCI_CHIP_SKYLAKE_ULT_GT3|| \
+(devid) == PCI_CHIP_SKYLAKE_HALO_GT3   || \
+(devid) == PCI_CHIP_SKYLAKE_SRV_GT3)
+
+#define IS_SKYLAKE(devid)  (IS_SKL_GT1(devid) || \
+IS_SKL_GT2(devid) || \
+IS_SKL_GT3(devid))
+
+#define IS_GEN9(devid) IS_SKYLAKE(devid)
+
 #define IS_9XX(dev)(IS_GEN3(dev) || \
 IS_GEN4(dev) || \
 IS_GEN5(dev) || \
 IS_GEN6(dev) || \
 IS_GEN7(dev) || \
-IS_GEN8(dev))
+IS_GEN8(dev) || \
+IS_GEN9(dev))


 #endif /* _INTEL_CHIPSET_H */
-- 
1.8.3.1

[Intel-gfx] [PATCH] intel: Don't leak the test page in an has_userptr() error path

[Damien Lespiau]

On Fri, Sep 19, 2014 at 02:31:56PM +0100, Tvrtko Ursulin wrote:
> 
> Reviewed-by: Tvrtko Ursulin 
> 

Thanks for the review, pushed the patch.

-- 
Damien

[PATCH] intel: Don't leak the test page in an has_userptr() error path

[Damien Lespiau]

When handling the error on GEM_CLOSE, we weren't freeing the allocated
page. Plug that.

Signed-off-by: Damien Lespiau 
---
 intel/intel_bufmgr_gem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index f378e5c..ce35bd4 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3383,9 +3383,8 @@ retry:

close_bo.handle = userptr.handle;
ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, _bo);
-   if (ret == 0) {
-   free(ptr);
-   } else {
+   free(ptr);
+   if (ret) {
fprintf(stderr, "Failed to release test userptr object! (%d) "
"i915 kernel driver may not be sane!\n", errno);
return false;
-- 
1.8.3.1

[Intel-gfx] [PATCH] intel: Add support for userptr objects

[Damien Lespiau]

On Thu, Jun 19, 2014 at 03:52:03PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Allow userptr objects to be created and used via libdrm_intel.
> 
> At the moment tiling and mapping to GTT aperture is not supported
> due hardware limitations across different generations and uncertainty
> about its usefulness.
> 
> v2: Improved error handling in feature detection per review comments.
> 
> Signed-off-by: Tvrtko Ursulin 
> ---

Reviewed-by: Damien Lespiau 

Pushed a slightly modified version of this patch as libdrm now has
explicit symbol visibility.

-- 
Damien

>  intel/intel_bufmgr.c  |  13 
>  intel/intel_bufmgr.h  |   5 ++
>  intel/intel_bufmgr_gem.c  | 163 
> +-
>  intel/intel_bufmgr_priv.h |  12 +++-
>  4 files changed, 191 insertions(+), 2 deletions(-)
> 
> diff --git a/intel/intel_bufmgr.c b/intel/intel_bufmgr.c
> index 905556f..7f3d795 100644
> --- a/intel/intel_bufmgr.c
> +++ b/intel/intel_bufmgr.c
> @@ -60,6 +60,19 @@ drm_intel_bo 
> *drm_intel_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>   return bufmgr->bo_alloc_for_render(bufmgr, name, size, alignment);
>  }
>  
> +drm_intel_bo *drm_intel_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
> +   const char *name, void *addr,
> +   uint32_t tiling_mode,
> +   uint32_t stride,
> +   unsigned long size,
> +   unsigned long flags)
> +{
> + if (bufmgr->bo_alloc_userptr)
> + return bufmgr->bo_alloc_userptr(bufmgr, name, addr, tiling_mode,
> + stride, size, flags);
> + return NULL;
> +}
> +
>  drm_intel_bo *
>  drm_intel_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>  int x, int y, int cpp, uint32_t *tiling_mode,
> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> index 9383c72..be83a56 100644
> --- a/intel/intel_bufmgr.h
> +++ b/intel/intel_bufmgr.h
> @@ -113,6 +113,11 @@ drm_intel_bo 
> *drm_intel_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>   const char *name,
>   unsigned long size,
>   unsigned int alignment);
> +drm_intel_bo *drm_intel_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
> + const char *name,
> + void *addr, uint32_t tiling_mode,
> + uint32_t stride, unsigned long size,
> + unsigned long flags);
>  drm_intel_bo *drm_intel_bo_alloc_tiled(drm_intel_bufmgr *bufmgr,
>  const char *name,
>  int x, int y, int cpp,
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 007a6d8..6dd3986 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -182,6 +182,11 @@ struct _drm_intel_bo_gem {
>   void *mem_virtual;
>   /** GTT virtual address for the buffer, saved across map/unmap cycles */
>   void *gtt_virtual;
> + /**
> +  * Virtual address of the buffer allocated by user, used for userptr
> +  * objects only.
> +  */
> + void *user_virtual;
>   int map_count;
>   drmMMListHead vma_list;
>  
> @@ -221,6 +226,11 @@ struct _drm_intel_bo_gem {
>   bool idle;
>  
>   /**
> +  * Boolean of whether this buffer was allocated with userptr
> +  */
> + bool is_userptr;
> +
> + /**
>* Size in bytes of this buffer and its relocation descendents.
>*
>* Used to avoid costly tree walking in
> @@ -847,6 +857,80 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, 
> const char *name,
>  tiling, stride);
>  }
>  
> +static drm_intel_bo *
> +drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
> + const char *name,
> + void *addr,
> + uint32_t tiling_mode,
> + uint32_t stride,
> + unsigned long size,
> + unsigned long flags)
> +{
> + drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
> + drm_intel_bo_gem *bo_gem;
> + int ret;
> + struct drm_i915_gem_userptr userptr;
> +
> + /* Tiling with userptr surfaces is not supported
> +  * on all hardw

Shareable bufmgr objects v4

[Damien Lespiau]

On Mon, Sep 15, 2014 at 02:12:25PM +0100, Chris Wilson wrote:
> On Fri, Sep 12, 2014 at 01:48:34PM +0100, Lionel Landwerlin wrote:
> > This is getting bigger than expected. Adding the locking that Chris
> > suggested on IRC.
> > 
> > Thanks for taking time to review Chris.
> 
> I'm happy with this series,
> Reviewed-by: Chris Wilson 

Just landed the series for Lionel (who doesn't have access to this git
repo) then.

-- 
Damien

[PATCH] drm: Improve debug output for drm_wait_one_vblank

[Damien Lespiau]

On Mon, Sep 15, 2014 at 02:05:56PM +0200, Daniel Vetter wrote:
> This replicates what we've done in i915 in
> 
> commit 31e4b89acbd7b19c9a8557e6e660a583a0b97daa
> Author: Damien Lespiau 
> Date:   Mon Aug 18 13:51:00 2014 +0100
> 
> drm/i915: Print the pipe on which the vblank wait times out
> 
> to make sure that when we switch i915 to drm_wait_one_vblank that the
> debug output doesn't regress.
> 
> Cc: Damien Lespiau 
> Cc: Thomas Wood 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Damien Lespiau 

-- 
Damien

> ---
>  drivers/gpu/drm/drm_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index e73cbdaa18df..5ef03c216a27 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1077,7 +1077,7 @@ void drm_wait_one_vblank(struct drm_device *dev, int 
> crtc)
>   u32 last;
>  
>   ret = drm_vblank_get(dev, crtc);
> - if (WARN_ON(ret))
> + if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", crtc, ret))
>   return;
>  
>   last = drm_vblank_count(dev, crtc);
> @@ -1086,7 +1086,7 @@ void drm_wait_one_vblank(struct drm_device *dev, int 
> crtc)
>last != drm_vblank_count(dev, crtc),
>msecs_to_jiffies(100));
>  
> - WARN_ON(ret == 0);
> + WARN(ret == 0, "vblank wait timed out on crtc %i\n", crtc);
>  
>   drm_vblank_put(dev, crtc);
>  }
> -- 
> 2.0.1
> 

[PATCH 1/7] drm: Renaming DP training vswing pre emph defines

[Damien Lespiau]

On Wed, Aug 27, 2014 at 03:11:08PM +0200, Thierry Reding wrote:
> > So we're left with
> > 
> >   #define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
> > 
> > Vs
> > 
> >   #define DP_TRAIN_VOLTAGE_SWING_LEVEL(x) ((x) << 0)
> > 
> > The second variant doesn't really bring much more clarity? Can we just
> > go with the first?
> 
> I think the parameterized version is more convenient, especially if you
> want to use that during training sequences and iterate over the levels.

That's a fair point, but today's code manages to do without that nicety.

I think these kind of refinements could go in series with code actually
using them on top.

> But I don't feel too strongly about it, so either way is fine with me.

Thanks, taking some of your time to provide feedback is always
appreciated!

-- 
Damien

drm/i915/bdw: Provide the BDW specific HDMI buffer translation table

[Damien Lespiau]

On Wed, Aug 27, 2014 at 04:04:46PM +0300, Dan Carpenter wrote:
> Hello Damien Lespiau,
> 
> The patch a26aa8baee6c: "drm/i915/bdw: Provide the BDW specific HDMI
> buffer translation table" from Aug 1, 2014, leads to the following
> static checker warning:
> 
>   drivers/gpu/drm/i915/intel_ddi.c:225 intel_prepare_ddi_buffers()
>   error: buffer overflow 'ddi_translations_hdmi' 24 <= 31
> 
> drivers/gpu/drm/i915/intel_ddi.c
>155  static void intel_prepare_ddi_buffers(struct drm_device *dev, enum 
> port port)
>156  {
>157  struct drm_i915_private *dev_priv = dev->dev_private;
>158  u32 reg;
>159  int i, n_hdmi_entries, hdmi_800mV_0dB;
>160  int hdmi_level = 
> dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> ^^
> The static checker thinks this is a number between 0-15.  I didn't check
> if that was correct.
> 
>161  const u32 *ddi_translations_fdi;
> 
> [ snip ]
> 
>186  ddi_translations_hdmi = bdw_ddi_translations_hdmi;
>187  n_hdmi_entries = 
> ARRAY_SIZE(bdw_ddi_translations_hdmi);
> ^^
> limit is the size of the array.
> 
>188  hdmi_800mV_0dB = 7;
>189  }
>190  
>191  switch (port) {
>192  case PORT_A:
>193  ddi_translations = ddi_translations_edp;
>194  break;
>195  case PORT_B:
>196  case PORT_C:
>197  ddi_translations = ddi_translations_dp;
>198  break;
>199  case PORT_D:
>200  if (intel_dp_is_edp(dev, PORT_D))
>201  ddi_translations = ddi_translations_edp;
>202  else
>203  ddi_translations = ddi_translations_dp;
>204  break;
>205  case PORT_E:
>206  ddi_translations = ddi_translations_fdi;
>207  break;
>208  default:
>209  BUG();
>210  }
>211  
>212  for (i = 0, reg = DDI_BUF_TRANS(port);
>213   i < ARRAY_SIZE(hsw_ddi_translations_fdi); i++) {
>214  I915_WRITE(reg, ddi_translations[i]);
>215  reg += 4;
>216  }
>217  
>218  /* Choose a good default if VBT is badly populated */
>219  if (hdmi_level == HDMI_LEVEL_SHIFT_UNKNOWN ||
>220  hdmi_level >= n_hdmi_entries)
>   ^^
> We cap it at the size of the array which is either 20 or 24.  The static
> checker thinks it's already capped at 15 so this is not a problem.
> 
> Should this be n_hdmi_entries / 2 or something?  Maybe a - 1 in there?

Indeed, n_hdmi_entries needed to be:

  n_hdmi_entries = ARRAY_SIZE(bdw_ddi_translations_hdmi) / 2;

This was fixed by:

  commit d6699dd3a7f696a80a5f8e5bb6ecf6ff6dd7c998
  Author: Damien Lespiau 
  Date:   Sat Aug 9 16:29:31 2014 +0100

  drm/i915: Fix wrong number of HDMI translation entries

-- 
Damien

[PATCH 1/7] drm: Renaming DP training vswing pre emph defines

[Damien Lespiau]

On Wed, Aug 27, 2014 at 08:47:54AM +0100, Damien Lespiau wrote:
> > An alternative would be to provide a second set of defines for eDP 1.4
> > where the name implies the meaning and then use them as appropriate.
> 
> We went through the idea as well and:
> 
> I actually think the nominal voltage swing and pre-emph values are quite
> misleading. The hw is free to implement a wildly different set of voltage
> swing/pre-emph values.
> 
> eDP 1.4 changes those nominal values as described in the cover letter,
> but there again, the actual hw implementation can choose fairly
> different values than the nominal ones.
> 
> Also, the DP 1.2 spec documents this field as (see address 103h):
> 
> TRAINING_LANE0_SET : Link Training Control_Lane0
>   Bits 1:0 = VOLTAGE SWING SET
> 00 ?Voltage swing level 0
> 01 ?Voltage swing level 1
> 10 ?Voltage swing level 2
> 11 ?Voltage swing level 3
> 
> So, in that sense, we're closer to the latest spec with those LEVEL_X
> defines.

I forgot to mention here that if we have separate defines for eDP 1.4,
then we lose the possibility to share training code with big DP and eDP
1.3, not something desirable.

-- 
Damien

[PATCH 1/7] drm: Renaming DP training vswing pre emph defines

[Damien Lespiau]

On Tue, Aug 26, 2014 at 01:28:19PM +0200, Thierry Reding wrote:
> On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal at intel.com wrote:
> > From: Sonika Jindal 
> > 
> > Adding new defines, older one will be removed in the last patch in the 
> > series.
> > This is to rename the defines to have levels instead of values for vswing 
> > and
> > pre-emph levels as the values may differ in other scenarios like low vswing 
> > of
> > eDP1.4 where the values are different.
> > 
> > Done using following cocci patch for each define:
> > @@
> > @@
> > 
> >  # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0)
> > + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
> 
> Could this perhaps be simply:
> 
>   #define DP_TRAIN_VOLTAGE_SWING(x) ((x) << 0)
> 
> As it is, there's no information about the value within the symbolic
> name anyway, so _LEVEL_* really isn't that useful and keeping several
> macros for each value seems isn't either.

The _LEVEL_ part is quite important IMHO, that's what changes between those
different defines, controlling a level shifter, somewhere.

So we're left with

  #define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)

Vs

  #define DP_TRAIN_VOLTAGE_SWING_LEVEL(x) ((x) << 0)

The second variant doesn't really bring much more clarity? Can we just
go with the first?

> An alternative would be to provide a second set of defines for eDP 1.4
> where the name implies the meaning and then use them as appropriate.

We went through the idea as well and:

I actually think the nominal voltage swing and pre-emph values are quite
misleading. The hw is free to implement a wildly different set of voltage
swing/pre-emph values.

eDP 1.4 changes those nominal values as described in the cover letter,
but there again, the actual hw implementation can choose fairly
different values than the nominal ones.

Also, the DP 1.2 spec documents this field as (see address 103h):

TRAINING_LANE0_SET : Link Training Control_Lane0
  Bits 1:0 = VOLTAGE SWING SET
00 ?Voltage swing level 0
01 ?Voltage swing level 1
10 ?Voltage swing level 2
11 ?Voltage swing level 3

So, in that sense, we're closer to the latest spec with those LEVEL_X
defines.

HTH,

-- 
Damien

drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging

[Damien Lespiau]

On Tue, Aug 26, 2014 at 12:46:32PM +0200, Martin Bugge (marbugge) wrote:
> Hello Damien
> 
> I'm writing to you as you seems to be one of latest maintainers of:
> 
> include/linux/hdmi.h
> drivers/video/hdmi.c
> 
> I wanted to add "Source Product Description information frame"
> logging for the drivers in
> 
> ./drivers/media/i2c/adv7604.c
> ./drivers/media/i2c/adv7842.c
> 
> But was advised to contact you and use some of the defines in
> include/linux/hdmi.h
> 
> As you can see in:
> 
> http://www.spinics.net/lists/linux-media/msg74727.html
> 
> Would you consider the addition of a hdmi_spd_infoframe_log function
> in hdmi.c a good idea ?
> 
> And the same goes for other HDMI Information frames.

I don't see why not. Currently the code there is somewhat geared for
writing HDMI info frames, not really reading back/debug.

The various structures are not a 1:1 mapping with how the fields are
actually represented in an info frame packet, so you get a _pack()
function to serialize the info frame structures. Something to
consider, then, for your logging API would be to either:

  - Give a buffer/size to your log() function and decode a raw buffer
  - make an _unpack() function that takes a raw buffer and translate it
into one of the various infoframe structure and then have a _log
function that operates on the "unpacked" structure.

Another thing to keep in mind is that those "unpacked" versions of the
infoframes don't actually have all the fields. eg.

   /*
 * Data byte 1, bit 4 has to be set if we provide the active format
 * aspect ratio
 */
if (frame->active_aspect & 0xf)
ptr[0] |= BIT(4);

[...]

ptr[1] = ((frame->colorimetry & 0x3) << 6) |
 ((frame->picture_aspect & 0x3) << 4) |
 (frame->active_aspect & 0xf);

You can see that active_aspect needs two fields set, a "active aspect valid"
bit (data byte 1, bit 4) and the actual active_aspect value. To prevent the
user of the API to generate invalid infoframes (ie have a active_aspect value
but forgetting to set the valid bit), I decided to "hide" the valid bit and set
it automatically if active_aspect is set. This isn't just theoritical, we've
had the bug that forgot the valid bit before.

That's the details I can think about atm, I don't seen any reason to not
improve that code to be able to dump info frames. I have a slight preference to
have an _unpack() version and have the _log() functions operate on the
"expoded" structures, maybe Thierry/Paulo (in Cc.) have some input as well.

HTH,

-- 
Damien

[Intel-gfx] [PATCH 2/7] drm/i915: Renaming DP training vswing pre emph defines

[Damien Lespiau]

On Fri, Aug 08, 2014 at 04:23:41PM +0530, sonika.jindal at intel.com wrote:
> From: Sonika Jindal 
> 
> Rename the defines to have levels instead of values for vswing and
> pre-emph levels as the values may differ in other scenarios like low vswing of
> eDP1.4 where the values are different.
> 
> Done using following cocci patch for each define:
> @@
> @@
> 
>  # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0)
> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
> 
> ...
> 
> Signed-off-by: Sonika Jindal 

Reviewed-by: Damien Lespiau 

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_bios.c |   16 +--
>  drivers/gpu/drm/i915/intel_dp.c   |  194 
> ++---
>  2 files changed, 105 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c 
> b/drivers/gpu/drm/i915/intel_bios.c
> index 031c565..e871f68 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -627,16 +627,16 @@ parse_edp(struct drm_i915_private *dev_priv, struct 
> bdb_header *bdb)
>  
>   switch (edp_link_params->preemphasis) {
>   case EDP_PREEMPHASIS_NONE:
> - dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPHASIS_0;
> + dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPH_LEVEL_0;
>   break;
>   case EDP_PREEMPHASIS_3_5dB:
> - dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPHASIS_3_5;
> + dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPH_LEVEL_1;
>   break;
>   case EDP_PREEMPHASIS_6dB:
> - dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPHASIS_6;
> + dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPH_LEVEL_2;
>   break;
>   case EDP_PREEMPHASIS_9_5dB:
> - dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPHASIS_9_5;
> + dev_priv->vbt.edp_preemphasis = DP_TRAIN_PRE_EMPH_LEVEL_3;
>   break;
>   default:
>   DRM_DEBUG_KMS("VBT has unknown eDP pre-emphasis value %u\n",
> @@ -646,16 +646,16 @@ parse_edp(struct drm_i915_private *dev_priv, struct 
> bdb_header *bdb)
>  
>   switch (edp_link_params->vswing) {
>   case EDP_VSWING_0_4V:
> - dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_400;
> + dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
>   break;
>   case EDP_VSWING_0_6V:
> - dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_600;
> + dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
>   break;
>   case EDP_VSWING_0_8V:
> - dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_800;
> + dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>   break;
>   case EDP_VSWING_1_2V:
> - dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_1200;
> + dev_priv->vbt.edp_vswing = DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>   break;
>   default:
>   DRM_DEBUG_KMS("VBT has unknown eDP voltage swing value %u\n",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 34e3c47..01f264c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2381,13 +2381,13 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
>   enum port port = dp_to_dig_port(intel_dp)->port;
>  
>   if (IS_VALLEYVIEW(dev))
> - return DP_TRAIN_VOLTAGE_SWING_1200;
> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>   else if (IS_GEN7(dev) && port == PORT_A)
> - return DP_TRAIN_VOLTAGE_SWING_800;
> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>   else if (HAS_PCH_CPT(dev) && port != PORT_A)
> - return DP_TRAIN_VOLTAGE_SWING_1200;
> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
>   else
> - return DP_TRAIN_VOLTAGE_SWING_800;
> + return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
>  }
>  
>  static uint8_t
> @@ -2398,49 +2398,49 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, 
> uint8_t voltage_swing)
>  
>   if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>   switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> - case DP_TRAIN_VOLTAGE_SWING_400:
> - return DP_TRAIN_PRE_EMPHASIS_9_5;
> - case DP_TRAIN_VOLTAGE_SWING_600:
> - return DP_TRAIN_PRE_EMPHASIS_6;
> - case DP_TRAIN_VOLTAGE_SWING_800:
> - return DP_TRAIN_PRE_EMPHASIS_3_5;
> -   

[Intel-gfx] [PATCH 1/7] drm: Renaming DP training vswing pre emph defines

[Damien Lespiau]

On Fri, Aug 08, 2014 at 04:23:40PM +0530, sonika.jindal at intel.com wrote:
> From: Sonika Jindal 
> 
> Adding new defines, older one will be removed in the last patch in the series.
> This is to rename the defines to have levels instead of values for vswing and
> pre-emph levels as the values may differ in other scenarios like low vswing of
> eDP1.4 where the values are different.
> 
> Done using following cocci patch for each define:
> @@
> @@
> 
>  # define DP_TRAIN_VOLTAGE_SWING_400 (0 << 0)
> + # define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
> 
> ...
> Cc: dri-devel at lists.freedesktop.org
> 
> Signed-off-by: Sonika Jindal 


Reviewed-by: Damien Lespiau 

-- 
Damien

> ---
>  include/drm/drm_dp_helper.h |8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index a21568b..3840a05 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -191,15 +191,23 @@
>  # define DP_TRAIN_VOLTAGE_SWING_SHIFT0
>  # define DP_TRAIN_MAX_SWING_REACHED  (1 << 2)
>  # define DP_TRAIN_VOLTAGE_SWING_400  (0 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
>  # define DP_TRAIN_VOLTAGE_SWING_600  (1 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_1 (1 << 0)
>  # define DP_TRAIN_VOLTAGE_SWING_800  (2 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_2 (2 << 0)
>  # define DP_TRAIN_VOLTAGE_SWING_1200 (3 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_3 (3 << 0)
>  
>  # define DP_TRAIN_PRE_EMPHASIS_MASK  (3 << 3)
>  # define DP_TRAIN_PRE_EMPHASIS_0 (0 << 3)
> +# define DP_TRAIN_PRE_EMPH_LEVEL_0   (0 << 3)
>  # define DP_TRAIN_PRE_EMPHASIS_3_5   (1 << 3)
> +# define DP_TRAIN_PRE_EMPH_LEVEL_1   (1 << 3)
>  # define DP_TRAIN_PRE_EMPHASIS_6 (2 << 3)
> +# define DP_TRAIN_PRE_EMPH_LEVEL_2   (2 << 3)
>  # define DP_TRAIN_PRE_EMPHASIS_9_5   (3 << 3)
> +# define DP_TRAIN_PRE_EMPH_LEVEL_3   (3 << 3)
>  
>  # define DP_TRAIN_PRE_EMPHASIS_SHIFT 3
>  # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED  (1 << 5)
> -- 
> 1.7.10.4
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

How to design a DRM KMS driver exposing 2D compositing?

[Damien Lespiau]

On Mon, Aug 11, 2014 at 03:07:33PM +0300, Pekka Paalanen wrote:
> > > there is some hardware than can do 2D compositing with an arbitrary
> > > number of planes. I'm not sure what the absolute maximum number of
> > > planes is, but for the discussion, let's say it is 100.
> > > 
> > > There are many complicated, dynamic constraints on how many, what size,
> > > etc. planes can be used at once. A driver would be able to check those
> > > before kicking the 2D compositing engine.
> > > 
> > > The 2D compositing engine in the best case (only few planes used) is
> > > able to composite on the fly in scanout, just like the usual overlay
> > > hardware blocks in CRTCs. When the composition complexity goes up, the
> > > driver can fall back to compositing into a buffer rather than on the
> > > fly in scanout. This fallback needs to be completely transparent to the
> > > user space, implying only additional latency if anything.
> > 
> > This looks like a fallback that would use GL to compose the intermediate
> > buffer. Any reason why that fallback can't be kicked from userspace?
> 
> It is not GL, and GL might not be available or desireable. It is still
> the same 2D compositing engine in hardware, but now running with
> off-screen target buffer, because it cannot anymore keep up with the
> continous pixel rate that the direct scanout would need.

I didn't mean this was GL, but just making the parallel, ie. we wouldn't
put a GL fallback into the kernel.

> If we were to use the 2D compositing engine from user space, we would
> be on the road to OpenWFC. IOW, there is no standard API for the
> user space to use yet, as far as I'm aware. ;-)
> 
> I'm just trying to avoid having to design a kernel driver ABI for a
> user space driver, then design/implement some standard user space
> API on top, and then go fix all compositors to actually use it instead
> of / with KMS.

It's no easy trade-off. For instance, if the compositor doesn't know
about some of the hw constraints you are talking about, it may ask the
kernel for a configuration that suddently will only allow 20 fps updates
(because of the bw limitation you're mentioning). And the compositor
just wouldn't know.

I can only speak for the hw I know, if you want to squeeze everything
you can from that simple (compared to the one you're talking about)
display hw, there's no choice, the compositor needs to know about the
constraints to make clever decisions (that's what we do on Android). But
then the appeal of a common interface is understandable.

(An answer that doesn't actually say anything interesting, oh well),

-- 
Damien

How to design a DRM KMS driver exposing 2D compositing?

[Damien Lespiau]

On Mon, Aug 11, 2014 at 01:38:55PM +0300, Pekka Paalanen wrote:
> Hi,

Hi,

> there is some hardware than can do 2D compositing with an arbitrary
> number of planes. I'm not sure what the absolute maximum number of
> planes is, but for the discussion, let's say it is 100.
> 
> There are many complicated, dynamic constraints on how many, what size,
> etc. planes can be used at once. A driver would be able to check those
> before kicking the 2D compositing engine.
> 
> The 2D compositing engine in the best case (only few planes used) is
> able to composite on the fly in scanout, just like the usual overlay
> hardware blocks in CRTCs. When the composition complexity goes up, the
> driver can fall back to compositing into a buffer rather than on the
> fly in scanout. This fallback needs to be completely transparent to the
> user space, implying only additional latency if anything.

This looks like a fallback that would use GL to compose the intermediate
buffer. Any reason why that fallback can't be kicked from userspace?

-- 
Damien

[PATCH] drm: Use the type of the array element when reallocating

[Damien Lespiau]

Static analysers find it 'suspicious', that we're trying to allocate memory for
elements of size sizeof(struct drm_fb_helper_connector) when the array is
defined as struct drm_fb_helper_connector **.

Use sizeof(struct drm_fb_helper_connector *) instead.

Note that the structure being defined as:

struct drm_fb_helper_connector {
struct drm_connector *connector;
};

This was still doing the right thing, but may not in the future if
additional fields are added.

Cc: Todd Previte 
Cc: Dave Airlie 
Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_fb_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 44da703..63d7b8e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -126,7 +126,7 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper 
*fb_helper, struct drm_

WARN_ON(!mutex_is_locked(_helper->dev->mode_config.mutex));
if (fb_helper->connector_count + 1 > 
fb_helper->connector_info_alloc_count) {
-   temp = krealloc(fb_helper->connector_info, sizeof(struct 
drm_fb_helper_connector) * (fb_helper->connector_count + 1), GFP_KERNEL);
+   temp = krealloc(fb_helper->connector_info, sizeof(struct 
drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
if (!temp)
return -ENOMEM;

-- 
1.8.3.1

[PATCH] drm: Don't return 0 for a value used as a denominator

[Damien Lespiau]

Static analysis will be unhappy if a function can theoretically return
0 and we're trying to divide by that value.

Mark that case that cannot occur as a BUG() instead.

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 352f5d6..b3adf14 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1772,7 +1772,7 @@ static int drm_dp_get_vc_payload_bw(int dp_link_bw, int 
dp_link_count)
case DP_LINK_BW_5_4:
return 10 * dp_link_count;
}
-   return 0;
+   BUG();
 }

 /**
-- 
1.8.3.1

[Intel-gfx] [PATCH 0/6] Rename DP training vswing/pre-emph defines

[Damien Lespiau]

On Tue, Aug 05, 2014 at 04:38:16PM +0530, sonika.jindal at intel.com wrote:
> From: Sonika Jindal 
> 
> Rename the defines to have levels instead of values for vswing and pre-emph
> levels as the values may differ in other scenarios like low vswing of eDP 1.4
> where the values are different.
> Updated in all the drivers as well

Hi Sonika,

Oops, another mess in this series :) If there's a need to rework the
series and add/remove patches, the best way to send a v2 is to actually
resend the whole series, otherwise someone will get it wrong and won't
apply the right patches.

So, could you resend the whole series again with the patches generated
by Daniel's review comment and with the explanation copy/pasted in all
the driver patches (as Jingoo Han asked). It's fair enough to track why
the rename was needed in driver-specific patches instead of relying on
the cover letter.

Thanks,

-- 
Damien

[PATCH] drm/mst: Don't use uninitialized fields of the sideband message header

[Damien Lespiau]

We could be using uninitialized fields of the header in
drm_dp_encode_sideband_msg_hdr(), for instance hdr->somt is set to 1 in
the first patcket but never set to 0 otherwise.

Always clear the header at the start then.

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 813b8d1..618526d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1290,6 +1290,8 @@ static int process_single_tx_qlock(struct 
drm_dp_mst_topology_mgr *mgr,
int len, space, idx, tosend;
int ret;

+   memset(, 0, sizeof(struct drm_dp_sideband_msg_hdr));
+
if (txmsg->state == DRM_DP_SIDEBAND_TX_QUEUED) {
txmsg->seqno = -1;
txmsg->state = DRM_DP_SIDEBAND_TX_START_SEND;
-- 
1.8.3.1

[PATCH] drm/mst: Avoid reading uninitialized value

[Damien Lespiau]

A static analysis tool tells me that we could try to read an
uninitialized 'ret' value. Plug that.

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 369d6c4..813b8d1 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1644,7 +1644,7 @@ int drm_dp_update_payload_part2(struct 
drm_dp_mst_topology_mgr *mgr)
 {
struct drm_dp_mst_port *port;
int i;
-   int ret;
+   int ret = 0;
mutex_lock(>payload_lock);
for (i = 0; i < mgr->max_payloads; i++) {

-- 
1.8.3.1

[Intel-gfx] [v2 00/11] Support for 180 degree HW rotation

[Damien Lespiau]

On Sun, Jul 06, 2014 at 01:41:12PM +0530, Jindal, Sonika wrote:
> >Missing from this series, your two documentation patches (we need to
> >bundle things up as a entity that makes sense for one of the maintainers
> >to pick it up (either Dave or Daniel)).
> >
> I was not aware that documentation patches should also be part of this.
> Because I was asked to send the igt testcase separately I thought
> documentation will also go separately. Documentation patches have
> also got reviewed-by tags. What do you suggest? Should I send the
> patchset again with documentation? Will Igt still be a separate
> post?

Please, resend the series entirely once again, with the latest review
tags added, the mutex locking around update_fbc() and the documentation
patches.

The reason with the documentation patches are part of this series is
that adding documentation is a requirement for adding new properties so
the whole DRM subsystem has a chance to converge towards a common set of
properties.

That series should also be annotated with a v3 in the cover letter.

Cheers,

-- 
Damien

[v2 11/11] drm: Resetting rotation property

[Damien Lespiau]

On Fri, Jul 04, 2014 at 03:14:03PM +0530, sonika.jindal at intel.com wrote:
> From: Sonika Jindal 
> 
> Reset rotation property to 0 wherever applicable
> 
> v2: Also calling set_property of the plane to set the rotation in the plane
> structure.
> 
> Cc: damien.lespiau at intel.com
> Signed-off-by: Sonika Jindal 

Reviewed-by: Damien Lespiau 

> ---
>  drivers/gpu/drm/drm_fb_helper.c |   16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index d5d8cea..30806b4 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -282,9 +282,23 @@ static bool restore_fbdev_mode(struct drm_fb_helper 
> *fb_helper)
>  
>   drm_warn_on_modeset_not_all_locked(dev);
>  
> - list_for_each_entry(plane, >mode_config.plane_list, head)
> + list_for_each_entry(plane, >mode_config.plane_list, head) {
> +
> + if (plane->rotation_property) {
> + int ret = 0;
> + if (plane->funcs->set_property)
> + ret = plane->funcs->set_property(plane,
> + plane->rotation_property,
> + BIT(DRM_ROTATE_0));
> + if (!ret)
> + drm_object_property_set_value(>base,
> + plane->rotation_property,
> + BIT(DRM_ROTATE_0));
> + }
> +
>   if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>   drm_plane_force_disable(plane);
> + }
>  
>   for (i = 0; i < fb_helper->crtc_count; i++) {
>   struct drm_mode_set *mode_set = 
> _helper->crtc_info[i].mode_set;
> -- 
> 1.7.10.4
> 

[Intel-gfx] [v2 00/11] Support for 180 degree HW rotation

[Damien Lespiau]

On Fri, Jul 04, 2014 at 03:13:52PM +0530, sonika.jindal at intel.com wrote:
> From: Sonika Jindal 
> 
> Enables 180 degree rotation for sprite and primary planes.
> Updated the primary plane rotation support as per the new universal plane
> design.
> 
> Most of these patches were already reviewed in intel-gfx in February 2014 
> thats
> why there is version history in few of them.
> 
> v2: Moved rotation_property to drm_plane. Added updation of FBC when rotation 
> is
> again set to 0.
> 
> Testcase: kms_rotation_crc
> This igt can be extended for clipped rotation cases. Right it only tests 180
> degree rotation for sprite and primary plane with crc check.
> 
> 
> Sonika Jindal (2):
>   drm/i915: Add 180 degree primary plane rotation support
>   drm: Resetting rotation property
> 
> Ville Syrj?l? (9):
>   drm: Move DRM_ROTATE bits out of omapdrm into drm_crtc.h
>   drm: Add support_bits parameter to drm_property_create_bitmask()
>   drm: Add drm_mode_create_rotation_property()
>   drm/omap: Switch omapdrm over to drm_mode_create_rotation_property()
>   drm: Add drm_rect rotation functions
>   drm: Add drm_rotation_simplify()
>   drm/i915: Add 180 degree sprite rotation support
>   drm/i915: Make intel_plane_restore() return an error
>   drm/i915: Add rotation property for sprites

Missing from this series, your two documentation patches (we need to
bundle things up as a entity that makes sense for one of the maintainers
to pick it up (either Dave or Daniel)).

-- 
Damien

[Intel-gfx] [v2 10/11] drm/i915: Add 180 degree primary plane rotation support

[Damien Lespiau]

On Fri, Jul 04, 2014 at 03:14:02PM +0530, sonika.jindal at intel.com wrote:
> +static int intel_primary_plane_set_property(struct drm_plane *plane,
> + struct drm_property *prop,
> + uint64_t val)
> +{
> + struct drm_device *dev = plane->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct intel_crtc *intel_crtc = to_intel_crtc(plane->crtc);
> + struct drm_crtc *crtc = _crtc->base;
> + uint64_t old_val;
> +
> + if (prop == plane->rotation_property) {
> + /* exactly one rotation angle please */
> + if (hweight32(val & 0xf) != 1)
> + return -EINVAL;
> +
> + old_val = intel_plane->rotation;
> + intel_plane->rotation = val;
> +
> + if (intel_crtc->active && intel_crtc->primary_enabled) {
> + intel_crtc_wait_for_pending_flips(crtc);
> +
> + /* FBC does not work on some platforms for rotated planes */
> + if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
> + if (dev_priv->fbc.plane == intel_crtc->plane &&
> + intel_plane->rotation != 
> BIT(DRM_ROTATE_0))
> + intel_disable_fbc(dev);
> + /* If rotation was set earlier and new rotation 
> is 0, we might
> +  * have disabled fbc earlier. So update it now 
> */
> + else if (intel_plane->rotation == 
> BIT(DRM_ROTATE_0) &&
> + old_val != BIT(DRM_ROTATE_0))
> + intel_update_fbc(dev);

I see a intel_update_fbc() called with the struct_mutext lock elsewhere,
don't we need it here as well?

mutex_lock(>struct_mutex);
intel_update_fbc(dev);
mutex_unlock(>struct_mutex);

-- 
Damien

[PATCH libdrm 1/2] intel: Sync the command parser version parameter from kernel

[Damien Lespiau]

On Thu, Jun 19, 2014 at 09:28:08AM -0700, Volkin, Bradley D wrote:
> On Thu, Jun 19, 2014 at 11:31:53AM +0100, Damien Lespiau wrote:
> > Cc: Bradley Volkin 
> > Signed-off-by: Damien Lespiau 
> 
> Thanks for taking care of this Damien.
> 
> Reviewed-by: Brad Volkin 

Thanks for the review, pushed both patches.

-- 
Damien

[PATCH libdrm 2/2] intel: Sync typo fix from the kernel sources.

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 include/drm/i915_drm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 980dbf8..8eb0cb3 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -722,7 +722,7 @@ struct drm_i915_gem_execbuffer2 {
  */
 #define I915_EXEC_IS_PINNED(1<<10)

-/** Provide a hint to the kernel that the command stream and auxilliary
+/** Provide a hint to the kernel that the command stream and auxiliary
  * state buffers already holds the correct presumed addresses and so the
  * relocation process may be skipped if no buffers need to be moved in
  * preparation for the execbuffer.
-- 
1.8.3.1

[PATCH libdrm 1/2] intel: Sync the command parser version parameter from kernel

[Damien Lespiau]

Cc: Bradley Volkin 
Signed-off-by: Damien Lespiau 
---
 include/drm/i915_drm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index 2f4eb8c..980dbf8 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -337,6 +337,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_EXEC_NO_RELOC25
 #define I915_PARAM_HAS_EXEC_HANDLE_LUT   26
 #define I915_PARAM_HAS_WT   27
+#define I915_PARAM_CMD_PARSER_VERSION   28

 typedef struct drm_i915_getparam {
int param;
-- 
1.8.3.1

[PATCH] drm/doc: Fix nouveau typo

[Damien Lespiau]

Signed-off-by: Damien Lespiau 
---
 Documentation/DocBook/drm.tmpl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index efef637..0854aed 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3045,7 +3045,7 @@ void intel_crt_init(struct drm_device *dev)
TBD


-   noveau
+   nouveau
NV10 Overlay
"colorkey"
RANGE
-- 
1.8.3.1

[PATCH 3/3] drm/i915: Fix the confusing comment about the ioctl limits

[Damien Lespiau]

It was reported that this comment was confusing, and indeed it is.

Signed-off-by: Damien Lespiau 
---
 include/uapi/drm/i915_drm.h | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ff57f07..eacd063 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -171,8 +171,12 @@ typedef struct _drm_i915_sarea {
 #define I915_BOX_TEXTURE_LOAD  0x8
 #define I915_BOX_LOST_CONTEXT  0x10

-/* I915 specific ioctls
- * The device specific ioctl range is 0x40 to 0x79.
+/*
+ * i915 specific ioctls.
+ *
+ * The device specific ioctl range is [DRM_COMMAND_BASE, DRM_COMMAND_END) ie
+ * [0x40, 0xa0) (a0 is excluded) and those defines are offsets from
+ * DRM_COMMAND_BASE.
  */
 #define DRM_I915_INIT  0x00
 #define DRM_I915_FLUSH 0x01
-- 
1.8.3.1

[PATCH 2/3] drm: Driver-specific ioctls range from 0x40 to 0x9f

[Damien Lespiau]

DRM_COMMAND_END is 0xa0, so the last driver ioctl is 0x9f, not 0x99.

Signed-off-by: Damien Lespiau 
---
 include/uapi/drm/drm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 9abbeb9..b0b8556 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -780,7 +780,7 @@ struct drm_prime_handle {

 /**
  * Device specific ioctls should only be in their respective headers
- * The device specific ioctl range is from 0x40 to 0x99.
+ * The device specific ioctl range is from 0x40 to 0x9f.
  * Generic IOCTLS restart at 0xA0.
  *
  * \sa drmCommandNone(), drmCommandRead(), drmCommandWrite(), and
-- 
1.8.3.1

[PATCH 1/3] drm: Remove DRM_ARRAY_SIZE() for ARRAY_SIZE()

[Damien Lespiau]

I cannot see a need to provide a DRM_ version of ARRAY_SIZE(), only used
in a few places. I suspect its usage has been spread by copy & paste
rather than anything else.

Let's just remove it for plain ARRAY_SIZE().

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/armada/armada_drv.c | 2 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +-
 drivers/gpu/drm/gma500/psb_drv.c| 2 +-
 drivers/gpu/drm/i810/i810_dma.c | 2 +-
 drivers/gpu/drm/i915/i915_dma.c | 2 +-
 drivers/gpu/drm/i915/i915_ioc32.c   | 2 +-
 drivers/gpu/drm/mga/mga_ioc32.c | 2 +-
 drivers/gpu/drm/mga/mga_state.c | 2 +-
 drivers/gpu/drm/nouveau/nouveau_ioc32.c | 2 +-
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +-
 drivers/gpu/drm/r128/r128_ioc32.c   | 2 +-
 drivers/gpu/drm/r128/r128_state.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_ioc32.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
 drivers/gpu/drm/radeon/radeon_state.c   | 2 +-
 drivers/gpu/drm/savage/savage_bci.c | 2 +-
 drivers/gpu/drm/sis/sis_mm.c| 2 +-
 drivers/gpu/drm/via/via_dma.c   | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
 include/drm/drmP.h  | 2 --
 20 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c 
b/drivers/gpu/drm/armada/armada_drv.c
index 567cfbd..8ab3cd1 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -402,7 +402,7 @@ static struct platform_driver armada_drm_platform_driver = {

 static int __init armada_drm_init(void)
 {
-   armada_drm_driver.num_ioctls = DRM_ARRAY_SIZE(armada_ioctls);
+   armada_drm_driver.num_ioctls = ARRAY_SIZE(armada_ioctls);
return platform_driver_register(_drm_platform_driver);
 }
 module_init(armada_drm_init);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 87461d4..ddb5003 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -569,7 +569,7 @@ static int exynos_drm_platform_probe(struct platform_device 
*pdev)
int ret;

pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-   exynos_drm_driver.num_ioctls = DRM_ARRAY_SIZE(exynos_ioctls);
+   exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);

 #ifdef CONFIG_DRM_EXYNOS_FIMD
ret = platform_driver_register(_driver);
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 59ea45e..6e8fe9e 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -477,7 +477,7 @@ static struct drm_driver driver = {
.lastclose = psb_driver_lastclose,
.preclose = psb_driver_preclose,

-   .num_ioctls = DRM_ARRAY_SIZE(psb_ioctls),
+   .num_ioctls = ARRAY_SIZE(psb_ioctls),
.device_is_agp = psb_driver_device_is_agp,
.irq_preinstall = psb_irq_preinstall,
.irq_postinstall = psb_irq_postinstall,
diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c
index aeace37..e88bac1 100644
--- a/drivers/gpu/drm/i810/i810_dma.c
+++ b/drivers/gpu/drm/i810/i810_dma.c
@@ -1251,7 +1251,7 @@ const struct drm_ioctl_desc i810_ioctls[] = {
DRM_IOCTL_DEF_DRV(I810_FLIP, i810_flip_bufs, DRM_AUTH|DRM_UNLOCKED),
 };

-int i810_max_ioctl = DRM_ARRAY_SIZE(i810_ioctls);
+int i810_max_ioctl = ARRAY_SIZE(i810_ioctls);

 /**
  * Determine if the device really is AGP or not.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 93c0e1a..7c63b18 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2025,7 +2025,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, 
DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };

-int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
+int i915_max_ioctl = ARRAY_SIZE(i915_ioctls);

 /*
  * This is really ugly: Because old userspace abused the linux agp interface to
diff --git a/drivers/gpu/drm/i915/i915_ioc32.c 
b/drivers/gpu/drm/i915/i915_ioc32.c
index 3c59584..2e0613e 100644
--- a/drivers/gpu/drm/i915/i915_ioc32.c
+++ b/drivers/gpu/drm/i915/i915_ioc32.c
@@ -208,7 +208,7 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
if (nr < DRM_COMMAND_BASE)
return drm_compat_ioctl(filp, cmd, arg);

-   if (nr < DRM_COMMAND_BASE + DRM_ARRAY_SIZE(i915_compat_ioctls))
+   if (nr < DRM_COMMAND_BASE + ARRAY_SIZE(i915_compat_ioctls))
fn = i915_compat_ioctls[nr - DRM_COMMAND_BASE];

if (fn != NULL)
diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
index 86b4bb8..729bfd5 100644
--- a/drivers/gpu/drm/mga/mga_ioc32.c
+++ b/drivers/gpu/drm/mga/mga_ioc32.c
@@ -214,7 +214,7 @@ long mga_compat_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
if (nr < DRM_COMMAND_BASE)
   

[PATCH 0/3] A couple of fixes about the ioctl number split

[Damien Lespiau]

It was reported a long time ago the various comments about the DRM/driver
specific ioctl split were confusing. So tried to fix that.

Patch #1 is a bonus patch that removes DRM_ARRAY_SIZE().

-- 
Damien

Damien Lespiau (3):
  drm: Remove DRM_ARRAY_SIZE() for ARRAY_SIZE()
  drm: Driver-specific ioctls range from 0x40 to 0x9f
  drm/i915: Fix the confusing comment about the ioctl limits

 drivers/gpu/drm/armada/armada_drv.c | 2 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +-
 drivers/gpu/drm/gma500/psb_drv.c| 2 +-
 drivers/gpu/drm/i810/i810_dma.c | 2 +-
 drivers/gpu/drm/i915/i915_dma.c | 2 +-
 drivers/gpu/drm/i915/i915_ioc32.c   | 2 +-
 drivers/gpu/drm/mga/mga_ioc32.c | 2 +-
 drivers/gpu/drm/mga/mga_state.c | 2 +-
 drivers/gpu/drm/nouveau/nouveau_ioc32.c | 2 +-
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +-
 drivers/gpu/drm/r128/r128_ioc32.c   | 2 +-
 drivers/gpu/drm/r128/r128_state.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_ioc32.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
 drivers/gpu/drm/radeon/radeon_state.c   | 2 +-
 drivers/gpu/drm/savage/savage_bci.c | 2 +-
 drivers/gpu/drm/sis/sis_mm.c| 2 +-
 drivers/gpu/drm/via/via_dma.c   | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
 include/drm/drmP.h  | 2 --
 include/uapi/drm/drm.h  | 2 +-
 include/uapi/drm/i915_drm.h | 8 ++--
 22 files changed, 26 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH] drm: Remove spurious ';'

[Damien Lespiau]

One small step after another, the never-ending crusade towards better
code continues.

Signed-off-by: Damien Lespiau 
---
 drivers/gpu/drm/ast/ast_post.c  | 4 ++--
 drivers/gpu/drm/exynos/exynos_dp_core.c | 2 +-
 drivers/gpu/drm/exynos/exynos_drm_dpi.c | 2 +-
 drivers/gpu/drm/gma500/mdfld_dsi_pkg_sender.c   | 2 +-
 drivers/gpu/drm/nouveau/core/engine/graph/ctxnvf0.c | 2 +-
 drivers/gpu/drm/nouveau/core/engine/graph/nv50.c| 2 +-
 drivers/gpu/drm/radeon/radeon_bios.c| 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 4e5ea38..38d437f 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -1083,7 +1083,7 @@ static void get_ddr3_info(struct ast_private *ast, struct 
ast2300_dram_param *pa
case AST_DRAM_4Gx16:
param->dram_config = 0x133;
break;
-   }; /* switch size */
+   } /* switch size */

switch (param->vram_size) {
default:
@@ -1454,7 +1454,7 @@ static void get_ddr2_info(struct ast_private *ast, struct 
ast2300_dram_param *pa
case AST_DRAM_4Gx16:
param->dram_config = 0x123;
break;
-   }; /* switch size */
+   } /* switch size */

switch (param->vram_size) {
default:
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c 
b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 5e05dbc..a8ffc8c 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1087,7 +1087,7 @@ static void exynos_dp_dpms(struct exynos_drm_display 
*display, int mode)
break;
default:
break;
-   };
+   }
dp->dpms_mode = mode;
 }

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index f1b8587..482127f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -152,7 +152,7 @@ static void exynos_dpi_dpms(struct exynos_drm_display 
*display, int mode)
break;
default:
break;
-   };
+   }
ctx->dpms_mode = mode;
 }

diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_pkg_sender.c 
b/drivers/gpu/drm/gma500/mdfld_dsi_pkg_sender.c
index 489ffd2..87885d8 100644
--- a/drivers/gpu/drm/gma500/mdfld_dsi_pkg_sender.c
+++ b/drivers/gpu/drm/gma500/mdfld_dsi_pkg_sender.c
@@ -148,7 +148,7 @@ static int handle_dsi_error(struct mdfld_dsi_pkg_sender 
*sender, u32 mask)
break;
case BIT(14):
/*wait for all fifo empty*/
-   /*wait_for_all_fifos_empty(sender)*/;
+   /*wait_for_all_fifos_empty(sender)*/
break;
case BIT(15):
dev_dbg(sender->dev->dev, "No Action required\n");
diff --git a/drivers/gpu/drm/nouveau/core/engine/graph/ctxnvf0.c 
b/drivers/gpu/drm/nouveau/core/engine/graph/ctxnvf0.c
index 0fab95e..dec03f0 100644
--- a/drivers/gpu/drm/nouveau/core/engine/graph/ctxnvf0.c
+++ b/drivers/gpu/drm/nouveau/core/engine/graph/ctxnvf0.c
@@ -842,7 +842,7 @@ nvf0_grctx_generate_mods(struct nvc0_graph_priv *priv, 
struct nvc0_grctx *info)
u16 magic3 = 0x0648;
magic[gpc][0]  = 0x1000 | (magic0 << 16) | offset;
magic[gpc][1]  = 0x | (magic1 << 16);
-   offset += 0x0324 * (priv->tpc_nr[gpc] - 1);;
+   offset += 0x0324 * (priv->tpc_nr[gpc] - 1);
magic[gpc][2]  = 0x1000 | (magic2 << 16) | offset;
magic[gpc][3]  = 0x | (magic3 << 16);
offset += 0x0324;
diff --git a/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c 
b/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c
index 2c7809e..1a2d564 100644
--- a/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/graph/nv50.c
@@ -901,7 +901,7 @@ nv50_graph_ctor(struct nouveau_object *parent, struct 
nouveau_object *engine,
nv_engine(priv)->sclass = nvaf_graph_sclass;
break;

-   };
+   }

/* unfortunate hw bug workaround... */
if (nv_device(priv)->chipset != 0x50 &&
diff --git a/drivers/gpu/drm/radeon/radeon_bios.c 
b/drivers/gpu/drm/radeon/radeon_bios.c
index 9ab3097..6a03624 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -626,7 +626,7 @@ static bool radeon_acpi_vfct_bios(struct radeon_device 
*rdev)
vhdr->DeviceID != rdev->pdev->device) {
DRM_INFO("ACPI VFCT table is not for this card\n");
goto out_unmap;
-   };
+   }

if (vfct->VBIOSImageOffset + sizeof(VFCT_IMAGE_HEADER) + 
vhdr->ImageLength > tbl_size) {
DRM_ERROR("ACPI VFCT image truncated\n");
-- 
1.8.3.1

[PATCH] drm/doc: Add the "type" plane property to the list of properties

[Damien Lespiau]

Matt aded this plane property before we had a table giving a summary of
the properties. Add it there.

Cc: Matt Roper 
Signed-off-by: Damien Lespiau 
---
 Documentation/DocBook/drm.tmpl | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 0854aed..ba405fe 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2466,7 +2466,7 @@ void intel_crt_init(struct drm_device *dev)
Description/Restrictions


-   DRM
+   DRM
Generic
?EDID?
BLOB | IMMUTABLE
@@ -2482,6 +2482,14 @@ void intel_crt_init(struct drm_device *dev)
Contains DPMS operation mode value.


+   Plane
+   ?type?
+   ENUM | IMMUTABLE
+   { "Overlay", "Primary", "Cursor" }
+   Plane
+   Plane type
+   
+   
DVI-I
?subconnector?
ENUM
-- 
1.8.3.1

[Intel-gfx] [PATCH] drivers/gpu/drm/i915/dma: style fixes

[Damien Lespiau]

On Mon, Jun 02, 2014 at 04:59:39PM +0200, Robin Schroer wrote:
> Fixed several double space pointer notations, and added one newline
> 
> Signed-off-by: Robin Schroer 

Reviewed-by: Damien Lespiau 

-- 
Damien