from:"Russell King"

On Sun, Jun 30, 2013 at 01:59:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
  +{
  +   struct drm_display_mode *adj = dcrtc-crtc.mode;
  +   uint32_t val = 0;
  +
  +   if (dcrtc-csc_yuv_mode == CSC_YUV_CCIR709)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   if (dcrtc-csc_rgb_mode == CSC_RGB_STUDIO)
  +   val |= CFG_CSC_RGB_STUDIO;
  +
  +   /*
  +* In auto mode, set the colorimetry, based upon the HDMI spec.
  +* 1280x720p, 1920x1080p and 1920x1080i use ITU709, others use
  +* ITU601.  It may be more appropriate to set this depending on
  +* the source - but what if the graphic frame is YUV and the
  +* video frame is RGB?
  +*/
  +   if ((adj-hdisplay == 1280  adj-vdisplay == 720 
  +!(adj-flags  DRM_MODE_FLAG_INTERLACE)) ||
  +   (adj-hdisplay == 1920  adj-vdisplay == 1080)) {
  +   if (dcrtc-csc_yuv_mode == CSC_AUTO)
  +   val |= CFG_CSC_YUV_CCIR709;
  +   }
  +
  +   /*
  +* We assume we're connected to a TV-like device, so the YUV-RGB
  +* conversion should produce a limited range.  We should set this
  +* depending on the connectors attached to this CRTC, and what
  +* kind of device they report being connected.
  +*/
  +   if (dcrtc-csc_rgb_mode == CSC_AUTO)
  +   val |= CFG_CSC_RGB_STUDIO;
 
 In the intel driver we check whether it's an cea mode with
 drm_match_cea_mode, e.g. in intel_hdmi.c:
 
   if (intel_hdmi-color_range_auto) {
   /* See CEA-861-E - 5.1 Default Encoding Parameters */
   if (intel_hdmi-has_hdmi_sink 
   drm_match_cea_mode(adjusted_mode)  1)
   intel_hdmi-color_range = HDMI_COLOR_RANGE_16_235;
   else
   intel_hdmi-color_range = 0;
   }
 
 (The color_range gets then propageted to the right place since different
 platforms have different ways to do that in intel hw).

Unfortunately, this disagrees with the HDMI v1.3a specification:

| Default Colorimetry
| 
| ...
| 
| 480p, 480i, 576p, 576i, 240p and 288p
| 
| The default colorimetry for all 480-line, 576-line, 240-line, and 288-line
| video formats described in CEA-861-D is based on SMPTE 170M.
| 
| 1080i, 1080p and 720p
| 
| The default colorimetry for high-definition video formats (1080i, 1080p and
| 720p) described in CEA-861-D is based on ITU-R BT.709-5.

As the HDMI spec refers to the CEA-861 specs, the HDMI spec overrides
CEA when dealing with HDMI specifics.

So, according to the HDMI specification, the default is that it's only
1080i, 1080p and 720p which are 709, the others are 601.  This does not
correspond with drm_match_cea_mode(adjusted_mode)  1.

Unfortunately, given DRM's structure, there's no way for the CRTC layer
to really know what its driving, and this setting is at the CRTC layer,
not the output layer.  It may be that you have the CRTC duplicated
between two different display devices with different characteristics,
which means you have to choose one - or just not bother.  I've chosen
the latter because it's a simpler solution, and is in no way any more
correct than any other solution.

This is also why these are exported to userspace via properties, so if
userspace knows better, it can deal with those issues.

  +struct armada_framebuffer *armada_framebuffer_create(struct drm_device 
  *dev,
  +   struct drm_mode_fb_cmd2 *mode, struct armada_gem_object *obj)
  +{
  +   struct armada_framebuffer *dfb;
  +   uint8_t format, config;
  +   int ret;
  +
  +   switch (mode-pixel_format) {
  +#define FMT(drm, fmt, mod) \
  +   case DRM_FORMAT_##drm:  \
  +   format = CFG_##fmt; \
  +   config = mod;   \
  +   break
  +   FMT(RGB565, 565,CFG_SWAPRB);
  +   FMT(BGR565, 565,0);
  +   FMT(ARGB1555,   1555,   CFG_SWAPRB);
  +   FMT(ABGR1555,   1555,   0);
  +   FMT(RGB888, 888PACK,CFG_SWAPRB);
  +   FMT(BGR888, 888PACK,0);
  +   FMT(XRGB,   X888,   CFG_SWAPRB);
  +   FMT(XBGR,   X888,   0);
  +   FMT(ARGB,   ,   CFG_SWAPRB);
  +   FMT(ABGR,   ,   0);
  +   FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);
  +   FMT(UYVY,   422PACK,CFG_YUV2RGB);
  +   FMT(VYUY,   422PACK,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVYU,   422PACK,CFG_YUV2RGB | CFG_SWAPYU);
  +   FMT(YUV422, 422,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU422, 422,CFG_YUV2RGB);
  +   FMT(YUV420, 420,CFG_YUV2RGB | CFG_SWAPUV);
  +   FMT(YVU420, 420,CFG_YUV2RGB);
  +   FMT(C8, PSEUDO8,0);
  +#undef FMT
  +   default:
  +   return ERR_PTR(-EINVAL);
  +   }
  +
  +   dfb = kzalloc(sizeof(*dfb), GFP_KERNEL);
  +   if (!dfb

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
 On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
  +   mutex_lock(dev-struct_mutex);
  +   free = drm_mm_search_free(priv-linear, size, align, 0);
  +   if (free)
  +   obj-linear = drm_mm_get_block(free, size, align);
  +   mutex_unlock(dev-struct_mutex);
 
 The two-stage search_free+get_block drm_mm interface is something I'd like
 to remove since it' complicates the interface for no gain. And the
 preallocation trick for atomic contexts is pretty racy as-is. Can you
 please convert this over to the insert_node interfaces which take a
 preallocate drm_mm_node?

Yes and no.  This is only racy if it is callable from atomic contexts,
which the above isn't, because it takes a mutex, and the above is the
only place which allocations against that pool are done.  Mutexes can't
be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
interfaces.

However, the insert_node interfaces appear not to solve any kind of race.
All they do is allow the kmalloc to be moved out of this region into
the user of this code sequence, while offering no additional locking or
safety.  So the mutex lock around a call to drm_mm_insert_node*() is
still required.

As the kmalloc() happens beneath the mutex lock, another thread can't race
with an allocation in the above code anyway.  The *only* reason I'll change
this is that it is nicer to the system not to hold locks while allocating
memory.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
 On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
  On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
   + mutex_lock(dev-struct_mutex);
   + free = drm_mm_search_free(priv-linear, size, align, 0);
   + if (free)
   + obj-linear = drm_mm_get_block(free, size, align);
   + mutex_unlock(dev-struct_mutex);
  
  The two-stage search_free+get_block drm_mm interface is something I'd like
  to remove since it' complicates the interface for no gain. And the
  preallocation trick for atomic contexts is pretty racy as-is. Can you
  please convert this over to the insert_node interfaces which take a
  preallocate drm_mm_node?
 
 Yes and no.  This is only racy if it is callable from atomic contexts,
 which the above isn't, because it takes a mutex, and the above is the
 only place which allocations against that pool are done.  Mutexes can't
 be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
 interfaces.
 
 However, the insert_node interfaces appear not to solve any kind of race.
 All they do is allow the kmalloc to be moved out of this region into
 the user of this code sequence, while offering no additional locking or
 safety.  So the mutex lock around a call to drm_mm_insert_node*() is
 still required.
 
 As the kmalloc() happens beneath the mutex lock, another thread can't race
 with an allocation in the above code anyway.  The *only* reason I'll change
 this is that it is nicer to the system not to hold locks while allocating
 memory.

Err.  There's bugs here in the other drivers such as i915 which follow
your suggestion.  The problem is this:

Every time they allocate using drm_mm_insert_node(), they kmalloc a new
struct drm_mm_node for this function to fill in and attach.

When they've done with the allocation, they call drm_mm_put_block().
This places the node onto the mm's unused_nodes list provided we don't
already have MM_UNUSED_TARGET nodes on that list.

So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
at which point we then start freeing any further nodes - and with no
way to pull these off that list, they all just sit there not being
used.

The only function which takes nodes off that list is drm_mm_kmalloc(),
which is declared statically, and so isn't available to drivers.

Are drivers which use the drm_mm_insert_node() function expected to
also use drm_mm_remove_node(), kfreeing the node after that call,
rather than using drm_mm_put_block() ?

Either way, I think someone needs to fix the other drivers and Cc those
patches to Stable...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

Right, so, incrementally, the changes are (this may not entirely apply
to the version I've posted because I have several patches on top of
that.)

I've also added locking to the calls to drm_mm_dump_table() as otherwise
these walk those lists without holding any locks what so ever, which
could mean that a block is removed while we're walking the list.

 drivers/gpu/drm/armada/armada_debugfs.c |   17 ++--
 drivers/gpu/drm/armada/armada_fb.c  |   43 ++-
 drivers/gpu/drm/armada/armada_fbdev.c   |   20 ++
 drivers/gpu/drm/armada/armada_gem.c |   29 +++--
 4 files changed, 68 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c 
b/drivers/gpu/drm/armada/armada_debugfs.c
index f42f3ab..60e1038 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -23,16 +23,27 @@ static int armada_debugfs_gem_offs_show(struct seq_file *m, 
void *data)
struct drm_info_node *node = m-private;
struct drm_device *dev = node-minor-dev;
struct drm_gem_mm *mm = dev-mm_private;
+   int ret;
+
+   mutex_lock(dev-struct_mutex);
+   ret = drm_mm_dump_table(m, mm-offset_manager);
+   mutex_unlock(dev-struct_mutex);
 
-   return drm_mm_dump_table(m, mm-offset_manager);
+   return ret;
 }
 
 static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 {
struct drm_info_node *node = m-private;
-   struct armada_private *priv = node-minor-dev-dev_private;
+   struct drm_device *dev = node-minor-dev;
+   struct armada_private *priv = dev-dev_private;
+   int ret;
 
-   return drm_mm_dump_table(m, priv-linear);
+   mutex_lock(dev-struct_mutex);
+   ret = drm_mm_dump_table(m, priv-linear);
+   mutex_unlock(dev-struct_mutex);
+
+   return ret;
 }
 
 static int armada_debugfs_reg_show(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/armada/armada_fb.c 
b/drivers/gpu/drm/armada/armada_fb.c
index 28965e3..97fbd61 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -79,6 +79,9 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,
 
dfb-fmt = format;
dfb-mod = config;
+   dfb-obj = obj;
+
+   drm_helper_mode_fill_fb_struct(dfb-fb, mode);
 
ret = drm_framebuffer_init(dev, dfb-fb, armada_fb_funcs);
if (ret) {
@@ -86,14 +89,13 @@ struct armada_framebuffer *armada_framebuffer_create(struct 
drm_device *dev,
return ERR_PTR(ret);
}
 
-   drm_helper_mode_fill_fb_struct(dfb-fb, mode);
-
/*
-* Take a reference on our object - the caller is expected
-* to drop their reference to it.
+* Take a reference on our object as we're successful - the
+* caller already holds a reference, which keeps us safe for
+* the above call, but the caller will drop their reference
+* to it.  Hence we need to take our own reference.
 */
drm_gem_object_reference(obj-obj);
-   dfb-obj = obj;
 
return dfb;
 }
@@ -111,39 +113,44 @@ static struct drm_framebuffer *armada_fb_create(struct 
drm_device *dev,
mode-pitches[2]);
 
/* We can only handle a single plane at the moment */
-   if (drm_format_num_planes(mode-pixel_format)  1)
-   return ERR_PTR(-EINVAL);
+   if (drm_format_num_planes(mode-pixel_format)  1) {
+   ret = -EINVAL;
+   goto err;
+   }
 
obj = armada_gem_object_lookup(dev, dfile, mode-handles[0]);
if (!obj) {
-   DRM_ERROR(failed to lookup gem object\n);
-   return ERR_PTR(-ENOENT);
+   ret = -ENOENT;
+   goto err;
}
 
if (obj-obj.import_attach  !obj-sgt) {
ret = armada_gem_map_import(obj);
if (ret)
-   goto unref;
+   goto err_unref;
}
 
/* Framebuffer objects must have a valid device address for scanout */
if (obj-dev_addr == DMA_ERROR_CODE) {
ret = -EINVAL;
-   goto unref;
+   goto err_unref;
}
 
dfb = armada_framebuffer_create(dev, mode, obj);
-   ret = IS_ERR(dfb) ? PTR_ERR(dfb) : 0;
+   if (IS_ERR(dfb)) {
+   ret = PTR_ERR(dfb);
+   goto err;
+   }
 
-unref:
drm_gem_object_unreference_unlocked(obj-obj);
 
-   if (ret) {
-   DRM_ERROR(failed to initialize framebuffer: %d\n, ret);
-   return ERR_PTR(ret);
-   }
-
return dfb-fb;
+
+ err_unref:
+   drm_gem_object_unreference_unlocked(obj-obj);
+ err:
+   DRM_ERROR(failed to initialize framebuffer: %d\n, ret);
+   return ERR_PTR(ret);
 }
 
 static void armada_output_poll_changed(struct drm_device *dev)
diff --git a/drivers/gpu/drm/armada/armada_fbdev.c

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

2013-07-01 Thread Russell King - ARM Linux

On Mon, Jul 01, 2013 at 10:01:30AM +1000, Dave Airlie wrote:
 OMG I'm working in a subsystem where stuff is being developed, with only
 a few resources! I know my full time job isn't maintaining a 500,000
 line subsystem,
 and the sub maintainers and developers do a great job refactoring
 where required.
 
 lots of other driver authors have made substantial changes to the drm core as
 they've written drivers, you spot one bit of refactoring and its a
 major shortcoming
 of the entire subsystem and development community.
 
 how about instead of writing:
 However, at least I've taken the time to _think_ about what I'm doing
 and realise that there _is_ scope here for the DRM core to improve,
 rather than burying this stuff deep inside my driver like everyone else
 has.  That's no reason to penalise patches from the good guys who think
 
 you go with
 I noticed this piece of functionality could be refactored, here is a
 patch adding them to
 the core, does anyone think its a good idea?
 
 As Daniel pointed out every driver submitted has refactored things,
 even exynos did a
 lot of work to be the first ARM driver we shipped, the DRM core
 doesn't write itself and
 I fully expect driver authors to be the ones that tell me what needs
 refactoring, since
 they are on the pointy end, but to state that you are the only person
 ever to think about
 things is frankly being a dick.
 
 Lets try for less dick, and more productive in future.

And you can stick your dick back where you got it from David.  Really,
your response is totally uncalled for.

Let's try realising that I only have very limited time to put into this
and unlike the other submitters who have been _paid_ to get their code
sorted, this is being done purely for free - which means it's really
low priority.  As you should already realise because I've stated already
that I *really* don't care whether it gets into mainline or not.

I am *NOT* planning on spending any time on this during the next week
as I have *paid* work to do, and probably not next weekend nor the
following week either.  So the next time that I'm likely to do anything
on this is in a fortnight or longer.

If you want stuff to be refactored in DRM, you need to find someone
with more time to do it.

And before you continue making a mountain out of a molehill, as you
seem to like doing, the let's make it a core thing is REALLY BLOODY
SIMPLE.  All it takes is for the header file to go into include/drm.
It's now available to anyone who wants to use that - and all that's
left is to sort out all those drivers *WHICH I CANT TEST* to use
that stuff.

And, frankly, when I was going to post this latest RFC, I gave serious
consideration to quite simply not posting it to dri-devel and copying
you or any other person listed as a maintainer for DRM, and only posting
it to the people interested in it on the ARM side, because I'd already
given up hope of it ever being merged into mainline.

Your totally over the top and rediculous response just confirms that.

Further postings will now omit you and dri-devel.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

2013-07-03 Thread Russell King - ARM Linux

On Tue, Jul 02, 2013 at 09:01:55PM +0300, Ville Syrjälä wrote:
 On Sun, Jun 30, 2013 at 07:29:27PM +0200, Daniel Vetter wrote:
  On Sun, Jun 30, 2013 at 2:52 PM, Russell King - ARM Linux
  li...@arm.linux.org.uk wrote:
   | Default Colorimetry
   |
   | ...
   |
   | 480p, 480i, 576p, 576i, 240p and 288p
   |
   | The default colorimetry for all 480-line, 576-line, 240-line, and 
   288-line
   | video formats described in CEA-861-D is based on SMPTE 170M.
   |
   | 1080i, 1080p and 720p
   |
   | The default colorimetry for high-definition video formats (1080i, 1080p 
   and
   | 720p) described in CEA-861-D is based on ITU-R BT.709-5.
 
 I think this was pretty much copy pasted from CEA-861-D which is very
 vague.
 
 CEA-861-E is a bit better, and more clearly states that if the sink can
 receive YCbCr, then the source should use it by default for CE formats,
 and the default colorimetry depends on whether it's SD or HD. It also
 states that when transmitting IT or CE formats as RGB, the color space
 is the one defined in the EDID. CEA-861-D only made that statement for
 IT formats, and left the RGB CE format case out in the cold.

Actually, what I'm doing there is probably wrong when you consider
what is going on:

 Overlay (YUV) - YUV-RGB colorspace conversion
  |
  v
 Graphic (RGB) ---(colorkey) HDMI

These bits control the YUV-RGB colorspace conversion.  The is it 601
or 709 colorspace question applies more to the colorspace of the
overlay image.  As far as I can tell, that is unspecified within our
normal video playback programs - there's provision to communicate that
information (they certainly don't seem to look for any kind of Xv
attribute).

The is it computer or studio RGB question (I think - I can't say
because the documentation is hellishly poor, and you now have as much
information on this as I do) refers to the colorspace of the RGB side.

So, maybe I should move the YUV colorspace setting to be a drm_plane
property?  But then how do we know what format it is supposed to be?
Do we just pick one and hope it's right?  Do we try to autodetect it
from the size of the drm_plane framebuffer?  What if something
downscales a HD YUV framebuffer to something smaller because the
display is smaller?

What I can say is that I've watched many hours of content with my driver
and at 720p output resolution, I prefer it converting the YUV between
709 to studio RGB - otherwise the blacks are too black and I find that
I have to adjust the brightness/contrast to bring the black levels up
compared to a standard TV broadcast.

 Oh and the other thing someone should do is fix the intel Xv code to
 use BT.709 CSC matrix for HD content. I believe that code is hardcoded
 for BT.601 currently, which may explain the last weirdness reported in
 that CEA bug or ours.

How do you propose to switch between the two?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

2013-07-13 Thread Russell King - ARM Linux

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:
 On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:
 On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Draked...@laptop.org  wrote:
 On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moinemoin...@free.fr  
 wrote:

 - the phandles to the clocks does not tell how the clock may be set by
the driver (it is an array index in the 510).

 In the other threads on clock selection, we decided that that exact
 information on how to select a clock (i.e. register bits/values) must
 be in the driver, not in the DT. What was suggested there is a
 well-documented scheme for clock naming, so the driver knows which
 clock is which. That is defined in the proposed DT binding though the
 valid clock names part. For an example driver implementation of this
 you can see my patch armada_drm clock selection - try 2.

 OK.

 Sorry to go back to this thread.

 I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
 driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
 modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0

 Hmm, a bit off topic question, does it work when the si5351 module gets
 removed ? I can't see anything preventing clock provider module from being
 removed why any of its clocks is used and clk_unregister() function is
 currently unimplemented.

When I designed the clk API, I arranged for things like clk_get() to
take a reference on the module if the clock was supplied by a module.
You'll see some evidence of that by looking back in the git history
for arch/arm/mach-integrator/include/mach/clkdev.h.

If the common clk API has been designed without any thought to clocks
supplied by modules and making sure that in-use clocks don't go away,
then it's going to be a real pain to sort that out.  I don't think
refcounting clocks makes sense (what do you do with an enabled clock
that you then remove the module for but it's still being used?  Do you
just shut it down anyway?  Do you leave it running?  What about
subsequent calls?).

I think this is one case where taking a reference on the module supplying
it makes total sense.

 (si5351). Normally, the external clock is used, but, sometimes, the
 si5351 module is not yet initialized when the drm driver starts. So,
 for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
 (40/3) instead of 148500. As a result, display and sound still work
 correctly on my TV set thru HDMI.

 So, it would be nice to have 2 usable clocks per LCD, until we find a
 good way to initialize the modules in the right order at startup time.

 Doesn't deferred probing help here ?

Indeed it does.  Just because one TV works with such a wrong clock does not
mean that all TVs and HDMI compatible monitors will do.  It's 11% out.

The reason that audio works is because of the way the HDMI transmitter works
- it can calculate the CTS value to send (by measuring it) and it sends that
value over the link so the TV can regenerate the correct audio clock from
the HDMI clock.

Whether that's going to be universally true, I don't know - it depends on
how much audio data gets sent via each frame.  As the HDMI clock is slower,
there would need to be more audio data sent.

  lcd0: lcd-controller@82 {
  compatible = marvell,dove-lcd0;
 [...]
  };

  lcd1: lcd-controller@81 {
  compatible = marvell,dove-lcd1;
 [...]
  };

 Using different compatible strings to indicate multiple instances of same
 hardware doesn't seem right. Unless LCD0, LCD1 are really different pieces

They aren't.  They're 100% identical in the Armada 510.

 of hardware incompatible with each other I think it would be more correct
 to use same compatible property and DT node aliases, similarly as is now
 done with e.g. I2C busses:

   aliases {
   lcd0 = lcd_0;  
   lcd1 = lcd_1;  
   };

   lcd_0: lcd-controller@82 {
   compatible = marvell,dove-lcd;

I'd much prefer marvell,armada-510-lcd rather than using the codenames for
the devices.  Otherwise we're going to run into totally different things
being used for different devices (eg, armada-xp...)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux

On Sat, Jul 13, 2013 at 08:25:15AM -0600, Daniel Drake wrote:
 I guess the IRE falls into the same category as the DCON - we won't
 implement it for now, but knowing where it might fit in is useful.

I don't see much need at the moment for IRE.  IRE isn't going to be
useful for general graphics rotation as it only supports up to 16bpp
RGB.

It looks to me like this module was designed to rotate YUV/camera
images.  If we wanted to rotate a video image, then it would probably
be more efficient to use this, but it will cause a conversion of the
image format in doing so.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux

On Sat, Jul 13, 2013 at 07:44:58PM +0200, Sebastian Hesselbarth wrote:
 On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
 When I designed the clk API, I arranged for things like clk_get() to
 take a reference on the module if the clock was supplied by a module.
 You'll see some evidence of that by looking back in the git history
 for arch/arm/mach-integrator/include/mach/clkdev.h.

 Last time I checked, clkdev API neither implements referencing nor
 unregister.

*Sigh*.

a613163dff04cbfcb7d66b06ef4a5f65498ee59b:
arch/arm/mach-integrator/include/mach/clkdev.h:
-struct clk {
-   unsigned long   rate;
-   const struct clk_ops*ops;
-   struct module   *owner;
-   const struct icst_params *params;
-   void __iomem*vcoreg;
-   void*data;
-};
-
-static inline int __clk_get(struct clk *clk)
-{
-   return try_module_get(clk-owner);
-}
-
-static inline void __clk_put(struct clk *clk)
-{
-   module_put(clk-owner);
-}

70ee65771424829fd092a1df9afcc7e24c94004b:
arch/arm/mach-integrator/impd1.c:
 static int impd1_probe(struct lm_device *dev)
...
-   for (i = 0; i  ARRAY_SIZE(impd1-vcos); i++) {
-   impd1-vcos[i].ops = impd1_clk_ops,
-   impd1-vcos[i].owner = THIS_MODULE,
-   impd1-vcos[i].params = impd1_vco_params,
-   impd1-vcos[i].data = impd1;
-   }
-   impd1-vcos[0].vcoreg = impd1-base + IMPD1_OSC1;
-   impd1-vcos[1].vcoreg = impd1-base + IMPD1_OSC2;
-
-   impd1-clks[0] = clkdev_alloc(impd1-vcos[0], NULL, lm%x:01000,
-   dev-id);
-   impd1-clks[1] = clkdev_alloc(fixed_14745600, NULL, lm%x:00100,
-   dev-id);
-   impd1-clks[2] = clkdev_alloc(fixed_14745600, NULL, lm%x:00200,
-   dev-id);
-   for (i = 0; i  ARRAY_SIZE(impd1-clks); i++)
-   clkdev_add(impd1-clks[i]);
...
 static void impd1_remove(struct lm_device *dev)
...
-   for (i = 0; i  ARRAY_SIZE(impd1-clks); i++)
-   clkdev_drop(impd1-clks[i]);

drivers/clk/clkdev.c:
/*
 * clkdev_drop - remove a clock dynamically allocated
 */
void clkdev_drop(struct clk_lookup *cl)
{
mutex_lock(clocks_mutex);
list_del(cl-node);
mutex_unlock(clocks_mutex);
kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);

No, of course, I'm imagining all the above!

Now, today, the IM-PD/1 support got broken in respect of ensuring that
things are properly refcounted in the rush to convert things to this
wonderful new common clk API - because it's oh soo much better.  Yes,
right, I'd forgotten the number one rule of kernel programming - always
sacrifice correctness when we get a new fantasic hip idea!  Silly me.

 This is on Mike's list and IIRC there already has been
 a proposal for unregister. Si5351 was the first clkdev driver ever
 that could possibly be unloaded, so there may be still some features
 missing.

Utter rubbish - it's not the first as you can see from the above.
Integrator had this support since the clk and clkdev APIs came along,
because the IM-PD/1 module was implemented as a module, and it has to
create and register clocks for its peripherals.

What you've found is a backwards step with the common clk API, nothing
more.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux

On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
 I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
 they're working on v4 with clock object reference counting. Presumably we
 need both clk_get() to be taking reference on the module and reference
 counted clk free, e.g. in cases where clock provider is a hot-pluggable
 device. It might be too paranoid though, I haven't seen hardware
 configurations where a clock source could be unplugged safely when whole
 system is running.

I'm not going to accept refcounting being thrown into clk_get().  The
clkdev API already has refcounting, as much as it needs to.  It just
needs people to use the hooks that I provided back in 2008 when I
created the clkdev API for doing _precisely_ this job.

Have a read through these commits, which backup my statement above:

0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API

and it will show you how to do refcounting.  The common clk API just
needs to stop defining __clk_get() and __clk_put() to be an empty
function and implement them appropriately for it's clk implementation,
like they were always meant to be.

__clk_get() and __clk_put() are the clk-implementation specific parts
of the clkdev API, because the clkdev API is utterly divorsed from the
internals of what a 'struct clk' actually is.  clkdev just treats a
'struct clk' as a completely opaque type and never bothers poking
about inside it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111

2013-07-26 Thread Russell King - ARM Linux

On Thu, Jul 25, 2013 at 02:21:59PM -0400, Rob Clark wrote:
 On Thu, Jul 25, 2013 at 1:17 PM,  tom.cook...@arm.com wrote:
  Known issues:
   * It uses KDS. We intend to switch to whatever implicit per-buffer
 synchronisation mechanism gets merged, once something is merged.
   * It abuses flags parameter of DRM_IOCTL_MODE_CREATE_DUMB to also
 allocate buffers for the GPU. Still not sure how to resolve this
 as we don't use DRM for our GPU driver.
 
 any thoughts/plans about a DRM GPU driver?  Ideally long term (esp.
 once the dma-fence stuff is in place), we'd have gpu-specific drm
 (gpu-only, no kms) driver, and SoC/display specific drm/kms driver,
 using prime/dmabuf to share between the two.

The PL111 primecell is just a scanout device.  It has no GPU embedded
in it.  It's just like the Armada/Dove DRM stuff - if a GPU is provided
it's an entirely separate piece of IP, and will likely remain a separate
non-DRM driver.

In the case of PL111, PL111 is not manufacturer specific so the GPU can
be any standalone GPU.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] drm/tilcdc drm/i2c/tda998x workaround for sync issues on TI SoC

2013-08-01 Thread Russell King - ARM Linux

On Wed, Jul 31, 2013 at 10:21:20PM +0200, Sebastian Hesselbarth wrote:
 Should we prepare a new patch set comprising the following patches?

 Russell King:
 drm/i2c: nxp-tda998x: fix EDID reading on TDA19988 devices
 drm/i2c: nxp-tda998x: ensure VIP output mux is properly set
 drm/i2c: nxp-tda998x: fix npix/nline programming
 drm/i2c: nxp-tda998x: prepare for video input configuration
 drm/i2c: nxp-tda998x: add video and audio input configuration

 Sebastian Hesselbarth:
 drm/i2c: tda998x: fix sync generation and calculation

Unless you've changed this one since I tested it, it needs fixes which
basically revert the vsync changes back to the original code.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/i2c: tda998x: ensure VIP output mux is properly set

On Tue, Aug 06, 2013 at 12:20:12AM +0200, Sebastian Hesselbarth wrote:
   switch (mode) {
   case DRM_MODE_DPMS_ON:
 + /* Write the default value MUX register */
 + reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);

This looks like an old version of my patch.  I ended up with this register
write at the bottom of tda998x_reset():

 drivers/gpu/drm/i2c/tda998x_drv.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..dc0428d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
 #define REG_VIP_CNTRL_5   REG(0x00, 0x25) /* write */
 # define VIP_CNTRL_5_CKCASE   (1  0)
 # define VIP_CNTRL_5_SP_CNT(x)(((x)  3)  1)
+#define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */
 #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */
 # define MAT_CONTRL_MAT_SC(x) (((x)  3)  0)
 # define MAT_CONTRL_MAT_BP(1  2)
@@ -415,6 +416,9 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_PLL_SCGR1,0x5b);
reg_write(encoder, REG_PLL_SCGR2,0x00);
reg_write(encoder, REG_PLL_SCG2, 0x10);
+
+   /* Ensure VP output bus muxes result in no swapping */
+   reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
 /* DRM encoder functions */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/i2c: tda998x: fix EDID reading on TDA19988 devices

