Re: [Intel-gfx] [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

2015-03-19 Thread Ander Conselvan De Oliveira 
On Wed, 2015-03-18 at 23:57 +, Konduru, Chandra wrote:
 
  -Original Message-
  From: Conselvan De Oliveira, Ander
  Sent: Wednesday, March 18, 2015 12:57 AM
  To: intel-gfx@lists.freedesktop.org
  Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
  Subject: [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy
  modeset code
  
  For the atomic conversion, the mode set paths need to be changed to rely on 
  an
  atomic state instead of using the staged config. By using an atomic state 
  for the
  legacy code, we will be able to convert the code base in small chunks.
  
  v2: Squash patch that adds stat argument to intel_set_mode(). (Ander)
  Make every caller of intel_set_mode() allocate state. (Daniel)
  Call drm_atomic_state_clear() in set config's error path. (Daniel)
  
  v3: Copy staged config to atomic state in force restore path. (Ander)
  
  v4: Don't update -new_config for disabled pipes in __intel_set_mode(),
  since it is expected to be NULL in that case. (Ander)
 
 Can you clarify why it is expected to be NULL?

This is part of the sanity checking implemented when the
intel_crtc-new_config pointer was introduced. If I understand
correctly, the idea was to reduce the usage of that pointer to only the
brief moment when it pointed at something different than the current
config. That way, if some piece of code relied on that pointer outside
of modeset, it would be easier to catch that bug.

  
  Signed-off-by: Ander Conselvan de Oliveira
  ander.conselvan.de.olive...@intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 200 +-
  -
   1 file changed, 165 insertions(+), 35 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c
  b/drivers/gpu/drm/i915/intel_display.c
  index 8458bf5..ce35647 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc 
  *crtc,
 struct intel_crtc_state *pipe_config);
  
   static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode
  *mode,
  - int x, int y, struct drm_framebuffer *old_fb);
  + int x, int y, struct drm_framebuffer *old_fb,
  + struct drm_atomic_state *state);
   static int intel_framebuffer_init(struct drm_device *dev,
struct intel_framebuffer *ifb,
struct drm_mode_fb_cmd2 *mode_cmd, @@
  -8802,6 +8803,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
  *connector,
  struct drm_device *dev = encoder-dev;
  struct drm_framebuffer *fb;
  struct drm_mode_config *config = dev-mode_config;
  +   struct drm_atomic_state *state = NULL;
  int ret, i = -1;
  
  DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n, @@
  -8883,6 +8885,12 @@ retry:
  old-load_detect_temp = true;
  old-release_fb = NULL;
  
  +   state = drm_atomic_state_alloc(dev);
  +   if (!state)
  +   return false;
  +
  +   state-acquire_ctx = ctx;
  +
  if (!mode)
  mode = load_detect_mode;
  
  @@ -8905,7 +8913,7 @@ retry:
  goto fail;
  }
  
  -   if (intel_set_mode(crtc, mode, 0, 0, fb)) {
  +   if (intel_set_mode(crtc, mode, 0, 0, fb, state)) {
  DRM_DEBUG_KMS(failed to set mode on load-detect pipe\n);
  if (old-release_fb)
  old-release_fb-funcs-destroy(old-release_fb);
  @@ -8924,6 +8932,11 @@ retry:
  else
  intel_crtc-new_config = NULL;
   fail_unlock:
  +   if (state) {
  +   drm_atomic_state_free(state);
  +   state = NULL;
  +   }
  +
  if (ret == -EDEADLK) {
  drm_modeset_backoff(ctx);
  goto retry;
  @@ -8936,22 +8949,34 @@ void intel_release_load_detect_pipe(struct
  drm_connector *connector,
  struct intel_load_detect_pipe *old,
  struct drm_modeset_acquire_ctx *ctx)  {
  +   struct drm_device *dev = connector-dev;
  struct intel_encoder *intel_encoder =
  intel_attached_encoder(connector);
  struct drm_encoder *encoder = intel_encoder-base;
  struct drm_crtc *crtc = encoder-crtc;
  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
  +   struct drm_atomic_state *state;
  
  DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
connector-base.id, connector-name,
encoder-base.id, encoder-name);
  
  if (old-load_detect_temp) {
  +   state = drm_atomic_state_alloc(dev);
  +   if (!state) {
  +   DRM_DEBUG_KMS(can't release load detect pipe\n);
  +   return;
  +   }
  +
  +   state-acquire_ctx = ctx;
  +
  to_intel_connector(connector)-new_encoder = NULL;
  intel_encoder-new_crtc = NULL;

[Intel-gfx] [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

2015-03-18 Thread Ander Conselvan de Oliveira 
For the atomic conversion, the mode set paths need to be changed to rely
on an atomic state instead of using the staged config. By using an
atomic state for the legacy code, we will be able to convert the code
base in small chunks.

v2: Squash patch that adds stat argument to intel_set_mode(). (Ander)
Make every caller of intel_set_mode() allocate state. (Daniel)
Call drm_atomic_state_clear() in set config's error path. (Daniel)

v3: Copy staged config to atomic state in force restore path. (Ander)

v4: Don't update -new_config for disabled pipes in __intel_set_mode(),
since it is expected to be NULL in that case. (Ander)

Signed-off-by: Ander Conselvan de Oliveira 
ander.conselvan.de.olive...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 200 +--
 1 file changed, 165 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 8458bf5..ce35647 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
   struct intel_crtc_state *pipe_config);
 
 static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
- int x, int y, struct drm_framebuffer *old_fb);
+ int x, int y, struct drm_framebuffer *old_fb,
+ struct drm_atomic_state *state);
 static int intel_framebuffer_init(struct drm_device *dev,
  struct intel_framebuffer *ifb,
  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -8802,6 +8803,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
*connector,
struct drm_device *dev = encoder-dev;
struct drm_framebuffer *fb;
struct drm_mode_config *config = dev-mode_config;
+   struct drm_atomic_state *state = NULL;
int ret, i = -1;
 
DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
@@ -8883,6 +8885,12 @@ retry:
old-load_detect_temp = true;
old-release_fb = NULL;
 
+   state = drm_atomic_state_alloc(dev);
+   if (!state)
+   return false;
+
+   state-acquire_ctx = ctx;
+
if (!mode)
mode = load_detect_mode;
 
@@ -8905,7 +8913,7 @@ retry:
goto fail;
}
 
-   if (intel_set_mode(crtc, mode, 0, 0, fb)) {
+   if (intel_set_mode(crtc, mode, 0, 0, fb, state)) {
DRM_DEBUG_KMS(failed to set mode on load-detect pipe\n);
if (old-release_fb)
old-release_fb-funcs-destroy(old-release_fb);
@@ -8924,6 +8932,11 @@ retry:
else
intel_crtc-new_config = NULL;
 fail_unlock:
+   if (state) {
+   drm_atomic_state_free(state);
+   state = NULL;
+   }
+
if (ret == -EDEADLK) {
drm_modeset_backoff(ctx);
goto retry;
@@ -8936,22 +8949,34 @@ void intel_release_load_detect_pipe(struct 
drm_connector *connector,
struct intel_load_detect_pipe *old,
struct drm_modeset_acquire_ctx *ctx)
 {
+   struct drm_device *dev = connector-dev;
struct intel_encoder *intel_encoder =
intel_attached_encoder(connector);
struct drm_encoder *encoder = intel_encoder-base;
struct drm_crtc *crtc = encoder-crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+   struct drm_atomic_state *state;
 
DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
  connector-base.id, connector-name,
  encoder-base.id, encoder-name);
 
if (old-load_detect_temp) {
+   state = drm_atomic_state_alloc(dev);
+   if (!state) {
+   DRM_DEBUG_KMS(can't release load detect pipe\n);
+   return;
+   }
+
+   state-acquire_ctx = ctx;
+
to_intel_connector(connector)-new_encoder = NULL;
intel_encoder-new_crtc = NULL;
intel_crtc-new_enabled = false;
intel_crtc-new_config = NULL;
-   intel_set_mode(crtc, NULL, 0, 0, NULL);
+   intel_set_mode(crtc, NULL, 0, 0, NULL, state);
+
+   drm_atomic_state_free(state);
 
if (old-release_fb) {
drm_framebuffer_unregister_private(old-release_fb);
@@ -10345,10 +10370,22 @@ static bool check_digital_port_conflicts(struct 
drm_device *dev)
return true;
 }
 
-static struct intel_crtc_state *
+static void
+clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
+{
+   struct drm_crtc_state tmp_state;
+
+   /* Clear only the intel specific part of the crtc state */
+   tmp_state = crtc_state-base;
+   memset(crtc_state, 0, sizeof *crtc_state);

Re: [Intel-gfx] [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy modeset code

2015-03-18 Thread Konduru, Chandra 


 -Original Message-
 From: Conselvan De Oliveira, Ander
 Sent: Wednesday, March 18, 2015 12:57 AM
 To: intel-gfx@lists.freedesktop.org
 Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
 Subject: [PATCH v4] drm/i915: Allocate a drm_atomic_state for the legacy
 modeset code
 
 For the atomic conversion, the mode set paths need to be changed to rely on an
 atomic state instead of using the staged config. By using an atomic state for 
 the
 legacy code, we will be able to convert the code base in small chunks.
 
 v2: Squash patch that adds stat argument to intel_set_mode(). (Ander)
 Make every caller of intel_set_mode() allocate state. (Daniel)
 Call drm_atomic_state_clear() in set config's error path. (Daniel)
 
 v3: Copy staged config to atomic state in force restore path. (Ander)
 
 v4: Don't update -new_config for disabled pipes in __intel_set_mode(),
 since it is expected to be NULL in that case. (Ander)

Can you clarify why it is expected to be NULL?

 
 Signed-off-by: Ander Conselvan de Oliveira
 ander.conselvan.de.olive...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 200 +-
 -
  1 file changed, 165 insertions(+), 35 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c
 b/drivers/gpu/drm/i915/intel_display.c
 index 8458bf5..ce35647 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -83,7 +83,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
  struct intel_crtc_state *pipe_config);
 
  static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode
 *mode,
 -   int x, int y, struct drm_framebuffer *old_fb);
 +   int x, int y, struct drm_framebuffer *old_fb,
 +   struct drm_atomic_state *state);
  static int intel_framebuffer_init(struct drm_device *dev,
 struct intel_framebuffer *ifb,
 struct drm_mode_fb_cmd2 *mode_cmd, @@
 -8802,6 +8803,7 @@ bool intel_get_load_detect_pipe(struct drm_connector
 *connector,
   struct drm_device *dev = encoder-dev;
   struct drm_framebuffer *fb;
   struct drm_mode_config *config = dev-mode_config;
 + struct drm_atomic_state *state = NULL;
   int ret, i = -1;
 
   DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n, @@
 -8883,6 +8885,12 @@ retry:
   old-load_detect_temp = true;
   old-release_fb = NULL;
 
 + state = drm_atomic_state_alloc(dev);
 + if (!state)
 + return false;
 +
 + state-acquire_ctx = ctx;
 +
   if (!mode)
   mode = load_detect_mode;
 
 @@ -8905,7 +8913,7 @@ retry:
   goto fail;
   }
 
 - if (intel_set_mode(crtc, mode, 0, 0, fb)) {
 + if (intel_set_mode(crtc, mode, 0, 0, fb, state)) {
   DRM_DEBUG_KMS(failed to set mode on load-detect pipe\n);
   if (old-release_fb)
   old-release_fb-funcs-destroy(old-release_fb);
 @@ -8924,6 +8932,11 @@ retry:
   else
   intel_crtc-new_config = NULL;
  fail_unlock:
 + if (state) {
 + drm_atomic_state_free(state);
 + state = NULL;
 + }
 +
   if (ret == -EDEADLK) {
   drm_modeset_backoff(ctx);
   goto retry;
 @@ -8936,22 +8949,34 @@ void intel_release_load_detect_pipe(struct
 drm_connector *connector,
   struct intel_load_detect_pipe *old,
   struct drm_modeset_acquire_ctx *ctx)  {
 + struct drm_device *dev = connector-dev;
   struct intel_encoder *intel_encoder =
   intel_attached_encoder(connector);
   struct drm_encoder *encoder = intel_encoder-base;
   struct drm_crtc *crtc = encoder-crtc;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 + struct drm_atomic_state *state;
 
   DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
 connector-base.id, connector-name,
 encoder-base.id, encoder-name);
 
   if (old-load_detect_temp) {
 + state = drm_atomic_state_alloc(dev);
 + if (!state) {
 + DRM_DEBUG_KMS(can't release load detect pipe\n);
 + return;
 + }
 +
 + state-acquire_ctx = ctx;
 +
   to_intel_connector(connector)-new_encoder = NULL;
   intel_encoder-new_crtc = NULL;
   intel_crtc-new_enabled = false;
   intel_crtc-new_config = NULL;
 - intel_set_mode(crtc, NULL, 0, 0, NULL);
 + intel_set_mode(crtc, NULL, 0, 0, NULL, state);
 +
 + drm_atomic_state_free(state);
 
   if (old-release_fb) {
   drm_framebuffer_unregister_private(old-release_fb);
 @@ -10345,10 +10370,22 @@ static bool check_digital_port_conflicts(struct
 drm_device *dev)
   return true;