[PATCH] drm/tilcdc: Add atomic and crtc headers to crtc.c

[Jyri Sarha]

On 09/22/16 09:18, Daniel Vetter wrote:
> On Wed, Sep 21, 2016 at 06:36:28AM -0700, Sean Paul wrote:
>> Also reorder alphabetically and fix up drm_flip_work header.
>>
>> Signed-off-by: Sean Paul 
> 
> Reviewed-by: Daniel Vetter 
> 

Picked this up. Thanks!

Best regards,
Jyri

>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
>> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 2087689..cb9df10 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -15,9 +15,11 @@
>>   * this program.  If not, see .
>>   */
>>  
>> -#include "drm_flip_work.h"
>> -#include 
>> +#include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>>  
>>  #include "tilcdc_drv.h"
>>  #include "tilcdc_regs.h"
>> -- 
>> 2.8.0.rc3.226.g39d4020
>>
> 

[GIT PULL] tilcdc 3rd set of changes for v4.9

[Jyri Sarha]

Hi Dave,
Please pull these collected fixes and cleanups from various sources. The
request was rebased on top the previous pull tilcdc request tag, so it
contains all the accumulated tilcdc changes for v4.9 so far.

Thanks,
Jyri

The following changes since commit 2e0965b06d90b9dba76198d026c4c2ee04443aca:

  drm/tilcdc: WARN if CRTC is touched without CRTC lock (2016-09-07
15:54:43 +0300)

are available in the git repository at:

  https://github.com/jsarha/linux tags/tilcdc-4.9-3

for you to fetch changes up to 55fd61dfb8e37e27dcd44503f9ac3d676a5c3896:

  drm/tilcdc: Return directly after a failed kfree_table_init() in
tilcdc_convert_slave_node() (2016-09-23 00:09:46 +0300)


3rd drm/tilcdc pull request for v4.9.


Baoyou Xie (2):
  drm/tilcdc: add missing header dependencies
  drm/tilcdc: mark symbols static where possible

Jyri Sarha (1):
  drm/tilcdc: Remove "default" from blue-and-red-wiring property binding

Markus Elfring (1):
  drm/tilcdc: Return directly after a failed kfree_table_init() in
tilcdc_convert_slave_node()

Sean Paul (1):
  drm/tilcdc: Add atomic and crtc headers to crtc.c

Wei Yongjun (1):
  drm/tilcdc: Fix non static symbol warning

 Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt | 6 +++---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c| 6 --
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 4 ++--
 drivers/gpu/drm/tilcdc/tilcdc_panel.c   | 1 +
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c| 8 
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c  | 1 +
 6 files changed, 15 insertions(+), 11 deletions(-)

[Bug 97879] [amdgpu] Rocket League: long hangs (several seconds) when loading assets (models/textures/shaders?)

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=97879

--- Comment #12 from Michel Dänzer  ---
(In reply to Andreas Hartmetz from comment #4)
> Assuming the problem was shader compilation, what could be done about it,
> though? Optimizing shader compilation by a factor of 10 seems unrealistic,

Not necessarily. Let's worry about that once the code taking so much time is
identified and analyzed. For that purpose, what's needed next is CPU profiles
of the stalls.


> a disk cache for shaders has been rejected (right?)

No, it hasn't.


> and would not always help, e.g. when somebody with a car that uses a new
> asset joins.

Right, there's only so much the driver stack can do when the game (engine)
keeps compiling shaders.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/539572fa/attachment.html>

linux-next: manual merge of the drm-misc tree with Linus' tree

[Stephen Rothwell]

Hi all,

Today's linux-next merge of the drm-misc tree got a conflict in:

  drivers/gpu/drm/drm_crtc.c

between commit:

  6f00975c6190 ("drm: Reject page_flip for !DRIVER_MODESET")

from Linus' tree and commit:

  43968d7b806d ("drm: Extract drm_plane.[hc]")

from the drm-misc tree.

I fixed it up (the latter incorporated the former, so I just used the
latter) and can carry the fix as necessary. This is now fixed as far as
linux-next is concerned, but any non trivial conflicts should be mentioned
to your upstream maintainer when your tree is submitted for merging.
You may also want to consider cooperating with the maintainer of the
conflicting tree to minimise any particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

[Bug 172421] radeon: allow to set the TMDS frequency by a special kernel parameter

[bugzilla-dae...@bugzilla.kernel.org]

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

Roland Scheidegger  changed:

   What|Removed |Added

 CC||rscheidegger at gmx.ch

--- Comment #13 from Roland Scheidegger  ---
(In reply to Elmar Stellnberger from comment #9)
> any other opinion by someone else on this issue?

FWIW enthusiasts love such things, but corporations do not. Hence you don't see
overclocking and similar unspecified stuff in drivers not maintained by
community generally.
Personally I've always thought the risk of damaging hardware with any kind of
overclocking is just about exactly zero as long as you don't increase voltage
levels (and you can handle the additional heat but that should be a non-issue
here). That's my limited understanding of the physics behind it :-).
I suspect part of the reason why it overclocks so well is also that this chip
should be DP 1.2 capable - meaning HBR2 mode which has a clock of 270Mhz (not
that you can really get cards with that chip which actually do have the DP
port...). Now the signalling is different with DP vs. HDMI/DVI but if I had to
take a guess I'd suspect the hw is mostly all the same.
But none of that is going to change the opinion of overclocking of anyone...

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

[PATCH] drm: Fix plane type uabi breakage

[Daniel Vetter]

Turns out assuming that only stuff in uabi is uabi is a bit naive, and
we have a bunch of properties for which the enum values are placed in
random headers. A proper fix would be to split out uapi include
headers, but meanwhile sprinkle at least some warning over them.

Fixes: 532b36712ddf ("drm/doc: Polish for drm_plane.[hc]")
Cc: Archit Taneja 
Cc: Sean Paul 
Signed-off-by: Daniel Vetter 
---
 include/drm/drm_blend.h |  3 +++
 include/drm/drm_plane.h | 19 +++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 868f0364e939..36baa175de99 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -33,6 +33,9 @@ struct drm_atomic_state;
  * Rotation property bits. DRM_ROTATE_ rotates the image by the
  * specified amount in degrees in counter clockwise direction. DRM_REFLECT_X 
and
  * DRM_REFLECT_Y reflects the image along the specified axis prior to rotation
+ *
+ * WARNING: These defines are UABI since they're exposed in the rotation
+ * property.
  */
 #define DRM_ROTATE_0   BIT(0)
 #define DRM_ROTATE_90  BIT(1)
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 256219bfd07b..43cf193e54d6 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -333,9 +333,20 @@ struct drm_plane_funcs {
  * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that they
  * wish to receive a universal plane list containing all plane types. See also
  * drm_for_each_legacy_plane().
+ *
+ * WARNING: The values of this enum is UABI since they're exposed in the "type"
+ * property.
  */
 enum drm_plane_type {
/**
+* @DRM_PLANE_TYPE_OVERLAY:
+*
+* Overlay planes represent all non-primary, non-cursor planes. Some
+* drivers refer to these types of planes as "sprites" internally.
+*/
+   DRM_PLANE_TYPE_OVERLAY,
+
+   /**
 * @DRM_PLANE_TYPE_PRIMARY:
 *
 * Primary planes represent a "main" plane for a CRTC.  Primary planes
@@ -353,14 +364,6 @@ enum drm_plane_type {
 * DRM_IOCTL_MODE_CURSOR2 IOCTLs.
 */
DRM_PLANE_TYPE_CURSOR,
-
-   /**
-* @DRM_PLANE_TYPE_OVERLAY:
-*
-* Overlay planes represent all non-primary, non-cursor planes. Some
-* drivers refer to these types of planes as "sprites" internally.
-*/
-   DRM_PLANE_TYPE_OVERLAY,
 };


-- 
2.7.4

[Intel-gfx] [PATCH] drm: Fix plane type uabi breakage

[Jani Nikula]

On Fri, 23 Sep 2016, Daniel Vetter  wrote:
> Turns out assuming that only stuff in uabi is uabi is a bit naive, and
> we have a bunch of properties for which the enum values are placed in
> random headers. A proper fix would be to split out uapi include
> headers, but meanwhile sprinkle at least some warning over them.
>
> Fixes: 532b36712ddf ("drm/doc: Polish for drm_plane.[hc]")
> Cc: Archit Taneja 
> Cc: Sean Paul 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Jani Nikula 


> ---
>  include/drm/drm_blend.h |  3 +++
>  include/drm/drm_plane.h | 19 +++
>  2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 868f0364e939..36baa175de99 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -33,6 +33,9 @@ struct drm_atomic_state;
>   * Rotation property bits. DRM_ROTATE_ rotates the image by the
>   * specified amount in degrees in counter clockwise direction. DRM_REFLECT_X 
> and
>   * DRM_REFLECT_Y reflects the image along the specified axis prior to 
> rotation
> + *
> + * WARNING: These defines are UABI since they're exposed in the rotation
> + * property.
>   */
>  #define DRM_ROTATE_0 BIT(0)
>  #define DRM_ROTATE_90BIT(1)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 256219bfd07b..43cf193e54d6 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -333,9 +333,20 @@ struct drm_plane_funcs {
>   * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that 
> they
>   * wish to receive a universal plane list containing all plane types. See 
> also
>   * drm_for_each_legacy_plane().
> + *
> + * WARNING: The values of this enum is UABI since they're exposed in the 
> "type"
> + * property.
>   */
>  enum drm_plane_type {
>   /**
> +  * @DRM_PLANE_TYPE_OVERLAY:
> +  *
> +  * Overlay planes represent all non-primary, non-cursor planes. Some
> +  * drivers refer to these types of planes as "sprites" internally.
> +  */
> + DRM_PLANE_TYPE_OVERLAY,
> +
> + /**
>* @DRM_PLANE_TYPE_PRIMARY:
>*
>* Primary planes represent a "main" plane for a CRTC.  Primary planes
> @@ -353,14 +364,6 @@ enum drm_plane_type {
>* DRM_IOCTL_MODE_CURSOR2 IOCTLs.
>*/
>   DRM_PLANE_TYPE_CURSOR,
> -
> - /**
> -  * @DRM_PLANE_TYPE_OVERLAY:
> -  *
> -  * Overlay planes represent all non-primary, non-cursor planes. Some
> -  * drivers refer to these types of planes as "sprites" internally.
> -  */
> - DRM_PLANE_TYPE_OVERLAY,
>  };

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm: Fix plane type uabi breakage

[Archit Taneja]

On 09/23/2016 12:05 PM, Daniel Vetter wrote:
> Turns out assuming that only stuff in uabi is uabi is a bit naive, and
> we have a bunch of properties for which the enum values are placed in
> random headers. A proper fix would be to split out uapi include
> headers, but meanwhile sprinkle at least some warning over them.
>
> Fixes: 532b36712ddf ("drm/doc: Polish for drm_plane.[hc]")
> Cc: Archit Taneja 
> Cc: Sean Paul 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Archit Taneja 

> ---
>  include/drm/drm_blend.h |  3 +++
>  include/drm/drm_plane.h | 19 +++
>  2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 868f0364e939..36baa175de99 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -33,6 +33,9 @@ struct drm_atomic_state;
>   * Rotation property bits. DRM_ROTATE_ rotates the image by the
>   * specified amount in degrees in counter clockwise direction. DRM_REFLECT_X 
> and
>   * DRM_REFLECT_Y reflects the image along the specified axis prior to 
> rotation
> + *
> + * WARNING: These defines are UABI since they're exposed in the rotation
> + * property.
>   */
>  #define DRM_ROTATE_0 BIT(0)
>  #define DRM_ROTATE_90BIT(1)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 256219bfd07b..43cf193e54d6 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -333,9 +333,20 @@ struct drm_plane_funcs {
>   * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that 
> they
>   * wish to receive a universal plane list containing all plane types. See 
> also
>   * drm_for_each_legacy_plane().
> + *
> + * WARNING: The values of this enum is UABI since they're exposed in the 
> "type"
> + * property.
>   */
>  enum drm_plane_type {
>   /**
> +  * @DRM_PLANE_TYPE_OVERLAY:
> +  *
> +  * Overlay planes represent all non-primary, non-cursor planes. Some
> +  * drivers refer to these types of planes as "sprites" internally.
> +  */
> + DRM_PLANE_TYPE_OVERLAY,
> +
> + /**
>* @DRM_PLANE_TYPE_PRIMARY:
>*
>* Primary planes represent a "main" plane for a CRTC.  Primary planes
> @@ -353,14 +364,6 @@ enum drm_plane_type {
>* DRM_IOCTL_MODE_CURSOR2 IOCTLs.
>*/
>   DRM_PLANE_TYPE_CURSOR,
> -
> - /**
> -  * @DRM_PLANE_TYPE_OVERLAY:
> -  *
> -  * Overlay planes represent all non-primary, non-cursor planes. Some
> -  * drivers refer to these types of planes as "sprites" internally.
> -  */
> - DRM_PLANE_TYPE_OVERLAY,
>  };
>
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Intel-gfx] [PATCH] drm: Fix plane type uabi breakage

[Sean Paul]

On Thu, Sep 22, 2016 at 11:44 PM, Jani Nikula
 wrote:
> On Fri, 23 Sep 2016, Daniel Vetter  wrote:
>> Turns out assuming that only stuff in uabi is uabi is a bit naive, and
>> we have a bunch of properties for which the enum values are placed in
>> random headers. A proper fix would be to split out uapi include
>> headers, but meanwhile sprinkle at least some warning over them.
>>
>> Fixes: 532b36712ddf ("drm/doc: Polish for drm_plane.[hc]")
>> Cc: Archit Taneja 
>> Cc: Sean Paul 
>> Signed-off-by: Daniel Vetter 
>
> Reviewed-by: Jani Nikula 
>

Thanks, applied to drm-misc

Sean

>
>> ---
>>  include/drm/drm_blend.h |  3 +++
>>  include/drm/drm_plane.h | 19 +++
>>  2 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>> index 868f0364e939..36baa175de99 100644
>> --- a/include/drm/drm_blend.h
>> +++ b/include/drm/drm_blend.h
>> @@ -33,6 +33,9 @@ struct drm_atomic_state;
>>   * Rotation property bits. DRM_ROTATE_ rotates the image by the
>>   * specified amount in degrees in counter clockwise direction. 
>> DRM_REFLECT_X and
>>   * DRM_REFLECT_Y reflects the image along the specified axis prior to 
>> rotation
>> + *
>> + * WARNING: These defines are UABI since they're exposed in the rotation
>> + * property.
>>   */
>>  #define DRM_ROTATE_0 BIT(0)
>>  #define DRM_ROTATE_90BIT(1)
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 256219bfd07b..43cf193e54d6 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -333,9 +333,20 @@ struct drm_plane_funcs {
>>   * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that 
>> they
>>   * wish to receive a universal plane list containing all plane types. See 
>> also
>>   * drm_for_each_legacy_plane().
>> + *
>> + * WARNING: The values of this enum is UABI since they're exposed in the 
>> "type"
>> + * property.
>>   */
>>  enum drm_plane_type {
>>   /**
>> +  * @DRM_PLANE_TYPE_OVERLAY:
>> +  *
>> +  * Overlay planes represent all non-primary, non-cursor planes. Some
>> +  * drivers refer to these types of planes as "sprites" internally.
>> +  */
>> + DRM_PLANE_TYPE_OVERLAY,
>> +
>> + /**
>>* @DRM_PLANE_TYPE_PRIMARY:
>>*
>>* Primary planes represent a "main" plane for a CRTC.  Primary planes
>> @@ -353,14 +364,6 @@ enum drm_plane_type {
>>* DRM_IOCTL_MODE_CURSOR2 IOCTLs.
>>*/
>>   DRM_PLANE_TYPE_CURSOR,
>> -
>> - /**
>> -  * @DRM_PLANE_TYPE_OVERLAY:
>> -  *
>> -  * Overlay planes represent all non-primary, non-cursor planes. Some
>> -  * drivers refer to these types of planes as "sprites" internally.
>> -  */
>> - DRM_PLANE_TYPE_OVERLAY,
>>  };
>
> --
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[GIT PULL] tilcdc 3rd set of changes for v4.9

[Sean Paul]

On Thu, Sep 22, 2016 at 2:37 PM, Jyri Sarha  wrote:
> Hi Dave,
> Please pull these collected fixes and cleanups from various sources. The
> request was rebased on top the previous pull tilcdc request tag, so it
> contains all the accumulated tilcdc changes for v4.9 so far.
>
> Thanks,
> Jyri
>
> The following changes since commit 2e0965b06d90b9dba76198d026c4c2ee04443aca:
>
>   drm/tilcdc: WARN if CRTC is touched without CRTC lock (2016-09-07
> 15:54:43 +0300)
>
> are available in the git repository at:
>
>   https://github.com/jsarha/linux tags/tilcdc-4.9-3
>
> for you to fetch changes up to 55fd61dfb8e37e27dcd44503f9ac3d676a5c3896:
>
>   drm/tilcdc: Return directly after a failed kfree_table_init() in
> tilcdc_convert_slave_node() (2016-09-23 00:09:46 +0300)
>
> 
> 3rd drm/tilcdc pull request for v4.9.
>
> 
> Baoyou Xie (2):
>   drm/tilcdc: add missing header dependencies
>   drm/tilcdc: mark symbols static where possible
>
> Jyri Sarha (1):
>   drm/tilcdc: Remove "default" from blue-and-red-wiring property binding
>
> Markus Elfring (1):
>   drm/tilcdc: Return directly after a failed kfree_table_init() in
> tilcdc_convert_slave_node()
>
> Sean Paul (1):
>   drm/tilcdc: Add atomic and crtc headers to crtc.c

I posted on the patch thread, but I've picked this up in drm-misc
since Daniel's plane/blending patchset depends on this fix.

Sean

>
> Wei Yongjun (1):
>   drm/tilcdc: Fix non static symbol warning
>
>  Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt | 6 +++---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c| 6 --
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 4 ++--
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c   | 1 +
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c| 8 
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c  | 1 +
>  6 files changed, 15 insertions(+), 11 deletions(-)

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

[Daniel Vetter]

On Thu, Sep 22, 2016 at 4:14 PM, Brian Starkey  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?
>>
>
> Yes, please revert this. There's a problem in the fbdev helper code
> which stops me fixing this quickly, so better to revert it.

Hm, what's the trouble wih fbdev? But yeah given this trouble I'd go
with a revert for now. For the real fix I guess we could just squash
it all in one, kinda pointless to go overboard for this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Sep 22, 2016 at 2:40 PM, Russell King - ARM Linux
 wrote:
> 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.


drm-misc has a different model than most other kernel trees. It
started out with drm-intel, and I'm going to present about it at KS.
One thing I want to fix with that discussion is some agreement on how
these trees should be handled in MAINTAINERS.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Sean Paul]

This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.

commit 6a2925ea12006911c8180a89feda6d040873ed18
Author: Brian Starkey 
Date:   Mon Jul 25 11:55:48 2016 +0100

drm/i2c: tda998x: don't register the connector

[seanpaul]
Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
until this is sorted.

Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d..9798d40 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1584,6 +1584,7 @@ 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);
 }

@@ -1655,10 +1656,16 @@ static int tda998x_bind(struct device *dev, struct 
device *master, void *data)
if (ret)
goto err_connector;

+   ret = drm_connector_register(&priv->connector);
+   if (ret)
+   goto err_sysfs;
+
drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);

return 0;

+err_sysfs:
+   drm_connector_cleanup(&priv->connector);
 err_connector:
drm_encoder_cleanup(&priv->encoder);
 err_encoder:
@@ -1671,6 +1678,7 @@ static void tda998x_unbind(struct device *dev, struct 
device *master,
 {
struct tda998x_priv *priv = dev_get_drvdata(dev);

+   drm_connector_unregister(&priv->connector);
drm_connector_cleanup(&priv->connector);
drm_encoder_cleanup(&priv->encoder);
tda998x_destroy(priv);
-- 
2.8.0.rc3.226.g39d4020

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Sean Paul]

On Thu, Sep 22, 2016 at 1:24 PM, Dan Carpenter  
wrote:
> On Thu, Sep 22, 2016 at 03:11:25PM +0200, SF Markus Elfring wrote:
>> > If you restricted yourself to fixing bugs only then you would maybe fix 
>> > more
>> > bugs than you introduce but as it you are making the kernel worse.
>>
>> Would you like to discuss the statistics for my failure (or success) rate
>> a bit more so that involved issues can be clarified in a constructive way?
>
> It should be that you target 20 bug fixes for each new regression that
> you add.
>
> Since you are just sending clean ups, every bug you introduce sets us
> further and further back.  There is no hope for improving the kernel
> because you are not even trying to fix 20 bugs, only introducing them.
>
> Once you fix 20 bugs, then you will be even and you can start sending
> cleanups again.  This is fair.
>

At the risk of piling on, but hopefully to benefit Markus going forward:

I will refrain from merging any more style/checkpatch/"code cleanup"
patches from Markus until we start getting real, tested, bug fixes.

Sean


> regards,
> dan carpenter
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Bug 94900] HD6950 GPU lockup loop with various steam games (octodad[always], saints row 4[always], dead island[always], grid autosport[sometimes])

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=94900

--- Comment #27 from Heiko  ---
Well, tiling and hyz were disabled per default as well, until bugs were ironed
out. So I'd go with the overall stability and use sbsafemath as default for now
(or just disable the calls to fold_assoc()).