On Tue, Aug 06, 2013 at 12:20:11AM +0200, Sebastian Hesselbarth wrote:
 From: Russell King rmk+ker...@arm.linux.org.uk
 
 TDA19988 devices need their RAM enabled in order to read EDID
 information.  Add support for this.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com

There was some debate about whether this is needed or not.  It seems that
if I don't run the NXP driver, it isn't needed, but if I have run the
NXP driver, then yes it is.  As it seems to do no harm, I think it's fine
to be submitted.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

On Tue, Aug 06, 2013 at 12:20:15AM +0200, Sebastian Hesselbarth wrote:
 - also calculate CTS

This is wrong...

 + /*
 +  * HDMI 1.3a, 7.2.2 CTS parameter:
 +  *  (avg cts) = (fTMDS * N) / (128 * fS)
 +  */
 + cts = n * mode-clock / p-audio_sample_rate;
 + cts *= 1000;
 + cts /= 128;

This only works if you can guarantee that the audio and video clocks are
synchronous - and that you know the audio sample rate.

I don't believe the audio and video clocks are synchronous on the cubox,
and also have no way to communicate the audio sample rate to the TDA998x
driver at present.

So, this calculation serves little purpose and wastes CPU cycles.

 +static ssize_t i2c_read_store(struct device *dev, struct device_attribute 
 *attr, const char *buf, size_t size)
 +{
 + struct drm_encoder *encoder = dev_get_drvdata(dev);
 + unsigned int page, addr;
 + unsigned char val;
 +
 + sscanf(buf, %x %x, page, addr);
 +
 + val = reg_read(encoder, REG(page, addr));
 +
 + printk(i2c read %02x @ page:%02x address:%02x\n, val, page, addr);
 + return size;
 +}
 +
 +static ssize_t i2c_write_store(struct device *dev, struct device_attribute 
 *attr, const char *buf, size_t size)
 +{
 + struct drm_encoder *encoder = dev_get_drvdata(dev);
 + unsigned int page, addr, mask, val;
 + unsigned char rval;
 +
 + sscanf(buf, %x %x %x %x, page, addr, mask, val);
 +
 + rval = reg_read(encoder, REG(page, addr));
 + rval = ~mask;
 + rval |= val  mask;
 + reg_write(encoder, REG(page, addr), rval);
 +
 + printk(i2c write %02x @ page:%02x address:%02x\n, rval, page, addr);
 + return size;
 +}
 +
 +static DEVICE_ATTR(i2c_read, S_IWUSR, NULL, i2c_read_store);
 +static DEVICE_ATTR(i2c_write, S_IWUSR, NULL, i2c_write_store);
 +
  static int
  tda998x_encoder_init(struct i2c_client *client,
   struct drm_device *dev,
 @@ -817,6 +,11 @@ tda998x_encoder_init(struct i2c_client *client,
  {
   struct drm_encoder *encoder = encoder_slave-base;
   struct tda998x_priv *priv;
 +/* debug */
 + device_create_file(client-dev, dev_attr_i2c_read);
 + device_create_file(client-dev, dev_attr_i2c_write);
 + dev_set_drvdata(client-dev, encoder);
 +/* debug end */

The above should probably be dropped from this patch; that's for debugging
and is unrelated to the rest of this patch.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/i2c: tda998x: fix sync generation and calculation

On Tue, Aug 06, 2013 at 12:20:16AM +0200, Sebastian Hesselbarth wrote:
 + de_pix_s = mode-htotal - mode-hdisplay;
 + de_pix_e = de_pix_s + mode-hdisplay;
 + hs_pix_s = mode-hsync_start - mode-hdisplay;
 + hs_pix_e = hs_pix_s + mode-hsync_end - mode-hsync_start;

I still think the above is over-complicating the calculation and making it
less readable.

The values in 'mode' represent this timing representation:
0=hdshdehss  hse   ht
|-|-|---||

What we want to do is to rotate that around to this:
0 hss  hse   hds   ht=hde
|-|---||-|

From that, you can see quite clearly that the end of the displayed line
is now at htotal, and the start of the displayed line (hds) is hdisplay
clocks before that.  So:

de_pix_e = mode-htotal;
de_pix_s = de_pix_e - mode-hdisplay;

Everything else (hss, hse) has moved to the left by hdisplay clocks, so:

hs_pix_s = mode-hsync_start - mode-hdisplay;
hs_pix_e = mode-hsync_end - mode-hdisplay;

That's what you get if you simplify your calculations as well.  If we then
go back and look at the original code:

-   hs_start   = mode-hsync_start - mode-hdisplay;
-   hs_end = mode-hsync_end - mode-hdisplay;
-   de_start   = mode-htotal - mode-hdisplay;
-   de_end = mode-htotal;

it's what was originally there, so I don't see any point in touching that
calculation.

We also have:

-   ref_pix = 3 + hs_start;
+   ref_pix  = 3 + mode-hsync_start - mode-hdisplay;

which we can see is also the same calculation in essence.  I don't think
the change helps readability.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/i2c: tda998x: add video and audio input configuration

On Tue, Aug 06, 2013 at 12:20:15AM +0200, Sebastian Hesselbarth wrote:
 @@ -0,0 +1,23 @@
 +#ifndef __TDA998X_H__
 +#define __TDA998X_H__
 +
 +enum tda998x_audio_format {
 + AFMT_I2S,
 + AFMT_SPDIF,
 +};
 +
 +struct tda998x_encoder_params {
 + int audio_cfg;
 + int audio_clk_cfg;
 + enum tda998x_audio_format audio_format;
 + int audio_sample_rate;
 + char audio_frame[6];
 + int swap_a, mirr_a;
 + int swap_b, mirr_b;
 + int swap_c, mirr_c;
 + int swap_d, mirr_d;
 + int swap_e, mirr_e;
 + int swap_f, mirr_f;
 +};

You really should pick my version of this header file from the message
I sent earlier:

e1ulloe-00058q...@rmk-pc.arm.linux.org.uk
[PATCH RFC 7/8] drm/i2c: nxp-tda998x: add video and audio input 
configuration

if you're going to suggest that it's something I produced.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/i2c: tda998x: fix sync generation and calculation

On Tue, Aug 06, 2013 at 12:20:16AM +0200, Sebastian Hesselbarth wrote:
 This fixes the wrong sync generation and sync calculation of TDA998x
 for HS/VS-based sync detection.
 
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com

The plus point with this is that interlaced modes (1080i) do work with
the TDA998x again, so I think that the vertical calculations are
probably correct.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/8] Several NXP TDA998x patches

2013-08-15 Thread Russell King - ARM Linux

On Wed, Aug 14, 2013 at 09:43:25PM +0200, Sebastian Hesselbarth wrote:
 This patch set picks up several patches sent during the past months
 related with NXP TDA998x HDMI transmitter driver. The patches have
 been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
 with audio playing on S/PDIF.
 
 I bumped all patches to v2, although only patches 2, 5, and 6 have
 changed. I also fixed a typo in commit line of patch 8. As Darren
 Etheridge already gave his Tested-by and tilcdc related patches have
 not changed, I added it to all patches for v2, too.
 
 I have not added Russell King's Tested-by, as I hope he will have a
 close look at what I changed after his review comments.
 
 Darren Etheridge (1):
   drm/tilcdc fixup mode to workaround sync for tda998x
 
 Russell King (5):
   drm/i2c: tda998x: fix EDID reading on TDA19988 devices
   drm/i2c: tda998x: ensure VIP output mux is properly set
   drm/i2c: tda998x: fix npix/nline programming
   drm/i2c: tda998x: prepare for video input configuration
   drm/i2c: tda998x: add video and audio input configuration
 
 Sebastian Hesselbarth (2):
   drm/i2c: tda998x: fix sync generation and calculation
   drm/i2c: tda998x: prepare for broken sync workaround
 
  drivers/gpu/drm/i2c/tda998x_drv.c |  481 
 +++--
  drivers/gpu/drm/tilcdc/tilcdc_crtc.c  |7 +-
  drivers/gpu/drm/tilcdc/tilcdc_slave.c |   27 +-
  include/drm/i2c/tda998x.h |   30 ++
  4 files changed, 467 insertions(+), 78 deletions(-)
  create mode 100644 include/drm/i2c/tda998x.h
 
 ---
 Cc: David Airlie airl...@linux.ie
 Cc: Darren Etheridge detheri...@ti.com
 Cc: Rob Clark robdcl...@gmail.com
 Cc: Russell King rmk+ker...@arm.linux.org.uk
 Cc: Daniel Vetter daniel.vet...@ffwll.ch
 Cc: dri-devel@lists.freedesktop.org
 Cc: linux-ker...@vger.kernel.org

Okay, I'm happy with this set.  Patches 1-7 can get a:

Tested-by: Russell King rmk+ker...@arm.linux.org.uk

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

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-19 Thread Russell King - ARM Linux

On Mon, Aug 19, 2013 at 09:23:17AM +1000, Dave Airlie wrote:
 On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth
 sebastian.hesselba...@gmail.com wrote:
  From: Russell King rmk+ker...@arm.linux.org.uk
 
  This patch adds tda998x specific parameters to allow it to be configured
  for different boards using it. Also, this implements rudimentary audio
  support for S/PDIF attached controllers.
 
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
  Tested-by: Darren Etheridge detheri...@ti.com
  ---
 
 I've merged the series,

Thanks.

 this one generates a warning though:
   CC [M]  drivers/gpu/drm/i2c/tda998x_drv.o
 /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:
 In function ‘tda998x_encoder_mode_set’:
 /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
 warning: ‘clksel_fs’ may be used uninitialized in this function
 [-Wmaybe-uninitialized]
 /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30:
 note: ‘clksel_fs’ was declared here
 /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
 warning: ‘clksel_aip’ may be used uninitialized in this function
 [-Wmaybe-uninitialized]
 /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18:
 note: ‘clksel_aip’ was declared here
 
 It doesn't seem like a real problem, since the function is unlikely to
 be called any way to make that case happen.

Ok, I'll squash those warnings by a slight rearrangement of the code.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-21 Thread Russell King - ARM Linux

On Wed, Aug 21, 2013 at 08:33:55PM +0200, Jean-Francois Moine wrote:
 On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
  +static void
  +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int 
  cnt)
  +{
  +   struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
  +   uint8_t buf[cnt+1];
  +   int ret;
  +
  +   buf[0] = REG2ADDR(reg);
  +   memcpy(buf[1], p, cnt);
  +
  +   set_page(encoder, reg);
  +
  +   ret = i2c_master_send(client, buf, cnt + 1);
  +   if (ret  0)
  +   dev_err(client-dev, Error %d writing to 0x%x\n, ret, reg);
  +}
  +
 
 It seems simpler to reserve one byte in the caller buffer for the
 register address and avoid a memcpy.

And by doing so create an obtuse API.  No thanks.

  +static void
  +tda998x_write_aif(struct drm_encoder *encoder, struct 
  tda998x_encoder_params *p)
  +{
  +   uint8_t buf[PB(5) + 1];
  +
  +   buf[HB(0)] = 0x84;
  +   buf[HB(1)] = 0x01;
  +   buf[HB(2)] = 10;
 
 Why don't you use the constants which are defined in hdmi.h?

Because I was not aware of them.

  +   /* Write the CTS and N values */
  +   buf[0] = 0x44;
  +   buf[1] = 0x42;
  +   buf[2] = 0x01;
 
 The CTS value is strange, but that does not matter: its generation is
 automatic (see above).

Correct - however not strange, if you've read the HDMI specification.

  +   buf[3] = n;
  +   buf[4] = n  8;
  +   buf[5] = n  16;
  +   reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
  +
  +   /* Set CTS clock reference */
  +   reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
  +
  +   /* Reset CTS generator */
  +   reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
  +   reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
  +
  +   /* Write the channel status */
  +   buf[0] = 0x04;
  +   buf[1] = 0x00;
  +   buf[2] = 0x00;
  +   buf[3] = 0xf1;
  +   reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
   [snip]
 
 From what I understood in the NXP driver, buf[3] depends on the sample
 rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz.

Correct, but the driver has no way to know this.  The above reflects how
the NXP driver on the Cubox implements this (which we know works for
multiple different sample rates).  This is because we use SPDIF, which
encodes the sample rate in the channel status information, which is then
presumably passed through by the TDA998x.  I wouldn't know, I don't have
a HDMI debugger.

What I do know is that it works for multiple sample rates.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

2013-08-21 Thread Russell King - ARM Linux

On Wed, Aug 21, 2013 at 08:26:46PM +0200, Jean-Francois Moine wrote:
 On Wed Aug 14 12:43:29 PDT 2013, Sebastian Hesselbarth wrote:
  From: Russell King rmk+kernel at arm.linux.org.uk
  
  The video-input-port (VIP) is highly configurable. This prepares
  current driver to allow to configure VIP configuration, as some
  boards connect lcd controller and TDA998x pin-swapped and depend
  on VIP to swap the pins by register configuration.
  
  Signed-off-by: Russell King rmk+kernel at arm.linux.org.uk
  Tested-by: Darren Etheridge detheridge at ti.com
  Tested-by: Sebastian Hesselbarth sebastian.hesselbarth at gmail.com
   [snip]
 
 AFAIK, the TI boards have no pin-swapped, nor has the Cubox (there is
 no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
 of the Dove lcd for RGB or YUV formats).
 
 Which board needs a special VIP configuration?

If you run the NXP driver, and then run this driver, things get messed
up - which has already been covered months ago when this patch was first
brought up.

It's there to ensure that the TDA998x is correctly configured no matter
what it's previous state is, and prevent the thing being fragile as hell.
No, reset doesn't restore its settings, only a power cycle does.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

2013-08-22 Thread Russell King - ARM Linux

On Thu, Aug 22, 2013 at 08:53:13AM +0200, Jean-Francois Moine wrote:
 On Wed, 21 Aug 2013 23:36:05 +0100
 Russell King - ARM Linux li...@arm.linux.org.uk wrote:
 
   AFAIK, the TI boards have no pin-swapped, nor has the Cubox (there is
   no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
   of the Dove lcd for RGB or YUV formats).
   
   Which board needs a special VIP configuration?  
  
  If you run the NXP driver, and then run this driver, things get messed
  up - which has already been covered months ago when this patch was first
  brought up.
  
  It's there to ensure that the TDA998x is correctly configured no matter
  what it's previous state is, and prevent the thing being fragile as hell.
 
 The NXP driver will never go to the mainline, so, I don't see the
 problem. If you want to use it to test some other drivers, you should
 better patch it instead of adding useless code in the TDA998x driver.

Sorry, you're wrong.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

2013-08-22 Thread Russell King - ARM Linux

On Thu, Aug 22, 2013 at 07:33:43AM -0400, Rob Clark wrote:
 On Thu, Aug 22, 2013 at 2:53 AM, Jean-Francois Moine moin...@free.fr wrote:
  On Wed, 21 Aug 2013 23:36:05 +0100
  Russell King - ARM Linux li...@arm.linux.org.uk wrote:
 
   AFAIK, the TI boards have no pin-swapped, nor has the Cubox (there is
   no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
   of the Dove lcd for RGB or YUV formats).
  
   Which board needs a special VIP configuration?
 
  If you run the NXP driver, and then run this driver, things get messed
  up - which has already been covered months ago when this patch was first
  brought up.
 
  It's there to ensure that the TDA998x is correctly configured no matter
  what it's previous state is, and prevent the thing being fragile as hell.
 
  The NXP driver will never go to the mainline, so, I don't see the
  problem. If you want to use it to test some other drivers, you should
  better patch it instead of adding useless code in the TDA998x driver.
 
 I don't think it really matters for the end user if NXP isn't
 mainline.  If they are jumping between vendor kernel and mainline, and
 inheriting some state left over from the NXP driver in vendor kernel,
 it makes debugging very confusing.  It would be less of an issue if a
 warm reset actually reset the tda998x part, but that is not the case,
 it is better to rely less on the hw state when the driver is loaded,
 IMHO.

Absolutely right, thanks for backing up what I've said.  I've done exactly
that - switching between the NXP driver and the mainline driver to debug
other problems, and not having the TDA998x setup correctly just makes the
job much harder and time consuming.

I keep both drivers available in my internal git tree so that I can switch
between them when necessary.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)

2013-09-02 Thread Russell King - ARM Linux

On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote:
 From: Russell King rmk+ker...@arm.linux.org.uk
 
 This patch adds tda998x specific parameters to allow it to be configured
 for different boards using it. Also, this implements rudimentary audio
 support for S/PDIF attached controllers.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
 Tested-by: Darren Etheridge detheri...@ti.com
 ---

It looks like there's been a bug introduced in this patch (which wasn't
in my original).

 @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int 
 mode)
  
   switch (mode) {
   case DRM_MODE_DPMS_ON:
 - /* enable audio and video ports */
 - reg_write(encoder, REG_ENA_AP, 0xff);
 + /* enable video ports, audio will be enabled later */
   reg_write(encoder, REG_ENA_VP_0, 0xff);
   reg_write(encoder, REG_ENA_VP_1, 0xff);
   reg_write(encoder, REG_ENA_VP_2, 0xff);

I also disabled the writing to REG_ENA_AP in the DPMS off path as well,
which clears this register.

That seems to be missing from this patch, and it means that when the
display is placed into DPMS-off mode, the audio inputs are disabled,
never to be re-enabled.  There is no need to disable the audio input
in DPMS-off mode.

8=
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] drm/i2c: Fix broken TDA998x audio