And you're still able to revert with R600_DEBUG settings, if there is need for
that particular performance boost. Are there benchmark results to show the
gains for sb[no]safemath?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/0d43c18d/attachment.html>

[Bug 172421] radeon: allow to set the TMDS frequency by a special kernel parameter

[bugzilla-dae...@bugzilla.kernel.org]

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

--- Comment #14 from Christian König  ---
(In reply to Roland Scheidegger from comment #13)
> Personally I've always thought the risk of damaging hardware with any kind
> of overclocking is just about exactly zero as long as you don't increase
> voltage levels

Unfortunately this is exactly what happens here. The clock is generated by a
voltage controlled oscillator and for the desired resolution you need to over
clock it by about 30-40%.

That in turn means you raise the voltage way over the nominal limit.

Those oscillators are designed to handle voltages about 250% over the nominal
level without frying immediately, but that says absolutely nothing about the
aging of the circuit under those conditions.

The PLL we are talking about here clearly isn't designed for that level of
operation and even the closed source driver (which are otherwise rather
friendly to overclocking) don't let the user override this absolute limit.

So this is a clearly NAK from my side.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

> I think the "if (node)" in the of_node_put() is there on purpose,

Yes, of course.

Does such an implementation detail correspond to a general software design 
pattern?


> because it potentially saves the caller one extra if()-statement

This can occasionally happen.


> and keeps the caller code simpler.

A special view on software simplicity can also lead to questionable intermediate
function implementation, can't it?


> Keeping the goto labels in right order needs precision

I can agree to this view.


> and can lead to subtle errors.

The management of jump labels is just another software development challenge
as usual, isn't it?


> Sometimes there is no way to avoid that,

How do you think about to clarify the constraints which you imagine a bit more?


> but here there is.

I disagree to this conclusion.

Would you like to care a bit more for efficiency and software correctness
around the discussed exception handling?

Regards,
Markus

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Dave Airlie]

On 23 September 2016 at 17:25, Sean Paul  wrote:
> On Thu, Sep 22, 2016 at 1:24 PM, Dan Carpenter  
> wrote:
>> On Thu, Sep 22, 2016 at 03:11:25PM +0200, SF Markus Elfring wrote:
>>> > If you restricted yourself to fixing bugs only then you would maybe fix 
>>> > more
>>> > bugs than you introduce but as it you are making the kernel worse.
>>>
>>> Would you like to discuss the statistics for my failure (or success) rate
>>> a bit more so that involved issues can be clarified in a constructive way?
>>
>> It should be that you target 20 bug fixes for each new regression that
>> you add.
>>
>> Since you are just sending clean ups, every bug you introduce sets us
>> further and further back.  There is no hope for improving the kernel
>> because you are not even trying to fix 20 bugs, only introducing them.
>>
>> Once you fix 20 bugs, then you will be even and you can start sending
>> cleanups again.  This is fair.
>>
>
> At the risk of piling on, but hopefully to benefit Markus going forward:
>
> I will refrain from merging any more style/checkpatch/"code cleanup"
> patches from Markus until we start getting real, tested, bug fixes.

I'd prefer if everyone on dri-devel just ignored Markus at this stage,

If you are going to pick up his patches, please spend time making sure they
are correct and tested, as he doesn't seem to.

Markus, please contact the list in advance in future before posting a bunch
of patches that don't fix any problems.

Dave.

[PATCH 0/2] Use drm-core helpers to wait for updates

[Andrzej Hajda]

Hi Inki,

These two patches replaces exynos-drm specific mechanism for waiting
for vblanks with the one provided by drm-core.
The patchset has nice diffstat and fixes some issues,
described in datail in patches.

As the patches touches exynos-drm core I have tested them in different
scenarios on different pipelines:
- TM2: vidi, tv, dsi,
- Odroid-u3: vidi, tv,
- Universal-c210: vidi, tv, rgb.

In all cases it worked.

Regards
Andrzej


Andrzej Hajda (2):
  drm/exynos/vidi: use timer for vblanks instead of sleeping worker
  drm/exynos: fix pending update handling

 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 +---
 drivers/gpu/drm/exynos/exynos_drm_drv.c  | 44 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h  |  2 -
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 ++--
 4 files changed, 22 insertions(+), 106 deletions(-)

-- 
2.7.4

[PATCH 1/2] drm/exynos/vidi: use timer for vblanks instead of sleeping worker

[Andrzej Hajda]

VIDI driver uses fake vblank handler to generate vblank events.
It was implemented using worker which slept for vblank time, additionally
it did not work if there were no page flips. The patch replaces it with
timer, uses drm_crtc_vblank_(on|off) helpers to manage it and fixes
behavior for non-page-flip cases.
This change allows further improvements of vblank in exynos-drm framework.

Signed-off-by: Andrzej Hajda 
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 ++--
 1 file changed, 20 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c 
b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index e8f6c92..a91dad6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 

 #include 

@@ -28,6 +29,9 @@
 #include "exynos_drm_plane.h"
 #include "exynos_drm_vidi.h"

+/* VIDI uses fixed refresh rate of 50Hz */
+#define VIDI_REFRESH_TIME (1000 / 50)
+
 /* vidi has totally three virtual windows. */
 #define WINDOWS_NR 3

@@ -43,12 +47,9 @@ struct vidi_context {
struct exynos_drm_plane planes[WINDOWS_NR];
struct edid *raw_edid;
unsigned intclkdiv;
-   unsigned long   irq_flags;
unsigned intconnected;
-   boolvblank_on;
boolsuspended;
-   booldirect_vblank;
-   struct work_struct  work;
+   struct timer_list   timer;
struct mutexlock;
int pipe;
 };
@@ -102,30 +103,14 @@ static int vidi_enable_vblank(struct exynos_drm_crtc 
*crtc)
if (ctx->suspended)
return -EPERM;

-   if (!test_and_set_bit(0, &ctx->irq_flags))
-   ctx->vblank_on = true;
-
-   ctx->direct_vblank = true;
-
-   /*
-* in case of page flip request, vidi_finish_pageflip function
-* will not be called because direct_vblank is true and then
-* that function will be called by crtc_ops->update_plane callback
-*/
-   schedule_work(&ctx->work);
+   mod_timer(&ctx->timer,
+   jiffies + msecs_to_jiffies(VIDI_REFRESH_TIME) - 1);

return 0;
 }

 static void vidi_disable_vblank(struct exynos_drm_crtc *crtc)
 {
-   struct vidi_context *ctx = crtc->ctx;
-
-   if (ctx->suspended)
-   return;
-
-   if (test_and_clear_bit(0, &ctx->irq_flags))
-   ctx->vblank_on = false;
 }

 static void vidi_update_plane(struct exynos_drm_crtc *crtc,
@@ -140,9 +125,6 @@ static void vidi_update_plane(struct exynos_drm_crtc *crtc,

addr = exynos_drm_fb_dma_addr(state->fb, 0);
DRM_DEBUG_KMS("dma_addr = %pad\n", &addr);
-
-   if (ctx->vblank_on)
-   schedule_work(&ctx->work);
 }

 static void vidi_enable(struct exynos_drm_crtc *crtc)
@@ -153,17 +135,17 @@ static void vidi_enable(struct exynos_drm_crtc *crtc)

ctx->suspended = false;

-   /* if vblank was enabled status, enable it again. */
-   if (test_and_clear_bit(0, &ctx->irq_flags))
-   vidi_enable_vblank(ctx->crtc);
-
mutex_unlock(&ctx->lock);
+
+   drm_crtc_vblank_on(&crtc->base);
 }

 static void vidi_disable(struct exynos_drm_crtc *crtc)
 {
struct vidi_context *ctx = crtc->ctx;

+   drm_crtc_vblank_off(&crtc->base);
+
mutex_lock(&ctx->lock);

ctx->suspended = true;
@@ -190,28 +172,17 @@ static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
.update_plane = vidi_update_plane,
 };

-static void vidi_fake_vblank_handler(struct work_struct *work)
+static void vidi_fake_vblank_timer(unsigned long arg)
 {
-   struct vidi_context *ctx = container_of(work, struct vidi_context,
-   work);
+   struct vidi_context *ctx = (void *)arg;
int win;

if (ctx->pipe < 0)
return;

-   /* refresh rate is about 50Hz. */
-   usleep_range(16000, 2);
-
-   mutex_lock(&ctx->lock);
-
-   if (ctx->direct_vblank) {
-   drm_crtc_handle_vblank(&ctx->crtc->base);
-   ctx->direct_vblank = false;
-   mutex_unlock(&ctx->lock);
-   return;
-   }
-
-   mutex_unlock(&ctx->lock);
+   if (drm_crtc_handle_vblank(&ctx->crtc->base))
+   mod_timer(&ctx->timer,
+   jiffies + msecs_to_jiffies(VIDI_REFRESH_TIME) - 1);

for (win = 0 ; win < WINDOWS_NR ; win++) {
struct exynos_drm_plane *plane = &ctx->planes[win];
@@ -489,6 +460,9 @@ static int vidi_bind(struct device *dev, struct device 
*master, void *data)

 static void vidi_unbind(struct device *dev, struct device *master, void *data)
 {
+   struct vidi_context *ctx = dev_get_drvdata(dev)

[PATCH 2/2] drm/exynos: fix pending update handling

[Andrzej Hajda]

Exynos DRM devices update their registers at vblank time. Exynos-DRM uses
custom mechanism to wait for vblank. This mechanism is error prone -
variables are not updated atomically. As a result in certain circumstances
user space can try to free buffers which are still in use by hardware,
in such cases IOMMU can throw OOPS.
The patch instead of fixing the mechanism replaces it with drm core helper.

Signed-off-by: Andrzej Hajda 
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 +---
 drivers/gpu/drm/exynos/exynos_drm_drv.c  | 44 +---
 drivers/gpu/drm/exynos/exynos_drm_drv.h  |  2 --
 3 files changed, 2 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 785ffa6..5b6845b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -134,8 +134,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct 
drm_device *drm_dev,
exynos_crtc->ops = ops;
exynos_crtc->ctx = ctx;

-   init_waitqueue_head(&exynos_crtc->wait_update);
-
crtc = &exynos_crtc->base;

private->crtc[pipe] = crtc;
@@ -175,13 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device 
*dev, unsigned int pipe)
exynos_crtc->ops->disable_vblank(exynos_crtc);
 }

-void exynos_drm_crtc_wait_pending_update(struct exynos_drm_crtc *exynos_crtc)
-{
-   wait_event_timeout(exynos_crtc->wait_update,
-  (atomic_read(&exynos_crtc->pending_update) == 0),
-  msecs_to_jiffies(50));
-}
-
 void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
struct exynos_drm_plane *exynos_plane)
 {
@@ -190,9 +181,6 @@ void exynos_drm_crtc_finish_update(struct exynos_drm_crtc 
*exynos_crtc,

exynos_plane->pending_fb = NULL;

-   if (atomic_dec_and_test(&exynos_crtc->pending_update))
-   wake_up(&exynos_crtc->wait_update);
-
spin_lock_irqsave(&crtc->dev->event_lock, flags);
if (exynos_crtc->event)
drm_crtc_send_vblank_event(crtc, exynos_crtc->event);
@@ -235,10 +223,8 @@ void exynos_drm_crtc_cancel_page_flip(struct drm_crtc 
*crtc,
spin_lock_irqsave(&crtc->dev->event_lock, flags);

e = exynos_crtc->event;
-   if (e && e->base.file_priv == file) {
+   if (e && e->base.file_priv == file)
exynos_crtc->event = NULL;
-   atomic_dec(&exynos_crtc->pending_update);
-   }

spin_unlock_irqrestore(&crtc->dev->event_lock, flags);

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 877d2efa..a1bc13f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -45,37 +45,11 @@ struct exynos_atomic_commit {
u32 crtcs;
 };

-static void exynos_atomic_wait_for_commit(struct drm_atomic_state *state)
-{
-   struct drm_crtc_state *crtc_state;
-   struct drm_crtc *crtc;
-   int i, ret;
-
-   for_each_crtc_in_state(state, crtc, crtc_state, i) {
-   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-
-   if (!crtc->state->enable)
-   continue;
-
-   ret = drm_crtc_vblank_get(crtc);
-   if (ret)
-   continue;
-
-   exynos_drm_crtc_wait_pending_update(exynos_crtc);
-   drm_crtc_vblank_put(crtc);
-   }
-}
-
 static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
 {
struct drm_device *dev = commit->dev;
struct exynos_drm_private *priv = dev->dev_private;
struct drm_atomic_state *state = commit->state;
-   struct drm_plane *plane;
-   struct drm_crtc *crtc;
-   struct drm_plane_state *plane_state;
-   struct drm_crtc_state *crtc_state;
-   int i;

drm_atomic_helper_commit_modeset_disables(dev, state);

@@ -89,25 +63,9 @@ static void exynos_atomic_commit_complete(struct 
exynos_atomic_commit *commit)
 * have the relevant clocks enabled to perform the update.
 */

-   for_each_crtc_in_state(state, crtc, crtc_state, i) {
-   struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-
-   atomic_set(&exynos_crtc->pending_update, 0);
-   }
-
-   for_each_plane_in_state(state, plane, plane_state, i) {
-   struct exynos_drm_crtc *exynos_crtc =
-   to_exynos_crtc(plane->crtc);
-
-   if (!plane->crtc)
-   continue;
-
-   atomic_inc(&exynos_crtc->pending_update);
-   }
-
drm_atomic_helper_commit_planes(dev, state, false);

-   exynos_atomic_wait_for_commit(state);
+   drm_atomic_helper_wait_for_vblanks(dev, state);

drm_atomic_helper_cleanup_planes(dev, state);

diff --git 

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

>> Would you like to discuss the statistics for my failure (or success) rate
>> a bit more so that involved issues can be clarified in a constructive way?
> 
> It should be that you target 20 bug fixes for each new regression
> that you add.

How do you think about to clarify any concrete "regression" a bit more?


> There is no hope for improving the kernel

I have got an other impression. - I am trying to help also for this goal.


> because you are not even trying to fix 20 bugs,

Under which circumstances would you dare to acknowledge once more
that I improved anything for which you care about?


> only introducing them.

It's a pity that you interpret some of my contributions in this way.


> Once you fix 20 bugs, then you will be even and you can start sending
> cleanups again.  This is fair.

How much will the suggested software refactorings influence the kind of
error counter that you prefer so far?

Regards,
Markus

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> I will refrain from merging any more style/checkpatch/"code cleanup"
> patches from Markus until we start getting real, tested, bug fixes.

Can such a kind of feedback be also interpreted in the way that you insist
to keep some weaknesses which I tried to point in the Linux source code out
for another while?

How do you think about to clarify your ranking for various update candidates
in this software?

Regards,
Markus

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

> Markus, please contact the list in advance in future before posting a bunch
> of patches that don't fix any problems.

I am trying to improve various open issues also in Linux source files.

Unfortunately, some of the proposed changes might not fit to your software
development attention at the moment.
How are the chances that corresponding change acceptance will evolve a bit more
after a while?

Regards,
Markus

[PATCH v2 2/4] drm/bridge: adv7511: Initialize regulators

[Archit Taneja]

Maintain a table of regulator names expect by ADV7511 and ADV7533.
Use regulator_bulk_* api to configure these.

Initialize and enable the regulators during probe itself. Controlling
these dynamically is left for later.

Cc: dri-devel at lists.freedesktop.org
Cc: Laurent Pinchart 
Signed-off-by: Archit Taneja 
---
v2:
- Use regulator_bulk_* API to configure regulators as suggested by Laurent.
- Set up regulators for ADV7511 too.

 drivers/gpu/drm/bridge/adv7511/adv7511.h |  4 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 85 +---
 2 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h 
b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 161c923..83ebdaa 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 

 #include 
 #include 
@@ -327,6 +328,9 @@ struct adv7511 {

struct gpio_desc *gpio_pd;

+   struct regulator_bulk_data *supplies;
+   int num_supplies;
+
/* ADV7533 DSI RX related params */
struct device_node *host_node;
struct mipi_dsi_device *dsi;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8ed3906..f7e79ed 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -839,6 +839,57 @@ static struct drm_bridge_funcs adv7511_bridge_funcs = {
  * Probe & remove
  */

+static const char * const adv7511_supply_names[] = {
+   "avdd",
+   "dvdd",
+   "pvdd",
+   "v3p3",
+   "bgvdd",
+};
+
+static const char * const adv7533_supply_names[] = {
+   "avdd",
+   "dvdd",
+   "pvdd",
+   "a2vdd",
+   "v3p3",
+   "v1p2",
+};
+
+static int adv7511_init_regulators(struct adv7511 *adv)
+{
+   struct device *dev = &adv->i2c_main->dev;
+   const char * const *supply_names;
+   int i, ret;
+
+   if (adv->type == ADV7511) {
+   supply_names = adv7511_supply_names;
+   adv->num_supplies = ARRAY_SIZE(adv7511_supply_names);
+   } else {
+   supply_names = adv7533_supply_names;
+   adv->num_supplies = ARRAY_SIZE(adv7533_supply_names);
+   }
+
+   adv->supplies = devm_kcalloc(dev, adv->num_supplies,
+sizeof(*adv->supplies), GFP_KERNEL);
+   if (!adv->supplies)
+   return -ENOMEM;
+
+   for (i = 0; i < adv->num_supplies; i++)
+   adv->supplies[i].supply = supply_names[i];
+
+   ret = devm_regulator_bulk_get(dev, adv->num_supplies, adv->supplies);
+   if (ret)
+   return ret;
+
+   return regulator_bulk_enable(adv->num_supplies, adv->supplies);
+}
+
+static void adv7511_uninit_regulators(struct adv7511 *adv)
+{
+   regulator_bulk_disable(adv->num_supplies, adv->supplies);
+}
+
 static int adv7511_parse_dt(struct device_node *np,
struct adv7511_link_config *config)
 {
@@ -939,6 +990,7 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
if (!adv7511)
return -ENOMEM;

+   adv7511->i2c_main = i2c;
adv7511->powered = false;
adv7511->status = connector_status_disconnected;

@@ -956,13 +1008,21 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
if (ret)
return ret;

+   ret = adv7511_init_regulators(adv7511);
+   if (ret) {
+   dev_err(dev, "failed to init regulators\n");
+   return ret;
+   }
+
/*
 * The power down GPIO is optional. If present, toggle it from active to
 * inactive to wake up the encoder.
 */
adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
-   if (IS_ERR(adv7511->gpio_pd))
-   return PTR_ERR(adv7511->gpio_pd);
+   if (IS_ERR(adv7511->gpio_pd)) {
+   ret = PTR_ERR(adv7511->gpio_pd);
+   goto uninit_regulators;
+   }

if (adv7511->gpio_pd) {
mdelay(5);
@@ -970,12 +1030,14 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
}

adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config);
-   if (IS_ERR(adv7511->regmap))
-   return PTR_ERR(adv7511->regmap);
+   if (IS_ERR(adv7511->regmap)) {
+   ret = PTR_ERR(adv7511->regmap);
+   goto uninit_regulators;
+   }

ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val);
if (ret)
-   return ret;
+   goto uninit_regulators;
dev_dbg(dev, "Rev. %d\n", val);

if (adv7511->type == ADV7511)
@@ -985,7 +1047,7 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
else
ret = adv7533_

[PATCH 1/2 v4] drm/bridge: adv7511: Add Audio support.

[Archit Taneja]

On 09/17/2016 04:47 AM, John Stultz wrote:
> This patch adds support to Audio for both adv7511 and adv7533
> bridge chips.
>
> This patch was originally from [1] by Lars-Peter Clausen 
> and was adapted by Archit Taneja  and
> Srinivas Kandagatla .
>
> Then I heavily reworked it to use the hdmi-codec driver. And also
> folded in some audio packet initialization done by Andy Green
> . So credit to them, but blame to me.
>
> [1] 
> https://github.com/analogdevicesinc/linux/blob/xcomm_zynq/drivers/gpu/drm/i2c/adv7511_audio.c
>
> Cc: David Airlie 
> Cc: Archit Taneja 
> Cc: Laurent Pinchart 
> Cc: Wolfram Sang 
> Cc: Srinivas Kandagatla 
> Cc: "Ville Syrjälä" 
> Cc: Boris Brezillon 
> Cc: Andy Green 
> Cc: Dave Long 
> Cc: Guodong Xu 
> Cc: Zhangfei Gao 
> Cc: Mark Brown 
> Cc: Lars-Peter Clausen 
> Cc: Jose Abreu 
> Cc: dri-devel at lists.freedesktop.org
> Acked-by: Lars-Peter Clausen 
> Signed-off-by: John Stultz 

Reviewed-by: Archit Taneja 

> ---
> v4:
> * Kconfig tweaks suggested by Lars-Peter Clausen
> v3:
> * Allowed audio support to be configured in or out, as suggested by Laurent
> * Minor cleanups suggested by Laurent
> * Folded in Andy's audio packet initialization patch, as suggested by Archit
>
> drivers/gpu/drm/bridge/adv7511/Kconfig |   8 +
>  drivers/gpu/drm/bridge/adv7511/Makefile|   1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  16 ++
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 213 
> +
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |   4 +
>  5 files changed, 242 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig 
> b/drivers/gpu/drm/bridge/adv7511/Kconfig
> index d2b0499..2fed567 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
> @@ -6,6 +6,14 @@ config DRM_I2C_ADV7511
>   help
> Support for the Analog Device ADV7511(W) and ADV7513 HDMI encoders.
>
> +config DRM_I2C_ADV7511_AUDIO
> + bool "ADV7511 HDMI Audio driver"
> + depends on DRM_I2C_ADV7511 && SND_SOC
> + select SND_SOC_HDMI_CODEC
> + help
> +   Support the ADV7511 HDMI Audio interface. This is used in
> +   conjunction with the AV7511  HDMI driver.
> +
>  config DRM_I2C_ADV7533
>   bool "ADV7533 encoder"
>   depends on DRM_I2C_ADV7511
> diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile 
> b/drivers/gpu/drm/bridge/adv7511/Makefile
> index 9019327..5ba6755 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> @@ -1,3 +1,4 @@
>  adv7511-y := adv7511_drv.o
> +adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
>  adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h 
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 161c923..992d76c 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -309,6 +309,8 @@ struct adv7511 {
>   struct drm_display_mode curr_mode;
>
>   unsigned int f_tmds;
> + unsigned int f_audio;
> + unsigned int audio_source;
>
>   unsigned int current_edid_segment;
>   uint8_t edid_buf[256];
> @@ -334,6 +336,7 @@ struct adv7511 {
>   bool use_timing_gen;
>
>   enum adv7511_type type;
> + struct platform_device *audio_pdev;
>  };
>
>  #ifdef CONFIG_DRM_I2C_ADV7533
> @@ -389,4 +392,17 @@ static inline int adv7533_parse_dt(struct device_node 
> *np, struct adv7511 *adv)
>  }
>  #endif
>
> +#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO
> +int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511);
> +void adv7511_audio_exit(struct adv7511 *adv7511);
> +#else /*CONFIG_DRM_I2C_ADV7511_AUDIO */
> +static inline int adv7511_audio_init(struct device *dev, struct adv7511 
> *adv7511)
> +{
> + return 0;
> +}
> +static inline void adv7511_audio_exit(struct adv7511 *adv7511)
> +{
> +}
> +#endif /* CONFIG_DRM_I2C_ADV7511_AUDIO */
> +
>  #endif /* __DRM_I2C_ADV7511_H__ */
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> new file mode 100644
> index 000..5ce29a5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> @@ -0,0 +1,213 @@
> +/*
> + * Analog Devices ADV7511 HDMI transmitter driver
> + *
> + * Copyright 2012 Analog Devices Inc.
> + * Copyright (c) 2016, Linaro Limited
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "adv7511.h"
> +
> +static void adv7511_calc_cts_n(unsigned int f_tmds, unsigned int fs,
> +unsigned int *cts, unsigned int *n)
> +{
> + switch (fs) {
> + case 32000:
> + *n = 4096;
> + break;
> + case 44100:
> + *n = 6272;
> + break;
> + case 48000:
> + *n = 6144;
> 

[PATCH 2/2 v4] drm/bridge: adv7511: Enable the audio data and clock pads on adv7533

[Archit Taneja]

On 09/17/2016 04:47 AM, John Stultz wrote:
> From: Srinivas Kandagatla 
>
> This patch enables the Audio Data and Clock pads to the adv7533 bridge.
> Without this patch audio can not be played.
>
> Cc: David Airlie 
> Cc: Archit Taneja 
> Cc: Laurent Pinchart 
> Cc: Wolfram Sang 
> Cc: Srinivas Kandagatla 
> Cc: "Ville Syrjälä" 
> Cc: Boris Brezillon 
> Cc: Andy Green 
> Cc: Dave Long 
> Cc: Guodong Xu 
> Cc: Zhangfei Gao 
> Cc: Mark Brown 
> Cc: Lars-Peter Clausen 
> Cc: Jose Abreu 
> Cc: dri-devel at lists.freedesktop.org

Reviewed-by: Archit Taneja 

> Signed-off-by: Srinivas Kandagatla 
> Signed-off-by: John Stultz 
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7533.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> index 5eebd15..6798ecf 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> @@ -29,6 +29,7 @@ static const struct reg_sequence 
> adv7533_cec_fixed_registers[] = {
>   { 0x17, 0xd0 },
>   { 0x24, 0x20 },
>   { 0x57, 0x11 },
> + { 0x05, 0xc8 },
>  };
>
>  static const struct regmap_config adv7533_cec_regmap_config = {
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

drm/exynos: fimd: pagefault when restarting X

[Andrzej Hajda]

Hi Tobias,

On 06.05.2016 09:48, Tobias Jakobi wrote:
> Hello Andrzej,
>
>
> Andrzej Hajda wrote:
>> Hi Tobias,
>>
>> On 05/05/2016 07:27 PM, Tobias Jakobi wrote:
>>> Hello,
>>>
>>> here's another issue I experience when enabling FIMD on the ODROID-X2.
>>>
>>> I can trigger a IOMMU pagefault by starting X once, quitting, and
>>> restarting X again.
>>>

Finally I have tested RGB output on similar target - Odroid-U3. It seems
it has set very low fimd clocks, as a result refresh rate is very low
(5-10 fps)
and vblank waiting code timeouts, as a result framebuffers are prematurely
removed.
Adding assigned-clocks to fimd node should probably help.
Anyway I will try to add check to fimd driver preventing such situations.

Regards
Andrzej

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

[liviu.du...@arm.com]

On Fri, Sep 23, 2016 at 12:18:12AM -0700, Sean Paul wrote:
> This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.
> 
> commit 6a2925ea12006911c8180a89feda6d040873ed18
> Author: Brian Starkey 
> Date:   Mon Jul 25 11:55:48 2016 +0100
> 
> drm/i2c: tda998x: don't register the connector
> 
> [seanpaul]
> Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
> until this is sorted.

I would argue that the comment is not correct, patch is fine, it is
just the dependent code is not ready to work with the patch.

Otherwise, thanks for doing this and sorry for the noise.

Best regards,
Liviu

> 
> Signed-off-by: Sean Paul 
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 088900d..9798d40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1584,6 +1584,7 @@ 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);
>  }
>  
> @@ -1655,10 +1656,16 @@ static int tda998x_bind(struct device *dev, struct 
> device *master, void *data)
>   if (ret)
>   goto err_connector;
>  
> + ret = drm_connector_register(&priv->connector);
> + if (ret)
> + goto err_sysfs;
> +
>   drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
>  
>   return 0;
>  
> +err_sysfs:
> + drm_connector_cleanup(&priv->connector);
>  err_connector:
>   drm_encoder_cleanup(&priv->encoder);
>  err_encoder:
> @@ -1671,6 +1678,7 @@ static void tda998x_unbind(struct device *dev, struct 
> device *master,
>  {
>   struct tda998x_priv *priv = dev_get_drvdata(dev);
>  
> + drm_connector_unregister(&priv->connector);
>   drm_connector_cleanup(&priv->connector);
>   drm_encoder_cleanup(&priv->encoder);
>   tda998x_destroy(priv);
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

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

[Liviu Dudau]

On Fri, Sep 23, 2016 at 09:05:50AM +0200, Daniel Vetter wrote:
> On Thu, Sep 22, 2016 at 4:14 PM, Brian Starkey  
> 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?
> >>
> >
> > Yes, please revert this. There's a problem in the fbdev helper code
> > which stops me fixing this quickly, so better to revert it.
> 
> Hm, what's the trouble wih fbdev? But yeah given this trouble I'd go
> with a revert for now. For the real fix I guess we could just squash
> it all in one, kinda pointless to go overboard for this.
> -Daniel

rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
enabled, but we never got the call to turn off the CRTC. Brian is still
tracking through the fbdev emulation to figure out the cause for that.

Best regards,
Liviu

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[PATCH 00/14] GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

First of all please stop sending your patches as a reply to an earlier 
and completely unrelated series.

Second please prefix all TTM related patches with "drm/ttm:".

Additional to that I don't really see the point in renaming some of the 
jump labels, if you call it "restart" or "lock_restart" doesn't make 
much difference.

Regards,
Christian.

Am 22.09.2016 um 19:32 schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Thu, 22 Sep 2016 19:00:01 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (14):
>Use kmalloc_array() in two functions
>Rename a jump label in ttm_alloc_new_pages()
>Rename jump labels in ttm_page_pool_free()
>Rename a jump label in ttm_page_pool_get_pages()
>Use kmalloc_array() in two more functions
>Rename a jump label in ttm_dma_pool_alloc_new_pages()
>Rename jump labels in ttm_dma_page_pool_free()
>Rename a jump label in ttm_dma_pool_shrink_scan()
>Return directly after a failed kzalloc() in ttm_dma_page_alloc_init()
>Return directly after a failed kobject_init_and_add() in 
> ttm_dma_page_alloc_init()
>Return an error code only as a constant in ttm_dma_pool_init()
>Less function calls in ttm_dma_pool_init() after error detection
>Delete unnecessary variable initialisations in ttm_dma_pool_init()
>Mark an array of text strings as "const" in ttm_dma_pool_init()
>
>   drivers/gpu/drm/ttm/ttm_page_alloc.c | 30 -
>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 58 
> +++-
>   2 files changed, 42 insertions(+), 46 deletions(-)
>

[PATCH] drm/i915: Before pageflip, also wait for shared dmabuf fences.

[Michel Dänzer]

On 22/09/16 10:22 PM, Christian König wrote:
> Am 22.09.2016 um 15:05 schrieb Daniel Vetter:
>> On Thu, Sep 22, 2016 at 2:44 PM, Christian König
>>  wrote:
 - explicit fencing: Userspace passes around distinct fence objects for
 any work going on on the gpu. The kernel doesn't insert any stall of
 it's own (except for moving buffer objects around ofc). This is what
 Android. This also seems to be what amdgpu is doing within one
 process/owner.
>>>
>>> No, that is clearly not my understanding of explicit fencing.
>>>
>>> Userspace doesn't necessarily need to pass around distinct fence objects
>>> with all of it's protocols and the kernel is still responsible for
>>> inserting
>>> stalls whenever an userspace protocol or application requires this
>>> semantics.
>>>
>>> Otherwise you will never be able to use explicit fencing on the Linux
>>> desktop with protocols like DRI2/DRI3.
>> This is about mixing them. Explicit fencing still means userspace has
>> an explicit piece, separate from buffers, (either sync_file fd, or a
>> driver-specific cookie, or similar).
>>
>>> I would expect that every driver in the system waits for all fences of a
>>> reservation object as long as it isn't told otherwise by providing a
>>> distinct fence object with the IOCTL in question.
>> Yup agreed. This way if your explicitly-fencing driver reads a shared
>> buffer passed over a protocol that does implicit fencing (like
>> DRI2/3), then it will work.
>>
>> The other interop direction is explicitly-fencing driver passes a
>> buffer to a consumer which expects implicit fencing. In that case you
>> must attach the right fence to the exclusive slot, but _only_ in that
>> case.
> 
> Ok well sounds like you are close to understand why I can't do exactly
> this: There simply is no right fence I could attach.
> 
> When amdgpu makes the command submissions it doesn't necessarily know
> that the buffer will be exported and shared with another device later on.
> 
> So when the buffer is exported and given to the other device you might
> have a whole bunch of fences which run concurrently and not in any
> serial order.

I feel like you're thinking too much of buffers shared between GPUs as
being short-lived and only shared late. In the use-cases I know about,
shared buffers are created separately and shared ahead of time, the
actual rendering work is done to non-shared buffers and then just copied
to the shared buffers for transfer between GPUs. These copies are always
performed by the same context in such a way that they should always be
performed by the same HW engine and thus implicitly serialized.

Do you have any specific use-cases in mind where buffers are only shared
between GPUs after the rendering operations creating the buffer contents
to be shared have already been submitted?


>> Otherwise you end up stalling your explicitly-fencing userspace,
>> since implicit fencing doesn't allow more than 1 writer. For amdgpu
>> one possible way to implement this might be to count how many users a
>> dma-buf has, and if it's more than just the current context set the
>> exclusive fence. Or do an uabi revision and let userspace decide (or
>> at least overwrite it).
> 
> I mean I can pick one fence and wait for the rest to finish manually,
> but that would certainly defeat the whole effort, doesn't it?

I'm afraid it's not clear to me why it would. Can you elaborate?


>> But the current approach in amdgpu_sync.c of declaring a fence as
>> exclusive after the fact (if owners don't match) just isn't how
>> reservation_object works. You can of course change that, but that
>> means you must change all drivers implementing support for implicit
>> fencing of dma-buf. Fixing amdgpu will be easier ;-)
> 
> Well as far as I can see there is no way I can fix amdgpu in this case.
> 
> The handling clearly needs to be changed on the receiving side of the
> reservation objects if I don't completely want to disable concurrent
> access to BOs in amdgpu.

Anyway, we need a solution for this between radeon and amdgpu, and I
don't think a solution which involves those drivers using reservation
object semantics between them which are different from all other drivers
is a good idea.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> Additional to that I don't really see the point in renaming some of the jump 
> labels,

I am suggesting changes for another collateral software evolution.


> if you call it "restart" or "lock_restart" doesn't make much difference.

Do other identifiers fit better to a specification from the document 
"CodingStyle"
like the following?

"…
Choose label names which say what the goto does or why the goto exists.
…"


Does this wording need any more adjustments?

Regards,
Markus

[Intel-gfx] [PATCH] drm: Convert prime dma-buf <-> handle to rhashtable

[Sean Paul]

On Thu, Sep 22, 2016 at 7:43 AM, Chris Wilson  
wrote:
> Currently we use a linear walk to lookup a handle and return a dma-buf,
> and vice versa. A long overdue TODO task is to convert that to a
> hashtable. Since the initial implementation of dma-buf/prime, we now
> have resizeable hashtables we can use (and now a future task is to RCU
> enable the lookup!).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94631
> Signed-off-by: Chris Wilson 

Looks like a nice improvement

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_prime.c | 94 
> +++--
>  include/drm/drmP.h  |  5 ++-
>  2 files changed, 77 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 780589b420a4..ad077def660d 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -28,6 +28,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -61,9 +62,11 @@
>   */
>
>  struct drm_prime_member {
> -   struct list_head entry;
> struct dma_buf *dma_buf;
> uint32_t handle;
> +
> +   struct rhash_head dma_buf_rht;
> +   struct rhash_head handle_rht;
>  };
>
>  struct drm_prime_attachment {
> @@ -71,10 +74,31 @@ struct drm_prime_attachment {
> enum dma_data_direction dir;
>  };
>
> +static const struct rhashtable_params dma_buf_params = {
> +   .head_offset = offsetof(struct drm_prime_member, dma_buf_rht),
> +   .key_len = sizeof(struct dma_buf *),
> +   .key_offset = offsetof(struct drm_prime_member, dma_buf),
> +   .hashfn = jhash,
> +   .nulls_base = 1u << RHT_BASE_SHIFT,
> +   .automatic_shrinking = true,
> +   .nelem_hint = 2,
> +};
> +
> +static const struct rhashtable_params handle_params = {
> +   .head_offset = offsetof(struct drm_prime_member, handle_rht),
> +   .key_len = sizeof(uint32_t),
> +   .key_offset = offsetof(struct drm_prime_member, handle),
> +   .hashfn = jhash,
> +   .nulls_base = 1u << RHT_BASE_SHIFT,
> +   .automatic_shrinking = true,
> +   .nelem_hint = 2,
> +};
> +
>  static int drm_prime_add_buf_handle(struct drm_prime_file_private 
> *prime_fpriv,
> struct dma_buf *dma_buf, uint32_t handle)
>  {
> struct drm_prime_member *member;
> +   int err;
>
> member = kmalloc(sizeof(*member), GFP_KERNEL);
> if (!member)
> @@ -83,8 +107,28 @@ static int drm_prime_add_buf_handle(struct 
> drm_prime_file_private *prime_fpriv,
> get_dma_buf(dma_buf);
> member->dma_buf = dma_buf;
> member->handle = handle;
> -   list_add(&member->entry, &prime_fpriv->head);
> +
> +   err = rhashtable_insert_fast(&prime_fpriv->dma_bufs,
> +&member->dma_buf_rht,
> +dma_buf_params);
> +   if (err)
> +   goto err_dma_buf;
> +
> +   err = rhashtable_insert_fast(&prime_fpriv->handles,
> +&member->handle_rht,
> +handle_params);
> +   if (err)
> +   goto err_dma_rht;
> +
> return 0;
> +
> +err_dma_rht:
> +   rhashtable_remove_fast(&prime_fpriv->dma_bufs,
> +  &member->dma_buf_rht,
> +  dma_buf_params);
> +err_dma_buf:
> +   dma_buf_put(dma_buf);
> +   return err;
>  }
>
>  static struct dma_buf *drm_prime_lookup_buf_by_handle(struct 
> drm_prime_file_private *prime_fpriv,
> @@ -92,10 +136,10 @@ static struct dma_buf 
> *drm_prime_lookup_buf_by_handle(struct drm_prime_file_priv
>  {
> struct drm_prime_member *member;
>
> -   list_for_each_entry(member, &prime_fpriv->head, entry) {
> -   if (member->handle == handle)
> -   return member->dma_buf;
> -   }
> +   member = rhashtable_lookup_fast(&prime_fpriv->handles,
> +   &handle, handle_params);
> +   if (member)
> +   return member->dma_buf;
>
> return NULL;
>  }
> @@ -106,12 +150,13 @@ static int drm_prime_lookup_buf_handle(struct 
> drm_prime_file_private *prime_fpri
>  {
> struct drm_prime_member *member;
>
> -   list_for_each_entry(member, &prime_fpriv->head, entry) {
> -   if (member->dma_buf == dma_buf) {
> -   *handle = member->handle;
> -   return 0;
> -   }
> +   member = rhashtable_lookup_fast(&prime_fpriv->dma_bufs,
> +   &dma_buf, dma_buf_params);
> +   if (member) {
> +   *handle = member->handle;
> +   return 0;
> }
> +
> return -ENOENT;
>  }
>
> @@ -166,14 +211,21 @@ static void drm_gem_map_detach(struct dma_buf *dma_buf,
>  void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private 
> *prime_fpriv,
>   

[PATCH] drm/udl: fix line iterator in damage handling

[David Herrmann]

The udl damage handler is supposed to render 'height' lines, but its
iterator has an obvious typo that makes it miss most lines if the
rectangle does not cover 0/0.

Fix the damage handler to correctly render all lines.

This is a fallout from:

commit e375882406d0cc24030746638592004755ed4ae0
Author: Noralf Trønnes 
Date:   Thu Apr 28 17:18:37 2016 +0200

drm/udl: Use drm_fb_helper deferred_io support

Tested-by: poma 
Reviewed-by: Daniel Vetter 
Signed-off-by: David Herrmann 
---
 drivers/gpu/drm/udl/udl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 9688bfa..611b6b9 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -122,7 +122,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, 
int y,
return 0;
cmd = urb->transfer_buffer;

-   for (i = y; i < height ; i++) {
+   for (i = y; i < y + height ; i++) {
const int line_offset = fb->base.pitches[0] * i;
const int byte_offset = line_offset + (x * bpp);
const int dev_byte_offset = (fb->base.width * bpp * i) + (x * 
bpp);
-- 
2.10.0

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Jyri Sarha]

On 09/23/16 10:36, SF Markus Elfring wrote:
>> I think the "if (node)" in the of_node_put() is there on purpose,
> 
> Yes, of course.
> 
> Does such an implementation detail correspond to a general software design 
> pattern?
> 

Yes it does. For instance standard malloc()/free() implementation [1].

> 
>> because it potentially saves the caller one extra if()-statement
> 
> This can occasionally happen.
> 
> 
>> and keeps the caller code simpler.
> 
> A special view on software simplicity can also lead to questionable 
> intermediate
> function implementation, can't it?
> 

I don't really follow. But in any case I do not see anything
questionable in the current tilcdc_convert_slave_node() implementation.

> 
>> Keeping the goto labels in right order needs precision
> 
> I can agree to this view.
> 
> 
>> and can lead to subtle errors.
> 
> The management of jump labels is just another software development challenge
> as usual, isn't it?
> 

Yes. But usually it pays of to avoid complexity when possible.

> 
>> Sometimes there is no way to avoid that,
> 
> How do you think about to clarify the constraints which you imagine a bit 
> more?
> 

If the the of_node_put() behaviour would not be specified with null
pointer as parameter, there would be such a constraint.

I am beginning to have a feeling that this discussion is not going anywhere.

> 
>> but here there is.
> 
> I disagree to this conclusion.
> 
> Would you like to care a bit more for efficiency and software correctness
> around the discussed exception handling?
> 

No, I would not. I think we have reached the bottom of this discussion.
For the moment I have more important tasks to do.

Best regards,
Jyri

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html

GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 12:20 schrieb SF Markus Elfring:
>> Additional to that I don't really see the point in renaming some of the jump 
>> labels,
> I am suggesting changes for another collateral software evolution.
>
>
>> if you call it "restart" or "lock_restart" doesn't make much difference.
> Do other identifiers fit better to a specification from the document 
> "CodingStyle"
> like the following?

No, not really.

>
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
>
> Does this wording need any more adjustments?

Of hand I can't find any better wording.

It's just the names like "out" or "restart" perfectly explain why the 
labels exists. So they fulfill this requirement from the coding style as 
far as I can see.

So why do you want to change them?

Regards,
Christian.

>
> Regards,
> Markus

[PATCH] drm/tilcdc: Add atomic and crtc headers to crtc.c

[Jyri Sarha]

On 09/23/16 09:23, Sean Paul wrote:
> On Thu, Sep 22, 2016 at 2:04 PM, Jyri Sarha  wrote:
>> On 09/22/16 09:18, Daniel Vetter wrote:
>>> On Wed, Sep 21, 2016 at 06:36:28AM -0700, Sean Paul wrote:
 Also reorder alphabetically and fix up drm_flip_work header.

 Signed-off-by: Sean Paul 
>>>
>>> Reviewed-by: Daniel Vetter 
>>>
>>
>> Picked this up. Thanks!
>>
> 
> Hi Jyri,
> I already picked this up through drm-misc to preface Daniel's clean-up
> patch series.
> 
> Could you drop this from your tree and we'll take it through -misc?
> 

Argh I sent the pull request already. However, Dave has not pulled it
yet, so I'll make another and ask him the ignore one with your patch.

BR,
Jyri

> Sean
> 
> 
>> Best regards,
>> Jyri
>>
 ---
  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

 diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
 b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 index 2087689..cb9df10 100644
 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
 @@ -15,9 +15,11 @@
   * this program.  If not, see .
   */

 -#include "drm_flip_work.h"
 -#include 
 +#include 
  #include 
 +#include 
 +#include 
 +#include 

  #include "tilcdc_drv.h"
  #include "tilcdc_regs.h"
 --
 2.8.0.rc3.226.g39d4020

>>>
>>

Please ignore this: [GIT PULL] tilcdc 3rd set of changes for v4.9

[Jyri Sarha]

Dave,
Please ignore this pull request. I'll send another soon.

The "drm/tilcdc: Add atomic and crtc headers to crtc.c" is already
coming trough drm-misc.

Best regards,
Jyri

On 09/23/16 00:37, Jyri Sarha wrote:
> Hi Dave,
> Please pull these collected fixes and cleanups from various sources. The
> request was rebased on top the previous pull tilcdc request tag, so it
> contains all the accumulated tilcdc changes for v4.9 so far.
> 
> Thanks,
> Jyri
> 
> The following changes since commit 2e0965b06d90b9dba76198d026c4c2ee04443aca:
> 
>   drm/tilcdc: WARN if CRTC is touched without CRTC lock (2016-09-07
> 15:54:43 +0300)
> 
> are available in the git repository at:
> 
>   https://github.com/jsarha/linux tags/tilcdc-4.9-3
> 
> for you to fetch changes up to 55fd61dfb8e37e27dcd44503f9ac3d676a5c3896:
> 
>   drm/tilcdc: Return directly after a failed kfree_table_init() in
> tilcdc_convert_slave_node() (2016-09-23 00:09:46 +0300)
> 
> 
> 3rd drm/tilcdc pull request for v4.9.
> 
> 
> Baoyou Xie (2):
>   drm/tilcdc: add missing header dependencies
>   drm/tilcdc: mark symbols static where possible
> 
> Jyri Sarha (1):
>   drm/tilcdc: Remove "default" from blue-and-red-wiring property binding
> 
> Markus Elfring (1):
>   drm/tilcdc: Return directly after a failed kfree_table_init() in
> tilcdc_convert_slave_node()
> 
> Sean Paul (1):
>   drm/tilcdc: Add atomic and crtc headers to crtc.c
> 
> Wei Yongjun (1):
>   drm/tilcdc: Fix non static symbol warning
> 
>  Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt | 6 +++---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c| 6 --
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 4 ++--
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c   | 1 +
>  drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c| 8 
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c  | 1 +
>  6 files changed, 15 insertions(+), 11 deletions(-)
> 

[PATCH] drm/exynos/fimd: add clock rate checking

[Andrzej Hajda]

In case of some platforms fimd clocks can be configured to
very low values, as a result refresh rate can be very low and
driver/drm-core will timeout waiting for vblanks, it will result
in premature removal of framebuffers and will cause oopses.
The patch adds atomic_check callback to fimd to prevent setting
such modes.

Reported-by: Tobias Jakobi 
Signed-off-by: Andrzej Hajda 
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 41 ++--
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index d472164..0e74999 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -198,6 +198,7 @@ struct fimd_context {
atomic_twait_vsync_event;
atomic_twin_updated;
atomic_ttriggering;
+   int clkdiv;

const struct fimd_driver_data *driver_data;
struct drm_encoder *encoder;
@@ -389,15 +390,18 @@ static void fimd_clear_channels(struct exynos_drm_crtc 
*crtc)
pm_runtime_put(ctx->dev);
 }

-static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
-   const struct drm_display_mode *mode)
+
+static int fimd_atomic_check(struct exynos_drm_crtc *crtc,
+   struct drm_crtc_state *state)
 {
-   unsigned long ideal_clk;
-   u32 clkdiv;
+   struct drm_display_mode *mode = &state->adjusted_mode;
+   struct fimd_context *ctx = crtc->ctx;
+   unsigned long ideal_clk, lcd_rate;
+   int clkdiv;

if (mode->clock == 0) {
-   DRM_ERROR("Mode has zero clock value.\n");
-   return 0xff;
+   DRM_INFO("Mode has zero clock value.\n");
+   return -EINVAL;
}

ideal_clk = mode->clock * 1000;
@@ -410,10 +414,23 @@ static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
ideal_clk *= 2;
}

+   lcd_rate = clk_get_rate(ctx->lcd_clk);
+   if (2 * lcd_rate < ideal_clk) {
+   DRM_INFO("sclk_fimd clock too low(%lu) for requested pixel 
clock(%lu)\n",
+lcd_rate, ideal_clk);
+   return -EINVAL;
+   }
+
/* Find the clock divider value that gets us closest to ideal_clk */
-   clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(ctx->lcd_clk), ideal_clk);
+   clkdiv = DIV_ROUND_CLOSEST(lcd_rate, ideal_clk);
+   if (clkdiv >= 0x200) {
+   DRM_INFO("requested pixel clock(%lu) too low\n", ideal_clk);
+   return -EINVAL;
+   }
+
+   ctx->clkdiv = (clkdiv < 0x100) ? clkdiv : 0xff;

-   return (clkdiv < 0x100) ? clkdiv : 0xff;
+   return 0;
 }

 static void fimd_setup_trigger(struct fimd_context *ctx)
@@ -442,7 +459,7 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
const struct fimd_driver_data *driver_data = ctx->driver_data;
void *timing_base = ctx->regs + driver_data->timing_base;
-   u32 val, clkdiv;
+   u32 val;

if (ctx->suspended)
return;
@@ -543,9 +560,8 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
if (ctx->driver_data->has_clksel)
val |= VIDCON0_CLKSEL_LCD;

-   clkdiv = fimd_calc_clkdiv(ctx, mode);
-   if (clkdiv > 1)
-   val |= VIDCON0_CLKVAL_F(clkdiv - 1) | VIDCON0_CLKDIR;
+   if (ctx->clkdiv > 1)
+   val |= VIDCON0_CLKVAL_F(ctx->clkdiv - 1) | VIDCON0_CLKDIR;

writel(val, ctx->regs + VIDCON0);
 }
@@ -939,6 +955,7 @@ static const struct exynos_drm_crtc_ops fimd_crtc_ops = {
.update_plane = fimd_update_plane,
.disable_plane = fimd_disable_plane,
.atomic_flush = fimd_atomic_flush,
+   .atomic_check = fimd_atomic_check,
.te_handler = fimd_te_handler,
 };

-- 
2.7.4

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

>> A special view on software simplicity can also lead to questionable 
>> intermediate
>> function implementation, can't it?
> 
> I don't really follow. But in any case I do not see anything
> questionable in the current tilcdc_convert_slave_node() implementation.

I identified update candidates there like the following.

1. Delayed checking for null pointers
…
if (!slave || !of_device_is_available(lcdc))
…

2. Usage of a single jump label for (too many?) cases
…
goto out;
…

   Can the corresponding exception handling become also a bit more efficient?


>> Would you like to care a bit more for efficiency and software correctness
>> around the discussed exception handling?
> 
> No, I would not.

Thanks for this information.

I hope that the software situation can also be improved around
this design aspect somehow.


> For the moment I have more important tasks to do.

I know also that various open issues are competing for your software
development attention as usual.

Regards,
Markus

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Thu, Sep 22, 2016 at 2:38 PM, SF Markus Elfring
 wrote:
>>> The of_node_put() function was called in some cases
>>> by the tilcdc_convert_slave_node() function during error handling
>>> even if the passed variable contained a null pointer.
>>>
>>> * Adjust jump targets according to the Linux coding style convention.
>>>
>>> * Split a condition check for resource detection failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>
>> I don't really agree with this patch.
>
> This kind of feedback can be fine at first glance.
>
>
>> There is no harm in calling of_node_put() with NULL as an argument
>
> The cost of additional function calls will be eventually not noticed
> just because they belong to an exception handling implementation so far.

iirc, there are Coccinelle rules that find code with unnecessary null
checks and removes them..

Although you probably made this complex enough that cocinelle would
not find it.  That is not a complement.  One should not make error
handling/cleanup more complex than needed.

BR,
-R

>
>> and because of that there is no point in making the function more complex
>
> There is inherent software complexity involved.
>
>
>> and harder to maintain.
>
> How do you think about to discuss this aspect a bit more?
>
>
> I suggest to reconsider this design detail if it is really acceptable
> for the safe implementation of such a software module.
>
> * How much will it matter in general that one function call was performed
>   in this use case without checking its return value immediately?
>
> * Should it usually be determined quicker if a required resource
>   could be acquired before trying the next allocation?
>
> Regards,
> Markus
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau  wrote:
> rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> enabled, but we never got the call to turn off the CRTC. Brian is still
> tracking through the fbdev emulation to figure out the cause for that.

fbdev emulation doesn't do that for you. If you need/want to shut down
all the crtcs on driver unload, you need to do that yourself. There's
atomic helpers to do that for you that for you.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/udl: fix udl_handle_damage

[Daniel Vetter]

Noralf accidentally made a small mistake for real subrectangles in his
patch to convert udl to the shared fb dirty handling code.

Fixes: e375882406d0 ("drm/udl: Use drm_fb_helper deferred_io support")
Cc:  # v4.7+
Cc: Noralf Trønnes 
Cc: Gerd Hoffmann 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/udl/udl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 9688bfa92ccd..611b6b9bb3cb 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -122,7 +122,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, 
int y,
return 0;
cmd = urb->transfer_buffer;

-   for (i = y; i < height ; i++) {
+   for (i = y; i < y + height ; i++) {
const int line_offset = fb->base.pitches[0] * i;
const int byte_offset = line_offset + (x * bpp);
const int dev_byte_offset = (fb->base.width * bpp * i) + (x * 
bpp);
-- 
2.7.4

[ADV7393] DRM Encoder Slave or DRM Bridge

[Tomi Valkeinen]

On 22/09/16 16:22, Vikas Patil wrote:

> Could you help me to understand if I could use “interlace=false”?
> ADV7393 seems to be supporting non-interlaced mode. From datasheet:
> “The ADV7390/ADV7391/ADV7392/ADV7393 support an SD noninterlaced mode.
> Using this mode, progressive inputs at twice the frame rate of NTSC
> and PAL (240p/59.94 Hz and 288p/50 Hz, respectively) can be input into
> the ADV7390/ ADV7391/ADV7392/ADV7393. The SD noninterlaced mode can be
> enabled using Subaddress 0x88, Bit 1.”

Difficult to say... So OMAP4+ DSS hardware does support interlace output
for DPI. The driver has never supported it, nor do I have any hardware
to test it. It might be quite easy to add, though.

If I read the above snippet right, to use progressive input, the DISPC
needs to output at double refresh rate. So probably what you would have
to do is in ADV driver, you have your set_timings function, where you
should double the pix clock before you pass the timings forward to the
DISPC's DPI driver.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/268bba3d/attachment.sig>

[ADV7393] DRM Encoder Slave or DRM Bridge

[Tomi Valkeinen]

On 23/09/16 13:08, Vikas Patil wrote:
> Hi Tomi,
> 
> I added the missing check for "OMAP_DISPLAY_TYPE_VENC" in function
> omap_connector_detect @ gpu/drm/omapdrm/omap_connector.c and now
> modetest  seems to be showing correct status and connections.

Is there a cable detection support in the ADV hardware & driver? If not,
then the cable connection status is "unknown". It should still work if
the output is enabled manually. I think. I don't have any boards with
analog tv out..

But yes, your change is an easy hack to force the output on.

> But still I could not see kmscube on panel and can see some flicker is
> going on display. I think I need to understand about what display
> timing I could use as interlace doesn't seems to be supported as I
> mentioned above.

Yes, sounds like a video timings issue.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/51340f52/attachment.sig>

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

[Sean Paul]

On Fri, Sep 23, 2016 at 2:32 AM,   wrote:
> On Fri, Sep 23, 2016 at 12:18:12AM -0700, Sean Paul wrote:
>> This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.
>>
>> commit 6a2925ea12006911c8180a89feda6d040873ed18
>> Author: Brian Starkey 
>> Date:   Mon Jul 25 11:55:48 2016 +0100
>>
>> drm/i2c: tda998x: don't register the connector
>>
>> [seanpaul]
>> Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
>> until this is sorted.
>
> I would argue that the comment is not correct, patch is fine, it is
> just the dependent code is not ready to work with the patch.
>
> Otherwise, thanks for doing this and sorry for the noise.
>

Applied to drm-misc

Sean

> Best regards,
> Liviu
>
>>
>> Signed-off-by: Sean Paul 
>> ---
>>  drivers/gpu/drm/i2c/tda998x_drv.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index 088900d..9798d40 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -1584,6 +1584,7 @@ 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);
>>  }
>>
>> @@ -1655,10 +1656,16 @@ static int tda998x_bind(struct device *dev, struct 
>> device *master, void *data)
>>   if (ret)
>>   goto err_connector;
>>
>> + ret = drm_connector_register(&priv->connector);
>> + if (ret)
>> + goto err_sysfs;
>> +
>>   drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
>>
>>   return 0;
>>
>> +err_sysfs:
>> + drm_connector_cleanup(&priv->connector);
>>  err_connector:
>>   drm_encoder_cleanup(&priv->encoder);
>>  err_encoder:
>> @@ -1671,6 +1678,7 @@ static void tda998x_unbind(struct device *dev, struct 
>> device *master,
>>  {
>>   struct tda998x_priv *priv = dev_get_drvdata(dev);
>>
>> + drm_connector_unregister(&priv->connector);
>>   drm_connector_cleanup(&priv->connector);
>>   drm_encoder_cleanup(&priv->encoder);
>>   tda998x_destroy(priv);
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>
> --
> 
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---
> ¯\_(ツ)_/¯

GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> It's just the names like "out" or "restart" perfectly explain why the labels 
> exists.

I have got an other impression.


> So they fulfill this requirement from the coding style as far as I can see.

Short identifiers might look more convenient in some cases because
they are quicker to type.


> So why do you want to change them?

1. I suggest to select identifiers also for jump labels which are more 
meaningful
   and eventually unique for some function implementations.

2. How do you think about to add a single space character before any label?

Regards,
Markus

GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 13:07 schrieb SF Markus Elfring:
>> It's just the names like "out" or "restart" perfectly explain why the labels 
>> exists.
> I have got an other impression.
>
>
>> So they fulfill this requirement from the coding style as far as I can see.
> Short identifiers might look more convenient in some cases because
> they are quicker to type.
>
>
>> So why do you want to change them?
> 1. I suggest to select identifiers also for jump labels which are more 
> meaningful
> and eventually unique for some function implementations.

I completely disagree. A longer identifier is not necessarily more 
meaningful than a shorter one.

The difference between calling a label "retry" and "lock_retry" is 
negligible, doesn't improve readability as far as I can see and is 
actually incorrect because the main meaning of the label is that we 
don't take the lock but rather that we restart the allocation operation.

Calling the label "unlock" instead of "out" is arguable a little better, 
but nothing I would call a major improvement either.

So that is a clear NAK to all those patches.

> 2. How do you think about to add a single space character before any label?

Bad as well. Why would anybody want to do this?

Christian.

>
> Regards,
> Markus

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

> iirc, there are Coccinelle rules that find code with unnecessary null
> checks and removes them.

This kind of software change is not needed here.

I find that a corresponding return value check happens one function call
too late.


> Although you probably made this complex enough that cocinelle would
> not find it.  That is not a complement.

I imagine that scripts for the semantic patch language can find more
source code places where questionable disjunctions are used so far.
Would you dare to split any more condition checks?


> One should not make error handling/cleanup more complex than needed.

I see a need to improve not only correctness there but also a bit of
software efficiency.

Regards,
Markus

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Fri, Sep 23, 2016 at 7:19 AM, SF Markus Elfring
 wrote:
>> iirc, there are Coccinelle rules that find code with unnecessary null
>> checks and removes them.
>
> This kind of software change is not needed here.
>
> I find that a corresponding return value check happens one function call
> too late.
>

I think you misunderstand my point.. which is that additional
conditional checks wrapping a function call to something that already
checks for null should be removed.. not introduced.

>
>> Although you probably made this complex enough that cocinelle would
>> not find it.  That is not a complement.
>
> I imagine that scripts for the semantic patch language can find more
> source code places where questionable disjunctions are used so far.
> Would you dare to split any more condition checks?
>
>
>> One should not make error handling/cleanup more complex than needed.
>
> I see a need to improve not only correctness there but also a bit of
> software efficiency.

If you can measure any performance difference and present some results
(esp. considering that this is something that just happens when the
driver is loaded), then we'll talk.  Until then, please don't send
this sort of patch.  Thank you.

BR,
-R

> Regards,
> Markus

[Intel-gfx] [PATCH 07/15] drm/omap: Use per-plane rotation property

[Tomi Valkeinen]

On 12/08/16 19:04, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 04:33:32PM +0300, Ville Syrjälä wrote:
>> On Thu, Aug 11, 2016 at 02:32:44PM +0300, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 22/07/16 16:43, ville.syrjala at linux.intel.com wrote:
>>>> From: Ville Syrjälä 
>>>>
>>>> The global mode_config.rotation_property is going away, switch over to
>>>> per-plane rotation_property.
>>>>
>>>> Not sure I got the annoying crtc rotation_property handling right.
>>>> Might work, or migth not.
>>>
>>> I think something is funny with this patch or the series. I fetched your
>>> branch, and with your series, it looks like the primary planes lose all
>>> their props. modetest says:
>>>
>>> could not get plane 26 properties: Invalid argument
>>> could not get plane 30 properties: Invalid argument
>>
>> Hmm. Weird. Is it really the get props ioctl that fails?
>>
>> The first EINVAL I can spot there is
>> if (!obj->properties) {
>>   ret = -EINVAL;
>>   goto out_unref;
>>  }
>> which definitely makes no sense since this is assigned
>> as plane->base.properties = &plane->properties. So can't be that unless
>> we manage to clear the pointer somehow after the init.
>>
>> The only other direct EINVAL I see there is if
>>  drm_object_property_get_value(obj->properties->properties[i])
>> fails to find the passed prop in the properties array. Which clearly
>> can't happen since we got it from the array in the first place. Also,
>> clearly that code is rather inefficient, perhaps someone should rewrite
>> it a bit.
>>
>> Can't quite see how this could fail for the plane in other ways. But I
>> might be blind.
> 
> I tried to think on this a bit more, and the only think I came up with was
> that we end up doing the drm_plane_create_rotation_property() twice for the
> primary planes. I tried that on i915 but it'd didn't result in anything bad
> AFAICS. Would leak a bit, but so what :P
> 
> Dunno, I guess you could try something like:
> 
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -211,11 +211,12 @@ void omap_plane_install_properties(struct drm_plane 
> *plane,
> struct omap_drm_private *priv = dev->dev_private;
>  
> if (priv->has_dmm) {
> -   drm_plane_create_rotation_property(plane,
> -  BIT(DRM_ROTATE_0),
> -  BIT(DRM_ROTATE_0) | 
> BIT(DRM_ROTATE_90) |
> -  BIT(DRM_ROTATE_180) | 
> BIT(DRM_ROTATE_270) |
> -  BIT(DRM_REFLECT_X) | 
> BIT(DRM_REFLECT_Y));
> +   if (!plane->rotation_property)
> +   drm_plane_create_rotation_property(plane,
> +  BIT(DRM_ROTATE_0),
> +  BIT(DRM_ROTATE_0) 
> | BIT(DRM_ROTATE_90) |
> +  
> BIT(DRM_ROTATE_180) | BIT(DRM_ROTATE_270) |
> +  BIT(DRM_REFLECT_X) 
> | BIT(DRM_REFLECT_Y));

This fixes the problem... Obviously something gets confused if the
property is created twice. Perhaps the first one gets stored somewhere,
and gets used somehow, even if the latter property is the "real" one,
attached to the plane? Just a guess...

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160923/ed1ad629/attachment.sig>

GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

> Calling the label "unlock" instead of "out" is arguable a little better,

Thanks that you can follow a renaming for this direction in principle.


> but nothing I would call a major improvement either.

This was not my intention for such an use case.

I am proposing some small software updates according to such a design pattern.


> So that is a clear NAK to all those patches.

Do you reject also update steps like the following then?

* drm/ttm: Use kmalloc_array() in two (or four?) functions"

* drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

* Would you like to improve the usage of the variables "n" and "t"
  in the function "ttm_dma_pool_init" any further as Joe Perches suggested it?


>> 2. How do you think about to add a single space character before any label?
> 
> Bad as well. Why would anybody want to do this?

Do you find another software evolution interesting according to a recent commit?

"docs: Remove space-before-label guidance from CodingStyle"
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a


Regards,
Markus

[PATCH] drm/tilcdc: fix wrong error handling

[Jyri Sarha]

On 09/23/16 14:47, Sean Paul wrote:
> On Fri, Sep 23, 2016 at 3:52 AM, Daniel Schultz  
> wrote:
>> When 'component_bind_all' fails it should not try to unbind components
>> in the error handling. This will produce a null pointer kernel panic when
>> no component exist.
>>
>> This patch changes the order of the error handling. Now, it will only
>> unbind components if the are bound. Otherwise, the module will jump to
>> an error label below.
>>
>> Signed-off-by: Daniel Schultz 
> 
> Reviewed-by: Sean Paul 
> 

Thanks, for both. Should I pick this one :)?

BR,
Jyri

>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c 
>> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> index d278093..d491610 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -315,13 +315,13 @@ fail_irq_uninstall:
>>  fail_vblank_cleanup:
>> drm_vblank_cleanup(dev);
>>
>> -fail_mode_config_cleanup:
>> -   drm_mode_config_cleanup(dev);
>> -
>>  fail_component_cleanup:
>> if (priv->is_componentized)
>> component_unbind_all(dev->dev, dev);
>>
>> +fail_mode_config_cleanup:
>> +   drm_mode_config_cleanup(dev);
>> +
>>  fail_external_cleanup:
>> tilcdc_remove_external_encoders(dev);
>>
>> --
>> 1.9.1
>>

[PATCH] drm/i915: Before pageflip, also wait for shared dmabuf fences.

[Daniel Vetter]

On Fri, Sep 23, 2016 at 07:00:25PM +0900, Michel Dänzer wrote:
> On 22/09/16 10:22 PM, Christian König wrote:
> > Am 22.09.2016 um 15:05 schrieb Daniel Vetter:
> >> On Thu, Sep 22, 2016 at 2:44 PM, Christian König
> >>  wrote:
>  - explicit fencing: Userspace passes around distinct fence objects for
>  any work going on on the gpu. The kernel doesn't insert any stall of
>  it's own (except for moving buffer objects around ofc). This is what
>  Android. This also seems to be what amdgpu is doing within one
>  process/owner.
> >>>
> >>> No, that is clearly not my understanding of explicit fencing.
> >>>
> >>> Userspace doesn't necessarily need to pass around distinct fence objects
> >>> with all of it's protocols and the kernel is still responsible for
> >>> inserting
> >>> stalls whenever an userspace protocol or application requires this
> >>> semantics.
> >>>
> >>> Otherwise you will never be able to use explicit fencing on the Linux
> >>> desktop with protocols like DRI2/DRI3.
> >> This is about mixing them. Explicit fencing still means userspace has
> >> an explicit piece, separate from buffers, (either sync_file fd, or a
> >> driver-specific cookie, or similar).
> >>
> >>> I would expect that every driver in the system waits for all fences of a
> >>> reservation object as long as it isn't told otherwise by providing a
> >>> distinct fence object with the IOCTL in question.
> >> Yup agreed. This way if your explicitly-fencing driver reads a shared
> >> buffer passed over a protocol that does implicit fencing (like
> >> DRI2/3), then it will work.
> >>
> >> The other interop direction is explicitly-fencing driver passes a
> >> buffer to a consumer which expects implicit fencing. In that case you
> >> must attach the right fence to the exclusive slot, but _only_ in that
> >> case.
> > 
> > Ok well sounds like you are close to understand why I can't do exactly
> > this: There simply is no right fence I could attach.
> > 
> > When amdgpu makes the command submissions it doesn't necessarily know
> > that the buffer will be exported and shared with another device later on.
> > 
> > So when the buffer is exported and given to the other device you might
> > have a whole bunch of fences which run concurrently and not in any
> > serial order.
> 
> I feel like you're thinking too much of buffers shared between GPUs as
> being short-lived and only shared late. In the use-cases I know about,
> shared buffers are created separately and shared ahead of time, the
> actual rendering work is done to non-shared buffers and then just copied
> to the shared buffers for transfer between GPUs. These copies are always
> performed by the same context in such a way that they should always be
> performed by the same HW engine and thus implicitly serialized.
> 
> Do you have any specific use-cases in mind where buffers are only shared
> between GPUs after the rendering operations creating the buffer contents
> to be shared have already been submitted?

Yeah, it should be known which buffer you use (at least in userspace,
maybe not in the kernel) for which you need implicit fencing. At least
with DRI2/3 it's really obvious which buffers are shared. Same holds for
external images and other imported buffers.

Yes that means you need to keep track of a few things in userspace, and
you need to add a special flag to CS to make sure the kernel does set the
exclusive fence.

> >> Otherwise you end up stalling your explicitly-fencing userspace,
> >> since implicit fencing doesn't allow more than 1 writer. For amdgpu
> >> one possible way to implement this might be to count how many users a
> >> dma-buf has, and if it's more than just the current context set the
> >> exclusive fence. Or do an uabi revision and let userspace decide (or
> >> at least overwrite it).
> > 
> > I mean I can pick one fence and wait for the rest to finish manually,
> > but that would certainly defeat the whole effort, doesn't it?
> 
> I'm afraid it's not clear to me why it would. Can you elaborate?
> 
> 
> >> But the current approach in amdgpu_sync.c of declaring a fence as
> >> exclusive after the fact (if owners don't match) just isn't how
> >> reservation_object works. You can of course change that, but that
> >> means you must change all drivers implementing support for implicit
> >> fencing of dma-buf. Fixing amdgpu will be easier ;-)
> > 
> > Well as far as I can see there is no way I can fix amdgpu in this case.
> > 
> > The handling clearly needs to be changed on the receiving side of the
> > reservation objects if I don't completely want to disable concurrent
> > access to BOs in amdgpu.
> 
> Anyway, we need a solution for this between radeon and amdgpu, and I
> don't think a solution which involves those drivers using reservation
> object semantics between them which are different from all other drivers
> is a good idea.

Afaik there's also amd+intel machines out there, so really the only option
is to either fix amdgpu to correctly 

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[SF Markus Elfring]

>> I see a need to improve not only correctness there but also a bit of
>> software efficiency.
> 
> If you can measure any performance difference and present some results
> (esp. considering that this is something that just happens when the
> driver is loaded), then we'll talk.

Are you really interested to increase your software development attention
for a quicker exception handling implementation in this function?


> Until then, please don't send this sort of patch.  Thank you.

Will benchmark statistics change such a change rejection ever?

Regards,
Markus

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

[Lucas Stach]

Am Freitag, den 23.09.2016, 12:58 +0200 schrieb Daniel Vetter:
> On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau 
> wrote:
> > 
> > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is
> > still
> > enabled, but we never got the call to turn off the CRTC. Brian is
> > still
> > tracking through the fbdev emulation to figure out the cause for
> > that.
> 
> fbdev emulation doesn't do that for you. If you need/want to shut
> down
> all the crtcs on driver unload, you need to do that yourself. There's
> atomic helpers to do that for you that for you.

Which reminds me that my patch (drm/atomic-helper: add unlocked disable
all outputs helper) to add such a helper wasn't applied. Probably my
own fault by being non-responsive to Seans question.

Regards,
Lucas

[PATCH 1/2] drm/exynos/vidi: use timer for vblanks instead of sleeping worker

[Gustavo Padovan]

2016-09-23 Andrzej Hajda :

> VIDI driver uses fake vblank handler to generate vblank events.
> It was implemented using worker which slept for vblank time, additionally
> it did not work if there were no page flips. The patch replaces it with
> timer, uses drm_crtc_vblank_(on|off) helpers to manage it and fixes
> behavior for non-page-flip cases.
> This change allows further improvements of vblank in exynos-drm framework.
> 
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 
> ++--
>  1 file changed, 20 insertions(+), 46 deletions(-)

Reviewed-by: Gustavo Padovan 

Gustavo

[PATCH 2/2] drm/exynos: fix pending update handling

[Gustavo Padovan]

2016-09-23 Andrzej Hajda :

> Exynos DRM devices update their registers at vblank time. Exynos-DRM uses
> custom mechanism to wait for vblank. This mechanism is error prone -
> variables are not updated atomically. As a result in certain circumstances
> user space can try to free buffers which are still in use by hardware,
> in such cases IOMMU can throw OOPS.
> The patch instead of fixing the mechanism replaces it with drm core helper.
> 
> Signed-off-by: Andrzej Hajda 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 +---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  | 44 
> +---
>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |  2 --
>  3 files changed, 2 insertions(+), 60 deletions(-)

Reviewed-by: Gustavo Padovan 

Gustavo

[PATCH v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Laurent Pinchart]

Hi Daniel,

On Wednesday 21 Sep 2016 09:31:58 Daniel Vetter wrote:
> On Thu, Sep 08, 2016 at 05:44:26PM +0300, Laurent Pinchart wrote:
> > The driver is the last users of the drm_fb_get_bpp_depth() function. It
> > should ideally be converted to use struct drm_mode_fb_cmd2 instead of
> > the legacy struct drm_mode_fb_cmd internally, but that will require
> > broad changes across the code base. As a first step, replace
> > drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
> > the function to drivers.
> > 
> > The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
> > vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
> > functions that currently print an error if the pixel format is
> > unsupported.
> > 
> > Signed-off-by: Laurent Pinchart 
> > Reviewed-by: Sinclair Yeh 
> > ---
> > 
> >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > Cc: VMware Graphics 
> > Cc: Sinclair Yeh 
> > Cc: Thomas Hellstrom 
> > 
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index bf28ccc150df..c965514b82be
> > 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > @@ -980,14 +980,22 @@ static struct drm_framebuffer
> > *vmw_kms_fb_create(struct drm_device *dev,> 
> > struct vmw_dma_buffer *bo = NULL;
> > struct ttm_base_object *user_obj;
> > struct drm_mode_fb_cmd mode_cmd;
> > 
> > +   const struct drm_format_info *info;
> > 
> > int ret;
> > 
> > +   info = drm_format_info(mode_cmd2->pixel_format);
> > +   if (!info || !info->depth) {
> > +   DRM_ERROR("Unsupported framebuffer format %s\n",
> > + drm_get_format_name(mode_cmd2->pixel_format));
> > +   return ERR_PTR(-EINVAL);
> > +   }
> > +
> > 
> > mode_cmd.width = mode_cmd2->width;
> > mode_cmd.height = mode_cmd2->height;
> > mode_cmd.pitch = mode_cmd2->pitches[0];
> > mode_cmd.handle = mode_cmd2->handles[0];
> > 
> > -   drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
> > -   &mode_cmd.bpp);
> > +   mode_cmd.depth = info->depth;
> > +   mode_cmd.bpp = info->cpp[0] * 8;
> 
> I think this should use drm_helper_mode_fill_fb_struct instead.

I would do that if there was a struct drm_framebuffer to fill, but this piece 
of code converts from struct drm_mode_fb_cmd2 to drm_mode_fb_cmd that is then 
used all over the place internally. This should be fixed, but I think that's 
out of scope for this patch series.

> > /**
> > 
> >  * This code should be conditioned on Screen Objects not being used.

-- 
Regards,

Laurent Pinchart

[PATCH v4 12/14] drm: vmwgfx: Replace drm_fb_get_bpp_depth() with drm_format_info()

[Daniel Vetter]

On Fri, Sep 23, 2016 at 03:40:17PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wednesday 21 Sep 2016 09:31:58 Daniel Vetter wrote:
> > On Thu, Sep 08, 2016 at 05:44:26PM +0300, Laurent Pinchart wrote:
> > > The driver is the last users of the drm_fb_get_bpp_depth() function. It
> > > should ideally be converted to use struct drm_mode_fb_cmd2 instead of
> > > the legacy struct drm_mode_fb_cmd internally, but that will require
> > > broad changes across the code base. As a first step, replace
> > > drm_fb_get_bpp_depth() with drm_format_info() in order to stop exporting
> > > the function to drivers.
> > > 
> > > The new DRM_ERROR() message comes from the vmw_create_dmabuf_proxy(),
> > > vmw_kms_new_framebuffer_surface() and vmw_kms_new_framebuffer_dmabuf()
> > > functions that currently print an error if the pixel format is
> > > unsupported.
> > > 
> > > Signed-off-by: Laurent Pinchart 
> > > Reviewed-by: Sinclair Yeh 
> > > ---
> > > 
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > Cc: VMware Graphics 
> > > Cc: Sinclair Yeh 
> > > Cc: Thomas Hellstrom 
> > > 
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index bf28ccc150df..c965514b82be
> > > 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > @@ -980,14 +980,22 @@ static struct drm_framebuffer
> > > *vmw_kms_fb_create(struct drm_device *dev,> 
> > >   struct vmw_dma_buffer *bo = NULL;
> > >   struct ttm_base_object *user_obj;
> > >   struct drm_mode_fb_cmd mode_cmd;
> > > 
> > > + const struct drm_format_info *info;
> > > 
> > >   int ret;
> > > 
> > > + info = drm_format_info(mode_cmd2->pixel_format);
> > > + if (!info || !info->depth) {
> > > + DRM_ERROR("Unsupported framebuffer format %s\n",
> > > +   drm_get_format_name(mode_cmd2->pixel_format));
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > +
> > > 
> > >   mode_cmd.width = mode_cmd2->width;
> > >   mode_cmd.height = mode_cmd2->height;
> > >   mode_cmd.pitch = mode_cmd2->pitches[0];
> > >   mode_cmd.handle = mode_cmd2->handles[0];
> > > 
> > > - drm_fb_get_bpp_depth(mode_cmd2->pixel_format, &mode_cmd.depth,
> > > - &mode_cmd.bpp);
> > > + mode_cmd.depth = info->depth;
> > > + mode_cmd.bpp = info->cpp[0] * 8;
> > 
> > I think this should use drm_helper_mode_fill_fb_struct instead.
> 
> I would do that if there was a struct drm_framebuffer to fill, but this piece 
> of code converts from struct drm_mode_fb_cmd2 to drm_mode_fb_cmd that is then 
> used all over the place internally. This should be fixed, but I think that's 
> out of scope for this patch series.

Oh right, I didn't realize that ...

Reviewed-by: Daniel Vetter 

> 
> > >   /**
> > >   
> > >* This code should be conditioned on Screen Objects not being used.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Brian Starkey]

On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
>On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau  wrote:
>> rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
>> enabled, but we never got the call to turn off the CRTC. Brian is still
>> tracking through the fbdev emulation to figure out the cause for that.
>
>fbdev emulation doesn't do that for you. If you need/want to shut down
>all the crtcs on driver unload, you need to do that yourself. There's
>atomic helpers to do that for you that for you.

The problem is a sort-of circular dependency between ->lastclose (at
least the common implementation of it), unregister and disabling
fbdev.

I want to move drm_dev_unregister() to be the first thing we do at
rmmod-time. However we need to disable fbdev first, otherwise
->lastclose restores the fbdev mode, guaranteeing that vsync is turned
on for drm_vsync_cleanup() to then WARN on.

There's a slightly different (perceived) problem - the one that Liviu
mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
You say it's not the fbdev helpers' responsibility to teardown their
modeset, but regardless I have nowhere to disable the CRTC if I want
to do drm_dev_unregister() first; and if the CRTC isn't disabled
there's always a chance of hitting the same vsync WARN even without
fbdev.

We *could* add an ->unload and disable everything there, but as that's
deprecated I'm guessing there should be another way.
Perhaps we should track ->firstopen/->lastclose pairs so we can detect
that ->lastclose is being called from unregister and use it to
disable everything in that case.

drm_vblank_cleanup() seems to have been carried over to unregister
from drm_put_dev(), but drm_dev_register() doesn't call
drm_vblank_init() so it seems a little strange to have it there.
I can see other drivers I'd expect to hit the same WARN but I don't
have HW to test it on.

Thanks,
Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>

[PATCH 01/11] drm/amdgpu: Remove call to reservation_object_test_signaled_rcu before wait

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:24AM +0100, Chris Wilson wrote:
> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> need to handle such conversion in the caller. The only challenge are
> those callers that wish to differentiate the error code between the
> nonblocking busy check and potentially blocking wait.
> 
> Signed-off-by: Chris Wilson 
> Cc: Alex Deucher 
> Cc: Christian König 

Reviewed-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 88fbed2389c0..a3e6f883ac2c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -407,10 +407,8 @@ int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, 
> void *data,
>   return -ENOENT;
>   }
>   robj = gem_to_amdgpu_bo(gobj);
> - if (timeout == 0)
> - ret = reservation_object_test_signaled_rcu(robj->tbo.resv, 
> true);
> - else
> - ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, 
> true, timeout);
> + ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, true,
> +   timeout);
>  
>   /* ret == 0 means not signaled,
>* ret > 0 means signaled
> -- 
> 2.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 02/11] drm/etnaviv: Remove manual call to reservation_object_test_signaled_rcu before wait

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:25AM +0100, Chris Wilson wrote:
> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> need to handle such conversion in the caller. The only challenge are
> those callers that wish to differentiate the error code between the
> nonblocking busy check and potentially blocking wait.
> 
> Signed-off-by: Chris Wilson 
> Cc: Lucas Stach 
> Cc: Russell King 
> Cc: Christian Gmeiner 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 24 ++--
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 5ce3603e6eac..9ffca2478e02 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -409,20 +409,16 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, 
> u32 op,
>   struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
>   struct drm_device *dev = obj->dev;
>   bool write = !!(op & ETNA_PREP_WRITE);
> - int ret;
> -
> - if (op & ETNA_PREP_NOSYNC) {
> - if (!reservation_object_test_signaled_rcu(etnaviv_obj->resv,
> -   write))
> - return -EBUSY;
> - } else {
> - unsigned long remain = etnaviv_timeout_to_jiffies(timeout);
> -
> - ret = reservation_object_wait_timeout_rcu(etnaviv_obj->resv,
> -   write, true, remain);
> - if (ret <= 0)
> - return ret == 0 ? -ETIMEDOUT : ret;
> - }
> + unsigned long remain =
> + op & ETNA_PREP_NOSYNC ? 0 : etnaviv_timeout_to_jiffies(timeout);
> + long lret;
> +
> + lret = reservation_object_wait_timeout_rcu(etnaviv_obj->resv,
> +write, true, remain);
> + if (lret < 0)
> + return lret;
> + else if (lret == 0)
> + return remain == 0 ? -EBUSY : -ETIMEDOUT;
>  
>   if (etnaviv_obj->flags & ETNA_BO_CACHED) {
>   if (!etnaviv_obj->sgt) {
> -- 
> 2.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 03/11] drm/msm: Remove call to reservation_object_test_signaled_rcu before wait

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:26AM +0100, Chris Wilson wrote:
> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> need to handle such conversion in the caller. The only challenge are
> those callers that wish to differentiate the error code between the
> nonblocking busy check and potentially blocking wait.
> 
> v2: 9 is only 0 in German.
> 
> Signed-off-by: Chris Wilson 
> Cc: Rob Clark 

Reviewed-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/msm/msm_gem.c | 22 ++
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 6cd4af443139..45796a88d353 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -584,18 +584,16 @@ int msm_gem_cpu_prep(struct drm_gem_object *obj, 
> uint32_t op, ktime_t *timeout)
>  {
>   struct msm_gem_object *msm_obj = to_msm_bo(obj);
>   bool write = !!(op & MSM_PREP_WRITE);
> -
> - if (op & MSM_PREP_NOSYNC) {
> - if (!reservation_object_test_signaled_rcu(msm_obj->resv, write))
> - return -EBUSY;
> - } else {
> - int ret;
> -
> - ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
> - true, timeout_to_jiffies(timeout));
> - if (ret <= 0)
> - return ret == 0 ? -ETIMEDOUT : ret;
> - }
> + unsigned long remain =
> + op & MSG_PREP_NOSYNC ? 0 : timeout_to_jiffies(timeout);
> + long ret;
> +
> + ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
> +   true,  remain);
> + if (ret == 0)
> + return remain == 0 ? -EBUSY : -ETIMEDOUT;
> + else if (ret < 0)
> + return ret;
>  
>   /* TODO cache maintenance */
>  
> -- 
> 2.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

GPU-DRM-TTM: Fine-tuning for several function implementations

[Dan Carpenter]

On Fri, Sep 23, 2016 at 12:20:54PM +0200, SF Markus Elfring wrote:
> > if you call it "restart" or "lock_restart" doesn't make much difference.
> 
> Do other identifiers fit better to a specification from the document 
> "CodingStyle"
> like the following?
> 
> "…
> Choose label names which say what the goto does or why the goto exists.
> …"
>
> 
> Does this wording need any more adjustments?

No.  I wrote that and "restart" seems like a pretty clear name to me.  I
never wrote that you should harrass people with your nonsense patches.
In fact, I have asked you over and over again to stop.

regards,
dan carpenter

[Intel-gfx] [PATCH 04/11] drm/nouveau: Remove call to reservation_object_test_signaled_rcu before wait

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:27AM +0100, Chris Wilson wrote:
> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> need to handle such conversion in the caller. The only challenge are
> those callers that wish to differentiate the error code between the
> nonblocking busy check and potentially blocking wait.
> 
> Signed-off-by: Chris Wilson 
> Cc: Ben Skeggs 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/nouveau/nouveau_gem.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c 
> b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 72e2399bce39..b90e21ff1ed8 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -861,6 +861,7 @@ nouveau_gem_ioctl_cpu_prep(struct drm_device *dev, void 
> *data,
>   struct nouveau_bo *nvbo;
>   bool no_wait = !!(req->flags & NOUVEAU_GEM_CPU_PREP_NOWAIT);
>   bool write = !!(req->flags & NOUVEAU_GEM_CPU_PREP_WRITE);
> + long lret;
>   int ret;
>  
>   gem = drm_gem_object_lookup(file_priv, req->handle);
> @@ -868,19 +869,15 @@ nouveau_gem_ioctl_cpu_prep(struct drm_device *dev, void 
> *data,
>   return -ENOENT;
>   nvbo = nouveau_gem_object(gem);
>  
> - if (no_wait)
> - ret = reservation_object_test_signaled_rcu(nvbo->bo.resv, 
> write) ? 0 : -EBUSY;
> - else {
> - long lret;
> + lret = reservation_object_wait_timeout_rcu(nvbo->bo.resv, write, true,
> +no_wait ? 0 :30 * HZ);
> + if (!lret)
> + ret = -EBUSY;
> + else if (lret > 0)
> + ret = 0;
> + else
> + ret = lret;
>  
> - lret = reservation_object_wait_timeout_rcu(nvbo->bo.resv, 
> write, true, 30 * HZ);
> - if (!lret)
> - ret = -EBUSY;
> - else if (lret > 0)
> - ret = 0;
> - else
> - ret = lret;
> - }
>   nouveau_bo_sync_for_cpu(nvbo);
>   drm_gem_object_unreference_unlocked(gem);
>  
> -- 
> 2.9.3
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 05/11] drm/vmwgfx: Remove call to reservation_object_test_signaled_rcu before wait

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:28AM +0100, Chris Wilson wrote:
> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
> need to handle such conversion in the caller. The only challenge are
> those callers that wish to differentiate the error code between the
> nonblocking busy check and potentially blocking wait.
> 
> Signed-off-by: Chris Wilson 
> Cc: Sinclair Yeh 
> Cc: Thomas Hellstrom 
> Reviewed-by: Sinclair Yeh 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c 
> b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index 6a328d507a28..1a85fb2d4dc6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -574,10 +574,8 @@ static int vmw_user_dmabuf_synccpu_grab(struct 
> vmw_user_dma_buffer *user_bo,
>   bool nonblock = !!(flags & drm_vmw_synccpu_dontblock);
>   long lret;
>  
> - if (nonblock)
> - return reservation_object_test_signaled_rcu(bo->resv, 
> true) ? 0 : -EBUSY;
> -
> - lret = reservation_object_wait_timeout_rcu(bo->resv, true, 
> true, MAX_SCHEDULE_TIMEOUT);
> + lret = reservation_object_wait_timeout_rcu(bo->resv, true, true,
> +nonblock ? 0 : 
> MAX_SCHEDULE_TIMEOUT);
>   if (!lret)
>   return -EBUSY;
>   else if (lret < 0)
> -- 
> 2.9.3
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 06/11] dma-buf: Introduce fence_get_rcu_safe()

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:29AM +0100, Chris Wilson wrote:
> This variant of fence_get_rcu() takes an RCU protected pointer to a
> fence and carefully returns a reference to the fence ensuring that it is
> not reallocated as it does. This is required when mixing fences and
> SLAB_DESTROY_BY_RCU - although it serves a more pedagogical function atm
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org
> ---
>  include/linux/fence.h | 56 
> ++-
>  1 file changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 0d763053f97a..c9c5ba98c302 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -183,6 +183,16 @@ void fence_release(struct kref *kref);
>  void fence_free(struct fence *fence);
>  
>  /**
> + * fence_put - decreases refcount of the fence
> + * @fence:   [in]fence to reduce refcount of
> + */
> +static inline void fence_put(struct fence *fence)
> +{
> + if (fence)
> + kref_put(&fence->refcount, fence_release);
> +}
> +
> +/**
>   * fence_get - increases refcount of the fence
>   * @fence:   [in]fence to increase refcount of
>   *
> @@ -210,13 +220,49 @@ static inline struct fence *fence_get_rcu(struct fence 
> *fence)
>  }
>  
>  /**
> - * fence_put - decreases refcount of the fence
> - * @fence:   [in]fence to reduce refcount of
> + * fence_get_rcu_safe  - acquire a reference to an RCU tracked fence
> + * @fence:   [in]pointer to fence to increase refcount of
> + *
> + * Function returns NULL if no refcount could be obtained, or the fence.
> + * This function handles acquiring a reference to a fence that may be
> + * reallocated within the RCU grace period (such as with 
> SLAB_DESTROY_BY_RCU),
> + * so long as the caller is using RCU on the pointer to the fence.
> + *
> + * An alternative mechanism is to employ a seqlock to protect a bunch of
> + * fences, such as used by struct reservation_object. When using a seqlock,
> + * the seqlock must be taken before and checked after a reference to the
> + * fence is acquired (as shown here).
> + *
> + * The caller is required to hold the RCU read lock.

Would be good to cross reference the various fence_get functions a bit
better in the docs. But since the docs aren't yet pulled into the rst/html
output, that doesn't matter that much. Hence as-is:

Reviewed-by: Daniel Vetter 

>   */
> -static inline void fence_put(struct fence *fence)
> +static inline struct fence *fence_get_rcu_safe(struct fence * __rcu *fencep)
>  {
> - if (fence)
> - kref_put(&fence->refcount, fence_release);
> + do {
> + struct fence *fence;
> +
> + fence = rcu_dereference(*fencep);
> + if (!fence || !fence_get_rcu(fence))
> + return NULL;
> +
> + /* The atomic_inc_not_zero() inside fence_get_rcu()
> +  * provides a full memory barrier upon success (such as now).
> +  * This is paired with the write barrier from assigning
> +  * to the __rcu protected fence pointer so that if that
> +  * pointer still matches the current fence, we know we
> +  * have successfully acquire a reference to it. If it no
> +  * longer matches, we are holding a reference to some other
> +  * reallocated pointer. This is possible if the allocator
> +  * is using a freelist like SLAB_DESTROY_BY_RCU where the
> +  * fence remains valid for the RCU grace period, but it
> +  * may be reallocated. When using such allocators, we are
> +  * responsible for ensuring the reference we get is to
> +  * the right fence, as below.
> +  */
> + if (fence == rcu_access_pointer(*fencep))
> + return rcu_pointer_handoff(fence);
> +
> + fence_put(fence);
> + } while (1);
>  }
>  
>  int fence_signal(struct fence *fence);
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 07/11] dma-buf: Restart reservation_object_get_fences_rcu() after writes

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:30AM +0100, Chris Wilson wrote:
> In order to be completely generic, we have to double check the read
> seqlock after acquiring a reference to the fence. If the driver is
> allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
> within an RCU grace period a fence may be freed and reallocated. The RCU
> read side critical section does not prevent this reallocation, instead
> we have to inspect the reservation's seqlock to double check if the
> fences have been reassigned as we were acquiring our reference.
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Christian König 
> Cc: Alex Deucher 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org
> ---
>  drivers/dma-buf/reservation.c | 71 
> +++
>  1 file changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 723d8af988e5..10fd441dd4ed 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -280,18 +280,24 @@ int reservation_object_get_fences_rcu(struct 
> reservation_object *obj,
> unsigned *pshared_count,
> struct fence ***pshared)
>  {
> - unsigned shared_count = 0;
> - unsigned retry = 1;
> - struct fence **shared = NULL, *fence_excl = NULL;
> - int ret = 0;
> + struct fence **shared = NULL;
> + struct fence *fence_excl;
> + unsigned shared_count;
> + int ret = 1;

Personally I'd go with ret = -EBUSY here, but that's a bikeshed.

Reviewed-by: Daniel Vetter 
>  
> - while (retry) {
> + do {
>   struct reservation_object_list *fobj;
>   unsigned seq;
> + unsigned i;
>  
> - seq = read_seqcount_begin(&obj->seq);
> + shared_count = i = 0;
>  
>   rcu_read_lock();
> + seq = read_seqcount_begin(&obj->seq);
> +
> + fence_excl = rcu_dereference(obj->fence_excl);
> + if (fence_excl && !fence_get_rcu(fence_excl))
> + goto unlock;
>  
>   fobj = rcu_dereference(obj->fence);
>   if (fobj) {
> @@ -309,52 +315,37 @@ int reservation_object_get_fences_rcu(struct 
> reservation_object *obj,
>   }
>  
>   ret = -ENOMEM;
> - shared_count = 0;
>   break;
>   }
>   shared = nshared;
> - memcpy(shared, fobj->shared, sz);
>   shared_count = fobj->shared_count;
> - } else
> - shared_count = 0;
> - fence_excl = rcu_dereference(obj->fence_excl);
> -
> - retry = read_seqcount_retry(&obj->seq, seq);
> - if (retry)
> - goto unlock;
> -
> - if (!fence_excl || fence_get_rcu(fence_excl)) {
> - unsigned i;
>  
>   for (i = 0; i < shared_count; ++i) {
> - if (fence_get_rcu(shared[i]))
> - continue;
> -
> - /* uh oh, refcount failed, abort and retry */
> - while (i--)
> - fence_put(shared[i]);
> -
> - if (fence_excl) {
> - fence_put(fence_excl);
> - fence_excl = NULL;
> - }
> -
> - retry = 1;
> - break;
> + shared[i] = rcu_dereference(fobj->shared[i]);
> + if (!fence_get_rcu(shared[i]))
> + break;
>   }
> - } else
> - retry = 1;
> + }
> +
> + if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) {
> + while (i--)
> + fence_put(shared[i]);
> + fence_put(fence_excl);
> + goto unlock;
> + }
>  
> + ret = 0;
>  unlock:
>   rcu_read_unlock();
> - }
> - *pshared_count = shared_count;
> - if (shared_count)
> - *pshared = shared;
> - else {
> - *pshared = NULL;
> + } while (ret);
> +
> + if (!shared_count) {
>   kfree(shared);
> + shared = NULL;
>   }
> +
> + *pshared_count = shared_count;
> + *pshared = shared;
>   *pfence_excl = fence_excl;
>  
>   return ret;
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection

[Rob Clark]

On Fri, Sep 23, 2016 at 8:17 AM, SF Markus Elfring
 wrote:
>>> I see a need to improve not only correctness there but also a bit of
>>> software efficiency.
>>
>> If you can measure any performance difference and present some results
>> (esp. considering that this is something that just happens when the
>> driver is loaded), then we'll talk.
>
> Are you really interested to increase your software development attention
> for a quicker exception handling implementation in this function?
>

No one is interested in making error handling more complex for a
non-existent benefit ;-)

>
>> Until then, please don't send this sort of patch.  Thank you.
>
> Will benchmark statistics change such a change rejection ever?

If you could demonstrate a real benefit to additional complexity for
something that actually matters (ie. not something like this that runs
once at bootup) I would care.

I don't recommend that you waste your time, since there is
approximately nothing in modesetting path that happens with a high
enough frequency to benefit from such a micro-optimization.
Especially not initialization code that runs once at boot up.

Fixing actual bugs is useful and valuable.  Premature "optimization"
at the expense of extra complexity is very much not useful.

BR,
-R

> Regards,
> Markus

GPU-DRM-TTM: Fine-tuning for several function implementations

[Christian König]

Am 23.09.2016 um 13:49 schrieb SF Markus Elfring:
>> Calling the label "unlock" instead of "out" is arguable a little better,
> Thanks that you can follow a renaming for this direction in principle.
>
>
>> but nothing I would call a major improvement either.
> This was not my intention for such an use case.
>
> I am proposing some small software updates according to such a design pattern.
>
>
>> So that is a clear NAK to all those patches.
> Do you reject also update steps like the following then?
>
> * drm/ttm: Use kmalloc_array() in two (or four?) functions"
>
> * drm/ttm: Less function calls in ttm_dma_pool_init() after error detection

The reason behind the advise to use kmalloc_array() is to avoid overruns 
when one of the parameters come from an IOCTL and so are controllable by 
user space.

Those overruns where the source of numerous security problems, but in 
this case the parameters don't come from an IOCTL and aren't user space 
controllable.

So this change actually doesn't make to much sense either, but I'm 
leaning towards accepting them for coding style consistency.

Regards,
Christian.

> * Would you like to improve the usage of the variables "n" and "t"
>in the function "ttm_dma_pool_init" any further as Joe Perches suggested 
> it?
>
>
>>> 2. How do you think about to add a single space character before any label?
>> Bad as well. Why would anybody want to do this?
> Do you find another software evolution interesting according to a recent 
> commit?
>
> "docs: Remove space-before-label guidance from CodingStyle"
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a
>
>
> Regards,
> Markus

[PATCH] drm/radeon: Fix negative cursor position

[Takashi Iwai]

radeon_cursor_move_unlock() contains a workaround for AVIVO chips that
are older than DCE6 when the cursor ends on 128 pixel boundary.  It
decreases the position when the calculated end position is on 128
pixel boundary.  However, it hits also the condition where x=-1 and
width=1 are passed, since cursor_end is 0 (which is aligned with 128,
too).  Then the value gets decreased and it hits WARN_ON_ONCE()
below, eventually screws up the GPU.

The fix is simply to skip the workaround when x is already zero.
Also, remove the superfluous WARN_ON_ON() for the negative value check
that wouldn't happen any longer after this change.

Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1000433
Cc: 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/radeon/radeon_cursor.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c 
b/drivers/gpu/drm/radeon/radeon_cursor.c
index 2a10e24b34b1..4e436eb49a56 100644
--- a/drivers/gpu/drm/radeon/radeon_cursor.c
+++ b/drivers/gpu/drm/radeon/radeon_cursor.c
@@ -193,10 +193,8 @@ static int radeon_cursor_move_locked(struct drm_crtc 
*crtc, int x, int y)
if (w <= 0) {
w = 1;
cursor_end = x - xorigin + w;
-   if (!(cursor_end & 0x7f)) {
+   if (x > 0 && !(cursor_end & 0x7f))
x--;
-   WARN_ON_ONCE(x < 0);
-   }
}
}
}
-- 
2.10.0

[Intel-gfx] [PATCH 03/11] drm/msm: Remove call to reservation_object_test_signaled_rcu before wait

[Rob Clark]

On Fri, Sep 23, 2016 at 8:55 AM, Daniel Vetter  wrote:
> On Mon, Aug 29, 2016 at 08:08:26AM +0100, Chris Wilson wrote:
>> Since fence_wait_timeout_reservation_object_wait_timeout_rcu() with a
>> timeout of 0 becomes reservation_object_test_signaled_rcu(), we do not
>> need to handle such conversion in the caller. The only challenge are
>> those callers that wish to differentiate the error code between the
>> nonblocking busy check and potentially blocking wait.
>>
>> v2: 9 is only 0 in German.
>>
>> Signed-off-by: Chris Wilson 
>> Cc: Rob Clark 
>
> Reviewed-by: Daniel Vetter 

fyi, this is in my msm-next pull-req sent last week..  (with one small
s/MSG_PREP_NOSYNC/MSM_PREP_NOSYNC/ fixup ;-))

BR,
-R


>> ---
>>  drivers/gpu/drm/msm/msm_gem.c | 22 ++
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index 6cd4af443139..45796a88d353 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -584,18 +584,16 @@ int msm_gem_cpu_prep(struct drm_gem_object *obj, 
>> uint32_t op, ktime_t *timeout)
>>  {
>>   struct msm_gem_object *msm_obj = to_msm_bo(obj);
>>   bool write = !!(op & MSM_PREP_WRITE);
>> -
>> - if (op & MSM_PREP_NOSYNC) {
>> - if (!reservation_object_test_signaled_rcu(msm_obj->resv, 
>> write))
>> - return -EBUSY;
>> - } else {
>> - int ret;
>> -
>> - ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
>> - true, timeout_to_jiffies(timeout));
>> - if (ret <= 0)
>> - return ret == 0 ? -ETIMEDOUT : ret;
>> - }
>> + unsigned long remain =
>> + op & MSG_PREP_NOSYNC ? 0 : timeout_to_jiffies(timeout);
>> + long ret;
>> +
>> + ret = reservation_object_wait_timeout_rcu(msm_obj->resv, write,
>> +   true,  remain);
>> + if (ret == 0)
>> + return remain == 0 ? -EBUSY : -ETIMEDOUT;
>> + else if (ret < 0)
>> + return ret;
>>
>>   /* TODO cache maintenance */
>>
>> --
>> 2.9.3
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/udl: fix line iterator in damage handling

[Eric Engestrom]

On Fri, Sep 23, 2016 at 12:36:02PM +0200, David Herrmann wrote:
> The udl damage handler is supposed to render 'height' lines, but its
> iterator has an obvious typo that makes it miss most lines if the
> rectangle does not cover 0/0.
> 
> Fix the damage handler to correctly render all lines.
> 
> This is a fallout from:
> 
> commit e375882406d0cc24030746638592004755ed4ae0
> Author: Noralf Trønnes 
> Date:   Thu Apr 28 17:18:37 2016 +0200
> 
> drm/udl: Use drm_fb_helper deferred_io support
> 
> Tested-by: poma 
> Reviewed-by: Daniel Vetter 
> Signed-off-by: David Herrmann 

Reviewed-by: Eric Engestrom 

> ---
>  drivers/gpu/drm/udl/udl_fb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 9688bfa..611b6b9 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -122,7 +122,7 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, 
> int y,
>   return 0;
>   cmd = urb->transfer_buffer;
>  
> - for (i = y; i < height ; i++) {
> + for (i = y; i < y + height ; i++) {
>   const int line_offset = fb->base.pitches[0] * i;
>   const int byte_offset = line_offset + (x * bpp);
>   const int dev_byte_offset = (fb->base.width * bpp * i) + (x * 
> bpp);
> -- 
> 2.10.0

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

[Daniel Vetter]

On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
> On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau  
> > wrote:
> > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> > > enabled, but we never got the call to turn off the CRTC. Brian is still
> > > tracking through the fbdev emulation to figure out the cause for that.
> > 
> > fbdev emulation doesn't do that for you. If you need/want to shut down
> > all the crtcs on driver unload, you need to do that yourself. There's
> > atomic helpers to do that for you that for you.
> 
> The problem is a sort-of circular dependency between ->lastclose (at
> least the common implementation of it), unregister and disabling
> fbdev.
> 
> I want to move drm_dev_unregister() to be the first thing we do at
> rmmod-time. However we need to disable fbdev first, otherwise
> ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
> on for drm_vsync_cleanup() to then WARN on.
> 
> There's a slightly different (perceived) problem - the one that Liviu
> mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
> You say it's not the fbdev helpers' responsibility to teardown their
> modeset, but regardless I have nowhere to disable the CRTC if I want
> to do drm_dev_unregister() first; and if the CRTC isn't disabled
> there's always a chance of hitting the same vsync WARN even without
> fbdev.

Just disable all crtc in a suitable place (after drm_dev_unregister,
before you tear down fbdev).
> 
> We *could* add an ->unload and disable everything there, but as that's
> deprecated I'm guessing there should be another way.
> Perhaps we should track ->firstopen/->lastclose pairs so we can detect
> that ->lastclose is being called from unregister and use it to
> disable everything in that case.

Hm, maybe we should simply not call ->lastclose for kms drivers. That is
kinda only a hack for ums/dri1 drivers.

But even with that gone you might still unload while fbdev is enabled, so
this won't fix it all.

> drm_vblank_cleanup() seems to have been carried over to unregister
> from drm_put_dev(), but drm_dev_register() doesn't call
> drm_vblank_init() so it seems a little strange to have it there.
> I can see other drivers I'd expect to hit the same WARN but I don't
> have HW to test it on.

Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
should push it into all callers instead I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[GIT PULL] tilcdc 3rd set of changes for v4.9 (second attempt)

[Jyri Sarha]

Hi Dave,
Please pull these collected fixes and cleanups from various sources. The
request was rebased on top the previous tilcdc pull request tag, so it
contains all the accumulated tilcdc changes for v4.9 so far.

Thanks,
Jyri

The following changes since commit 2e0965b06d90b9dba76198d026c4c2ee04443aca:

  drm/tilcdc: WARN if CRTC is touched without CRTC lock (2016-09-07
15:54:43 +0300)

are available in the git repository at:

  https://github.com/jsarha/linux tags/tilcdc-4.9-3.1

for you to fetch changes up to 7b993855dfd5d87e07ac84285d3d9bb0c743dede:

  drm/tilcdc: fix wrong error handling (2016-09-23 15:12:57 +0300)


Second attempt for 3rd drm/tilcdc pull request for v4.9.


Baoyou Xie (2):
  drm/tilcdc: add missing header dependencies
  drm/tilcdc: mark symbols static where possible

Daniel Schultz (1):
  drm/tilcdc: fix wrong error handling

Jyri Sarha (1):
  drm/tilcdc: Remove "default" from blue-and-red-wiring property binding

Markus Elfring (1):
  drm/tilcdc: Return directly after a failed kfree_table_init() in
tilcdc_convert_slave_node()

Wei Yongjun (1):
  drm/tilcdc: Fix non static symbol warning

 Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt |  6 +++---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 10 +-
 drivers/gpu/drm/tilcdc/tilcdc_panel.c   |  1 +
 drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c|  8 
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c  |  1 +
 5 files changed, 14 insertions(+), 12 deletions(-)

[PATCH] drm/sun4i: rgb: Enable panel after controller

[Maxime Ripard]

On Thu, Sep 22, 2016 at 08:03:31AM +1000, Jonathan Liu wrote:
> Hi Maxime,
> 
> On Thursday, 22 September 2016, Maxime Ripard  free-electrons.
> com> wrote:
> 
> > On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote:
> > > The panel should be enabled after the controller so that the panel
> > > prepare/enable delays are properly taken into account. Similarly, the
> > > panel should be disabled before the controller so that the panel
> > > unprepare/disable delays are properly taken into account.
> > >
> > > This is useful for avoiding visual glitches.
> >
> > This is not really taking any delays into account, especially since
> > drm_panel_enable and prepare are suppose to block until their
> > operation is complete.
> 
> 
> drm_panel_prepare turns on power to the LCD using enable-gpios property of
> the panel and then blocks for prepare delay. The prepare delay for panel
> can be set to how long it takes between the time the panel is powered to
> when it is ready to receive images. If backlight property is specified the
> backlight will be off while the panel is powered on.
> 
> drm_panel_enable blocks for enable delay and then turns on the backlight.
> The enable delay can be set to how long it takes for panel to start making
> the image visible after receiving the first valid frame. For example if the
> panel starts off as white and the TFT takes some time to initialize to
> black before it shows the image being received.
> 
> Refer to drivers/gpu/drm/panel-panel.simple.c for details.

[PATCH 08/11] dma-buf: Restart reservation_object_wait_timeout_rcu() after writes

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:31AM +0100, Chris Wilson wrote:
> In order to be completely generic, we have to double check the read
> seqlock after acquiring a reference to the fence. If the driver is
> allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
> within an RCU grace period a fence may be freed and reallocated. The RCU
> read side critical section does not prevent this reallocation, instead
> we have to inspect the reservation's seqlock to double check if the
> fences have been reassigned as we were acquiring our reference.
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Christian König 
> Cc: Alex Deucher 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org

Reviewed-by: Daniel Vetter 

> ---
>  drivers/dma-buf/reservation.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 10fd441dd4ed..3369e4668e96 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -388,9 +388,6 @@ retry:
>   if (fobj)
>   shared_count = fobj->shared_count;
>  
> - if (read_seqcount_retry(&obj->seq, seq))
> - goto unlock_retry;
> -
>   for (i = 0; i < shared_count; ++i) {
>   struct fence *lfence = rcu_dereference(fobj->shared[i]);
>  
> @@ -413,9 +410,6 @@ retry:
>   if (!shared_count) {
>   struct fence *fence_excl = rcu_dereference(obj->fence_excl);
>  
> - if (read_seqcount_retry(&obj->seq, seq))
> - goto unlock_retry;
> -
>   if (fence_excl &&
>   !test_bit(FENCE_FLAG_SIGNALED_BIT, &fence_excl->flags)) {
>   if (!fence_get_rcu(fence_excl))
> @@ -430,6 +424,11 @@ retry:
>  
>   rcu_read_unlock();
>   if (fence) {
> + if (read_seqcount_retry(&obj->seq, seq)) {
> + fence_put(fence);
> + goto retry;
> + }
> +
>   ret = fence_wait_timeout(fence, intr, ret);
>   fence_put(fence);
>   if (ret > 0 && wait_all && (i + 1 < shared_count))
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm/exynos: use drm core to handle page-flip event

[Andrzej Hajda]

Exynos DRM framework handled page-flip event with custom code.
The patch replaces it with drm-core vblank queue.

Signed-off-by: Andrzej Hajda 
---
Hi Inki,

This patch is next step of vblank re-factoring in Exynos-DRM.
I have tested the code on fimd, vidi, decon5433, mixer.
I hope I have not introduced bugs.

Regards
Andrzej
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 11 ---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c|  9 --
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 42 +--
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  2 --
 drivers/gpu/drm/exynos/exynos_drm_fimd.c  | 15 +-
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  1 -
 drivers/gpu/drm/exynos/exynos_drm_vidi.c  | 10 ---
 drivers/gpu/drm/exynos/exynos_mixer.c |  9 --
 8 files changed, 21 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c 
b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index ac21b40..6ca1f31 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -551,7 +551,6 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
 {
struct decon_context *ctx = dev_id;
u32 val;
-   int win;

if (!test_bit(BIT_CLKS_ENABLED, &ctx->flags))
goto out;
@@ -560,16 +559,6 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
val &= VIDINTCON1_INTFRMDONEPEND | VIDINTCON1_INTFRMPEND;

if (val) {
-   for (win = ctx->first_win; win < WINDOWS_NR ; win++) {
-   struct exynos_drm_plane *plane = &ctx->planes[win];
-
-   if (!plane->pending_fb)
-   continue;
-
-   exynos_drm_crtc_finish_update(ctx->crtc, plane);
-   }
-
-   /* clear */
writel(val, ctx->addr + DECON_VIDINTCON1);
drm_crtc_handle_vblank(&ctx->crtc->base);
}
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c 
b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 7f9901b..f4d5a21 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -603,7 +603,6 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)
 {
struct decon_context *ctx = (struct decon_context *)dev_id;
u32 val, clear_bit;
-   int win;

val = readl(ctx->regs + VIDINTCON1);

@@ -617,14 +616,6 @@ static irqreturn_t decon_irq_handler(int irq, void *dev_id)

if (!ctx->i80_if) {
drm_crtc_handle_vblank(&ctx->crtc->base);
-   for (win = 0 ; win < WINDOWS_NR ; win++) {
-   struct exynos_drm_plane *plane = &ctx->planes[win];
-
-   if (!plane->pending_fb)
-   continue;
-
-   exynos_drm_crtc_finish_update(ctx->crtc, plane);
-   }

/* set wait vsync event to zero and wake up queue. */
if (atomic_read(&ctx->wait_vsync_event)) {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c 
b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 5b6845b..2530bf5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -69,8 +69,6 @@ static void exynos_crtc_atomic_begin(struct drm_crtc *crtc,
 {
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);

-   exynos_crtc->event = crtc->state->event;
-
if (exynos_crtc->ops->atomic_begin)
exynos_crtc->ops->atomic_begin(exynos_crtc);
 }
@@ -79,9 +77,24 @@ static void exynos_crtc_atomic_flush(struct drm_crtc *crtc,
 struct drm_crtc_state *old_crtc_state)
 {
struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+   struct drm_pending_vblank_event *event;
+   unsigned long flags;

if (exynos_crtc->ops->atomic_flush)
exynos_crtc->ops->atomic_flush(exynos_crtc);
+
+   event = crtc->state->event;
+   if (event) {
+   crtc->state->event = NULL;
+
+   spin_lock_irqsave(&crtc->dev->event_lock, flags);
+   if (drm_crtc_vblank_get(crtc) == 0)
+   drm_crtc_arm_vblank_event(crtc, event);
+   else
+   drm_crtc_send_vblank_event(crtc, event);
+   spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+   }
+
 }

 static const struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
@@ -173,22 +186,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device 
*dev, unsigned int pipe)
exynos_crtc->ops->disable_vblank(exynos_crtc);
 }

-void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
-   struct exynos_drm_plane *exynos_plane)
-{
-   struct drm_crtc *crtc = &exynos_crtc->base;
-   unsigned long flags;
-
-   exynos_plane->pe

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[Emil Velikov]

On 23 September 2016 at 09:50, SF Markus Elfring
 wrote:
>> Markus, please contact the list in advance in future before posting a bunch
>> of patches that don't fix any problems.
>
> I am trying to improve various open issues also in Linux source files.
>
That the fact that you see issues (in these particular cases) while
others do not indicates that the commit summary could be explained
better.

A good commit summary should provide enough information to do that and
make people _want_ the patch. From my limited experience through your
patches (just skimmed a few) you seems to be describing what the patch
does as opposed to why it does it and why should one find it
interesting/wanted.

You might want to read through the following [1] [2] and many others
that exist out there.

-Emil

[1] http://chris.beams.io/posts/git-commit/
[2] http://who-t.blogspot.fi/2009/12/on-commit-messages.html

[PATCH 06/11] dma-buf: Introduce fence_get_rcu_safe()

[Markus Heiser]

Am 23.09.2016 um 14:59 schrieb Daniel Vetter :

>> 
>> /**
>> - * fence_put - decreases refcount of the fence
>> - * @fence:  [in]fence to reduce refcount of
>> + * fence_get_rcu_safe  - acquire a reference to an RCU tracked fence
>> + * @fence:  [in]pointer to fence to increase refcount of
>> + *
>> + * Function returns NULL if no refcount could be obtained, or the fence.
>> + * This function handles acquiring a reference to a fence that may be
>> + * reallocated within the RCU grace period (such as with 
>> SLAB_DESTROY_BY_RCU),
>> + * so long as the caller is using RCU on the pointer to the fence.
>> + *
>> + * An alternative mechanism is to employ a seqlock to protect a bunch of
>> + * fences, such as used by struct reservation_object. When using a seqlock,
>> + * the seqlock must be taken before and checked after a reference to the
>> + * fence is acquired (as shown here).
>> + *
>> + * The caller is required to hold the RCU read lock.
> 
> Would be good to cross reference the various fence_get functions a bit
> better in the docs. But since the docs aren't yet pulled into the rst/html
> output, that doesn't matter that much

Hi Daniel ... I'am working on ;-)

* 
http://return42.github.io/sphkerneldoc/linux_src_doc/include/linux/fence_h.html

-- Markus --

[PATCH 09/11] dma-buf: Restart reservation_object_test_signaled_rcu() after writes

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:32AM +0100, Chris Wilson wrote:
> In order to be completely generic, we have to double check the read
> seqlock after acquiring a reference to the fence. If the driver is
> allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then
> within an RCU grace period a fence may be freed and reallocated. The RCU
> read side critical section does not prevent this reallocation, instead
> we have to inspect the reservation's seqlock to double check if the
> fences have been reassigned as we were acquiring our reference.
> 
> Signed-off-by: Chris Wilson 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Christian König 
> Cc: Alex Deucher 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org

Reviewed-by: Daniel Vetter 

> ---
>  drivers/dma-buf/reservation.c | 30 ++
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 3369e4668e96..e74493e7332b 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -474,12 +474,13 @@ bool reservation_object_test_signaled_rcu(struct 
> reservation_object *obj,
> bool test_all)
>  {
>   unsigned seq, shared_count;
> - int ret = true;
> + int ret;
>  
> + rcu_read_lock();
>  retry:
> + ret = true;
>   shared_count = 0;
>   seq = read_seqcount_begin(&obj->seq);
> - rcu_read_lock();
>  
>   if (test_all) {
>   unsigned i;
> @@ -490,46 +491,35 @@ retry:
>   if (fobj)
>   shared_count = fobj->shared_count;
>  
> - if (read_seqcount_retry(&obj->seq, seq))
> - goto unlock_retry;
> -
>   for (i = 0; i < shared_count; ++i) {
>   struct fence *fence = rcu_dereference(fobj->shared[i]);
>  
>   ret = reservation_object_test_signaled_single(fence);
>   if (ret < 0)
> - goto unlock_retry;
> + goto retry;
>   else if (!ret)
>   break;
>   }
>  
> - /*
> -  * There could be a read_seqcount_retry here, but nothing cares
> -  * about whether it's the old or newer fence pointers that are
> -  * signaled. That race could still have happened after checking
> -  * read_seqcount_retry. If you care, use ww_mutex_lock.
> -  */
> + if (read_seqcount_retry(&obj->seq, seq))
> + goto retry;
>   }
>  
>   if (!shared_count) {
>   struct fence *fence_excl = rcu_dereference(obj->fence_excl);
>  
> - if (read_seqcount_retry(&obj->seq, seq))
> - goto unlock_retry;
> -
>   if (fence_excl) {
>   ret = reservation_object_test_signaled_single(
>   fence_excl);
>   if (ret < 0)
> - goto unlock_retry;
> + goto retry;
> +
> + if (read_seqcount_retry(&obj->seq, seq))
> + goto retry;
>   }
>   }
>  
>   rcu_read_unlock();
>   return ret;
> -
> -unlock_retry:
> - rcu_read_unlock();
> - goto retry;
>  }
>  EXPORT_SYMBOL_GPL(reservation_object_test_signaled_rcu);
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

GPU-DRM-TTM: Fine-tuning for several function implementations

[SF Markus Elfring]

>> Do other identifiers fit better to a specification from the document 
>> "CodingStyle"
>> like the following?
>>
>> "…
>> Choose label names which say what the goto does or why the goto exists.
>> …"
>>
>>
>> Does this wording need any more adjustments?
> 
> No.

I have got an other impression.

The terse description can trigger disagreements about the "what" and "why",
can't it?


> I wrote that and "restart" seems like a pretty clear name to me.

This identifier might be good enough to some degree.
I imagined that it would become better by the addition of a bit of information
from the jump target.


> I never wrote that you should harrass people with your nonsense patches.

This is true in principle.

But your adjustment for the document "CodingStyle" supported also a 
reconsideration
of the corresponding identifier selection.

Some developers disagreed with a proposed renaming while others reacted
in a positive way.


> In fact, I have asked you over and over again to stop.

This happened under different software update contexts occasionally.

Regards,
Markus

[PATCH] dma-buf/fence-array: get signaled state when signaling is disabled

[Christian König]

Am 23.09.2016 um 13:30 schrieb Gustavo Padovan:
> 2016-09-22 Christian König :
>
>> Am 22.09.2016 um 13:16 schrieb Gustavo Padovan:
>>> 2016-09-22 Christian König :
>>>
 Dropping the rest of the patch, cause that really doesn't make sense any
 more.

 Am 22.09.2016 um 12:40 schrieb Gustavo Padovan:
>> E.g. for example it is illegal to do something like
>>> "while(!fence_is_signaled(f)) sleep();" without enabling signaling 
>>> before
>>> doing this.
>>>
>>> Could just be a misunderstanding, but the comments on your patch 
>>> actually
>>> sounds a bit like somebody is trying to do exactly that.
> I think the usecase in mind here is poll(fd, timeout=0)
 Exactly as I feared. Even if userspace polls with timeout=0 you still need
 to call enable_signaling().

 Otherwise you can run into a situation where userspace only uses timeout=0
 and so never activates the signaling check in the driver.

 This would in turn result in an endless loop on implementations where the
 driver never signals fences on their own.
>>> Polling is optional, userspace may never call it. And DRM/KMS or GPU
>>> drivers will be doing fence_wait() themselves so signaling is enabled at
>>> some point.
>> No they won't. We have an use case where we clearly want to avoid that as
>> much as possible because and so the driver never calls enable_signaling() on
>> it's own.
>>
>> Exposing this poll function to userspace without enabling signaling is a
>> clear NAK from my side.
> Okay. So you are NAK'ing the does_not_pool_wait change. Should we revert
> that one then? It is already broken.

Yeah, that would probably a good idea. The AMD driver changes which 
really rely on this aren't upstream yet, so if you point me to the 
commit hash I could revert that as well when we send that out.

On the other hand the idea behind fence_is_signaled() is really that you 
check the status multiple times after enabling signaling. So I would 
prefer if you would revert this change preliminary.

Double checking this patch (and thinking about it a bit more) reveals 
that it is most likely correct. So feel free to commit this one if it is 
still needed for something.

Regards,
Christian.

>
> Gustavo
>

[Linaro-mm-sig] [PATCH 10/11] dma-buf: Use seqlock to close RCU race in test_signaled_single

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:33AM +0100, Chris Wilson wrote:
> With the seqlock now extended to cover the lookup of the fence and its
> testing, we can perform that testing solely under the seqlock guard and
> avoid the effective locking and serialisation of acquiring a reference to
> the request.  As the fence is RCU protected we know it cannot disappear
> as we test it, the same guarantee that made it safe to acquire the
> reference previously.  The seqlock tests whether the fence was replaced
> as we are testing it telling us whether or not we can trust the result
> (if not, we just repeat the test until stable).
> 
> Signed-off-by: Chris Wilson 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org

Not entirely sure this is safe for non-i915 drivers. We might now call
->signalled on a zombie fence (i.e. refcount == 0, but not yet kfreed).
i915 can do that, but other drivers might go boom.

I think in generic code we can't do these kind of tricks unfortunately.
-Daniel

> ---
>  drivers/dma-buf/reservation.c | 32 
>  1 file changed, 4 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index e74493e7332b..1ddffa5adb5a 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -442,24 +442,6 @@ unlock_retry:
>  }
>  EXPORT_SYMBOL_GPL(reservation_object_wait_timeout_rcu);
>  
> -
> -static inline int
> -reservation_object_test_signaled_single(struct fence *passed_fence)
> -{
> - struct fence *fence, *lfence = passed_fence;
> - int ret = 1;
> -
> - if (!test_bit(FENCE_FLAG_SIGNALED_BIT, &lfence->flags)) {
> - fence = fence_get_rcu(lfence);
> - if (!fence)
> - return -1;
> -
> - ret = !!fence_is_signaled(fence);
> - fence_put(fence);
> - }
> - return ret;
> -}
> -
>  /**
>   * reservation_object_test_signaled_rcu - Test if a reservation object's
>   * fences have been signaled.
> @@ -474,7 +456,7 @@ bool reservation_object_test_signaled_rcu(struct 
> reservation_object *obj,
> bool test_all)
>  {
>   unsigned seq, shared_count;
> - int ret;
> + bool ret;
>  
>   rcu_read_lock();
>  retry:
> @@ -494,10 +476,8 @@ retry:
>   for (i = 0; i < shared_count; ++i) {
>   struct fence *fence = rcu_dereference(fobj->shared[i]);
>  
> - ret = reservation_object_test_signaled_single(fence);
> - if (ret < 0)
> - goto retry;
> - else if (!ret)
> + ret = fence_is_signaled(fence);
> + if (!ret)
>   break;
>   }
>  
> @@ -509,11 +489,7 @@ retry:
>   struct fence *fence_excl = rcu_dereference(obj->fence_excl);
>  
>   if (fence_excl) {
> - ret = reservation_object_test_signaled_single(
> - fence_excl);
> - if (ret < 0)
> - goto retry;
> -
> + ret = fence_is_signaled(fence_excl);
>   if (read_seqcount_retry(&obj->seq, seq))
>   goto retry;
>   }
> -- 
> 2.9.3
> 
> ___
> Linaro-mm-sig mailing list
> Linaro-mm-sig at lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 11/11] dma-buf: Do a fast lockless check for poll with timeout=0

[Daniel Vetter]

On Mon, Aug 29, 2016 at 08:08:34AM +0100, Chris Wilson wrote:
> Currently we install a callback for performing poll on a dma-buf,
> irrespective of the timeout. This involves taking a spinlock, as well as
> unnecessary work, and greatly reduces scaling of poll(.timeout=0) across
> multiple threads.
> 
> We can query whether the poll will block prior to installing the
> callback to make the busy-query fast.
> 
> Single thread: 60% faster
> 8 threads on 4 (+4 HT) cores: 600% faster
> 
> Still not quite the perfect scaling we get with a native busy ioctl, but
> poll(dmabuf) is faster due to the quicker lookup of the object and
> avoiding drm_ioctl().
> 
> Signed-off-by: Chris Wilson 
> Cc: Sumit Semwal 
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org
> Reviewed-by: Daniel Vetter 

Need to strike the r-b here, since Christian König pointed out that
objects won't magically switch signalling on.
-Daniel

> ---
>  drivers/dma-buf/dma-buf.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index cf04d249a6a4..c7a7bc579941 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -156,6 +156,18 @@ static unsigned int dma_buf_poll(struct file *file, 
> poll_table *poll)
>   if (!events)
>   return 0;
>  
> + if (poll_does_not_wait(poll)) {
> + if (events & POLLOUT &&
> + !reservation_object_test_signaled_rcu(resv, true))
> + events &= ~(POLLOUT | POLLIN);
> +
> + if (events & POLLIN &&
> + !reservation_object_test_signaled_rcu(resv, false))
> + events &= ~POLLIN;
> +
> + return events;
> + }
> +
>  retry:
>   seq = read_seqcount_begin(&resv->seq);
>   rcu_read_lock();
> -- 
> 2.9.3
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Linaro-mm-sig] [PATCH 10/11] dma-buf: Use seqlock to close RCU race in test_signaled_single

[Chris Wilson]

On Fri, Sep 23, 2016 at 03:49:26PM +0200, Daniel Vetter wrote:
> On Mon, Aug 29, 2016 at 08:08:33AM +0100, Chris Wilson wrote:
> > With the seqlock now extended to cover the lookup of the fence and its
> > testing, we can perform that testing solely under the seqlock guard and
> > avoid the effective locking and serialisation of acquiring a reference to
> > the request.  As the fence is RCU protected we know it cannot disappear
> > as we test it, the same guarantee that made it safe to acquire the
> > reference previously.  The seqlock tests whether the fence was replaced
> > as we are testing it telling us whether or not we can trust the result
> > (if not, we just repeat the test until stable).
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Sumit Semwal 
> > Cc: linux-media at vger.kernel.org
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: linaro-mm-sig at lists.linaro.org
> 
> Not entirely sure this is safe for non-i915 drivers. We might now call
> ->signalled on a zombie fence (i.e. refcount == 0, but not yet kfreed).
> i915 can do that, but other drivers might go boom.

All fences must be under RCU guard, or is that the sticking point? Given
that, the problem is fence reallocation within the RCU grace period. If
we can mandate that fence reallocation must be safe for concurrent
fence->ops->*(), we can use this technique to avoid the serialisation
barrier otherwise required. In the simple stress test, the difference is
an order of magnitude, and test_signaled_rcu is often on a path where
every memory barrier quickly adds up (at least for us).

So is it just that you worry that others using SLAB_DESTROY_BY_RCU won't
ensure their fence is safe during the reallocation?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/bridge: analogix_dp: Add analogix_dp_psr_supported

[Tomeu Vizoso]

So users know whether PSR should be enabled or not.

Signed-off-by: Tomeu Vizoso 
Cc: Sean Paul 
Cc: Yakir Yang 
Cc: Archit Taneja 
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 
 include/drm/bridge/analogix_dp.h   | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index bf992460a6c7..91d8540ac8f0 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -98,6 +98,14 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device 
*dp)
return 0;
 }

+int analogix_dp_psr_supported(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   return dp->psr_support;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_psr_supported);
+
 int analogix_dp_enable_psr(struct device *dev)
 {
struct analogix_dp_device *dp = dev_get_drvdata(dev);
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 5f498ca07eea..c99d6eaef1ac 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -38,6 +38,7 @@ struct analogix_dp_plat_data {
 struct drm_connector *);
 };

+int analogix_dp_psr_supported(struct device *dev);
 int analogix_dp_enable_psr(struct device *dev);
 int analogix_dp_disable_psr(struct device *dev);

-- 
2.7.4

[PATCH 2/2] drm/rockchip: analogix_dp: Refuse to enable PSR if panel doesn't support it

[Tomeu Vizoso]

There's no point in enabling PSR when the panel doesn't support it.

This also avoids a problem when PSR gets enabled when a CRTC is being
disabled, because sometimes in that situation the DSP_HOLD_VALID_INTR
interrupt on which we wait will never arrive. This was observed on
RK3288 with a panel without PSR (veyron-jaq Chromebook).

It's very easy to reproduce by running the kms_rmfb test in IGT a few
times.

Signed-off-by: Tomeu Vizoso 
Cc: Sean Paul 
Cc: Yakir Yang 
Cc: Archit Taneja 
---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c 
b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index e83be157cc2a..8548e8271639 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -85,6 +85,9 @@ static void analogix_dp_psr_set(struct drm_encoder *encoder, 
bool enabled)
struct rockchip_dp_device *dp = to_dp(encoder);
unsigned long flags;

+   if (!analogix_dp_psr_supported(dp->dev))
+   return;
+
dev_dbg(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");

spin_lock_irqsave(&dp->psr_lock, flags);
-- 
2.7.4

[Intel-gfx] [PATCH 11/11] dma-buf: Do a fast lockless check for poll with timeout=0

[Chris Wilson]

On Fri, Sep 23, 2016 at 03:50:44PM +0200, Daniel Vetter wrote:
> On Mon, Aug 29, 2016 at 08:08:34AM +0100, Chris Wilson wrote:
> > Currently we install a callback for performing poll on a dma-buf,
> > irrespective of the timeout. This involves taking a spinlock, as well as
> > unnecessary work, and greatly reduces scaling of poll(.timeout=0) across
> > multiple threads.
> > 
> > We can query whether the poll will block prior to installing the
> > callback to make the busy-query fast.
> > 
> > Single thread: 60% faster
> > 8 threads on 4 (+4 HT) cores: 600% faster
> > 
> > Still not quite the perfect scaling we get with a native busy ioctl, but
> > poll(dmabuf) is faster due to the quicker lookup of the object and
> > avoiding drm_ioctl().
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Sumit Semwal 
> > Cc: linux-media at vger.kernel.org
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: linaro-mm-sig at lists.linaro.org
> > Reviewed-by: Daniel Vetter 
> 
> Need to strike the r-b here, since Christian König pointed out that
> objects won't magically switch signalling on.

The point being here that we don't even want to switch signaling on! :)

Christian's point was that not all fences guarantee forward progress
irrespective of whether signaling is enabled or not, and fences are not
required to guarantee forward progress without signaling even if they
provide an ops->signaled().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

GPU-DRM-QXL: Move three assignments in qxl_device_init()

[SF Markus Elfring]

>> I am trying to improve various open issues also in Linux source files.
>>
> That the fact that you see issues (in these particular cases) while
> others do not

I guess that the discussed "story" affects more challenges in communication
and different opinions about where to invest software development attention.


> indicates that the commit summary could be explained better.

I imagine that there are a few improvement possibilities left over.


> A good commit summary should provide enough information to do that

This advice is generally fine.


> and make people _want_ the patch.

I guess that this expectation can become a research topic for some knowledge 
fields,
can't it?

There are update suggestion where the probability for acceptance is higher
than for others.

Some maintainers have got their own difficulties with changes when they 
categorise
them as "ordinary clean-up".


> From my limited experience through your patches (just skimmed a few)

Thanks for your general interest.


> you seems to be describing what the patch does

My collection of update suggestions is evolving over some source code search 
patterns.

I find that this approach fits to the recommended imperative wording style
according to document "SubmittingPatches", doesn't it?

I dared also some deviations or variations already.


> as opposed to why it does it and why should one find it interesting/wanted.

I am trying to express also this information to some degree.

Regards,
Markus

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

[Brian Starkey]

On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
>On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
>> On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
>> > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau  
>> > wrote:
>> > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
>> > > enabled, but we never got the call to turn off the CRTC. Brian is still
>> > > tracking through the fbdev emulation to figure out the cause for that.
>> >
>> > fbdev emulation doesn't do that for you. If you need/want to shut down
>> > all the crtcs on driver unload, you need to do that yourself. There's
>> > atomic helpers to do that for you that for you.
>>
>> The problem is a sort-of circular dependency between ->lastclose (at
>> least the common implementation of it), unregister and disabling
>> fbdev.
>>
>> I want to move drm_dev_unregister() to be the first thing we do at
>> rmmod-time. However we need to disable fbdev first, otherwise
>> ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
>> on for drm_vsync_cleanup() to then WARN on.
>>
>> There's a slightly different (perceived) problem - the one that Liviu
>> mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
>> You say it's not the fbdev helpers' responsibility to teardown their
>> modeset, but regardless I have nowhere to disable the CRTC if I want
>> to do drm_dev_unregister() first; and if the CRTC isn't disabled
>> there's always a chance of hitting the same vsync WARN even without
>> fbdev.
>
>Just disable all crtc in a suitable place (after drm_dev_unregister,
>before you tear down fbdev).

I think this is predicated on first removing the drm_vblank_cleanup
call.

>>
>> We *could* add an ->unload and disable everything there, but as that's
>> deprecated I'm guessing there should be another way.
>> Perhaps we should track ->firstopen/->lastclose pairs so we can detect
>> that ->lastclose is being called from unregister and use it to
>> disable everything in that case.
>
>Hm, maybe we should simply not call ->lastclose for kms drivers. That is
>kinda only a hack for ums/dri1 drivers.
>

To be clear (and in response to Russell's question) - you mean
only the call to ->lastclose in drm_dev_unregister, not in general?

>But even with that gone you might still unload while fbdev is 
>enabled, so
>this won't fix it all.
>

Yeah it will be tidier, but I don't think it actually fixes anything.

>> drm_vblank_cleanup() seems to have been carried over to unregister
>> from drm_put_dev(), but drm_dev_register() doesn't call
>> drm_vblank_init() so it seems a little strange to have it there.
>> I can see other drivers I'd expect to hit the same WARN but I don't
>> have HW to test it on.
>
>Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
>should push it into all callers instead I think.

OK so two things to do - remove drm_vblank_cleanup() from
drm_dev_unregister(), and then do the teardown like so:

drm_dev_unregister();
drm_crtc_force_disable_all(); // or atomic equivalent
fbdev_teardown();
...

Seems good to me. Are there any ordering constraints you're aware of
for drm_vblank_cleanup()? Or you think just putting it after
drm_dev_unregister() should be OK?

Thanks,
-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>

[Intel-gfx] [PATCH 11/11] dma-buf: Do a fast lockless check for poll with timeout=0

[Chris Wilson]

On Fri, Sep 23, 2016 at 03:50:44PM +0200, Daniel Vetter wrote:
> On Mon, Aug 29, 2016 at 08:08:34AM +0100, Chris Wilson wrote:
> > Currently we install a callback for performing poll on a dma-buf,
> > irrespective of the timeout. This involves taking a spinlock, as well as
> > unnecessary work, and greatly reduces scaling of poll(.timeout=0) across
> > multiple threads.
> > 
> > We can query whether the poll will block prior to installing the
> > callback to make the busy-query fast.
> > 
> > Single thread: 60% faster
> > 8 threads on 4 (+4 HT) cores: 600% faster
> > 
> > Still not quite the perfect scaling we get with a native busy ioctl, but
> > poll(dmabuf) is faster due to the quicker lookup of the object and
> > avoiding drm_ioctl().
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Sumit Semwal 
> > Cc: linux-media at vger.kernel.org
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: linaro-mm-sig at lists.linaro.org
> > Reviewed-by: Daniel Vetter 
> 
> Need to strike the r-b here, since Christian König pointed out that
> objects won't magically switch signalling on.

Propagating a flag through to sync_file is trivial, but not through to
the dma_buf->resv. Looks like dma-buf will be without a fast busy query,
which I guess in the grand scheme of things (i.e. dma-buf itself is not
intended to be used as a fence) is not that important.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[RFC] drm/exynos: g2d: runpm fixing attempt

[Tobias Jakobi]

Hello,

I have already talked to Marek in private about this. The latest runpm patch 
(b05984e21a7e000bf5074ace00d7a574944b2c16) cripples G2D operation. I have tried 
to come up with a way to fix this and also to improve runpm behaviour while at 
it.

Marek pointed out that the current issue, i.e. the reason for the 
aforementioned patch, is that sleep ops (suspend/resume) are now called during 
runpm suspend time. I think that my approach should handle this situation.

In any case, feedback is much appreciated.

With best wishes,
Tobias

Tobias Jakobi (1):
  drm/exynos: g2d: fix runtime PM

 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 235 +---
 1 file changed, 186 insertions(+), 49 deletions(-)

-- 
2.7.3

[RFC] drm/exynos: g2d: fix runtime PM

[Tobias Jakobi]

The commit b05984e21a7e000bf5074ace00d7a574944b2c16 broke
operation of the G2D. After this commit the following
happens.
- exynos_g2d_exec_ioctl() prepares a runqueue node and
  calls g2d_exec_runqueue()
- g2d_exec_runqueue() calls g2d_dma_start() which gets
  runtime PM sync
- runtime PM calls g2d_runtime_resume()
- g2d_runtime_resume() calls g2d_exec_runqueue()

Luckily for us this doesn't really loop, but creates a
mutex lockup, which the kernel even predicts.

Anyway, we fix this by reintroducing dedicated sleep
operations again, and only letting runtime PM control
the gate clock.

This is not enough to fix the issue though.
- We switch runtime PM to autosuspend. Currently clocks get
  disabled, and then re-enabled again in the runqueue worker
  when a node is completed and the next is started.
  With the upcoming introduction of IOMMU runtime PM this
  situations gets worse, since now also the IOMMU goes
  through a disable/enable cycle before the next node is
  started.
- We consolidate all runtime PM management to the runqueue
  worker.
- We introduce g2d_wait_finish() which waits until the currently
  processed runqueue node is finished.
  If this takes too long, the engine is forcibly reset. This
  is necessary to properly close the driver in case the engine
  should hang with read/write access to some area of memory.
  In this situation we can't properly unmap GEM/userptr
  objects, since this might create a pagefault.
- Sleep suspend issues a suspend of the runqueue and then calls
  g2d_wait_finished(), which returns the engine in the idle state.
  The current runqueue node gets completed, all other queued
  nodes stay in the queue. There is no hardware state that
  needs to be retained.
- Sleep resume just pokes the runqueue worker, which, should
  something be in queue, continues its work, or just exits.

Signed-off-by: Tobias Jakobi 
---
 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 235 +---
 1 file changed, 186 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 6eca8bb..c4f7026 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -138,6 +138,18 @@ enum g2d_reg_type {
MAX_REG_TYPE_NR
 };

+enum g2d_flag_bits {
+   /*
+* If set, suspends the runqueue worker after the currently
+* processed node is finished.
+*/
+   G2D_BIT_SUSPEND_RUNQUEUE,
+   /*
+* If set, indicates that the engine is currently busy.
+*/
+   G2D_BIT_ENGINE_BUSY,
+};
+
 /* cmdlist data structure */
 struct g2d_cmdlist {
u32 head;
@@ -226,7 +238,7 @@ struct g2d_data {
struct workqueue_struct *g2d_workq;
struct work_struct  runqueue_work;
struct exynos_drm_subdrvsubdrv;
-   boolsuspended;
+   unsigned long   flags;

/* cmdlist */
struct g2d_cmdlist_node *cmdlist_node;
@@ -246,6 +258,12 @@ struct g2d_data {
unsigned long   max_pool;
 };

+static inline void g2d_hw_reset(struct g2d_data *g2d)
+{
+   writel(G2D_R | G2D_SFRCLEAR, g2d->regs + G2D_SOFT_RESET);
+   clear_bit(G2D_BIT_ENGINE_BUSY, &g2d->flags);
+}
+
 static int g2d_init_cmdlist(struct g2d_data *g2d)
 {
struct device *dev = g2d->dev;
@@ -803,12 +821,8 @@ static void g2d_dma_start(struct g2d_data *g2d,
struct g2d_cmdlist_node *node =
list_first_entry(&runqueue_node->run_cmdlist,
struct g2d_cmdlist_node, list);
-   int ret;
-
-   ret = pm_runtime_get_sync(g2d->dev);
-   if (ret < 0)
-   return;

+   set_bit(G2D_BIT_ENGINE_BUSY, &g2d->flags);
writel_relaxed(node->dma_addr, g2d->regs + G2D_DMA_SFR_BASE_ADDR);
writel_relaxed(G2D_DMA_START, g2d->regs + G2D_DMA_COMMAND);
 }
@@ -831,9 +845,6 @@ static void g2d_free_runqueue_node(struct g2d_data *g2d,
 {
struct g2d_cmdlist_node *node;

-   if (!runqueue_node)
-   return;
-
mutex_lock(&g2d->cmdlist_mutex);
/*
 * commands in run_cmdlist have been completed so unmap all gem
@@ -847,29 +858,63 @@ static void g2d_free_runqueue_node(struct g2d_data *g2d,
kmem_cache_free(g2d->runqueue_slab, runqueue_node);
 }

-static void g2d_exec_runqueue(struct g2d_data *g2d)
+/**
+ * g2d_remove_runqueue_nodes - remove items from the list of runqueue nodes
+ * @g2d: G2D state object
+ * @file: if not zero, only remove items with this DRM file
+ *
+ * Has to be called under runqueue lock.
+ */
+static void g2d_remove_runqueue_nodes(struct g2d_data *g2d, struct drm_file* 
file)
 {
-   g2d->runqueue_node = g2d_get_runqueue_node(g2d);
-   if (g2d->runqueue_node)
-   g2d_dma_start(g2d, g2d->runqueue_node);
+   struct g2d_runqueue_node *node, *n;

[RFC] drm/exynos: g2d: fix runtime PM

[Tobias Jakobi]

Tobias Jakobi wrote:
> The commit b05984e21a7e000bf5074ace00d7a574944b2c16 broke
> operation of the G2D. After this commit the following
> happens.
> - exynos_g2d_exec_ioctl() prepares a runqueue node and
>   calls g2d_exec_runqueue()
> - g2d_exec_runqueue() calls g2d_dma_start() which gets
>   runtime PM sync
> - runtime PM calls g2d_runtime_resume()
> - g2d_runtime_resume() calls g2d_exec_runqueue()
> 
> Luckily for us this doesn't really loop, but creates a
> mutex lockup, which the kernel even predicts.
> 
> Anyway, we fix this by reintroducing dedicated sleep
> operations again, and only letting runtime PM control
> the gate clock.
> 
> This is not enough to fix the issue though.
> - We switch runtime PM to autosuspend. Currently clocks get
>   disabled, and then re-enabled again in the runqueue worker
>   when a node is completed and the next is started.
>   With the upcoming introduction of IOMMU runtime PM this
>   situations gets worse, since now also the IOMMU goes
>   through a disable/enable cycle before the next node is
>   started.
> - We consolidate all runtime PM management to the runqueue
>   worker.
> - We introduce g2d_wait_finish() which waits until the currently
>   processed runqueue node is finished.
>   If this takes too long, the engine is forcibly reset. This
>   is necessary to properly close the driver in case the engine
>   should hang with read/write access to some area of memory.
>   In this situation we can't properly unmap GEM/userptr
>   objects, since this might create a pagefault.
> - Sleep suspend issues a suspend of the runqueue and then calls
>   g2d_wait_finished(), which returns the engine in the idle state.
This should read 'g2d_wait_finish()'.



>   The current runqueue node gets completed, all other queued
>   nodes stay in the queue. There is no hardware state that
>   needs to be retained.
> - Sleep resume just pokes the runqueue worker, which, should
>   something be in queue, continues its work, or just exits.
> 
> Signed-off-by: Tobias Jakobi 

[PATCH] drm/tilcdc: Remove "default" from blue-and-red-wiring property binding

[Rob Herring]

On Thu, Sep 15, 2016 at 12:34:51PM +0300, Jyri Sarha wrote:
> Remove "default" keyword from blue-and-red-wiring devicetree property
> binding document. The code does not support and there is no intention
> to support it.
> 
> Reported-by: Rob Herring 
> Signed-off-by: Jyri Sarha 
> ---
> I sent the pull request for the color errata changes before I received
> the review mail[1] from Rob Herring. This fixes the issue reported in
> the mail.

Acked-by: Rob Herring 

[Intel-gfx] [PATCH 11/11] dma-buf: Do a fast lockless check for poll with timeout=0

[Chris Wilson]

On Fri, Sep 23, 2016 at 03:50:44PM +0200, Daniel Vetter wrote:
> On Mon, Aug 29, 2016 at 08:08:34AM +0100, Chris Wilson wrote:
> > Currently we install a callback for performing poll on a dma-buf,
> > irrespective of the timeout. This involves taking a spinlock, as well as
> > unnecessary work, and greatly reduces scaling of poll(.timeout=0) across
> > multiple threads.
> > 
> > We can query whether the poll will block prior to installing the
> > callback to make the busy-query fast.
> > 
> > Single thread: 60% faster
> > 8 threads on 4 (+4 HT) cores: 600% faster
> > 
> > Still not quite the perfect scaling we get with a native busy ioctl, but
> > poll(dmabuf) is faster due to the quicker lookup of the object and
> > avoiding drm_ioctl().
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Sumit Semwal 
> > Cc: linux-media at vger.kernel.org
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: linaro-mm-sig at lists.linaro.org
> > Reviewed-by: Daniel Vetter 
> 
> Need to strike the r-b here, since Christian König pointed out that
> objects won't magically switch signalling on.

Oh, it also means that

commit fb8b7d2b9d80e1e71f379e57355936bd2b024be9
Author: Jammy Zhou 
Date:   Wed Jan 21 18:35:47 2015 +0800

reservation: wait only with non-zero timeout specified (v3)

When the timeout value passed to reservation_object_wait_timeout_rcu
is zero, no wait should be done if the fences are not signaled.

Return '1' for idle and '0' for busy if the specified timeout is '0'
to keep consistent with the case of non-zero timeout.

v2: call fence_put if not signaled in the case of timeout==0

v3: switch to reservation_object_test_signaled_rcu

Signed-off-by: Jammy Zhou 
Reviewed-by: Christian König 
Reviewed-by: Alex Deucher 
Reviewed-By: Maarten Lankhorst 
Signed-off-by: Sumit Semwal 

is wrong. And reservation_object_test_signaled_rcu() is unreliable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Bug 172421] radeon: allow to set the TMDS frequency by a special kernel parameter

[bugzilla-dae...@bugzilla.kernel.org]

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

--- Comment #15 from Roland Scheidegger  ---
(In reply to Christian König from comment #14)
> (In reply to Roland Scheidegger from comment #13)
> > Personally I've always thought the risk of damaging hardware with any kind
> > of overclocking is just about exactly zero as long as you don't increase
> > voltage levels
> 
> Unfortunately this is exactly what happens here. The clock is generated by a
> voltage controlled oscillator and for the desired resolution you need to
> over clock it by about 30-40%.
> 
> That in turn means you raise the voltage way over the nominal limit.

Oh interesting - didn't know voltage was directly tied to clock frequency here.
Makes sense then to not allow it (at least if that circuitry isn't shared with
DP, as the DP link runs at much higher clock (540Mhz actually), but I suppose
it's really different there).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.