In patch drm/i2c: tda998x: add video and audio input configuration in
its original version, disabling the audio input port was removed.  The
version which was submitted for merging had this change deleted, which
results in audio being non-functional.  Fix this by removing the write.
While here, update the comment too.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
--- 
 drivers/gpu/drm/i2c/tda998x_drv.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5742cfc..59878af 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_VIP_CNTRL_2, priv-vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
-   /* disable audio and video ports */
-   reg_write(encoder, REG_ENA_AP, 0x00);
+   /* disable video ports */
reg_write(encoder, REG_ENA_VP_0, 0x00);
reg_write(encoder, REG_ENA_VP_1, 0x00);
reg_write(encoder, REG_ENA_VP_2, 0x00);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 00/51] DMA mask changes

2013-09-20 Thread Russell King - ARM Linux

This started out as a request to look at the DMA mask situation, and how
to solve the issues which we have on ARM - notably how the DMA mask
should be setup.

However, I started off reviewing how the dma_mask and coherent_dma_mask
was being used, and what I found was rather messy, and in some cases
rather buggy.  I tried to get some of the bug fixes in before the last
merge window, but it seems that the maintainers preferred to have the
full solution rather than a simple -rc suitable bug fix.

So, this is an attempt to clean things up.

The first point here is that drivers performing DMA should be calling
dma_set_mask()/dma_set_coherent_mask() in their probe function to verify
that DMA can be performed.  Lots of ARM drivers omit this step; please
refer to the DMA API documentation on this subject.

What this means is that the DMA mask provided by bus code is a default
value - nothing more.  It doesn't have to accurately reflect what the
device is actually capable of.  Apart from the storage for dev-dma_mask
being initialised for any device which is DMA capable, there is no other
initialisation which is strictly necessary at device creation time.

Now, these cleanups address two major areas:
1. The setting of DMA masks, particularly when both the coherent and
   streaming DMA masks are set together.

2. The initialisation of DMA masks by drivers - this seems to be becoming
   a popular habbit, one which may not be entirely the right solution.
   Rather than having this scattered throughout the tree, I've pulled
   that into a central location (and called it coercing the DMA mask -
   because it really is about forcing the DMA mask to be that value.)

3. Finally, addressing the long held misbelief that DMA masks somehow
   correspond with physical addresses.  We already have established
   long ago that dma_addr_t values returned from the DMA API are the
   values which you program into the DMA controller, and so are the
   bus addresses.  It is _only_ sane that DMA masks are also bus
   related too, and not related to physical address spaces.

(3) is a very important point for LPAE systems, which may still have
less than 4GB of memory, but this memory is all located above the 4GB
physical boundary.  This means with the current model, any device
using a 32-bit DMA mask fails - even though the DMA controller is
still only a 32-bit DMA controller but the 32-bit bus addresses map
to system memory.  To put it another way, the bus addresses have a
4GB physical offset on them.

This email is only being sent to the mailing lists in question, not to
anyone personally.  The list of individuals is far to great to do that.
I'm hoping no mailing lists reject the patches based on the number of
recipients.

Patches based on v3.12-rc1.

 Documentation/DMA-API-HOWTO.txt   |   37 +--
 Documentation/DMA-API.txt |8 +++
 arch/arm/include/asm/dma-mapping.h|8 +++
 arch/arm/mm/dma-mapping.c |   49 ++--
 arch/arm/mm/init.c|   12 +++---
 arch/arm/mm/mm.h  |2 +
 arch/powerpc/kernel/vio.c |3 +-
 block/blk-settings.c  |8 ++--
 drivers/amba/bus.c|6 +--
 drivers/ata/pata_ixp4xx_cf.c  |5 ++-
 drivers/ata/pata_octeon_cf.c  |5 +-
 drivers/block/nvme-core.c |   10 ++---
 drivers/crypto/ixp4xx_crypto.c|   48 ++--
 drivers/dma/amba-pl08x.c  |5 ++
 drivers/dma/dw/platform.c |8 +--
 drivers/dma/edma.c|6 +--
 drivers/dma/pl330.c   |4 ++
 drivers/firmware/dcdbas.c |   23 +-
 drivers/firmware/google/gsmi.c|   13 +++--
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |6 ++-
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c  |5 +-
 drivers/media/platform/omap3isp/isp.c |6 +-
 drivers/media/platform/omap3isp/isp.h |3 -
 drivers/mmc/card/queue.c  |3 +-
 drivers/mmc/host/sdhci-acpi.c |5 +-
 drivers/net/ethernet/broadcom/b44.c   |3 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c  |8 +---
 drivers/net/ethernet/brocade/bna/bnad.c   |   13 ++
 drivers/net/ethernet/emulex/benet/be_main.c   |   12 +
 drivers/net/ethernet/intel/e1000/e1000_main.c |9 +---
 drivers/net/ethernet/intel/e1000e/netdev.c|   18 +++-
 drivers/net/ethernet/intel/igb/igb_main.c |   18 +++-
 drivers/net/ethernet/intel/igbvf/netdev.c |   18 +++-
 drivers/net/ethernet/intel/ixgb/ixgb_main.c   |   16 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

Re: [PATCH 42/51] DMA-API: usb: musb: use platform_device_register_full() to avoid directly messing with dma masks

On Fri, Sep 20, 2013 at 08:11:25AM -0500, Felipe Balbi wrote:
 Hi,
 
 On Fri, Sep 20, 2013 at 12:14:38AM +0100, Russell King wrote:
  Use platform_device_register_full() for those drivers which can, to
  avoid messing directly with DMA masks.  This can only be done when
  the driver does not need to access the allocated musb platform device
  from within its callbacks, which may be called during the musb
  device probing.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
 you want me to carry this one through my tree or you prefer getting my
 Acked-by ? Either way works for me:
 
 Acked-by: Felipe Balbi ba...@ti.com
 
 there's also the third option of me setting up a branch with only this
 patch and we both merge it, that'd also work.

I think this patch is sufficiently stand-alone that it should be fine
if you want to take it through your tree.  That may be better in the
long run to avoid conflicts with this patch and any future work in
this area during this cycle.

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

Re: [PATCH 01/51] DMA-API: provide a helper to set both DMA and coherent DMA masks

On Fri, Sep 20, 2013 at 02:21:37AM +0100, Ben Hutchings wrote:
 On Thu, 2013-09-19 at 22:25 +0100, Russell King wrote:
 [...]
  -dma_set_coherent_mask() will always be able to set the same or a
  -smaller mask as dma_set_mask(). However for the rare case that a
  +The coherent coherent mask will always be able to set the same or a
  +smaller mask as the streaming mask. However for the rare case that a
 [...]
 
 The new wording doesn't make sense; a mask doesn't set itself.  I would
 suggest:
 
 The coherent mask can always be set to the same or a smaller mask than
 the streaming mask.

Yes, the original sentence is not particularly good, but I think even
your modified version can be interpreted as a mask setting itself
for all the same reasons that the original can be (which mask does same
refer to?)

Even so, I prefer your version.  Thanks. :)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 24/51] DMA-API: dma: pl330: add dma_set_mask_and_coherent() call

On Fri, Sep 20, 2013 at 07:26:27PM +0200, Heiko Stübner wrote:
 Am Donnerstag, 19. September 2013, 23:49:01 schrieb Russell King:
  The DMA API requires drivers to call the appropriate dma_set_mask()
  functions before doing any DMA mapping.  Add this required call to
  the AMBA PL08x driver.
   ^--- copy and paste error - should of course be PL330

Fixed, thanks.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)

Ping?

On Mon, Sep 02, 2013 at 03:50:57PM +0100, Russell King - ARM Linux wrote:
 On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote:
  From: Russell King rmk+ker...@arm.linux.org.uk
  
  This patch adds tda998x specific parameters to allow it to be configured
  for different boards using it. Also, this implements rudimentary audio
  support for S/PDIF attached controllers.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
  Tested-by: Darren Etheridge detheri...@ti.com
  ---
 
 It looks like there's been a bug introduced in this patch (which wasn't
 in my original).
 
  @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int 
  mode)
   
  switch (mode) {
  case DRM_MODE_DPMS_ON:
  -   /* enable audio and video ports */
  -   reg_write(encoder, REG_ENA_AP, 0xff);
  +   /* enable video ports, audio will be enabled later */
  reg_write(encoder, REG_ENA_VP_0, 0xff);
  reg_write(encoder, REG_ENA_VP_1, 0xff);
  reg_write(encoder, REG_ENA_VP_2, 0xff);
 
 I also disabled the writing to REG_ENA_AP in the DPMS off path as well,
 which clears this register.
 
 That seems to be missing from this patch, and it means that when the
 display is placed into DPMS-off mode, the audio inputs are disabled,
 never to be re-enabled.  There is no need to disable the audio input
 in DPMS-off mode.
 
 8=
 From: Russell King rmk+ker...@arm.linux.org.uk
 Subject: [PATCH] drm/i2c: Fix broken TDA998x audio
 
 In patch drm/i2c: tda998x: add video and audio input configuration in
 its original version, disabling the audio input port was removed.  The
 version which was submitted for merging had this change deleted, which
 results in audio being non-functional.  Fix this by removing the write.
 While here, update the comment too.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 --- 
  drivers/gpu/drm/i2c/tda998x_drv.c |3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
 b/drivers/gpu/drm/i2c/tda998x_drv.c
 index 5742cfc..59878af 100644
 --- a/drivers/gpu/drm/i2c/tda998x_drv.c
 +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
 @@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int 
 mode)
   reg_write(encoder, REG_VIP_CNTRL_2, priv-vip_cntrl_2);
   break;
   case DRM_MODE_DPMS_OFF:
 - /* disable audio and video ports */
 - reg_write(encoder, REG_ENA_AP, 0x00);
 + /* disable video ports */
   reg_write(encoder, REG_ENA_VP_0, 0x00);
   reg_write(encoder, REG_ENA_VP_1, 0x00);
   reg_write(encoder, REG_ENA_VP_2, 0x00);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH 43/51] DMA-API: dma: edma.c: no need to explicitly initialize DMA masks

On Mon, Sep 23, 2013 at 03:55:33PM +0530, Vinod Koul wrote:
 On Fri, Sep 20, 2013 at 12:15:39AM +0100, Russell King wrote:
  register_platform_device_full() can setup the DMA mask provided the
  appropriate member is set in struct platform_device_info.  So lets
  make that be the case.  This avoids a direct reference to the DMA
  masks by this driver.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 Acked-by: Vinod Koul vinod.k...@intel.com
 
 This also brings me question that should we force the driver to use the
 dma_set_mask_and_coherent() API or they have below flexiblity too?

There's two issues here:
1. dma_set_mask_and_coherent() will only work if dev-dma_mask points at
   some storage for the mask.  This needs to have .dma_mask in the
   platform_device_info initialised.

2. Yes, this driver should also be calling the appropriate DMA mask setting
   functions in addition to having the mask initialized at device creation
   time.

Here's a replacement patch, though maybe it would be better to roll all
the additions of dma_set_mask_and_coherent() in drivers/dma into one
patch?  In other words, combine the addition of this with these two
patches:

dma: pl330: add dma_set_mask_and_coherent() call
dma: pl08x: add dma_set_mask_and_coherent() call

8=
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] DMA-API: dma: edma.c: no need to explicitly initialize DMA
 masks

register_platform_device_full() can setup the DMA mask provided the
appropriate member is set in struct platform_device_info.  So lets
make that be the case.  This avoids a direct reference to the DMA
masks by this driver.

While here, add the dma_set_mask_and_coherent() call which the DMA API
requires DMA-using drivers to call.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 drivers/dma/edma.c |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index ff50ff4..fd5e48c 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -631,6 +631,10 @@ static int edma_probe(struct platform_device *pdev)
struct edma_cc *ecc;
int ret;
 
+   ret = dma_set_mask_and_coherent(pdev-dev, DMA_BIT_MASK(32));
+   if (ret)
+   return ret;
+
ecc = devm_kzalloc(pdev-dev, sizeof(*ecc), GFP_KERNEL);
if (!ecc) {
dev_err(pdev-dev, Can't allocate controller\n);
@@ -702,11 +706,13 @@ static struct platform_device *pdev0, *pdev1;
 static const struct platform_device_info edma_dev_info0 = {
.name = edma-dma-engine,
.id = 0,
+   .dma_mask = DMA_BIT_MASK(32),
 };
 
 static const struct platform_device_info edma_dev_info1 = {
.name = edma-dma-engine,
.id = 1,
+   .dma_mask = DMA_BIT_MASK(32),
 };
 
 static int edma_init(void)
@@ -720,8 +726,6 @@ static int edma_init(void)
ret = PTR_ERR(pdev0);
goto out;
}
-   pdev0-dev.dma_mask = pdev0-dev.coherent_dma_mask;
-   pdev0-dev.coherent_dma_mask = DMA_BIT_MASK(32);
}
 
if (EDMA_CTLRS == 2) {
@@ -731,8 +735,6 @@ static int edma_init(void)
platform_device_unregister(pdev0);
ret = PTR_ERR(pdev1);
}
-   pdev1-dev.dma_mask = pdev1-dev.coherent_dma_mask;
-   pdev1-dev.coherent_dma_mask = DMA_BIT_MASK(32);
}
 
 out:
-- 
1.7.4.4


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

Re: [PATCH 36/51] DMA-API: usb: use dma_set_coherent_mask()

2013-09-24 Thread Russell King - ARM Linux

On Mon, Sep 23, 2013 at 02:27:39PM -0400, Alan Stern wrote:
 On Thu, 19 Sep 2013, Russell King wrote:
 
  The correct way for a driver to specify the coherent DMA mask is
  not to directly access the field in the struct device, but to use
  dma_set_coherent_mask().  Only arch and bus code should access this
  member directly.
  
  Convert all direct write accesses to using the correct API.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
  diff --git a/drivers/usb/host/ehci-platform.c 
  b/drivers/usb/host/ehci-platform.c
  index f6b790c..5b0cd2d 100644
  --- a/drivers/usb/host/ehci-platform.c
  +++ b/drivers/usb/host/ehci-platform.c
 
  @@ -91,8 +91,9 @@ static int ehci_platform_probe(struct platform_device 
  *dev)
  dev-dev.platform_data = ehci_platform_defaults;
  if (!dev-dev.dma_mask)
  dev-dev.dma_mask = dev-dev.coherent_dma_mask;
  -   if (!dev-dev.coherent_dma_mask)
  -   dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
  +   err = dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
  +   if (err)
  +   return err;
   
  pdata = dev_get_platdata(dev-dev);
 
 ehci-platform.c is a generic file, intended for use by multiple
 platforms.  Is it not possible that on some platforms, the arch or bus
 code may already have initialized the DMA masks?  Isn't something like 
 this (i.e., DMA bindings) planned for Device Tree?
 
 By eliminating the tests above and calling dma_set_coherent_mask or
 dma_coerce_mask_and_coherent unconditionally, this patch (and the next)
 would overwrite those initial settings.
 
 I don't know to what extent the same may be true for the other,
 platform-specific, drivers changed by this patch.  But it's something 
 to be aware of.

Please check the DMA API documentation:

=
For correct operation, you must interrogate the kernel in your device
probe routine to see if the DMA controller on the machine can properly
support the DMA addressing limitation your device has.  It is good
style to do this even if your device holds the default setting,
because this shows that you did think about these issues wrt. your
device.

The query is performed via a call to dma_set_mask():

int dma_set_mask(struct device *dev, u64 mask);

The query for consistent allocations is performed via a call to
dma_set_coherent_mask():

int dma_set_coherent_mask(struct device *dev, u64 mask);
=

So, All drivers which use DMA are supposed to issue the appropriate
calls to check the DMA masks before they perform DMA, even if the
default is correct.

And yes, this is all part of sorting out the DT mess - we should
start not from the current mess (which is really totally haphazard)
but from the well established position of how the DMA API _should_
be used.  What that means is that all drivers should be issuing
these calls as appropriate today.

The default mask setup when the device is created is just that -
it's a default mask, and it should not be used to decide anything
about the device.  That's something which the driver should compute
on its own accord and then inform the various other layers via the
appropriate call.

Remember, on PCI, even when we have 64-bit, we do not declare PCI
devices with a 64-bit DMA mask: that's up to PCI device drivers to
_try_ to set a 64-bit DMA mask, which when successful _allows_ them
to use 64-bit DMA.  If it fails, they have to only use 32-bit.  If
they want a smaller mask, the _driver_ has to set the smaller mask,
not the device creating code.

The reason we're into this (particularly on ARM) is that we got lazy
because we could get by with declaring a DMA mask at device creation
time, since all devices were statically declared.  Now it's time to
get rid of those old lazy ways and start doing things correctly and
to the requirements of the APIs.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 28/51] DMA-API: sound: fix dma mask handling in a lot of drivers

2013-09-26 Thread Russell King - ARM Linux

On Thu, Sep 26, 2013 at 09:51:23AM +0200, Takashi Iwai wrote:
 Hi,
 
 sorry for the lat response, as I've been traveling in the last weeks.
 
 At Thu, 19 Sep 2013 22:53:02 +0100,
 Russell King wrote:
  
  This code sequence is unsafe in modules:
  
  static u64 mask = DMA_BIT_MASK(something);
  ...
  if (!dev-dma_mask)
  dev-dma_mask = mask;
  
  as if a module is reloaded, the mask will be pointing at the original
  module's mask address, and this can lead to oopses.  Moreover, they
  all follow this with:
  
  if (!dev-coherent_dma_mask)
  dev-coherent_dma_mask = mask;
  
  where 'mask' is the same value as the statically defined mask, and this
  bypasses the architecture's check on whether the DMA mask is possible.
  
  Fix these issues by using the new dma_coerce_coherent_and_mask()
  function.
  
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 
 Applied with Mark's ack now.

Which is a very stupid thing to do because you won't have
dma_coerce_coherent_and_mask() in your tree, so all these drivers
will fail to build for you.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 39/51] DMA-API: others: use dma_set_coherent_mask()

2013-09-26 Thread Russell King - ARM Linux

On Thu, Sep 26, 2013 at 04:21:36PM +0530, Archit Taneja wrote:
 Hi,

 On Friday 20 September 2013 04:41 AM, Russell King wrote:
 The correct way for a driver to specify the coherent DMA mask is
 not to directly access the field in the struct device, but to use
 dma_set_coherent_mask().  Only arch and bus code should access this
 member directly.

 Convert all direct write accesses to using the correct API.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
   drivers/ata/pata_ixp4xx_cf.c |5 -
   drivers/gpu/drm/exynos/exynos_drm_drv.c  |6 +-
   drivers/gpu/drm/omapdrm/omap_dmm_tiler.c |5 +++--
   3 files changed, 12 insertions(+), 4 deletions(-)

 diff --git a/drivers/ata/pata_ixp4xx_cf.c b/drivers/ata/pata_ixp4xx_cf.c
 index 1ec53f8..ddf470c 100644
 --- a/drivers/ata/pata_ixp4xx_cf.c
 +++ b/drivers/ata/pata_ixp4xx_cf.c
 @@ -144,6 +144,7 @@ static int ixp4xx_pata_probe(struct platform_device 
 *pdev)
  struct ata_host *host;
  struct ata_port *ap;
  struct ixp4xx_pata_data *data = dev_get_platdata(pdev-dev);
 +int ret;

  cs0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  cs1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 @@ -157,7 +158,9 @@ static int ixp4xx_pata_probe(struct platform_device 
 *pdev)
  return -ENOMEM;

  /* acquire resources and fill host */
 -pdev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
 +ret = dma_set_coherent_mask(pdev-dev, DMA_BIT_MASK(32));
 +if (ret)
 +return ret;

  data-cs0 = devm_ioremap(pdev-dev, cs0-start, 0x1000);
  data-cs1 = devm_ioremap(pdev-dev, cs1-start, 0x1000);
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
 b/drivers/gpu/drm/exynos/exynos_drm_drv.c
 index bb82ef7..81192d0 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
 @@ -286,7 +286,11 @@ static struct drm_driver exynos_drm_driver = {

   static int exynos_drm_platform_probe(struct platform_device *pdev)
   {
 -pdev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
 +int ret;
 +
 +ret = dma_set_coherent_mask(pdev-dev, DMA_BIT_MASK(32));
 +if (ret)
 +return ret;

  return drm_platform_init(exynos_drm_driver, pdev);
   }
 diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c 
 b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
 index acf6678..701c4c1 100644
 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
 +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
 @@ -664,8 +664,9 @@ static int omap_dmm_probe(struct platform_device *dev)
  }

  /* set dma mask for device */
 -/* NOTE: this is a workaround for the hwmod not initializing properly */
 -dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
 +ret = dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
 +if (ret)
 +goto fail;

 Tested with omapdrm on omap4 panda es board.

Just wish to make sure - that's code for:

Tested-by: Archit Taneja arc...@ti.com

to be added?

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

Re: [PATCH 00/51] DMA mask changes

2013-09-29 Thread Russell King - ARM Linux

On Thu, Sep 26, 2013 at 10:23:08PM +0200, Rafał Miłecki wrote:
 2013/9/19 Russell King - ARM Linux li...@arm.linux.org.uk:
  This email is only being sent to the mailing lists in question, not to
  anyone personally.  The list of individuals is far to great to do that.
  I'm hoping no mailing lists reject the patches based on the number of
  recipients.
 
 Huh, I think it was enough to send only 3 patches to the b43-dev@. Like:
 [PATCH 01/51] DMA-API: provide a helper to set both DMA and coherent DMA masks
 [PATCH 14/51] DMA-API: net: b43: (...)
 [PATCH 15/51] DMA-API: net: b43legacy: (...)
 ;)
 
 I believe Joe has some nice script for doing it that way. When fixing
 some coding style / formatting, he sends only related patches to the
 given ML.

If I did that, then the mailing lists would not get the first patch,
because almost none of the lists would be referred to by patch 1.

Moreover, people complain when they don't have access to the full
patch series - they assume patches are missing for some reason, and
they ask for the rest of the series.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFCv1 12/12] ARM: add get_user() support for 8 byte types

On Sun, Oct 06, 2013 at 10:09:34AM -0400, Rob Clark wrote:
 On Sun, Oct 6, 2013 at 4:53 AM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Sat, Oct 05, 2013 at 08:45:50PM -0400, Rob Clark wrote:
  A new atomic modeset/pageflip ioctl being developed in DRM requires
  get_user() to work for 64bit types (in addition to just put_user()).
 
  v1: original
  v2: pass correct size to check_uaccess, and better handling of narrowing
  double word read with __get_user_xb() (Russell King's suggestion)
 
  I thought we had decided not to support this for 32-bit because x86_32
  had problems here as well:
 
 I don't remember.. actually I didn't even realize it wasn't merged
 until I tried to build with the atomic modeset ioctl.
 
 Ville did have a patch to add it for x86_32, and that patch appears to
 be merged (96477b4c), so I just assume it wasn't removed since then.

Okay, the code I quoted is for __get_user() not get_user().

This is roughly what we (hpa/myself) came up with for ARM which did get
things working, but itw as x86 which made us decide not to persue this.
I've added the __get_user_8 implementation to this.  The __inttype()
thing was hpa's idea.

 arch/arm/include/asm/uaccess.h |   17 +
 arch/arm/lib/getuser.S |   33 -
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 72abdc5..747f2cb 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -93,6 +93,9 @@ static inline void set_fs(mm_segment_t fs)
: cc); \
flag; })
 
+#define __inttype(x) \
+   __typeof__(__builtin_choose_expr(sizeof(x)  sizeof(0UL), 0ULL, 0UL))
+
 /*
  * Single-value transfer routines.  They automatically use the right
  * size if we just have the right pointer type.  Note that the functions
@@ -107,14 +110,16 @@ static inline void set_fs(mm_segment_t fs)
 extern int __get_user_1(void *);
 extern int __get_user_2(void *);
 extern int __get_user_4(void *);
+extern int __get_user_8(void *);
 
-#define __GUP_CLOBBER_1lr, cc
+#define __GUP_CLOBBER_1 lr, cc
 #ifdef CONFIG_CPU_USE_DOMAINS
-#define __GUP_CLOBBER_2ip, lr, cc
+#define __GUP_CLOBBER_2 ip, lr, cc
 #else
 #define __GUP_CLOBBER_2 lr, cc
 #endif
-#define __GUP_CLOBBER_4lr, cc
+#define __GUP_CLOBBER_4 lr, cc
+#define __GUP_CLOBBER_8 lr, cc
 
 #define __get_user_x(__r2,__p,__e,__l,__s) \
   __asm__ __volatile__ (   \
@@ -129,7 +134,7 @@ extern int __get_user_4(void *);
({  \
unsigned long __limit = current_thread_info()-addr_limit - 1; \
register const typeof(*(p)) __user *__p asm(r0) = (p);\
-   register unsigned long __r2 asm(r2);  \
+   register __inttype(*p) __r2 asm(r2);  \
register unsigned long __l asm(r1) = __limit; \
register int __e asm(r0); \
switch (sizeof(*(__p))) {   \
@@ -142,6 +147,9 @@ extern int __get_user_4(void *);
case 4: \
__get_user_x(__r2, __p, __e, __l, 4);   \
break;  \
+   case 8: \
+   __get_user_x(__r2, __p, __e, __l, 8);   \
+   break;  \
default: __e = __get_user_bad(); break; \
}   \
x = (typeof(*(p))) __r2;\
@@ -150,6 +158,7 @@ extern int __get_user_4(void *);
 
 #define get_user(x,p)  \
({  \
+   __chk_user_ptr(ptr);\
might_fault();  \
__get_user_check(x,p);  \
 })
diff --git a/arch/arm/lib/getuser.S b/arch/arm/lib/getuser.S
index 9b06bb4..3583c83 100644
--- a/arch/arm/lib/getuser.S
+++ b/arch/arm/lib/getuser.S
@@ -18,7 +18,7 @@
  * Inputs: r0 contains the address
  * r1 contains the address limit, which must be preserved
  * Outputs:r0 is the error code
- * r2 contains the zero-extended value
+ * r2/r3 contains the zero-extended value
  * lr corrupted
  *
  * No other registers must be altered.  (see asm/uaccess.h
@@ -32,6 +32,14 @@
 #include asm/errno.h
 #include asm/domain.h
 
+#ifdef __ARMEB__
+#define rlo8

[PATCH 0/5] Armada DRM stuff

The Armada DRM drivers again, along with the TDA998x HDMI output support,
and an illustration to show how to add Armada 610 support (and others.)

Rob has looked at this a couple of times since its last posting, and
has provided additional useful feedback which has been incorporated.
I believe all the major issues have been addressed now.

 drivers/gpu/drm/Kconfig |2 +
 drivers/gpu/drm/Makefile|1 +
 drivers/gpu/drm/armada/Kconfig  |   24 +
 drivers/gpu/drm/armada/Makefile |8 +
 drivers/gpu/drm/armada/armada_510.c |   87 +++
 drivers/gpu/drm/armada/armada_610.c |   49 ++
 drivers/gpu/drm/armada/armada_crtc.c| 1099 +++
 drivers/gpu/drm/armada/armada_crtc.h|   83 +++
 drivers/gpu/drm/armada/armada_debugfs.c |  183 +
 drivers/gpu/drm/armada/armada_drm.h |  114 
 drivers/gpu/drm/armada/armada_drv.c |  428 
 drivers/gpu/drm/armada/armada_fb.c  |  170 +
 drivers/gpu/drm/armada/armada_fb.h  |   24 +
 drivers/gpu/drm/armada/armada_fbdev.c   |  202 ++
 drivers/gpu/drm/armada/armada_gem.c |  616 +
 drivers/gpu/drm/armada/armada_gem.h |   52 ++
 drivers/gpu/drm/armada/armada_hw.h  |  326 +
 drivers/gpu/drm/armada/armada_ioctlP.h  |   18 +
 drivers/gpu/drm/armada/armada_output.c  |  158 +
 drivers/gpu/drm/armada/armada_output.h  |   39 ++
 drivers/gpu/drm/armada/armada_overlay.c |  477 ++
 drivers/gpu/drm/armada/armada_slave.c   |  139 
 drivers/gpu/drm/armada/armada_slave.h   |   26 +
 drivers/gpu/drm/i2c/tda998x_drv.c   |2 +
 include/drm/drm_crtc.h  |   17 +
 include/uapi/drm/armada_drm.h   |   45 ++
 26 files changed, 4389 insertions(+), 0 deletions(-)

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

Re: [PATCH 3/5] DRM: Armada: Add support for ARGB 32x64 or 64x32 hardware cursors

On Mon, Oct 07, 2013 at 12:09:22PM +0200, Jean-Francois Moine wrote:
 On Mon, 7 Oct 2013 10:40:08 +0100
 Russell King - ARM Linux li...@arm.linux.org.uk wrote:
 
This patch adds ARGB hardware cursor support to the DRM driver for the
Marvell Armada SoCs.  ARGB cursors are supported at either 32x64 or
64x32 resolutions.  
 [snip]
   
   I don't see the interest of such cursors. Actually, most often, the
   cursors are 64x64 and 'A' is either 0 or 0xff. As the Armada 510
   supports 64x64 cursors with transparency, it would be more useful to
   implement these ones...  
  
  Sorry, you're completely wrong there.  Xorg uses an alphablended cursor.
  This patch is a result of trialling each of the Armada's cursor options
  and this is the only one which results in a reasonable looking cursor.
 
 Strange. I am using the 64x64 cursor with transparency of the 510 for
 many months and I never saw any problem.

When I tried it, all cursors had variable alpha, and converting the
alpha to a single transparency bit made the cursor look rubbish against
certain backgrounds.

 If you absolutely want to have all transparency shades, you should
 accept 64x64 cursors and test if they may be rendered as 64x32 or
 32x64, i.e. test that there is only pure transparency in the non-
 rendered rectangles...

That is policy, and that is something which can be done by the X server
rather than the kernel.  The kernel exposes the hardware capabilities,
which are to allow a 64x32 or a 32x64 cursor.  Userspace can make the
decision dynamically which it wishes to use.

However, what you're suggesting is dangerous.  What do you do when you're
presented with a cursor which is 64x64 ?  Do you:

(a) not display it
(b) crash the X server

There isn't a fallback to using software cursors available in the X server
because there's no error reporting for a failed hardware cursor update.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] DRM: Armada: add support for drm tda19988 driver

On Mon, Oct 07, 2013 at 12:48:20PM +0200, Jean-Francois Moine wrote:
 On Mon, 7 Oct 2013 10:44:04 +0100
 Russell King - ARM Linux li...@arm.linux.org.uk wrote:
 
  On Mon, Oct 07, 2013 at 11:18:07AM +0200, Jean-Francois Moine wrote:
   [snip]
   It seems we are going backwards: as the Armada based boards will soon
   move to full DT (mvebu), you are making an exception for the Cubox, so
   that there should be Cubox specific kernels. I don't like that...
  
  *Ignored*.  You know why.
 
 Sorry. I don't see why. May you explain again?

I don't run DT because DT lacks most of the features I require on the
cubox.  Therefore I can't develop for DT.  Simple.

diff --git a/drivers/gpu/drm/armada/armada_drv.c 
b/drivers/gpu/drm/armada/armada_drv.c
   [snip]
+static struct tda998x_encoder_params params = {
+   /* With 0x24, there is no translation between vp_out and int_vp
+   FB  LCD out PinsVIP Int Vp
+   R:23:16 R:7:0   VPC7:0  7:0 7:0[R]
+   G:15:8  G:15:8  VPB7:0  23:16   23:16[G]
+   B:7:0   B:23:16 VPA7:0  15:815:8[B]
+   */
+   .swap_a = 2,
+   .swap_b = 3,
+   .swap_c = 4,
+   .swap_d = 5,
+   .swap_e = 0,
+   .swap_f = 1,
   
   I still don't agree. You don't need to invert R - B for the Cubox at
   the tda998x level: this may be done setting as it should be the
   CFG_GRA_SWAPRB flag of the lcd register LCD_SPU_DMA_CTRL0.
  
  You are totally and utterly wrong there.  We need R and B presented on
  their correct lanes to the TDA998x so that the Armadas YUV-RGB
  conversion works.  Setting CFG_GRA_SWAPRB does not swap the YUV output
  to match, neither does setting any of the other bits.
  
  CFG_GRA_SWAPRB is all about the _graphics_ _framebuffer_ format, it's got
  nothing to do at all with how the output is wired.
 
 Then, may you explain why you must swap R/B on simple RGB output?
 
 From your [PATCH 2/5] DRM: Armada: Add Armada DRM driver:
 
 + FMT(RGB888, 888PACK,CFG_SWAPRB);
 + FMT(BGR888, 888PACK,0);

I think I explained above.  This is to do with the *graphics* *framebuffer*
*format* in *memory*.  This has nothing to do with how the hardware is
wired up.

With no swapping, the 888 format is defined by the documentation to be:

[7:0] = red
[15:8] = green
[23:16] = blue

Now, if you look this up in the drm_fourcc.h header file, this corresponds
with this entry:

#define DRM_FORMAT_BGR888   fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R 
little endian */

Hence, this is the BGR888 format with no swapping.  Therefore, my second
line is correct.

With CFG_SWAPRB set, the format in hardware becomes:

[7:0] = blue
[15:8] = green
[23:16] = red

Again, if you look this up in the drm_fourcc.h header, you get:

#define DRM_FORMAT_RGB888   fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B 
little endian */

So, the two entries you quote above are 100% correct.

 + FMT(YUYV,   422PACK,CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV);

Where is the RGB swap there?  Again, this is to do with swapping the
format to match the hardware.  Please read the documentation on the
framebuffer formats.

The native 422PACK format is:
[7:0] = U
[15:8] = Y0
[23:16] = V
[31:24] = Y1

and we need to set both CFG_SWAPYU and CFG_SWAPUV to get this to be:
[7:0] = V
[15:8] = Y1
[23:16] = U
[31:24] = Y0

which is what the YUYV format wants.

 Rabeeh did the most he could to have a working Cubox. He used bad
 written drivers and he had not the time to think about how the drivers
 could be enhanced.

What does I2S give us in terms of enhancement which SPDIF does not,
remembering that audio is transmitted over HDMI in a format which closely
resembles SPDIF (but with a 28-bit subframe size.)

 Here is a small story about i2s/spdif: once, I set the tda998x to use
 the spdif input, and at this time, I was using the dummy codec.
 This codec accepts the format 32_LE, as does the audio device, but the
 output cannot go to spdif. Result: no hdmi sound.

How ASoC works in this regard is that the capabilities are the _union_ of
the codec and the cpu DAIs.

The SPDIF transmitter codec supports 16, 20 and 24 bits per sample but not
32.  Quite simply, that's because the SPDIF format does not allow 32-bits
per sample.  Therefore, 32_LE is not available for use with audio output,
and userspace must convert down to something which the hardware does
support.

Because I2S can support 32-bit audio is a poor reason, because you can't
pass 32-bit audio via HDMI due to a subframe being limited to 28 bits - up
to 24 bits for the sample and 4 bits of control/status information.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] DRM: Armada: add support for drm tda19988 driver

On Mon, Oct 07, 2013 at 02:03:27PM +0200, Jean-Francois Moine wrote:
 On Mon, 7 Oct 2013 12:09:02 +0100
 Russell King - ARM Linux li...@arm.linux.org.uk wrote:
 
   Here is a small story about i2s/spdif: once, I set the tda998x to use
   the spdif input, and at this time, I was using the dummy codec.
   This codec accepts the format 32_LE, as does the audio device, but the
   output cannot go to spdif. Result: no hdmi sound.  
  
  How ASoC works in this regard is that the capabilities are the _union_ of
  the codec and the cpu DAIs.
  
  The SPDIF transmitter codec supports 16, 20 and 24 bits per sample but not
  32.  Quite simply, that's because the SPDIF format does not allow 32-bits
  per sample.  Therefore, 32_LE is not available for use with audio output,
  and userspace must convert down to something which the hardware does
  support.
  
  Because I2S can support 32-bit audio is a poor reason, because you can't
  pass 32-bit audio via HDMI due to a subframe being limited to 28 bits - up
  to 24 bits for the sample and 4 bits of control/status information.
 
 OK. So, to resume, if both i2s and spdif are furnished by the audio
 subsystem, the tda998x must use spdif only (with the spdif codec).

This is going off-topic because the TDA998x bindings are a subject for
the TDA998x driver, not the Armada driver.  This data structure merely
exists to provide the required data for the TDA998x in absence of DT
information.

To do the job properly - which is what should be done _whenever_ DT
bindings are being considered - you have to look at the hardware with
an overview of its capabilities, never at the implementation.

So, the TDA998x family needs to be looked at in isolation, and its DT
bindings need to be considered on its own merit.

The TDA998x can accept a SPDIF signal on a single pin (on AP6), with or
without a clock (on AP5), or it can accept I2S: I2S clock in APCLK, I2S
word select on AP0, and up to four I2S streams on AP1..4 and auxillary
(internal test) data in AP7.  AP7 should therefore not be used.

The TDA19988 is slightly different, because it doesn't have soo many
AP pins.  Here, SPDIF can be supplied on either AP1 or AP2 with the
optional clock on AP3.  I2S is similar to the above but only AP1 and
AP2 accept streams.

I think the I2S is a special case - I'm not aware of anything which
outputs _four_ synchronised I2S data streams with a common word select.
Nevertheless, it's something that needs to be considered when creating
DT bindings.

So, we have some quite different capabilities depending on the device.
For I2S, we can get away with this:

tda998x {
compatible = nxp,tda19988, nxp,tda998x;
i2s-source = i2s_source N;
};

where N is the number of coherent I2S streams supplied.  For the TDA998x,
N can be 1 to 4.  For TDA19988, N can be 1 or 2 but no more.

For SPDIF, the situation is more complex.  The TDA19988 could in theory
have two streams connected, and one of those streams can be selected.
So, maybe:

tda998x {
compatible = nxp,tda998x;
spdif-source = spdif_source;
spdif-has-mclk;
};

and for the 19988:

tda19988 {
compatible = nxp,tda19988;
spdif0-source = spdif_source0;
spdif0-has-mclk;
spdif1-source = spdif_source1;
spdif1-has-mclk;
};

 Also, from your video swap explanation, the DT will contain some video
 property(ies) TBD.

The TDA998x needs some properties to define how it is connected to the
rest of the hardware - which pins are connected to what in terms of the
colour channel information.  As I hope I've made clear to you, this has
no bearing on the settings in the Dove for the SWAPRB bits.  This is
purely only to do with the TDA998x.

So, this also needs to be presented as properties for the TDA998x driver.

 The remaining question is: do we need more audio parameters?
 
 - the audio_cfg value is defined by i2s/spdif (3 or 4)

This should be discovered from the DT binding information, because which
AP pins get used depends on the signals being supplied to it.

 - the audio_frame[1] value is always 1 (2 channels)
 - audio_sample_rate is useless

audio_sample_rate comes in if we wish to use a fixed sample rate and
have coherent video and audio clocks derived from a common source.  That
allows fixed CTS and N values to be used based on the audio sample rate
being supplied as the clocks have a fixed known relationship (and aren't
going to drift.)

However, you are right that this should not be specified in DT - it
should come from the audio subsystem.  However, that's something which
is a very very long way away from being possible (if ever) with Linux
since I don't see the DPCM issues ever getting resolved.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] DRM: Armada: Add support for ARGB 32x64 or 64x32 hardware cursors

On Mon, Oct 07, 2013 at 03:29:15PM +0300, Siarhei Siamashka wrote:
On Mon, 7 Oct 2013 11:32:50 +0100
Russell King - ARM Linux li...@arm.linux.org.uk wrote:
However, what you're suggesting is dangerous. What do you do when you're
presented with a cursor which is 64x64 ? Do you:

(a) not display it
(b) crash the X server

There isn't a fallback to using software cursors available in the X server
because there's no error reporting for a failed hardware cursor update.

Hmm, maybe I'm missing something, but doesn't returning FALSE from
UseHWCursorARGB just make the X server fallback to using a software
cursor?

https://github.com/ssvb/xf86-video-fbturbo/blob/689c08256555/src/sunxi_disp_hwcursor.c#L72

I'm just doing this. And also have hooks to notify the DRI2 code about
the status of the cursor. In the case if the hardware cursor is not
enabled, hardware overlays can't be safely used with the software
cursor and we need to fallback to CPU/GPU copy instead of zero-copy
buffers swapping.

And I fully agree that it is the responsibility of the kernel to expose
as much of the hardware capabilities as possible (without compromising
reliability and/or security).

If you wish to go in below the level presented by
hw/xfree86/modes/xf86Cursors.c (as you are doing), then yes you can do
this, and it's something which maybe should be looked into. You have a
good reason to do this (due to your hardware cursor being incompatible
with overlays).

We have no such restriction - the only issue here is the odd size of
cursors. Practically, choosing one or other of 64x32 or 32x64 works
fine for the cases I've seen. I haven't seen any cursors which are
64x64 yet. I'm not saying they don't exist though! I can imagine that
some accessibility options might want to display a large cursor.

Now, the way I handle this is that the kernel allows _either_ a
(1 to 32)x(1 to 64) or (1 to 64)x(1 to 32) cursor. Both width and
height larger than 32 gets rejected with -EINVAL:

+ /* maximum size is 64x32 or 32x64 */
+ if (w 64 || h 64 || (w 32 h 32))
+ return -ENOMEM;

which is quite reasonable for this hardware. However, there is no way
to export this from DRM - adding maximum cursor size properties wouldn't
really represent this.

The only question is whether DRM should export some capabilities to
indicate what's possible with the cursor, so that X knows about this
quirk of this hardware. Overall, I suspect that it's just rare and
not worth the effort. I think it's just easier to pick one of these
and stick with it, and if we happen to encounter a cursor larger than
our chosen dimentions, XFree86 will already automatically fall back
to s/w cursor.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] Armada DRM stuff

2013-10-09 Thread Russell King - ARM Linux

On Mon, Oct 07, 2013 at 11:47:55PM +0200, Sebastian Hesselbarth wrote:
 On 10/07/2013 12:07 AM, Russell King - ARM Linux wrote:
 The Armada DRM drivers again, along with the TDA998x HDMI output support,
 and an illustration to show how to add Armada 610 support (and others.)

 Rob has looked at this a couple of times since its last posting, and
 has provided additional useful feedback which has been incorporated.
 I believe all the major issues have been addressed now.

 Tested-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com

 on Marvell Armada 510, SolidRun CuBox with quick-hack DT support added.

 Let's please get this driver mainlined and start working on proper DT
 support for it. Thanks for driver and the patience to work out all
 issues!

Sebastian,

I've added your tested-by now, and also updated the TDA998x patch to use
the HDMI definitions that Jean-Francois pointed out (which is a really
small delta, so isn't worth resending the patch set just for that).
Thanks.

All the points brought up in previous reviews concerning the physical
address exposure, and the location of the DRM helpers have all been
addressed, so it is now ready to be merged into mainline.  So now the
question is how to proceed with it.

However, I see no response from any DRM developers, even though they are
around - they've been responding on a different thread about driver
model/sysfs abuse by DRM.

I'm going to pull it into my nightly build tree so it can have some
testing with non-dove builds, and if there's still no response from
DRM people next week, I'll put it in to linux-next via my tree so at
least it can get some exposure and integration validation there.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] DRM: Armada: Add Armada DRM driver

2013-10-11 Thread Russell King - ARM Linux

On Thu, Oct 10, 2013 at 05:25:15PM -0400, Rob Clark wrote:
 On Sun, Oct 6, 2013 at 6:08 PM, Russell King
 rmk+ker...@arm.linux.org.uk wrote:
  +   /*
  +* If we have an overlay plane associated with this CRTC, disable
  +* it before the modeset to avoid its coordinates being outside
  +* the new mode parameters.  DRM doesn't provide help with this.
  +*/
 
 Getting a bit off topic, but yeah.. I'd be in favor of clarifying
 things by having crtc helpers disable planes on setcrtc (rather than
 only doing this on fbdev/lastclose restore).  I
 don't know of any userspace that this would cause problems for.  But
 not really sure if it would be considered an interface break or just
 fixing a bug.. since we have different behavior on different
 drivers, I'd lean towards the latter.

The reasoning here is if the overlay plane is larger than the mode being
set, we will end up having the hardware programmed in an inconsistent
state - the overlay plane position/width/height can be outside of the
active region, which is invalid for the hardware.  So, it's safer to
disable the plane and wait for the next plane to be sent and have that
validated against the new mode.

It's not like you would be able to see the plane immediately, because
most monitors blank while they retrain to the new mode settings.

  +static int armada_gem_vm_fault(struct vm_area_struct *vma, struct vm_fault 
  *vmf)
  +{
  +   struct armada_gem_object *obj = 
  drm_to_armada_gem(vma-vm_private_data);
  +   unsigned long addr = (unsigned long)vmf-virtual_address;
  +   unsigned long pfn = obj-phys_addr  PAGE_SHIFT;
  +   int ret;
  +
  +   pfn += (addr - vma-vm_start)  PAGE_SHIFT;
  +   ret = vm_insert_pfn(vma, addr, pfn);
  +
  +   switch (ret) {
  +   case -EIO:
  +   case -EAGAIN:
  +   set_need_resched();
 
 probably this thread is applicable here too?
 
 https://lkml.org/lkml/2013/9/12/417
 
 (although.. we have at least a few  slightly differet variants on the
 same errno - VM_FAULT_x switch in different drivers.. maybe we should
 do something better)

Hmm.  It seems today's vm_insert_pfn() has the following error return
values:

-EFAULT - virtual address outside vma
-EINVAL - track_pfn_insert() failure
-ENOMEM - failed to get locked pte
-EBUSY - entry already exists in pte

So it seems my handling -EIO, -EAGAIN, -ERESTARTSYS and -EINTR are all
redundant and can be removed.  I'm not sure if it makes sense for this
to be generic - it looks like it's only Intel, gma500 and me who use
this, and Intel handles more error codes (due to it calling other
functions.)

I'll respin dropping those four unnecessary error codes, and post the
delta for this.

  +/* Prime support */
  +struct sg_table *
  +armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
  +   enum dma_data_direction dir)
  +{
  +   struct drm_gem_object *obj = attach-dmabuf-priv;
  +   struct armada_gem_object *dobj = drm_to_armada_gem(obj);
  +   struct scatterlist *sg;
  +   struct sg_table *sgt;
  +   int i, num;
  +
  +   sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
  +   if (!sgt)
  +   return NULL;
  +
  +   if (dobj-obj.filp) {
  +   struct address_space *mapping;
  +   gfp_t gfp;
  +   int count;
  +
  +   count = dobj-obj.size / PAGE_SIZE;
  +   if (sg_alloc_table(sgt, count, GFP_KERNEL))
  +   goto free_sgt;
  +
  +   mapping = file_inode(dobj-obj.filp)-i_mapping;
  +   gfp = mapping_gfp_mask(mapping);
  +
  +   for_each_sg(sgt-sgl, sg, count, i) {
  +   struct page *page;
  +
  +   page = shmem_read_mapping_page_gfp(mapping, i, gfp);
 
 you could almost use drm_gem_get_pages().. although I guess you
 otherwise have no need for the page[] array?

Correct.  The page[] array would just be an additional unnecessary
allocation, and therefore would be an additional unnecessary point of
failure.

  +#define DRM_ARMADA_GEM_CREATE  0x00
  +#define DRM_ARMADA_GEM_MMAP0x02
  +#define DRM_ARMADA_GEM_PWRITE  0x03
  +
  +#define ARMADA_IOCTL(dir, name, str) \
  +   DRM_##dir(DRM_COMMAND_BASE + DRM_ARMADA_##name, struct 
  drm_armada_##str)
  +
  +struct drm_armada_gem_create {
 
 Any reason not to throw in a 'uint32_t flags' field?  At least then
 you could indicate what sort of backing memory you want, and do things
 like allocate linear scanout buffer w/ your gem_create rather than
 having to use dumb_create.  Maybe not something you need now, but
 seems like eventually you'll come up with some reason or another to
 want to pass some flags into gem_create.

I thought one of the points of the dumb_create stuff was so that there
is a standard API for dumb scanout buffers across all DRM drivers.  It
has that advantage, and as I understand it, a generic KMS driver for X

Re: [PATCH 2/5] DRM: Armada: Add Armada DRM driver

2013-10-11 Thread Russell King - ARM Linux

On Thu, Oct 10, 2013 at 06:23:26PM -0400, Rob Clark wrote:
 On Thu, Oct 10, 2013 at 5:59 PM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Thu, Oct 10, 2013 at 05:25:15PM -0400, Rob Clark wrote:
  probably this thread is applicable here too?
 
  https://lkml.org/lkml/2013/9/12/417
 
  (although.. we have at least a few  slightly differet variants on the
  same errno - VM_FAULT_x switch in different drivers.. maybe we should
  do something better)
 
  Hmm.  It seems today's vm_insert_pfn() has the following error return
  values:
 
  -EFAULT - virtual address outside vma
  -EINVAL - track_pfn_insert() failure
  -ENOMEM - failed to get locked pte
  -EBUSY - entry already exists in pte
 
  So it seems my handling -EIO, -EAGAIN, -ERESTARTSYS and -EINTR are all
  redundant and can be removed.  I'm not sure if it makes sense for this
  to be generic - it looks like it's only Intel, gma500 and me who use
  this, and Intel handles more error codes (due to it calling other
  functions.)
 
 I just noticed msm and omapdrm are missing the -EBUSY case (have some
 patches I need to send), which was why I mentioned this.  They do have
 other error cases from other fxns, so maybe something generic/common
 doesn't make sense..  but I realized i915 fixed the same issue a while
 back, so somewhere common has the advantage of somehow not forgetting
 to fix other drivers ;-)

Here's the promised delta:

 drivers/gpu/drm/armada/armada_gem.c |5 -
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c 
b/drivers/gpu/drm/armada/armada_gem.c
index 02de7a4..1e74f70 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -25,12 +25,7 @@ static int armada_gem_vm_fault(struct vm_area_struct *vma, 
struct vm_fault *vmf)
ret = vm_insert_pfn(vma, addr, pfn);
 
switch (ret) {
-   case -EIO:
-   case -EAGAIN:
-   set_need_resched();
case 0:
-   case -ERESTARTSYS:
-   case -EINTR:
case -EBUSY:
return VM_FAULT_NOPAGE;
case -ENOMEM:

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

Re: [PATCH 1/2] drm/i915: abstract the conversion of device-minor out to a macro

2013-10-12 Thread Russell King - ARM Linux

On Fri, Oct 11, 2013 at 03:05:56PM +1000, Dave Airlie wrote:
 From: Dave Airlie airl...@redhat.com
 
 This will make the next patch to change how this works a lot cleaner.
 
 Signed-off-by: Dave Airlie airl...@redhat.com
 ---
  drivers/gpu/drm/i915/i915_sysfs.c | 32 +---
  1 file changed, 17 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
 b/drivers/gpu/drm/i915/i915_sysfs.c
 index 44f4c1a..5caffb7 100644
 --- a/drivers/gpu/drm/i915/i915_sysfs.c
 +++ b/drivers/gpu/drm/i915/i915_sysfs.c
 @@ -32,6 +32,8 @@
  #include intel_drv.h
  #include i915_drv.h
  
 +#define dev_to_drm_minor(d) container_of((d), struct drm_minor, kdev)

Maybe this should be in include/drm somewhere?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: convert drivers to use of_graph_get_remote_node

2017-02-06 Thread Russell King - ARM Linux

On Mon, Feb 06, 2017 at 05:55:33PM +, Liviu Dudau wrote:
> OK, I will fix the driver if Rob's patch still requires it.

I don't think you ever needed it.  As Rob says, what you're testing
won't ever change unless you're using overlays - it's certainly not
dependent on the tda998x module being loaded or not, or even the
tda998x driver being bound or not.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/23] drm: armada: use vblank hooks in struct drm_crtc_funcs

2017-02-07 Thread Russell King - ARM Linux

On Tue, Feb 07, 2017 at 05:16:18PM +0800, Shawn Guo wrote:
> From: Shawn Guo <shawn@linaro.org>
> 
> The vblank hooks in struct drm_driver are deprecated and only meant for
> legacy drivers.  For modern drivers with DRIVER_MODESET flag, the hooks
> in struct drm_crtc_funcs should be used instead.  As the result,
> functions armada_drm_crtc_enable[disable]_irq() can be static, although
> they are moved around a bit to save forward declaration.
> 
> The armada_crtc pointer array in struct armada_private is still kept in
> there, because armada_debugfs.c still have reference to it.
> 
> Signed-off-by: Shawn Guo <shawn@linaro.org>
> Cc: Russell King <li...@armlinux.org.uk>

Acked-by: Russell King <rmk+ker...@armlinux.org.uk>

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/23] drm: remove drm_vblank_no_hw_counter assignment from driver code

2017-02-07 Thread Russell King - ARM Linux

On Tue, Feb 07, 2017 at 05:16:14PM +0800, Shawn Guo wrote:

For:

>  drivers/gpu/drm/armada/armada_drv.c | 1 -

Acked-by: Russell King <rmk+ker...@armlinux.org.uk>

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm: convert drivers to use of_graph_get_remote_node

2017-02-06 Thread Russell King - ARM Linux

On Mon, Feb 06, 2017 at 05:23:06PM +, Liviu Dudau wrote:
> On Mon, Feb 06, 2017 at 11:09:49AM -0600, Rob Herring wrote:
> > On Mon, Feb 06, 2017 at 10:29:33AM +, Liviu Dudau wrote:
> > > On Fri, Feb 03, 2017 at 09:36:33PM -0600, Rob Herring wrote:
> > > > -   /* add the remote encoder port as component */
> > > > -   port = of_graph_get_remote_port_parent(ep);
> > > > -   of_node_put(ep);
> > > > -   if (!port || !of_device_is_available(port)) {
> > > > -   of_node_put(port);
> > > > -   return -EAGAIN;
> > > 
> > > The HDLCD change looks reasonable except for this -EAGAIN business. I'll 
> > > have to
> > > test your changes on my setup to see how this affects having the encoder 
> > > as a module.
> > 
> > What are you expecting to happen with -EAGAIN? This one was a bit of an 
> > oddball.
> 
> When both the HDLCD and the TDA998x drivers are compiled as modules, the
> order in which they are inserted can be somewhat random (due to testing).

Not really "due to testing" but if you run a real distro, they tend to
have a multi-threaded behaviour when loading kernel modules at boot.

> It is at that time when you want the probe of HDLCD to be retried on the
> insmod-ing of the tda998x.ko rather than fail entirely.

-EAGAIN doesn't get you that, and in any case, solving that problem is
exactly why the component API exists - so that DRM only comes up once
all the necessary components are available.

-EAGAIN also doesn't get you that from inside a probe function - such
an error will be reported in the kernel log, and no further action
will be taken (the device driver probe will be failed, and not
automatically retried.

The only case that we automatically retry is if a driver returns
-EPROBE_DEFER.  Everything else causes a probe failure.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/23] drm: remove drm_vblank_no_hw_counter assignment from driver code

2017-02-07 Thread Russell King - ARM Linux

On Tue, Feb 07, 2017 at 12:42:15PM +0200, Laurent Pinchart wrote:
> On an unrelated note, for security reasons we should try to make the driver 
> structure static, or at least move ops to a static structure.

ITYM "const" not "static".

"static" doesn't get you anything from a security point of view.  "const"
gets you write protection, so code can't modify the function pointers.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] DRM OF graph clean-up

2017-02-05 Thread Russell King - ARM Linux

On Fri, Feb 03, 2017 at 09:36:30PM -0600, Rob Herring wrote:
> The Armada and Rockchip drivers remain oddballs with their own graph 
> parsing. I can't see how the armada driver even can work. There's 
> nothing to instantiate the armada-drm device either in DT or the kernel.

Correct, that's sitting out of tree because it requires either legacy
code in arch/arm/mach-dove at the moment, or stuff for DT.  Each time
that I looked at the DT reserved memory stuff I've ended up giving up
as it always seemed to be half complete, and the documentation was
confusing (seemingly referring to things that weren't merged.)

Maybe that's changed today, but I've not had a chance to look at it
again.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

2017-01-30 Thread Russell King - ARM Linux

On Mon, Jan 30, 2017 at 09:18:25PM +0100, Thierry Reding wrote:
> On Fri, Dec 09, 2016 at 12:21:22PM +0100, Christian Gmeiner wrote:
> > @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
> > __u64 bos;/* in, ptr to array of submit_bo's */
> > __u64 relocs; /* in, ptr to array of submit_reloc's */
> > __u64 stream; /* in, ptr to cmdstream */
> > +   __u64 readbacks;  /* in, ptr to array of submit_readback's */
> > +   __u32 nr_readbacks;   /* in, number of submit_readback's */
> > +   __u32 padding;
> >  };
> 
> What about ABI stability? How's this going to work when userspace uses
> old headers but runs against a new kernel? What about userspace using
> newer headers than the kernel?

+1, this is unacceptable.  We went through a period of making sure
that the ABI was going to be stable once we merged the driver into
the kernel, and the rule is that we don't ever break userspace.  This
does exactly that, so it's not permissible.

If this change is necessary, it needs to be a new ioctl number for the
call with the new structure, and the old ioctl has to keep working.

There are other users of etnaviv other than mesa that will break.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/10] drm/etnaviv: validate readback register address

2017-01-30 Thread Russell King - ARM Linux

On Mon, Jan 30, 2017 at 11:58:32AM +0100, Lucas Stach wrote:
> Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> > Reading some registers end in a system crash ala:
> > 
> >   Unhandled fault: external abort on non-linefetch (0x1028) at 0xfe641000
> >   Internal error: : 1028 [#1] PREEMPT ARM
> > 
> > Avoid those by register validation.
> 
> Avoiding crashes is one thing, but I believe this needs to go further
> and avoid reads from any register that isn't a performance counter. This
> probably isn't a big hole, but we want to avoid leaking the GPU state to
> userspace.

We certainly don't want to let people use this to read stuff like the
ID registers, bypassing the kernel.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/10] drm/etnaviv: make it possible to reconfigure perf counter

2017-01-30 Thread Russell King - ARM Linux

On Mon, Jan 30, 2017 at 12:01:18PM +0100, Lucas Stach wrote:
> Am Freitag, den 09.12.2016, 12:21 +0100 schrieb Christian Gmeiner:
> > -   if (r->flags) {
> > -   DRM_ERROR("readback flags not 0");
> > +   if (r->flags > ETNA_READBACK_PERF) {
> > +   DRM_ERROR("invalid readback flags");

This test should be more robust - while ETNA_READBACK_PERF may have a
value of '1', this is not how we should be checking for invalid _flags_.
It may work, but it's not the best solution.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [BUG] hdlcd gets confused about base address

2017-02-20 Thread Russell King - ARM Linux

On Mon, Feb 20, 2017 at 05:53:31PM +, Liviu Dudau wrote:
> Sorry, looks like this fell through the cracks of us trying to fix the
> other issues you were seeing. I'm attaching a patch, please let me know
> if this works for you.

I don't have time to test right now, but it looks similar to what I
had.

Only nit is - is dest_w used apart from being written once?  (That's
probably something for a separate patch.)

There's also the issue which Daniel Vetter followed up with to my
comment about this calculation - the plane state validation is
insufficient, and it would be a good idea if it used
drm_plane_helper_check_state().

Also, I've since noticed that it doesn't check state->rotation, and
I don't think this driver supports any rotation options.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [BUG] hdlcd gets confused about base address

2017-02-20 Thread Russell King - ARM Linux

On Mon, Feb 20, 2017 at 08:05:58PM +0200, Ville Syrjälä wrote:
> This stuff should be using the clipped coordinates, not the user
> coordinates. And it doesn't look like this guy is even calling the
> clip helper thing.
> 
> malidp seems to be calling that thing, but it still doesn't
> manage to program the hw with the right coordinates from what
> I can see.
> 
> /me feels a bit like a broken record...

If you mean drm_plane_helper_check_state(), then...

$ grep drm_plane_helper_check_state Documentation/gpu/ -r

So nothing there... but in drivers/gpu/drm/drm_plane_helper.c, there's
the following, and I think this really isn't helping people understand
what's required:

 * This helper library has two parts. The first part has support to implement
 * primary plane support on top of the normal CRTC configuration interface.
 * Since the legacy ->set_config interface ties the primary plane together with
 * the CRTC state this does not allow userspace to disable the primary plane
 * itself.  To avoid too much duplicated code use
 * drm_plane_helper_check_update() which can be used to enforce the same
 * restrictions as primary planes had thus. The default primary plane only
 * expose XRBG and ARGB as valid pixel formats for the attached
 * framebuffer.
 *
 * Drivers are highly recommended to implement proper support for primary
 * planes, and newly merged drivers must not rely upon these transitional
 * helpers.

Which functions are defined as "these transitional helpers" - the above
is rather ambiguous.  Is drm_plane_helper_check_state() a "transitional
helper" or is it not?  (It probably isn't, but the documentation does not
make that clear.)

It then goes on to:

 * The second part also implements transitional helpers which allow drivers to

So maybe the second paragraph needs to be moved after this line to
remove the confusion?

If you find that you're repeating something to many people, it's always
a good idea to re-read the documentation that's supposed to be giving
people guidance.

Now, when you say that we're supposed to program "clipped coordinates"
maybe you can give a hint what those are and where they come from?
Is that the vaguely documented "clip" parameter in
drm_plane_helper_check_state() ?

 * @clip: integer clipping coordinates

If it is, that doesn't really describe it, and neither does the
description of what the function does, nor what it returns:

 * Checks that a desired plane update is valid.  Drivers that provide
 * their own plane handling rather than helper-provided implementations may
 * still wish to call this function to avoid duplication of error checking
 * code.
 *
 * RETURNS:
 * Zero if update appears valid, error code on failure

So, some improvement there could go a long way towards eliminating
some of these issues...

Atomic modeset is hideously complex... having poor documentation doesn't
help.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [BUG] hdlcd gets confused about base address

2017-02-20 Thread Russell King - ARM Linux

On Fri, Nov 18, 2016 at 11:37:33PM +, Russell King - ARM Linux wrote:
> Something I also noticed is this:
> 
> scanout_start = gem->paddr + plane->state->fb->offsets[0] +
> plane->state->crtc_y * plane->state->fb->pitches[0] +
> plane->state->crtc_x * bpp / 8;
> 
> Surely this should be using src_[xy] (which are the position in the
> source - iow, memory, and not crtc_[xy] which is the position on the
> CRTC displayed window.  To put it another way, the src_* define the
> region of the source material that is mapped onto a rectangular area
> on the display defined by crtc_*.

This problem still exists...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: ensure atomic messages consistently include the name of the component

On Mon, Feb 13, 2017 at 02:37:31PM +0100, Maarten Lankhorst wrote:
> All for it, looks sane. The last hunk fails to apply because it's based on
> an older version of page_flip, but easy enough to fix.

Yes, it's based on v4.10-rc7.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: imxdrm issue on Hummingboard with LVDS enabled

On Sun, Feb 12, 2017 at 01:42:38PM +, Russell King - ARM Linux wrote:
> On Sun, Feb 12, 2017 at 01:18:54PM +0000, Russell King - ARM Linux wrote:
> > Hi,
> > 
> > Here's another issue with imxdrm.  I got this while trying to reproduce
> > Dan's problem by enabling the lvds output using the patch below
> > originally from Fabio, but updated a bit.  This doesn't occur if I leave
> > LVDS disabled.
> > 
> > The taint is due to the IMX capture modules from Steve, who chose to put
> > his code in drivers/staging/media rather than drivers/media.  However,
> > the last module to make the capture stuff active (imx-csi) which interfaces
> > the drivers/gpu/ipu-v3 code with the capture code has not been loaded.
> > 
> > [ cut here ]
> > WARNING: CPU: 1 PID: 1049 at 
> > /home/rmk/git/linux-rmk/drivers/gpu/drm/drm_atomic_helper.c:1149 
> > drm_atomic_helper_wait_for_vblanks+0x218/0x224
> > [CRTC:29] vblank wait timed out
> 
> Can someone please explain to me how you go from something like
> "[CRTC:29]" to something meaningful, such as identifying which
> exact CRTC failed here?
> 
> Given that the ID numbers given out by DRM for individual components
> come from the dev->mode_config.crtc_idr IDR, without knowing in
> exact detail the precise registration order of these objects with
> drm_mode_object_get(), printing "[CRTC:29]" is utterly meaningless -
> it conveys zero useful information.  DRM might as well be printing
> random numbers instead.
> 
> At least the pre-atomic DRM code printed crtc->name as well, which
> would at least indicate which _CRTC_ in numeric order of registration
> was the cause, which is slightly easier to guess which hardware CRTC
> in question caused the problem.
> 
> Some consistency in DRM land would be nice...

Eventually, we get:

WARNING: CPU: 1 PID: 1049 at 
/home/rmk/git/linux-rmk/drivers/gpu/drm/drm_atomic_helper.c:1151 
drm_atomic_helper_wait_for_vblanks+0x220/0x22c
[CRTC:26:crtc-0] vblank wait timed out

which is the "crtc" providing the HDMI output, and there is no HDMI
output anymore.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: imxdrm issue on SABRE Lite

On Sat, Feb 11, 2017 at 09:09:34PM +, Dan MacDonald wrote:
> [   43.997066] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:24:crtc-0] flip_done timed out
> [   55.517063] [drm:drm_atomic_helper_commit_cleanup_done] *ERROR*
> [CRTC:24:crtc-0] flip_done timed out

This seems to lay the foundation for the kernel to Oops itself later.
The problem seems to be this:

drm_atomic_helper_commit(state->dev, state, false)
- drm_atomic_helper_setup_commit(state, false)
  - foreach crtc in state
- commit->event = kzalloc()
- crtc_state->event = commit->event
- crtc_state->event->base.completion = >flip_done
...
- commit_tail(state)
  - funcs->atomic_commit_tail(state)
...
- drm_atomic_helper_commit_planes(dev, state,
DRM_PLANE_COMMIT_ACTIVE_ONLY |

DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODES$  - foreach active crtc in state
- funcs->atomic_begin(crtc, old_crtc_state)
  - ipu_crtc_atomic_begin()
- drm_crtc_vblank_on()
- if crtc->state->event
  - drm_crtc_arm_vblank_event(crtc, crtc->state->event)
  - crtc->state->event = NULL

At this point, the "commit->flip_done" completion is queued with the
event onto the vblank list.
...
  - drm_atomic_helper_commit_cleanup_done(state)
- foreach crtc in state
  - try_wait_for_completion(>hw_done)
  - wait_for_completion_timeout(>flip_done, 10sec)

This is where we get the timeout message.

  - drm_atomic_state_free(state)

This "clears" the commit state (calling drm_crtc_commit_put() on it)
which has the effect of kfree()'ing the structure containing the
flip_done, but which is still on the vblank list.

The next time we try to set a mode, the result is that a call to
drm_crtc_vblank_off() causes all queued events to be sent, including
the now kfree()'d flip_done completion, resulting in the reported
kernel oops.

It seems others are also suffering similar issues when the flip_done
completion times out with other drivers:

https://lkml.org/lkml/2016/12/1/171
https://bugs.freedesktop.org/show_bug.cgi?id=96781
https://lists.opensuse.org/opensuse-bugs/2016-10/msg03011.html
https://patchwork.kernel.org/patch/9280223/ (which is me...)

This is likely the same, although the timeout line was not captured:
https://bugzilla.redhat.com/show_bug.cgi?id=1415180
https://bodhi.fedoraproject.org/updates/kernel-4.8.7-200.fc24

So, can we please avoid killing the kernel when the hardware doesn't
quite behave as we want it to?

Right now, when we oops the kernel, we're leaking all the memory
associated with the atomic modeset, so if we stop oopsing the kernel
but still leak the memory, surely that would be an improvement?
Maybe something like the untested patch at the bottom of this mail?

It would give the opportunity to poke about on a failed system to
work out what happened and maybe why the hardware misbehaved.

The real answer is for the hardware to behave, but we can't always
have our cake.

Note - I can't reproduce Dan's problem here on 4.10-rc7 as I suspect
it needs multiple CRTCs/outputs running in the IPU to trigger it,
which Sabre lite has.  I'll try enabling the (disconnected) LVDS
output tomorrow (I have Fabio's LVDS patch knocking about), but I
suspect those with a deeper knowledge of the IPU need to investigate
what's going on.

> [  214.765689] imx-ipuv3 240.ipu: DC stop timeout after 50 ms
> [  214.825688] Unable to handle kernel NULL pointer dereference at
> virtual address 
> [  214.833783] pgd = ed1b8000
> [  214.836491] [] *pgd=4c974831
> [  214.840084] Internal error: Oops: 17 [#1] SMP ARM
> [  214.844789] Modules linked in: mousedev snd_soc_sgtl5000
> snd_soc_fsl_ssi snd_soc_imx_sgtl5000 imx_pcm_fiq imx_pcm_dma
> snd_soc_fsl_asrc snd_soc_fsl_asoc_card snd_soc_core dw_hdmi_ahb_audio
> snd_pcm_dmaengine caam_jr imx_ipuv3_crtc snd_ac97_codec coda
> v4l2_mem2mem videobuf2_dma_contig ac97_bus imx_ipu_v3
> snd_soc_imx_audmux snd_pcm videobuf2_vmalloc videobuf2_memops
> videobuf2_v4l2 videobuf2_core dw_hdmi_imx caam imx2_wdt ofpart spi_imx
> evdev dw_hdmi etnaviv imx_ldb pwm_imx snd_timer parallel_display
> uio_pdrv_genirq uio imxdrm sch_fq_codel ip_tables x_tables
> [  214.894338] CPU: 2 PID: 316 Comm: Xorg Tainted: GW
> 4.9.8-1-ARCH #1
> [  214.901735] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [  214.908264] task: ed2c4d00 task.stack: ed2a6000
> [  214.912803] PC is at __wake_up_common+0x1c/0x80
> [  214.917337] LR is at __wake_up_locked+0x14/0x1c
> [  214.921871] pc : []lr : []psr: a0070093
> [  214.921871] sp : ed2a7c68  ip :   fp : c0fa2a70
> [  214.933348] r10: c0f37384  r9 : 0003  r8 : 
> [  214.938574] r7 :   r6 : edbf3410  r5 : edbf3408  r4 : edbf340c
> [  214.945101] r3 :   r2 :   r1 :   r0 : edbf340c
> [  214.951630] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> Segment

imxdrm issue on Hummingboard with LVDS enabled

Hi,

Here's another issue with imxdrm.  I got this while trying to reproduce
Dan's problem by enabling the lvds output using the patch below
originally from Fabio, but updated a bit.  This doesn't occur if I leave
LVDS disabled.

The taint is due to the IMX capture modules from Steve, who chose to put
his code in drivers/staging/media rather than drivers/media.  However,
the last module to make the capture stuff active (imx-csi) which interfaces
the drivers/gpu/ipu-v3 code with the capture code has not been loaded.

[ cut here ]
WARNING: CPU: 1 PID: 1049 at 
/home/rmk/git/linux-rmk/drivers/gpu/drm/drm_atomic_helper.c:1149 
drm_atomic_helper_wait_for_vblanks+0x218/0x224
[CRTC:29] vblank wait timed out
Modules linked in: imx_camif(C) imx_ic(C) imx_smfc(C) caam_jr uvcvideo 
snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card snd_soc_imx_spdif imx_media(C) 
snd_soc_imx_audmux imx_mipi_csi2(C) imx_media_core(C) snd_soc_sgtl5000 imx219 
caam imx_sdma video_multiplexer imx2_wdt coda v4l2_mem2mem videobuf2_v4l2 
videobuf2_dma_contig snd_soc_fsl_spdif videobuf2_core snd_soc_fsl_ssi 
imx_pcm_dma videobuf2_vmalloc videobuf2_memops imx_thermal nfsd 
rc_pinnacle_pctv_hd dw_hdmi_cec dw_hdmi_ahb_audio imx_ldb etnaviv
CPU: 1 PID: 1049 Comm: Xorg Tainted: G C  4.10.0-rc7+ #2106
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace: 
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:600f0013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:c08db088 r5: r4:ee9a5c98 r3:d039ee40
[] (__warn) from [] (warn_slowpath_fmt+0x40/0x48)
 r10:d01d5018 r8:00b0 r7:d01d4000 r6:eea7d000 r5:eaab4480 r4:0001
[] (warn_slowpath_fmt) from [] 
(drm_atomic_helper_wait_for_vblanks+0x218/0x224)
 r3:001d r2:c08db1b4
[] (drm_atomic_helper_wait_for_vblanks) from [] 
(imx_drm_atomic_commit_tail+0x50/0x60)
 r10:d02be400 r9:eaab4a00 r8: r7:d01d4000 r6: r5:d01d4000
 r4:eaab4480
[] (imx_drm_atomic_commit_tail) from [] 
(commit_tail+0x48/0x94)
 r5:c0a456a4 r4:eaab4480
[] (commit_tail) from [] 
(drm_atomic_helper_commit+0xc4/0x150)
 r5: r4:eaab4480
[] (drm_atomic_helper_commit) from [] 
(drm_atomic_commit+0x4c/0x60)
 r8:ed57f000 r7:ee9a5dd8 r6:d01d4000 r5: r4:eaab4480 r3:c03daf04
[] (drm_atomic_commit) from [] 
(drm_atomic_helper_set_config+0x80/0xd0)
 r6:d01d5018 r5: r4:eaab4480 r3:0004
[] (drm_atomic_helper_set_config) from [] 
(drm_mode_set_config_internal+0x60/0xe4)
 r7:d01d5018 r6:d01d4000 r5:eaab4a00 r4:ed57f000
[] (drm_mode_set_config_internal) from [] 
(drm_mode_setcrtc+0xdc/0x47c)
 r7:0001 r6:d01d4000 r5:eaab4a00 r4:ee9a5e58
[] (drm_mode_setcrtc) from [] (drm_ioctl+0x204/0x41c)
 r10:c06864a2 r9:d01d4000 r8:c07495a0 r7:ee9a5e58 r6:d03e3400 r5:0068
 r4:
[] (drm_ioctl) from [] (do_vfs_ioctl+0x98/0x9a0)
 r10:ed4ba068 r9:ee9a4000 r8:be9fa5c0 r7:0008 r6:0008 r5:ee1a4500
 r4:c018961c
[] (do_vfs_ioctl) from [] (SyS_ioctl+0x3c/0x60)
 r10: r9:ee9a4000 r8:be9fa5c0 r7:0008 r6:c06864a2 r5:ee1a4500
 r4:ee1a4500
[] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c)
 r8:c000ff04 r7:0036 r6:c06864a2 r5:be9fa5c0 r4:b6c4ace8 r3:0001
---[ end trace e9cdd5f49e5cc87f ]---


diff --git a/arch/arm/boot/dts/imx6dl-hummingboard.dts 
b/arch/arm/boot/dts/imx6dl-hummingboard.dts
index d5c966031962..9f605d14c50f 100644
--- a/arch/arm/boot/dts/imx6dl-hummingboard.dts
+++ b/arch/arm/boot/dts/imx6dl-hummingboard.dts
@@ -48,3 +48,10 @@
model = "SolidRun HummingBoard Solo/DualLite";
compatible = "solidrun,hummingboard/dl", "fsl,imx6dl";
 };
+
+ {
+   assigned-clocks = < IMX6QDL_CLK_LDB_DI0_SEL>,
+ < IMX6QDL_CLK_LDB_DI1_SEL>;
+   assigned-clock-parents = < IMX6QDL_CLK_PLL3_USB_OTG>,
+< IMX6QDL_CLK_PLL3_USB_OTG>;
+};
diff --git a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi 
b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
index d6c2358ffad4..258107246d64 100644
--- a/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-hummingboard.dtsi
@@ -302,3 +302,28 @@
cd-gpios = < 4 GPIO_ACTIVE_LOW>;
status = "okay";
 };
+
+ {
+   status = "okay";
+
+   lvds-channel@1 {
+   fsl,data-mapping = "spwg";
+   fsl,data-width = <18>;
+   status = "okay";
+
+   display-timings {
+   native-mode = <>;
+   timing0: hsd100pxn1 {
+   clock-frequency = <6500>;
+   hactive = <1024>;
+   vactive = <768>;
+   hback-porch = <220>;
+   hfront-porch = <40>;
+   vback-porch = <21>;
+   vfront-porch = <7>;
+   hsync-len = <60>;
+

Re: imxdrm issue on Hummingboard with LVDS enabled

On Sun, Feb 12, 2017 at 01:18:54PM +, Russell King - ARM Linux wrote:
> Hi,
> 
> Here's another issue with imxdrm.  I got this while trying to reproduce
> Dan's problem by enabling the lvds output using the patch below
> originally from Fabio, but updated a bit.  This doesn't occur if I leave
> LVDS disabled.
> 
> The taint is due to the IMX capture modules from Steve, who chose to put
> his code in drivers/staging/media rather than drivers/media.  However,
> the last module to make the capture stuff active (imx-csi) which interfaces
> the drivers/gpu/ipu-v3 code with the capture code has not been loaded.
> 
> [ cut here ]
> WARNING: CPU: 1 PID: 1049 at 
> /home/rmk/git/linux-rmk/drivers/gpu/drm/drm_atomic_helper.c:1149 
> drm_atomic_helper_wait_for_vblanks+0x218/0x224
> [CRTC:29] vblank wait timed out

Can someone please explain to me how you go from something like
"[CRTC:29]" to something meaningful, such as identifying which
exact CRTC failed here?

Given that the ID numbers given out by DRM for individual components
come from the dev->mode_config.crtc_idr IDR, without knowing in
exact detail the precise registration order of these objects with
drm_mode_object_get(), printing "[CRTC:29]" is utterly meaningless -
it conveys zero useful information.  DRM might as well be printing
random numbers instead.

At least the pre-atomic DRM code printed crtc->name as well, which
would at least indicate which _CRTC_ in numeric order of registration
was the cause, which is slightly easier to guess which hardware CRTC
in question caused the problem.

Some consistency in DRM land would be nice...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: imxdrm issue on SABRE Lite

On Mon, Feb 13, 2017 at 09:05:33AM +0100, Thierry Reding wrote:
> On Sun, Feb 12, 2017 at 12:15:46AM +0000, Russell King - ARM Linux wrote:
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 21f992605541..46668d071d6a 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1201,9 +1201,8 @@ static void commit_tail(struct drm_atomic_state 
> > *state)
> > else
> > drm_atomic_helper_commit_tail(state);
> >  
> > -   drm_atomic_helper_commit_cleanup_done(state);
> > -
> > -   drm_atomic_state_free(state);
> > +   if (drm_atomic_helper_commit_cleanup_done(state) == 0)
> > +   drm_atomic_state_free(state);
> 
> Chris (Cc'ed) added reference counting to atomic state for v4.10, maybe
> that already fixes the issue?

No.  It's not the atomic state that's referenced, it's only a completion
within the drm_crtc_commit structure, which is completely separate from
the atomic state.  Moreover, the event code has no knowledge of commits,
so it can't "put" a reference count on it.  See:

void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
{
assert_spin_locked(>event_lock);

if (e->completion) {
/* ->completion might disappear as soon as it signalled. */
complete_all(e->completion);
e->completion = NULL;
}

vs the setup of the event done in drm_atomic_helper_setup_commit():

if (!crtc_state->event) {
commit->event = kzalloc(sizeof(*commit->event),
GFP_KERNEL);
if (!commit->event)
return -ENOMEM;

crtc_state->event = commit->event;
}

crtc_state->event->base.completion = >flip_done;

"commit" gets freed before drm_send_event_locked() is called (hence
the timeout message) and when drm_send_event_locked() is eventually
called via drm_vblank_off(), this causes a use-after-free bug.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: imxdrm issue on SABRE Lite

On Mon, Feb 13, 2017 at 08:55:53AM +, Chris Wilson wrote:
> On Mon, Feb 13, 2017 at 09:05:33AM +0100, Thierry Reding wrote:
> > On Sun, Feb 12, 2017 at 12:15:46AM +, Russell King - ARM Linux wrote:
> > > On Sat, Feb 11, 2017 at 09:09:34PM +, Dan MacDonald wrote:
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 21f992605541..46668d071d6a 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1201,9 +1201,8 @@ static void commit_tail(struct drm_atomic_state 
> > > *state)
> > >   else
> > >   drm_atomic_helper_commit_tail(state);
> > >  
> > > - drm_atomic_helper_commit_cleanup_done(state);
> > > -
> > > - drm_atomic_state_free(state);
> > > + if (drm_atomic_helper_commit_cleanup_done(state) == 0)
> > > + drm_atomic_state_free(state);
> > 
> > Chris (Cc'ed) added reference counting to atomic state for v4.10, maybe
> > that already fixes the issue?
> 
> I'm not confident it will, as there is not an independent ref on the
> state for the phases, and so a forced timeout still leaves a dangling
> pointer. The above chunk goes the opposite way and leaks the state to
> avoid the invalid deref, what we need is a ref around its existence on
> the dependency queue if that is outside the lifetime of the commit.

I said as much in my email - unfortunately, Thierry cut all that context.

Right now, we oops the kernel, which causes:

(a) the death of the calling process
(b) leaking of all memory associated with the modeset

What I'm proposing for the -stable kernels is to _improve_ the situation
by eliminating part of the problem, so it's possible to get a better
idea of which bit went wrong and which outputs have failed.

Fixing it properly is likely to be very invasive, since you'll need to
add reference counting to the drm_crtc_commit structure, a pointer
to that in the drm_pending_event structure, and ensure that the
reference count gets incremented at the appropriate time.  Incrementing
the reference count in drm_atomic_helper_setup_commit() certainly isn't
the right place, that would be at the sites which queue the event, but
they are scattered amongst all the atomic modeset drivers.

For reference, here's my complete patch I posted yesterday:

 drivers/gpu/drm/drm_atomic_helper.c | 15 +--
 include/drm/drm_atomic_helper.h |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 21f992605541..46668d071d6a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1201,9 +1201,8 @@ static void commit_tail(struct drm_atomic_state *state)
else
drm_atomic_helper_commit_tail(state);
 
-   drm_atomic_helper_commit_cleanup_done(state);
-
-   drm_atomic_state_free(state);
+   if (drm_atomic_helper_commit_cleanup_done(state) == 0)
+   drm_atomic_state_free(state);
 }
 
 static void commit_work(struct work_struct *work)
@@ -1591,12 +1590,12 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
+int drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
 {
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
struct drm_crtc_commit *commit;
-   int i;
+   int i, failed = 0;
long ret;
 
for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -1621,15 +1620,19 @@ void drm_atomic_helper_commit_cleanup_done(struct 
drm_atomic_state *state)
 * not hold a reference of its own. */
ret = wait_for_completion_timeout(>flip_done,
  10*HZ);
-   if (ret == 0)
-   DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
+   if (ret == 0) {
+   DRM_ERROR("[CRTC:%d:%s] flip_done timed out, memory 
leaked\n",
  crtc->base.id, crtc->name);
+   failed = -ETIMEDOUT;
+   }
 
spin_lock(>commit_lock);
 del_commit:
list_del(>commit_entry);
spin_unlock(>commit_lock);
}
+
+   return failed;
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b09fd9c..ee3d642c1feb 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_hel

Re: imxdrm issue on Hummingboard with LVDS enabled

On Mon, Feb 13, 2017 at 09:17:49AM +0100, Thierry Reding wrote:
> Of course, the above still requires that the core messages output the
> name along with the ID.

Thankfully, that's not a very big patch.  I'll post it later today.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-20 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 11:20:05AM +0300, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> > Moreover, as I've already said, converting tda998x to a DRM bridge
> > will not get rid of the encoder/connector part, because it _is_ a
> > connector in the DRM sense - it provides the detection and EDID
> > reading.
> > 
> > So, what would we achieve by splitting the driver into a DRM bridge
> > and DRM encoder/connector?
> 
> Please note that DRM bridge doesn't split the DRM connector out of the 
> bridge, 
> bridges instantiate drm_connector objects. In that sense they don't differ 
> much from the model implemented by the tda998x driver.

So, we what you're saying is that we from a component based DRM encoder
plus DRM connector to a component based DRM bridge plus DRM connector
and some nebulous DRM encoder provider.

How does the DRM encoder get associated with the DRM connector?  DRM
requires the presence of at least one DRM encoder for each DRM connector,
and the DRM connector needs to provide a .best_encoder method to pick
the appropriate DRM encoder.

If (as you said elsewhere in your message) that the display driver is
responsible for providing the DRM encoder in your view, how does a
bridge DRM connector get to that DRM encoder, and how does it know
which DRM encoder to pick?

> I however believe that connectors should be split out DRM bridge drivers, for 
> the simple reason that bridges can be chained. The output of a bridge isn't 
> guaranteed to be a connector but can be another bridge.

You've not said how that could be achieved with TDA998x, so I'm still
opposed to the idea.  Remember, you're the one asking for this, you
must have a view on how to achieve it if you want it to happen.

> > We can see this with what happened to the DW-HDMI driver - that still
> > needs the component helper, and it provides a DRM bridge, DRM encoder
> > and DRM connector parts.
> 
> To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the 
> glue layers implemented as part of the Rockchip and iMX display drivers that 
> do. Nonetheless, that's a mistake, the encoder should be created by the 
> display drivers.

If we're being precise, the "glue layer" creates the DRM encoder, but
the bridge code is responsible for binding itself to the DRM encoder,
and binding the DRM encoder to its associated DRM connector.

Let's assume it's a mistake.  Please illustrate how you would solve
this mistake.

> > The only reason it made sense to split the DW-HDMI driver was due to the
> > differences between the Rockchip and Freescale implementations.  Such
> > differences do not exist for TDA998x. So even here, your idea that "DRM
> > bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM
> > bridge component based" because of the problem that I'm illustrating here.
> > 
> > So, again, I ask: what's the point of needlessly splitting the code
> > between an encoder/connector and a bridge?
> 
> We need to standardize on one model. I don't care much about how the end 
> result is named, as long as it fulfils the task at hand.

I don't care about the name either, that's not what I'm asking here.

All I seem to be getting is some hand-waving "we want one model" and
"the current situation is a huge mess" (which is not a sentiment I agree
with) but with no technical response about how to achieve it.  Please do
the technical response bits and stop hand-waving.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-21 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
> 3) Rough conversion to bridge:

So I thought I might give this a try, and see what's needed to complete
the patch, but...

I thought we'd got past the dark ages of email clients screwing up
patches, but it seems not.  This patch is totally screwed that it's
not worth even sending - it has been:

- wrapped
- white-space damaged

which makes it pretty hard to undo all the damage.  Please use a
better mail client which doesn't screw up patches, or send it as a
plain text attachment.  Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: convert DT component matching to component_match_add_release()

2016-10-20 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 04:39:04PM -0400, Sean Paul wrote:
> On Wed, Oct 19, 2016 at 6:28 AM, Russell King
> <rmk+kernel at armlinux.org.uk> wrote:
> > Convert DT component matching to use component_match_add_release().
> >
> > Signed-off-by: Russell King <rmk+kernel at armlinux.org.uk>
> > ---
> > Can we please get this patch from May merged into the drm-misc or
> > whatever trees so that we don't end up with conflicts?  I've no idea
> > who looks after drm-misc, as they have _still_ failed to add
> > themselves to MAINTAINERS.
> 
> I think Daniel explained pretty clearly why this wasn't happening in
> the previous thread.
> 
> Next time you send a v2, can you please mark it as such and include a
> "Changes in v2" section?

Why - nothing's changed other than a rebase onto 4.9-rc1.  This isn't
a "I've changed it in XYZ way, so here's a new copy".  It's being
posted in the hope that someone will finally either comment on it or
merge the damn thing, rather than ignoring it was done when it was
last posted.

> >  drivers/gpu/drm/arm/hdlcd_drv.c |  3 ++-
> >  drivers/gpu/drm/arm/malidp_drv.c|  4 +++-
> >  drivers/gpu/drm/armada/armada_drv.c |  2 +-
> >  drivers/gpu/drm/drm_of.c| 28 
> > +++--
> >  drivers/gpu/drm/etnaviv/etnaviv_drv.c   |  5 +++--
> >  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  7 ---
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  4 +++-
> >  drivers/gpu/drm/msm/msm_drv.c   | 12 ++-
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  6 --
> >  drivers/gpu/drm/sti/sti_drv.c   |  5 +++--
> >  drivers/gpu/drm/sun4i/sun4i_drv.c   |  3 ++-
> >  drivers/gpu/drm/tilcdc/tilcdc_external.c|  4 +++-
> >  include/drm/drm_of.h| 12 +++
> >  13 files changed, 73 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c 
> > b/drivers/gpu/drm/arm/hdlcd_drv.c
> > index fb6a418ce6be..6477d1a65266 100644
> > --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> > @@ -453,7 +453,8 @@ static int hdlcd_probe(struct platform_device *pdev)
> > return -EAGAIN;
> > }
> >
> > -   component_match_add(>dev, , compare_dev, port);
> > +   drm_of_component_match_add(>dev, , compare_dev, port);
> > +   of_node_put(port);
> 
> There's no mention in your commit message about fixing these node leaks.

Isn't that kind-of the whole point of this patch?

> >
> > return component_master_add_with_match(>dev, 
> > _master_ops,
> >match);
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c 
> > b/drivers/gpu/drm/arm/malidp_drv.c
> > index 9280358b8f15..9f4739452a25 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -493,7 +493,9 @@ static int malidp_platform_probe(struct platform_device 
> > *pdev)
> > return -EAGAIN;
> > }
> >
> > -   component_match_add(>dev, , malidp_compare_dev, port);
> > +   drm_of_component_match_add(>dev, , malidp_compare_dev,
> > +  port);
> > +   of_node_put(port);
> > return component_master_add_with_match(>dev, 
> > _master_ops,
> >match);
> >  }
> > diff --git a/drivers/gpu/drm/armada/armada_drv.c 
> > b/drivers/gpu/drm/armada/armada_drv.c
> > index 1e0e68f608e4..94e46da9a758 100644
> > --- a/drivers/gpu/drm/armada/armada_drv.c
> > +++ b/drivers/gpu/drm/armada/armada_drv.c
> > @@ -254,7 +254,7 @@ static void armada_add_endpoints(struct device *dev,
> > continue;
> > }
> >
> > -   component_match_add(dev, match, compare_of, remote);
> > +   drm_of_component_match_add(dev, match, compare_of, remote);
> > of_node_put(remote);
> > }
> >  }
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index bc98bb94264d..47848ed8ca48 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -6,6 +6,11 @@
> >  #include 
> >  #include 
> >
> > +static void drm_release_of(struct device *dev, void *data)
> > +{
> > +   of_node_put(data);
> > +}
> > +
> >  /**
> >

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-21 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
> 
> 
> On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
> >On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
> >>
> >>
> >>On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
> >>>Hi Russell,
> >>>
> >>>On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> >>>>On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> >>>>>On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >>>>>>In any case, I don't agree with converting it to a DRM bridge - that
> >>>>>>will mean that we have to split the driver into two pieces, the bridge
> >>>>>>part handling the mode set specifics, and a connector/encoder part which
> >>>>>>handles the detection/edid stuff.
> >>>>>>
> >>>>>>We might as well keep the whole thing as the classical connector/encoder
> >>>>>>rather than introducing this additional layer of complexity.
> >>>>>
> >>>>>We have different ways to instantiate external HDMI encoders, and that's
> >>>>>not good. I believe everybody agrees that drm encoder slave has to go, so
> >>>>>that's already one problem solved (or rather solvable, it still requires
> >>>>>someone to do the work). We will then be left with two different methods,
> >>>>>drm bridge and non-bridge component-based instantiation. We need to
> >>>>>somehow merge the two, and I'm open to discussions on how the end result
> >>>>>should look like.
> >>>>
> >>>>I think you're idea really doesn't work - and I think your idea that
> >>>>component-based is somehow separate from other methods is wrong.
> >>>>
> >>>>Look at iMX for example - even converting all the code that could be
> >>>>a bridge to be a bridge will not get rid of its use of the component
> >>>>instantiation, because you still have other components that need to
> >>>>come from elsewhere.  The same is true of armada as well.
> >>>
> >>>Don't get me wrong, I'm certainly not against the component framework 
> >>>itself.
> >>>A way to bind multiple independent devices together is needed. We have a
> >>>similar framework in V4L2 called v4l2-async, and I'd like to see it moved 
> >>>to
> >>>the component framework at some point to unify code. Some changes to the
> >>>component framework might be needed to support needs of V4L2 that are
> >>>currently not applicable to DRM/KMS, but there's nothing major there.
> >>>
> >>>>Moreover, as I've already said, converting tda998x to a DRM bridge
> >>>>will not get rid of the encoder/connector part, because it _is_ a
> >>>>connector in the DRM sense - it provides the detection and EDID
> >>>>reading.
> >>>>
> >>>>So, what would we achieve by splitting the driver into a DRM bridge
> >>>>and DRM encoder/connector?
> >>>
> >>>Please note that DRM bridge doesn't split the DRM connector out of the 
> >>>bridge,
> >>>bridges instantiate drm_connector objects. In that sense they don't differ
> >>>much from the model implemented by the tda998x driver.
> >>>
> >>>I however believe that connectors should be split out DRM bridge drivers, 
> >>>for
> >>>the simple reason that bridges can be chained. The output of a bridge isn't
> >>>guaranteed to be a connector but can be another bridge. We managed not to 
> >>>have
> >>>to deal with that in a generic way so far in mainline, but we'll run into 
> >>>the
> >>>problem one of these days, and a solution will be needed. There's no rush
> >>>right now, and this is unrelated to converting tda998x to DRM bridge.
> >>>
> >>>>We would still need the component helper to manage the binding of
> >>>>the connector part, which would also then have to register a DRM
> >>>>bridge in addition to a DRM encoder and providing the DRM connector
> >>>>as we can't have two device drivers bound to the same i2c device.
> >>>>What we get is more complexity in the driver.
> >>>
> >>>DRM bridges indeed don't create encoders. That task is left to the display
> >>>driver. The reason is the same as

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-20 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
> 
> 
> On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
> >Hi Russell,
> >
> >On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> >>On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> >>>On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >>>>In any case, I don't agree with converting it to a DRM bridge - that
> >>>>will mean that we have to split the driver into two pieces, the bridge
> >>>>part handling the mode set specifics, and a connector/encoder part which
> >>>>handles the detection/edid stuff.
> >>>>
> >>>>We might as well keep the whole thing as the classical connector/encoder
> >>>>rather than introducing this additional layer of complexity.
> >>>
> >>>We have different ways to instantiate external HDMI encoders, and that's
> >>>not good. I believe everybody agrees that drm encoder slave has to go, so
> >>>that's already one problem solved (or rather solvable, it still requires
> >>>someone to do the work). We will then be left with two different methods,
> >>>drm bridge and non-bridge component-based instantiation. We need to
> >>>somehow merge the two, and I'm open to discussions on how the end result
> >>>should look like.
> >>
> >>I think you're idea really doesn't work - and I think your idea that
> >>component-based is somehow separate from other methods is wrong.
> >>
> >>Look at iMX for example - even converting all the code that could be
> >>a bridge to be a bridge will not get rid of its use of the component
> >>instantiation, because you still have other components that need to
> >>come from elsewhere.  The same is true of armada as well.
> >
> >Don't get me wrong, I'm certainly not against the component framework itself.
> >A way to bind multiple independent devices together is needed. We have a
> >similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
> >the component framework at some point to unify code. Some changes to the
> >component framework might be needed to support needs of V4L2 that are
> >currently not applicable to DRM/KMS, but there's nothing major there.
> >
> >>Moreover, as I've already said, converting tda998x to a DRM bridge
> >>will not get rid of the encoder/connector part, because it _is_ a
> >>connector in the DRM sense - it provides the detection and EDID
> >>reading.
> >>
> >>So, what would we achieve by splitting the driver into a DRM bridge
> >>and DRM encoder/connector?
> >
> >Please note that DRM bridge doesn't split the DRM connector out of the 
> >bridge,
> >bridges instantiate drm_connector objects. In that sense they don't differ
> >much from the model implemented by the tda998x driver.
> >
> >I however believe that connectors should be split out DRM bridge drivers, for
> >the simple reason that bridges can be chained. The output of a bridge isn't
> >guaranteed to be a connector but can be another bridge. We managed not to 
> >have
> >to deal with that in a generic way so far in mainline, but we'll run into the
> >problem one of these days, and a solution will be needed. There's no rush
> >right now, and this is unrelated to converting tda998x to DRM bridge.
> >
> >>We would still need the component helper to manage the binding of
> >>the connector part, which would also then have to register a DRM
> >>bridge in addition to a DRM encoder and providing the DRM connector
> >>as we can't have two device drivers bound to the same i2c device.
> >>What we get is more complexity in the driver.
> >
> >DRM bridges indeed don't create encoders. That task is left to the display
> >driver. The reason is the same as above: bridges can be chained (including
> >with an internal encoder that is not modelled as a bridge, and that's a case
> >we have today), while the KMS model exposes a single encoder to userspace.
> >Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> >Better solutions would have been to expose no encoder at all or all encoders
> >in the chain. We are however stuck with this model as we can't break the 
> >UABI.
> >For that reason a DRM encoder object doesn't represent an encoder but a chain
> >of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> >must be created at a higher level, in the display driver.
> 
> One more thing is that the

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-22 Thread Russell King - ARM Linux

On Fri, Oct 21, 2016 at 09:04:39PM +0200, Jean-Francois Moine wrote:
>  On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>   (sorry, I lost your original mail)
> > >>> DRM bridges indeed don't create encoders. That task is left to the 
> > >>> display
> > >>> driver. The reason is the same as above: bridges can be chained 
> > >>> (including
> > >>> with an internal encoder that is not modelled as a bridge, and that's a 
> > >>> case
> > >>> we have today), while the KMS model exposes a single encoder to 
> > >>> userspace.
> > >>> Exposing DRM encoder objects as part of the KMS UABI was probably a 
> > >>> mistake.
> > >>> Better solutions would have been to expose no encoder at all or all 
> > >>> encoders
> > >>> in the chain. We are however stuck with this model as we can't break 
> > >>> the UABI.
> > >>> For that reason a DRM encoder object doesn't represent an encoder but a 
> > >>> chain
> > >>> of encoders. As a DRM bridge models a single encoder, the DRM encoder 
> > >>> object
> > >>> must be created at a higher level, in the display driver.
> 
> I wonder why you created 'bridge's instead of simply adding links to
> the encoders? (that's what ASoC did: the audio CODECs are linked)
> This way, in simple cases (most cases), there would have been
>   crtc -> (encoder -> connector)
> instead of
>   crtc -> (bridge + encoder) -> (bridge + connector) 
> without any changes in the actual (encoder + connector)s.

Looking at drm_bridge_disable() and drm_bridge_enable(), the control
model appears to be:

crtc -> encoder -> connector
 `-> bridge
 `-> bridge
 `-> bridge

Bridges are always attached to an encoder, and there can be multiple
bridges attached to one encoder.  Bridges can't be attached to the
connector.

So, in the case of TDA998x, we end up with the TDA998x providing a
connector, because it has connector functionality, and providing a
bridge.  The encoder is left to the KMS driver, which adds additional
complexity (100+ lines) to each and every KMS driver, requiring the
KMS driver to have much more knowledge of what's attached to the
"CRTC", so it can create these encoders itself.  I still think this
is a backwards step - maybe one step forwards, two backwards.

Even if we were to provide helpers to create a dummy encoder to
work around the DRM bridge model short-comings, much of the 100+
lines is to do with working out whether or not we need to create an
encoder, and parsing the information we need to create that in a way
that existing DT doesn't break.

Then there's the fact that the bridge approach breaks non-DT at the
moment, because DRM bridge fundamentally requires DT.  This makes
DRM bridge useless for architectures which aren't DT aware, such as
x86.  So, converting drivers to be a DRM bridge effectively denies
their use on other architectures.  See drm_bridge.c, and look for
the references to bridge_list, noting that there are two: one which
adds to the bridge list, and one under #ifdef CONFIG_OF which looks
up a DT reference to a bridge.

There's another issue with TDA998x - we now have audio support in
TDA998x, and converting TDA998x to be a DRM bridge introduces some
fundamental (and as yet unsolved) races between the ASoC code and
the attachment of the DRM bridge to the DRM encoder, and the detachment
when the DRM bridge/connector gets cleaned up.  Right now, there's no
bridge callback when the encoder or drm_device goes away, so doing
stuff like:

static int tda998x_audio_get_eld(struct device *dev, void *data,
 uint8_t *buf, size_t len)
{
struct tda998x_priv *priv = dev_get_drvdata(dev);
struct drm_mode_config *config;
struct drm_connector *connector;
int ret = -ENODEV;

/* FIXME: This is racy */
if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
return ret;

config = >bridge.encoder->dev->mode_config;

mutex_lock(>mutex);
list_for_each_entry(connector, >connector_list, head) {
if (priv->bridge.encoder == connector->encoder) {
memcpy(buf, connector->eld,
   min(sizeof(connector->eld), len));
ret = 0;
}
}
mutex_unlock(>mutex);

feels very unsafe - nothing really guarantees the validity of the
priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
config->mutex lock does nothing to solve this.  The same problem
also exists in tda998x_audio_hw_params().

Anyway, the whole bridge conversion thing is moot until every user
of the TDA998x code has been updated to cope with the lack of
drm_connector_register() inside TDA998x - see Brian Starkey's
response to this patch set.  It's also moot if it breaks armada-drm.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at

[PATCH RFC 0/10] tda998x initial cleanups from bridge conversion

2016-10-23 Thread Russell King - ARM Linux

As part of the discussion about converting tda998x to a bridge, here's
some preparatory work for that, which includes a bunch of fixes.  I'm
sending this out _early_ as I'm not going to be working on any kernel
stuff next week (it's likely I won't even be reading email.)  So it may
be a little rough around the edges.

Essentially, this is a series of cleanups, complexity removal, and
avoiding races with the newly introduced audio support.  Even without
the bridge conversion, I think all these are still worthwhile to have.

This series of changes can also be found in my drm-tda998x-devel branch
as an unstable series of commits (iow, I'm going to rebase/rework these
at some point, so the commit IDs are not stable, so do not merge this.)

 git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel

 drivers/gpu/drm/i2c/tda998x_drv.c | 782 +++---
 1 file changed, 394 insertions(+), 388 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-22 Thread Russell King - ARM Linux

On Wed, Oct 19, 2016 at 10:46:48AM +0100, Brian Starkey wrote:
> Hi Jyri,
> 
> I believe this will break mali-dp and hdlcd, unless something changed
> while I wasn't looking. Please see this previous thread where I did
> the same thing and then had to have it reverted: [1]
> 
> Before removing this, we need to refactor (at least) mali-dp and hdlcd
> to move drm_dev_register() to the end of their ->bind() callback.
> 
> That could be done without moving drm_dev_unregister() to the start
> of ->unbind() if you really want to nuke the drm_connector_register()
> call, but to maintain symmetry (and introduce correctness) I was
> putting it off until I had a chance to remove drm_vblank_cleanup()
> from drm_dev_unregister() (because [2]).

So what is the status of this - when is it going to happen?  Without
this happening, I can't de-midlayer armada-drm, and I can't apply
these TDA998x patches.

As armada-drm stands at the moment, it can cope with the TDA998x
driver having the drm_connector_register(), or with it eliminated.
When armada-drm is de-midlayered without changing TDA998x, the
drm_connector_register() call in TDA998x produces a warning:

WARNING: CPU: 0 PID: 13 at lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8
kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)

I suspect that you will end up with the same problem when you move
the drm_dev_register() call after component_bind_all() - and I think
the answer is... you shouldn't have de-midlayered until TDA998x had
been updated to cope with de-midlayering, iow having had the
drm_connector_register() call removed.

Given that, I don't think we can avoid breaking mali-dp and hdlcd,
except by combining the change into a single patch, changing all three
drivers simultaneously (and any other driver which uses TDA998x which
has also been de-midlayered.)

So, what I would like to see is a single patch against Linus' 4.8.0
commit fixing mali-dp, hdlcd and any other driver, together with
removing drm_connector_register() from TDA998x.  This is so the patch
can be shared between all interested parties without forcing everyone
to 4.9-rc1.  Looking at the diff between 4.8 and 4.9-rc1 for
drivers/gpu/drm/arm, that shouldn't result in any merge conflicts -
and if you want to follow on from that with 4.9-rc1 development, you
can always merge 4.9-rc1 on top of that commit.

I'm happy to take such a patch and publish it via my git tree as part
of the TDA998x development if that helps - but either way we need it
shared between all parties.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: convert DT component matching to component_match_add_release()

2016-10-21 Thread Russell King - ARM Linux

On Thu, Oct 20, 2016 at 07:15:56PM -0400, Sean Paul wrote:
> On Thu, Oct 20, 2016 at 5:50 PM, Russell King - ARM Linux
>  wrote:
> > On Thu, Oct 20, 2016 at 04:39:04PM -0400, Sean Paul wrote:
> >> On Wed, Oct 19, 2016 at 6:28 AM, Russell King
> >> <rmk+kernel at armlinux.org.uk> wrote:
> >> > Convert DT component matching to use component_match_add_release().
> >> >
> >> > Signed-off-by: Russell King <rmk+kernel at armlinux.org.uk>
> >> > ---
> >> > Can we please get this patch from May merged into the drm-misc or
> >> > whatever trees so that we don't end up with conflicts?  I've no idea
> >> > who looks after drm-misc, as they have _still_ failed to add
> >> > themselves to MAINTAINERS.
> >>
> >> I think Daniel explained pretty clearly why this wasn't happening in
> >> the previous thread.
> >>
> >> Next time you send a v2, can you please mark it as such and include a
> >> "Changes in v2" section?
> >
> > Why - nothing's changed other than a rebase onto 4.9-rc1.  This isn't
> > a "I've changed it in XYZ way, so here's a new copy".
> 
> 
> Changes in v2: None
> 
> Is still useful since:
> a) the diffstat is different from v1, which necessitates me going
> through both versions to see what's changed from the previous reviews
> (only to find out myself that it's been rebased and a function has
> changed names)

Sorry, but I don't track in any fine detail what the changes are
between patches when I bring them forward: I have far too many patches
to do that (I have 300 right now) and I never know whether they're
going to result in a pull request or not.  It means finding some way
to manually attach a change log to each patch I send out, which will
be very time consuming.  I'd rather not send patches out than jump
through hoops like that, especially when the reason is that the
previous version was ignored.

> b) in June, you said you were going to roll a new version with the
> common OF bits extracted. it's nice to know at the outset that this
> has/hasn't happened

Now if you were following the rest of it, rather than just random parts
that suited you, you'd have noticed that I posted the new version, but
that caused more issues, so I publically said I was reverting back to
the original version.

> Also, prefacing the subject with [PATCH v2] or [PATCH RESEND] lets me
> know there is prior history with the change. Reading the previous
> version is helpful to see what reviewer's concerns were, and whether
> they've been addressed.

Yea, I'm very bad at changing the subject line in that way, and I'm
getting worse at it.  Sorry, I try my best, but I can't do better than
that.

> 
> 
> > It's being
> > posted in the hope that someone will finally either comment on it or
> > merge the damn thing, rather than ignoring it was done when it was
> > last posted.
> >
> >> >  drivers/gpu/drm/arm/hdlcd_drv.c |  3 ++-
> >> >  drivers/gpu/drm/arm/malidp_drv.c|  4 +++-
> >> >  drivers/gpu/drm/armada/armada_drv.c |  2 +-
> >> >  drivers/gpu/drm/drm_of.c| 28 
> >> > +++--
> >> >  drivers/gpu/drm/etnaviv/etnaviv_drv.c   |  5 +++--
> >> >  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  7 ---
> >> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  4 +++-
> >> >  drivers/gpu/drm/msm/msm_drv.c   | 12 ++-
> >> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  6 --
> >> >  drivers/gpu/drm/sti/sti_drv.c   |  5 +++--
> >> >  drivers/gpu/drm/sun4i/sun4i_drv.c   |  3 ++-
> >> >  drivers/gpu/drm/tilcdc/tilcdc_external.c|  4 +++-
> >> >  include/drm/drm_of.h| 12 +++
> >> >  13 files changed, 73 insertions(+), 22 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c 
> >> > b/drivers/gpu/drm/arm/hdlcd_drv.c
> >> > index fb6a418ce6be..6477d1a65266 100644
> >> > --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> >> > +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> >> > @@ -453,7 +453,8 @@ static int hdlcd_probe(struct platform_device *pdev)
> >> > return -EAGAIN;
> >> > }
> >> >
> >> > -   component_match_add(>dev, , compare_dev, port);
> >> > +   drm_of_component_match_add(>dev, , compare_dev, 
> >> > port);
> >> > +   of_node_put(port);
>

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

2016-10-30 Thread Russell King - ARM Linux

On Mon, Oct 24, 2016 at 10:39:27AM +0530, Archit Taneja wrote:
> Sorry about that. I'll post out a proper patch for this once we resolve
> the drm_bridge shortcomings you've mentioned. I can create one if you
> want to try it now.

As said elsewhere, I've been away, so I haven't had a chance to look
at anything.

I'm going to be spending the early part of this week catching up with
mail, but probably _not_ touching the kernel tree for any development
stuff until I've caught up.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

On Mon, Oct 24, 2016 at 08:53:04AM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 11:58:00AM +0530, Archit Taneja wrote:
> > On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> > > Looking at drm_bridge_disable() and drm_bridge_enable(), the control
> > > model appears to be:
> > > 
> > >   crtc -> encoder -> connector
> > >  `-> bridge
> > >`-> bridge
> > >`-> bridge
> > > 
> > > Bridges are always attached to an encoder, and there can be multiple
> > > bridges attached to one encoder.  Bridges can't be attached to the
> > > connector.
> 
> In helpers connectors are no-op objects. We _never_ call any connector
> callbacks when doing a modeset. Connectors are only used to probe output
> state, and as the userspace-visisble endpoint representation. Hence the
> real graph is
> 
> crtc -> encoder [ -> bridge [ -> bridge [...]]] -> connector
> 
> with the last bridge owning the connector. And that last bridge probably
> needs to store a pointer to its connector(s).

That model can't work for TDA998x if the TDA998x is followed by
another "bridge" (eg, to convert the TDMS signals to something else)
unless there's some way to tell a bridge that it isn't the last in
the chain.

However, my graph is accurate as it's reflecting the software
modelling - it reflects how the various objects are bound together in
DRM.  The DRM encoder has pointers to the DRM bridge, which has a
pointer to the next DRM bridge.  The DRM connector doesn't have any
pointers to the connector, only to the DRM encoder.  So, DRM bridges
are childs of the encoder, and the encoder (and attached encoder
bridge chain) can be selected by the DRM connector.

However, you are correct that for different "tasks" like mode setting,
or output probing, the representation is somewhat different - that's
not really what I was talking about though, and I certainly was not
talking about the userspace representation.

What I'm 100% concerned about is how this stuff looks in kernel space
and what the driver(s) should look like.

> > > So, in the case of TDA998x, we end up with the TDA998x providing a
> > > connector, because it has connector functionality, and providing a
> > > bridge.  The encoder is left to the KMS driver, which adds additional
> > > complexity (100+ lines) to each and every KMS driver, requiring the
> > > KMS driver to have much more knowledge of what's attached to the
> > > "CRTC", so it can create these encoders itself.  I still think this
> > > is a backwards step - maybe one step forwards, two backwards.
> 
> We've stubbed out everything that's in an encoder, you definitely don't
> need hundreds of lines to write one any more. If there's still a callback
> left around drm_encoder which has not yet suitable default handling, then
> that's a bug.

Sorry, but I do need exactly what I've written above, I can talk rather
definitively because I've actually got the code in front of me.  Most of
the additional lines is due to the complexity added to the KMS driver to
locate (actually for a third time) all the components in the system,
specifically parsing the DT tree to find the "encoders" (or rather the
TDA998x in this case), creating the DRM encoder objects, and binding the
TDA998x bridge.

Here's the _exact_ diffstat for the hacky conversion so far (including
something like the 10 patches I posted last weekend, which haven't had
any comments yet):

 drivers/gpu/drm/armada/armada_drv.c | 125 +
 drivers/gpu/drm/i2c/tda998x_drv.c   | 904 +---
 2 files changed, 560 insertions(+), 469 deletions(-)

The actual bridge conversion on its own is:

 drivers/gpu/drm/armada/armada_drv.c | 125 
 drivers/gpu/drm/i2c/tda998x_drv.c   | 139 ++--
 2 files changed, 180 insertions(+), 84 deletions(-)

> Note: Only applies to atomic though, I'm not going to bother with old
> legacy crtc helpers. I guess armada needs to switch to atomic, otherwise
> encoders are indeed a bit a pain.

That's not going to happen - and you know exactly why that's not going
to happen - I've tried to do it and it failed misterably with all sorts
of problems.  The idea that it can be done piecemeal as per your guide
is a falacy - it can't be.  There is no progressive way to do a
conversion.  It seems that KMS drivers need to be rewritten from
scratch, and that means there is a high risk of introducing lots of
new bugs.

I'm just not prepared to go through that - I'd much rather have a stable
kernel driver that actually works than spend the next six months rewriting
and debugging stuff just for the latest ideas about how stuff should be
done

[PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

On Mon, Oct 31, 2016 at 09:00:08AM +, Russell King - ARM Linux wrote:
> I need the patch against v4.8.  I'm happy to pick it up and add it
> to my drm-tda998x-devel branch, which you can then merge into
> drm-misc if you wish.

... or if Brian wants to send a git pull request to us with the
patch based on v4.8, we can all merge that instead.

Brian?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/5] drm/sun4i: Handle TV overscan

On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
> The first one is that this overscanning should be reported by the
> connector I guess? but this is really TV specific, so we need one way
> to let the user tell how the image is displayed on its side, and we
> cannot really autodetect it, and this needs to be done at runtime so
> that we can present some shiny interface to let it select which
> overscan ratio works for him/her.

See xbmc... they go through a nice shiny setup which includes adjusting
the visible area.  From what I remember, it has pointers on each corner
which you can adjust to be just visible on the screen, so xbmc knows
how much overscan there is, and xbmc itself reduces down to the user
set size.

> The second one is that we still need to expose the reduced modes to
> userspace, and not only the displayed size, so that the applications
> know what they must draw on. But I guess this could be adjusted by the
> core too.
> 
> In order to work consistently, I think all planes should be adjusted
> that way, so that relative coordinates are from the primary plane
> origin, and not the display origin. But that could be adjusted too by
> the core I guess.

I'm not sure about that - we want the graphics to be visible, but that
may not be appropriate for an video overlay frame.  It's quite common
for (eg) broadcast video to contain dead pixels or other artifacts on
the right hand side, and the broadcast video expects overscan to be
present.

I know this because I have run my TV with overscan disabled, even for
broadcast TV.

> The fourth one being the major one. Every time I raised the issue on
> IRC, the answer basically was "we don't care about analog", so I'm a
> bit pessimistic about whether dealing with this in the core would be
> accepted, hence why I chose to deal with this at the driver level.

Yea, that's quite sad, "analog" has become a dirty word, but really
this has nothing to do with "analog" at all - there are LCD TVs (and
some monitors) out there which take HDMI signals but refuse to
disable overscan no matter what you do to them if you provide them
with a "broadcast"  mode - so the analog excuse is very poor.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey  
> wrote:
> >>
> >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> b/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> index f4315bc..6e6fca2 100644
> >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> @@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
> >>> tda998x_connector_helper_funcs = {
> >>>
> >>>  static void tda998x_connector_destroy(struct drm_connector *connector)
> >>>  {
> >>> -   drm_connector_unregister(connector);
> >>> drm_connector_cleanup(connector);
> >>>  }
> >>>
> >>> @@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
> >>> struct device *master, void *data)
> >>> if (ret)
> >>> goto err_connector;
> >>>
> >>> -   ret = drm_connector_register(>connector);
> >>> -   if (ret)
> >>> -   goto err_sysfs;
> >>> -
> >>
> >>
> >> Instead of smashing all these patches into one, what about checking here
> >> for midlayer driver set with:
> >>
> >> /* register here for drivers still using midlayer load/unload */
> >> if (dev->driver->load)
> >> drm_connector_register(connector),
> >>
> >> Similar in other places. That way we wouldn't need to switch the world in
> >> one patch.
> >
> >
> > I don't think that helps. If we do that in isolation (first), then
> > mali-dp and hdlcd won't get their connectors registered because their
> > bind order is:
> >
> > drm_dev_register();
> > component_bind_all();
> >
> > If we change the mali-dp/hdlcd bind order first, then tda998x will
> > explode on drm_connector_register() until it's patched to remove that.
> >
> > As I mentioned in my mail to Russell, the only way I can see to avoid
> > patching all three drivers in one go is:
> >  1) Add (probably open-coded) drm_connector_register_all() to the end
> > of bind in hdlcd and mali-dp
> >  2) Patch tda998x to remove drm_connector_register()
> >  3) Reorder hdlcd/mali-dp bind and remove the connector registration
> > added in 1)
> >
> > We can do that, but it's extra churn for the same result, and none of
> > the 5 patches will really make sense in isolation anyway.
> 
> I thought there's also armada to take care of, which this patch would
> break?

NO NO NO NO NO.  I've said this several times.  Let's try it again,
and see if it sticks.

Because Armada has not been converted from a mid-layered driver, it
is _IMMUNE_ from any patch removing the drm_connector_register() call
in TDA998x.  It does _NOT_ break in any way.

Only those drivers which are de-mid-layered, and worked around the
drm_connector_register() call inside TDA998x (eg, mali) break, because
of the order in which they are _forced_ to call stuff.

In a de-mid-layered driver, with the drm_connector_register() call in
place in TDA998x, drm_dev_register() _MUST_ be called prior to
component_bind_all(), otherwise you get a WARN_ON() dump from the
kobject code.  With the drm_connector_register() call removed,
drm_dev_register() _MUST_ be called after component_bind_all() so that
the connector is registered.

It's the de-mid-layered drivers which are the problem here, not the
mid-layered ones like Armada.

> Maybe even another driver, so the hack would still be useful
> for those other drivers. And it would have been useful if malidp/hdlcd
> wouldn't have started out with the wrong init ordering ;-)

It's forced into the "wrong init ordering" due to the kobject WARN_ON.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c/tda998x: mark symbol static where possible

On Mon, Oct 24, 2016 at 04:27:45PM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 12:14:14PM +0200, Arnd Bergmann wrote:
> > On Saturday, October 22, 2016 5:14:42 PM CEST Baoyou Xie wrote:
> > > We get 1 warning when building kernel with W=1:
> > > drivers/gpu/drm/i2c/tda998x_drv.c:1292:5: warning: no previous prototype 
> > > for 'tda998x_audio_digital_mute' [-Wmissing-prototypes]
> > > 
> > > In fact, this function is only used in the file in which it is
> > > declared and don't need a declaration, but can be made static.
> > > So this patch marks this function with 'static'.
> > > 
> > > Signed-off-by: Baoyou Xie 
> > > 
> > 
> > Reviewed-by: Arnd Bergmann 
> 
> Applied to drm-misc, thanks.

Argh.  This conflicts with my patch series posted before I went away,
so now we're going to end up with merge conflicts... I suppose you're
happy to handle the conflict as you've bypassed waiting for an ack
from the driver maintainer...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/bridge: dw-hdmi: Set sink_is_hdmi and sink_has_audio in mode_set

On Sun, Oct 30, 2016 at 01:56:25PM +, James Le Cuirot wrote:
> These were previously set in dw_hdmi_connector_get_modes but this
> isn't called when the EDID is overridden. This can be seen in
> drm_helper_probe_single_connector_modes. Using the
> drm_kms_helper.edid_firmware parameter therefore always resulted in
> silence, even when providing the very same EDID that had previously
> been read from /sys.
> 
> I saw that other drivers set similar properties in mode_set rather
> than get_modes. radeon_audio_hdmi_mode_set is one such example. It
> calls radeon_connector_edid to retrieve the EDID so I drew inspiration
> from this for the fix.
> 
> I have tested this with a Utilite Pro on 4.9-rc3. I tried overriding
> the EDID with my own, not overriding the EDID, hotplugging the display
> after booting, and overriding the EDID with 1920x1080.bin. The latter
> has no audio parameters so no sound was heard as expected.

I'm not sure I particularly like this approach - the issue seems to
be that drm_helper_probe_single_connector_modes() can avoid calling
->get_modes(), at which point our ideas about the EDID-based
capabilities become stale.

I think it would be better to provide our own ->fill_modes implementation
which calls drm_helper_probe_single_connector_modes(), and then parses
the resulting EDID, rather than re-parsing it each time we set a mode.

We also need to apply this to the ELD as well - and several other
drivers are similarly buggy, and are going to need similar fixes (thanks
for pointing this problem out!)

> Notes:
> I do have some questions.
> 
> I don't know the significance of the mutex lock. I put my code inside
> it because I am modifying the hdmi properties. Is this necessary?
> Should it go before or after the lock instead?

It's there to ensure that ->previous_mode, ->disabled, and the power
management all operate atomically.

> I'm also wondering whether I should initially set both properties to
> false in case the EDID is missing but the driver didn't do this
> previously. Perhaps it should have?

The driver's private data is initially zero-ed, so that should be
unnecessary.

So maybe something like this instead - can you test please?

 drivers/gpu/drm/bridge/dw-hdmi.c | 30 +-
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 77ab47341658..878568af2d41 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge 
*bridge)
mutex_unlock(>mutex);
 }

+static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
+   uint32_t maxX, uint32_t maxY)
+{
+   struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
+connector);
+   struct edid *edid;
+   int ret;
+
+   ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+   edid = connector->edid_blob_ptr;
+   if (edid) {
+   hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+   hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
+   /* Store the ELD */
+   drm_edid_to_eld(connector, edid);
+   } else {
+   hdmi->sink_is_hdmi = false;
+   hdmi->sink_has_audio = false;
+   }
+
+   return ret;
+}
+
 static enum drm_connector_status
 dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct 
drm_connector *connector)
dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
edid->width_cm, edid->height_cm);

-   hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
-   hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
drm_mode_connector_update_edid_property(connector, edid);
ret = drm_add_edid_modes(connector, edid);
-   /* Store the ELD */
-   drm_edid_to_eld(connector, edid);
kfree(edid);
} else {
dev_dbg(hdmi->dev, "failed to get edid\n");
@@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector 
*connector)

 static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
.dpms = drm_atomic_helper_connector_dpms,
-   .fill_modes = drm_helper_probe_single_connector_modes,
+   .fill_modes = dw_hdmi_connector_fill_modes,
.detect = dw_hdmi_connector_detect,
.destroy = dw_hdmi_connector_destroy,
.force = dw_hdmi_connector_force,


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 1/8] drm/bridge: rgb-to-vga: Support an enable GPIO

On Thu, Oct 27, 2016 at 12:17:24AM +0200, Maxime Ripard wrote:
> Hi Rob,
> 
> On Wed, Oct 26, 2016 at 05:13:46PM -0500, Rob Herring wrote:
> > On Thu, Oct 20, 2016 at 11:43:37AM +0800, Chen-Yu Tsai wrote:
> > > Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> > > an enable pin on the bridge IC, or indirectly controlling a power
> > > switch.
> > > 
> > > Add support for it.
> > > 
> > > Signed-off-by: Chen-Yu Tsai 
> > > ---
> > >  .../bindings/display/bridge/dumb-vga-dac.txt   |  2 ++
> > >  drivers/gpu/drm/bridge/dumb-vga-dac.c  | 28 
> > > ++
> > >  2 files changed, 30 insertions(+)
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt 
> > > b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > index 003bc246a270..d3484822bf77 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > @@ -16,6 +16,8 @@ graph bindings specified in 
> > > Documentation/devicetree/bindings/graph.txt.
> > >  - Video port 0 for RGB input
> > >  - Video port 1 for VGA output
> > >  
> > > +Optional properties:
> > > +- enable-gpios: GPIO pin to enable or disable the bridge
> > 
> > This should also define the active state.
> > 
> > > +static void dumb_vga_enable(struct drm_bridge *bridge)
> > > +{
> > > + struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> > > +
> > > + if (vga->enable_gpio)
> > > + gpiod_set_value_cansleep(vga->enable_gpio, 1);
> > 
> > So the driver should allow either active high or low.
> 
> You mean like having a enable-active-high property? Isn't that
> redundant with the GPIO flags?

Correct - the gpiod APIs remove the need for drivers to know the
polarity of the signal, handling it inside the GPIO subsystem
instead, controlled either from the gpiod lookup tables in legacy
board files, or the DT specification for the GPIO.

So, in drivers, gpiod_set_value*(, 1) means "set gpio to active
level" and gpiod_set_value*(, 0) means "set gpio to inactive level".

Far nicer than all the bugs we've had with the legacy GPIO interfaces
with random different drivers implementing random different ways to
invert the signal, with all the pain that brings with it when a
platform comes along with a different inversion state.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH][resend] drm: bridge: add DesignWare HDMI I2S audio support

On Fri, Oct 28, 2016 at 01:22:21AM +, Kuninori Morimoto wrote:
> +static struct platform_driver snd_dw_hdmi_driver = {
> + .probe  = snd_dw_hdmi_probe,

The driver must have a .remove function, because the platform device it
is binding against can appear and disappear.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
> On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> > Hi Daniel,
> > 
> > On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey  
> > > wrote:
> > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > index f4315bc..6e6fca2 100644
> > > > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > @@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
> > > > > > tda998x_connector_helper_funcs = {
> > > > > > 
> > > > > >  static void tda998x_connector_destroy(struct drm_connector 
> > > > > > *connector)
> > > > > >  {
> > > > > > -   drm_connector_unregister(connector);
> > > > > > drm_connector_cleanup(connector);
> > > > > >  }
> > > > > > 
> > > > > > @@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
> > > > > > struct device *master, void *data)
> > > > > > if (ret)
> > > > > > goto err_connector;
> > > > > > 
> > > > > > -   ret = drm_connector_register(>connector);
> > > > > > -   if (ret)
> > > > > > -   goto err_sysfs;
> > > > > > -
> > > > > 
> > > > > 
> > > > > Instead of smashing all these patches into one, what about checking 
> > > > > here
> > > > > for midlayer driver set with:
> > > > > 
> > > > > /* register here for drivers still using midlayer load/unload 
> > > > > */
> > > > > if (dev->driver->load)
> > > > > drm_connector_register(connector),
> > > > > 
> > > > > Similar in other places. That way we wouldn't need to switch the 
> > > > > world in
> > > > > one patch.
> > > > 
> > > > 
> > > > I don't think that helps. If we do that in isolation (first), then
> > > > mali-dp and hdlcd won't get their connectors registered because their
> > > > bind order is:
> > > > 
> > > > drm_dev_register();
> > > > component_bind_all();
> > > > 
> > > > If we change the mali-dp/hdlcd bind order first, then tda998x will
> > > > explode on drm_connector_register() until it's patched to remove that.
> > > > 
> > > > As I mentioned in my mail to Russell, the only way I can see to avoid
> > > > patching all three drivers in one go is:
> > > >  1) Add (probably open-coded) drm_connector_register_all() to the end
> > > > of bind in hdlcd and mali-dp
> > > >  2) Patch tda998x to remove drm_connector_register()
> > > >  3) Reorder hdlcd/mali-dp bind and remove the connector registration
> > > > added in 1)
> > > > 
> > > > We can do that, but it's extra churn for the same result, and none of
> > > > the 5 patches will really make sense in isolation anyway.
> > > 
> > > I thought there's also armada to take care of, which this patch would
> > > break? Maybe even another driver, so the hack would still be useful
> > > for those other drivers.
> > 
> > OK it seems like this situation has got very confused. In short, this
> > patch does not break armada. Russell previously tested the tda998x
> > patch against armada and reported no issues.
> > Drivers with a ->load() callback don't need to register the connector
> > anymore, because drm_dev_register() does it for them.
> > 
> > As far as I know, this patch touching these three drivers is
> > sufficient to allow all current users of tda998x to continue using it,
> > whilst also allowing armada and tilcdc to be de-midlayered.
> 
> Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?

I need the patch against v4.8.  I'm happy to pick it up and add it
to my drm-tda998x-devel branch, which you can then merge into
drm-misc if you wish.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c: tda998x: don't register the connector

On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
>  wrote:
> > On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> >> Actually, could you please hold off picking this up? We need to make
> >> changes in mali-dp and hdlcd or this will mess up their registration.
> >> I will send those patches later today, but better if this all goes in
> >> together (whenever that ends up being).
> >
> > Sorry, but I'm annoyed with this - the impression being given was that
> > I was holding up this patch by not testing it on Armada, and I brought
> > up the issue about registration at the beginning of this.
> >
> > Now we're _just_ finding out that there are drivers where removing the
> > connector registration in tda998x causes them to break?  It's a bit
> > late to be checking your own drivers when you've been chasing me...
> >
> > Sorry, but it sounds like we're not ready to make this change - and as
> > it's the very last day that changes will appear in linux-next prior to
> > the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> > holding off until after the merge window is over, so we can get some
> > testing with these other two drivers with this change in place.
> >
> 
> sigh. I just pushed my queue to drm-misc, which included this patch.
> Sounds like I should revert?

Why did you do push it _without_ an ack from the maintainer of the
driver when the comments on the list were clearly indicating that it
was waiting for testing and such an ack?

There's something horribly wrong with the process here.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c: tda998x: don't register the connector

2016-09-21 Thread Russell King - ARM Linux

On Wed, Sep 21, 2016 at 09:57:38AM +0100, Brian Starkey wrote:
> Hi Russell,
> 
> Are you in a position to be able to test this now?

Normally, I'd say no, because I'd normally wait for 4.8 to be out before
moving the cubox tree up.  However, as we're close to 4.8, I've merged
4.8-rc7 in (and fixed the multitude of conflicts), and manually made the
changes in your patch.  Nothing seems to have broken, so I think we're
good.

Acked-by: Russell King <rmk+kernel at armlinux.org.uk>

Daniel, please take this change through the drm-misc tree as I'm unlikely
to have a branch which I can apply it to until after the merge window
opens.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c: tda998x: don't register the connector

On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> Actually, could you please hold off picking this up? We need to make
> changes in mali-dp and hdlcd or this will mess up their registration.
> I will send those patches later today, but better if this all goes in
> together (whenever that ends up being).

Sorry, but I'm annoyed with this - the impression being given was that
I was holding up this patch by not testing it on Armada, and I brought
up the issue about registration at the beginning of this.

Now we're _just_ finding out that there are drivers where removing the
connector registration in tda998x causes them to break?  It's a bit
late to be checking your own drivers when you've been chasing me...

Sorry, but it sounds like we're not ready to make this change - and as
it's the very last day that changes will appear in linux-next prior to
the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
holding off until after the merge window is over, so we can get some
testing with these other two drivers with this change in place.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c: tda998x: don't register the connector

On Thu, Sep 22, 2016 at 05:32:35AM -0700, Sean Paul wrote:
> On Thu, Sep 22, 2016 at 5:09 AM, Russell King - ARM Linux
>  wrote:
> > On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
> >> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
> >>  wrote:
> >> > On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> >> >> Actually, could you please hold off picking this up? We need to make
> >> >> changes in mali-dp and hdlcd or this will mess up their registration.
> >> >> I will send those patches later today, but better if this all goes in
> >> >> together (whenever that ends up being).
> >> >
> >> > Sorry, but I'm annoyed with this - the impression being given was that
> >> > I was holding up this patch by not testing it on Armada, and I brought
> >> > up the issue about registration at the beginning of this.
> >> >
> >> > Now we're _just_ finding out that there are drivers where removing the
> >> > connector registration in tda998x causes them to break?  It's a bit
> >> > late to be checking your own drivers when you've been chasing me...
> >> >
> >> > Sorry, but it sounds like we're not ready to make this change - and as
> >> > it's the very last day that changes will appear in linux-next prior to
> >> > the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> >> > holding off until after the merge window is over, so we can get some
> >> > testing with these other two drivers with this change in place.
> >> >
> >>
> >> sigh. I just pushed my queue to drm-misc, which included this patch.
> >> Sounds like I should revert?
> >
> > Why did you do push it _without_ an ack from the maintainer of the
> > driver when the comments on the list were clearly indicating that it
> > was waiting for testing and such an ack?
> >
> 
> Daniel Acked it on the list on 7/25, and you acked it yesterday asking
> it to be merged to -misc. What am I missing?

Sorry, I thought you were some random person maintaining some random
tree who'd submitted a pull request to be merged into drm-misc.  If
you are in fact the drm-misc maintainer, please add yourself to the
MAINTAINERS file so that everyone knows who you are.  Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm/i2c: tda998x: don't register the connector