date:20201126

[Why]

Set dpms off on the MST connector that was unplugged, for the side effect of
releasing some references held through deallocation of mst payload.

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 ++-
 drivers/gpu/drm/amd/display/dc/core/dc.c  | 13 
 drivers/gpu/drm/amd/display/dc/dc_stream.h|  1 +
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e213246e3f04..9966679d29e7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1984,6 +1984,32 @@ static void dm_gpureset_commit_state(struct dc_state 
*dc_state,
return;
 }
 
+static void dm_set_dpms_off(struct dc_link *link)
+{
+   struct dc_stream_state *stream_state;
+   struct amdgpu_dm_connector *aconnector = link->priv;
+   struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev);
+   struct dc_stream_update stream_update = {0};
+   bool dpms_off = true;
+
+   stream_update.dpms_off = _off;
+
+   mutex_lock(>dm.dc_lock);
+   stream_state = dc_stream_find_from_link(link);
+
+   if (stream_state == NULL) {
+   dm_error("Error finding stream state associated with link!\n");
+   mutex_unlock(>dm.dc_lock);
+   return;
+   }
+
+   stream_update.stream = stream_state;
+   dc_commit_updates_for_stream(stream_state->ctx->dc, NULL, 0,
+stream_state, _update,
+stream_state->ctx->dc->current_state);
+   mutex_unlock(>dm.dc_lock);
+}
+
 static int dm_resume(void *handle)
 {
struct amdgpu_device *adev = handle;
@@ -2434,8 +2460,11 @@ static void handle_hpd_irq(void *param)
drm_kms_helper_hotplug_event(dev);
 
} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) {
-   amdgpu_dm_update_connector_after_detect(aconnector);
+   if (new_connection_type == dc_connection_none &&
+   aconnector->dc_link->type == dc_connection_none)
+   dm_set_dpms_off(aconnector->dc_link);
 
+   amdgpu_dm_update_connector_after_detect(aconnector);
 
drm_modeset_lock_all(dev);
dm_restore_drm_connector_state(dev, connector);
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 903353389edb..58eb0d69873a 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2798,6 +2798,19 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc 
*dc, uint8_t i)
return NULL;
 }
 
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link)
+{
+   uint8_t i;
+   struct dc_context *ctx = link->ctx;
+
+   for (i = 0; i < ctx->dc->current_state->stream_count; i++) {
+   if (ctx->dc->current_state->streams[i]->link == link)
+   return ctx->dc->current_state->streams[i];
+   }
+
+   return NULL;
+}
+
 enum dc_irq_source dc_interrupt_to_irq_source(
struct dc *dc,
uint32_t src_id,
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h 
b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index bf090afc2f70..b7910976b81a 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct 
dc_stream_state *stream);
 
 uint8_t dc_get_current_stream_count(struct dc *dc);
 struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i);
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link);
 
 /*
  * Return the current frame counter.
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: turn DPMS off on mst connector unplug

2020-11-26 Thread Kazlauskas, Nicholas


On 2020-11-26 4:45 p.m., Aurabindo Pillai wrote:

[Why]

Set dpms off on the MST connector that was unplugged, for the side effect of
releasing some references held through deallocation of mst payload.

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++-
  drivers/gpu/drm/amd/display/dc/core/dc.c  | 17 ++
  drivers/gpu/drm/amd/display/dc/dc_stream.h|  1 +
  3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e213246e3f04..6203cbf3ee33 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1984,6 +1984,34 @@ static void dm_gpureset_commit_state(struct dc_state 
*dc_state,
return;
  }
  
+static void dm_set_dpms_off(struct dc_link *link)

+{
+   struct dc_stream_state *stream_state;
+   struct amdgpu_dm_connector *aconnector = link->priv;
+   struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev);
+   struct {
+   struct dc_surface_update surface_updates[MAX_SURFACES];


Let's remove the bundle and drop the surface_updates here. A 
surface_count of 0 should be sufficient to guard against NULL 
surface_updates array.



+   struct dc_stream_update stream_update;
+   } bundle = {0};
+   bool dpms_off = true;
+
+   bundle.stream_update.dpms_off = _off;
+
+   mutex_lock(>dm.dc_lock);
+   stream_state = dc_stream_find_from_link(link);
+   mutex_unlock(>dm.dc_lock);


This needs to be move under dc_commit_updates_for_stream(). It's not 
safe to call dc_commit_updates_for_stream while unlocked since it 
modifies global state.



+
+   if (stream_state == NULL) {
+   dm_error("Error finding stream state associated with link!\n");
+   return;
+   }
+
+   bundle.stream_update.stream = stream_state;
+   dc_commit_updates_for_stream(stream_state->ctx->dc, 
bundle.surface_updates, 0,
+stream_state, _update,
+stream_state->ctx->dc->current_state);
+}
+
  static int dm_resume(void *handle)
  {
struct amdgpu_device *adev = handle;
@@ -2434,8 +2462,11 @@ static void handle_hpd_irq(void *param)
drm_kms_helper_hotplug_event(dev);
  
  	} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) {

-   amdgpu_dm_update_connector_after_detect(aconnector);
+   if (new_connection_type == dc_connection_none &&
+   aconnector->dc_link->type == dc_connection_none)
+   dm_set_dpms_off(aconnector->dc_link);
  
+		amdgpu_dm_update_connector_after_detect(aconnector);
  
  		drm_modeset_lock_all(dev);

dm_restore_drm_connector_state(dev, connector);
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 903353389edb..7a2b2802faa2 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2798,6 +2798,23 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc 
*dc, uint8_t i)
return NULL;
  }
  
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link)

+{
+   uint8_t i;
+   struct dc_context *ctx = link->ctx;
+
+   for (i = 0; i< MAX_PIPES; i++) {
+   if (ctx->dc->current_state->streams[i] == NULL)
+   continue;


Drop the NULL check above and change MAX_PIPES to 
dc->current_state->stream_count.


Regards,
Nicholas Kazlauskas


+
+   if (ctx->dc->current_state->streams[i]->link == link) {
+   return ctx->dc->current_state->streams[i];
+   }
+   }
+
+   return NULL;
+}
+
  enum dc_irq_source dc_interrupt_to_irq_source(
struct dc *dc,
uint32_t src_id,
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h 
b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index bf090afc2f70..b7910976b81a 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct 
dc_stream_state *stream);
  
  uint8_t dc_get_current_stream_count(struct dc *dc);

  struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i);
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link);
  
  /*

   * Return the current frame counter.



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: turn DPMS off on mst connector unplug



On 2020-11-26 3:11 p.m., Kazlauskas, Nicholas wrote:
> On 2020-11-26 2:50 p.m., Aurabindo Pillai wrote:
>> [Why]
>>
>> Set dpms off on the MST connector that was unplugged, for the side
>> effect of
>> releasing some references held through deallocation of mst payload.
>>
>> Signed-off-by: Aurabindo Pillai 
>> Signed-off-by: Eryk Brol 
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++-
>>   1 file changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index e213246e3f04..fc984cf6e316 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct
>> dc_state *dc_state,
>>   return;
>>   }
>>   +static void dm_set_dpms_off(struct dc_link *link)
>> +{
>> +    struct {
>> +    struct dc_surface_update surface_updates[MAX_SURFACES];
>> +    struct dc_stream_update stream_update;
>> +    } * bundle;
>> +    struct dc_stream_state *stream_state;
>> +    struct dc_context *dc_ctx = link->ctx;
>> +    int i;
>> +
>> +    bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
>> +
>> +    if (!bundle) {
>> +    dm_error("Failed to allocate update bundle\n");
>> +    return;
>> +    }
>> +
>> +    bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL);
> 
> You don't need to allocate memory for the bool. You can just use a local
> here, DC doesn't need it after the call ends.
> 
> I think the same should apply to the surface updates as well. I'm not
> entirely sure how much stack the bundle takes up for a single stream
> here but it should be small enough that we can leave it on the stack.
> 
>> +
>> +    if (!bundle->stream_update.dpms_off) {
>> +    dm_error("Failed to allocate update bundle\n");
>> +    goto cleanup;
>> +    }
>> +
>> +    *bundle->stream_update.dpms_off = true;
>> +
>> +    for (i = 0; i< MAX_PIPES; i++) {
>> +    if (dc_ctx->dc->current_state->streams[i] == NULL)
>> +    continue;
>> +
>> +    if (dc_ctx->dc->current_state->streams[i]->link == link) {
>> +    stream_state = dc_ctx->dc->current_state->streams[i];
>> +    goto link_found;
>> +    }
>> +    }
> 
> We shouldn't be reading from dc->current_state directly in DM, it's
> essentially private state.
> 
> I think we should actually have a new helper here in dc_stream.h that's
> like:
> 
> struct dc_stream_state *dc_stream_find_from_link(const struct dc_link
> *link);
> 
> to replace this block of code.
> 
> Note that any time we touch current_state we also need to be locking -
> it looks like this function is missing the appropriate calls to:
> 
> mutex_lock(>dm.dc_lock);
> mutex_unlock(>dm.dc_lock);
> 
> Regards,
> Nicholas Kazlauskas
> 
> 

Thank you for the review. Sent a v2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2] drm/amd/display: turn DPMS off on mst connector unplug

[Why]

Set dpms off on the MST connector that was unplugged, for the side effect of
releasing some references held through deallocation of mst payload.

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++-
 drivers/gpu/drm/amd/display/dc/core/dc.c  | 17 ++
 drivers/gpu/drm/amd/display/dc/dc_stream.h|  1 +
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e213246e3f04..6203cbf3ee33 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1984,6 +1984,34 @@ static void dm_gpureset_commit_state(struct dc_state 
*dc_state,
return;
 }
 
+static void dm_set_dpms_off(struct dc_link *link)
+{
+   struct dc_stream_state *stream_state;
+   struct amdgpu_dm_connector *aconnector = link->priv;
+   struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev);
+   struct {
+   struct dc_surface_update surface_updates[MAX_SURFACES];
+   struct dc_stream_update stream_update;
+   } bundle = {0};
+   bool dpms_off = true;
+
+   bundle.stream_update.dpms_off = _off;
+
+   mutex_lock(>dm.dc_lock);
+   stream_state = dc_stream_find_from_link(link);
+   mutex_unlock(>dm.dc_lock);
+
+   if (stream_state == NULL) {
+   dm_error("Error finding stream state associated with link!\n");
+   return;
+   }
+
+   bundle.stream_update.stream = stream_state;
+   dc_commit_updates_for_stream(stream_state->ctx->dc, 
bundle.surface_updates, 0,
+stream_state, _update,
+stream_state->ctx->dc->current_state);
+}
+
 static int dm_resume(void *handle)
 {
struct amdgpu_device *adev = handle;
@@ -2434,8 +2462,11 @@ static void handle_hpd_irq(void *param)
drm_kms_helper_hotplug_event(dev);
 
} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) {
-   amdgpu_dm_update_connector_after_detect(aconnector);
+   if (new_connection_type == dc_connection_none &&
+   aconnector->dc_link->type == dc_connection_none)
+   dm_set_dpms_off(aconnector->dc_link);
 
+   amdgpu_dm_update_connector_after_detect(aconnector);
 
drm_modeset_lock_all(dev);
dm_restore_drm_connector_state(dev, connector);
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 903353389edb..7a2b2802faa2 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2798,6 +2798,23 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc 
*dc, uint8_t i)
return NULL;
 }
 
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link)
+{
+   uint8_t i;
+   struct dc_context *ctx = link->ctx;
+
+   for (i = 0; i< MAX_PIPES; i++) {
+   if (ctx->dc->current_state->streams[i] == NULL)
+   continue;
+
+   if (ctx->dc->current_state->streams[i]->link == link) {
+   return ctx->dc->current_state->streams[i];
+   }
+   }
+
+   return NULL;
+}
+
 enum dc_irq_source dc_interrupt_to_irq_source(
struct dc *dc,
uint32_t src_id,
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h 
b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index bf090afc2f70..b7910976b81a 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct 
dc_stream_state *stream);
 
 uint8_t dc_get_current_stream_count(struct dc *dc);
 struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i);
+struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link);
 
 /*
  * Return the current frame counter.
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: turn DPMS off on mst connector unplug

2020-11-26 Thread Kazlauskas, Nicholas


On 2020-11-26 2:50 p.m., Aurabindo Pillai wrote:

[Why]

Set dpms off on the MST connector that was unplugged, for the side effect of
releasing some references held through deallocation of mst payload.

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++-
  1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e213246e3f04..fc984cf6e316 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct dc_state 
*dc_state,
return;
  }
  
+static void dm_set_dpms_off(struct dc_link *link)

+{
+   struct {
+   struct dc_surface_update surface_updates[MAX_SURFACES];
+   struct dc_stream_update stream_update;
+   } * bundle;
+   struct dc_stream_state *stream_state;
+   struct dc_context *dc_ctx = link->ctx;
+   int i;
+
+   bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
+
+   if (!bundle) {
+   dm_error("Failed to allocate update bundle\n");
+   return;
+   }
+
+   bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL);


You don't need to allocate memory for the bool. You can just use a local 
here, DC doesn't need it after the call ends.


I think the same should apply to the surface updates as well. I'm not 
entirely sure how much stack the bundle takes up for a single stream 
here but it should be small enough that we can leave it on the stack.



+
+   if (!bundle->stream_update.dpms_off) {
+   dm_error("Failed to allocate update bundle\n");
+   goto cleanup;
+   }
+
+   *bundle->stream_update.dpms_off = true;
+
+   for (i = 0; i< MAX_PIPES; i++) {
+   if (dc_ctx->dc->current_state->streams[i] == NULL)
+   continue;
+
+   if (dc_ctx->dc->current_state->streams[i]->link == link) {
+   stream_state = dc_ctx->dc->current_state->streams[i];
+   goto link_found;
+   }
+   }


We shouldn't be reading from dc->current_state directly in DM, it's 
essentially private state.


I think we should actually have a new helper here in dc_stream.h that's 
like:


struct dc_stream_state *dc_stream_find_from_link(const struct dc_link 
*link);


to replace this block of code.

Note that any time we touch current_state we also need to be locking - 
it looks like this function is missing the appropriate calls to:


mutex_lock(>dm.dc_lock);
mutex_unlock(>dm.dc_lock);

Regards,
Nicholas Kazlauskas



+
+   dm_error("Cannot find link associated with the stream to be 
disabled\n");
+   return;
+
+link_found:
+   if (stream_state == NULL) {
+   dm_error("Stream state associated with the link is NULL\n");
+   return;
+   }
+
+   bundle->stream_update.stream = stream_state;
+
+   dc_commit_updates_for_stream(stream_state->ctx->dc, 
bundle->surface_updates, 0,
+stream_state, >stream_update,
+stream_state->ctx->dc->current_state);
+
+   kfree(bundle->stream_update.dpms_off);
+cleanup:
+   kfree(bundle);
+
+   return;
+}
+
  static int dm_resume(void *handle)
  {
struct amdgpu_device *adev = handle;
@@ -2434,8 +2492,11 @@ static void handle_hpd_irq(void *param)
drm_kms_helper_hotplug_event(dev);
  
  	} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) {

-   amdgpu_dm_update_connector_after_detect(aconnector);
+   if (new_connection_type == dc_connection_none &&
+   aconnector->dc_link->type == dc_connection_none)
+   dm_set_dpms_off(aconnector->dc_link);
  
+		amdgpu_dm_update_connector_after_detect(aconnector);
  
  		drm_modeset_lock_all(dev);

dm_restore_drm_connector_state(dev, connector);



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/display: turn DPMS off on mst connector unplug

[Why]

Set dpms off on the MST connector that was unplugged, for the side effect of
releasing some references held through deallocation of mst payload.

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e213246e3f04..fc984cf6e316 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct dc_state 
*dc_state,
return;
 }
 
+static void dm_set_dpms_off(struct dc_link *link)
+{
+   struct {
+   struct dc_surface_update surface_updates[MAX_SURFACES];
+   struct dc_stream_update stream_update;
+   } * bundle;
+   struct dc_stream_state *stream_state;
+   struct dc_context *dc_ctx = link->ctx;
+   int i;
+
+   bundle = kzalloc(sizeof(*bundle), GFP_KERNEL);
+
+   if (!bundle) {
+   dm_error("Failed to allocate update bundle\n");
+   return;
+   }
+
+   bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL);
+
+   if (!bundle->stream_update.dpms_off) {
+   dm_error("Failed to allocate update bundle\n");
+   goto cleanup;
+   }
+
+   *bundle->stream_update.dpms_off = true;
+
+   for (i = 0; i< MAX_PIPES; i++) {
+   if (dc_ctx->dc->current_state->streams[i] == NULL)
+   continue;
+
+   if (dc_ctx->dc->current_state->streams[i]->link == link) {
+   stream_state = dc_ctx->dc->current_state->streams[i];
+   goto link_found;
+   }
+   }
+
+   dm_error("Cannot find link associated with the stream to be 
disabled\n");
+   return;
+
+link_found:
+   if (stream_state == NULL) {
+   dm_error("Stream state associated with the link is NULL\n");
+   return;
+   }
+
+   bundle->stream_update.stream = stream_state;
+
+   dc_commit_updates_for_stream(stream_state->ctx->dc, 
bundle->surface_updates, 0,
+stream_state, >stream_update,
+stream_state->ctx->dc->current_state);
+
+   kfree(bundle->stream_update.dpms_off);
+cleanup:
+   kfree(bundle);
+
+   return;
+}
+
 static int dm_resume(void *handle)
 {
struct amdgpu_device *adev = handle;
@@ -2434,8 +2492,11 @@ static void handle_hpd_irq(void *param)
drm_kms_helper_hotplug_event(dev);
 
} else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) {
-   amdgpu_dm_update_connector_after_detect(aconnector);
+   if (new_connection_type == dc_connection_none &&
+   aconnector->dc_link->type == dc_connection_none)
+   dm_set_dpms_off(aconnector->dc_link);
 
+   amdgpu_dm_update_connector_after_detect(aconnector);
 
drm_modeset_lock_all(dev);
dm_restore_drm_connector_state(dev, connector);
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: fix check order in radeon_bo_move

2020-11-26 Thread Daniel Vetter

On Wed, Nov 25, 2020 at 3:34 PM Christian König
 wrote:
>
> Reorder the code to fix checking if blitting is available.

Might be good to explain why blitting might not be available, e.g.
suspend/resume and or chip death and stuff like that.

> Signed-off-by: Christian König 

Needs Fixes: 28a68f828266 ("drm/radeon/ttm: use multihop")

Btw

$ dim fixes [sha1]

generates that for you plus nice cc list of offenders. With the Fixes
line added:

Reviewed-by: Daniel Vetter 

At least I'm hanging onto the illusion that I understand what you did here :-)
-Daniel
> ---
>  drivers/gpu/drm/radeon/radeon_ttm.c | 54 +
>  1 file changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 0ca381b95d3d..2b598141225f 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -216,27 +216,15 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
> struct ttm_resource *old_mem = >mem;
> int r;
>
> -   if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> -new_mem->mem_type == TTM_PL_VRAM) ||
> -   (old_mem->mem_type == TTM_PL_VRAM &&
> -new_mem->mem_type == TTM_PL_SYSTEM)) {
> -   hop->fpfn = 0;
> -   hop->lpfn = 0;
> -   hop->mem_type = TTM_PL_TT;
> -   hop->flags = 0;
> -   return -EMULTIHOP;
> -   }
> -
> if (new_mem->mem_type == TTM_PL_TT) {
> r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
> if (r)
> return r;
> }
> -   radeon_bo_move_notify(bo, evict, new_mem);
>
> r = ttm_bo_wait_ctx(bo, ctx);
> if (r)
> -   goto fail;
> +   return r;
>
> /* Can't move a pinned BO */
> rbo = container_of(bo, struct radeon_bo, tbo);
> @@ -246,12 +234,12 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
> rdev = radeon_get_rdev(bo->bdev);
> if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
> ttm_bo_move_null(bo, new_mem);
> -   return 0;
> +   goto out;
> }
> if (old_mem->mem_type == TTM_PL_SYSTEM &&
> new_mem->mem_type == TTM_PL_TT) {
> ttm_bo_move_null(bo, new_mem);
> -   return 0;
> +   goto out;
> }
>
> if (old_mem->mem_type == TTM_PL_TT &&
> @@ -259,31 +247,37 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
> radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
> ttm_resource_free(bo, >mem);
> ttm_bo_assign_mem(bo, new_mem);
> -   return 0;
> +   goto out;
> }
> -   if (!rdev->ring[radeon_copy_ring_index(rdev)].ready ||
> -   rdev->asic->copy.copy == NULL) {
> -   /* use memcpy */
> -   goto memcpy;
> +   if (rdev->ring[radeon_copy_ring_index(rdev)].ready &&
> +   rdev->asic->copy.copy != NULL) {
> +   if ((old_mem->mem_type == TTM_PL_SYSTEM &&
> +new_mem->mem_type == TTM_PL_VRAM) ||
> +   (old_mem->mem_type == TTM_PL_VRAM &&
> +new_mem->mem_type == TTM_PL_SYSTEM)) {
> +   hop->fpfn = 0;
> +   hop->lpfn = 0;
> +   hop->mem_type = TTM_PL_TT;
> +   hop->flags = 0;
> +   return -EMULTIHOP;
> +   }
> +
> +   r = radeon_move_blit(bo, evict, new_mem, old_mem);
> +   } else {
> +   r = -ENODEV;
> }
>
> -   r = radeon_move_blit(bo, evict, new_mem, old_mem);
> if (r) {
> -memcpy:
> r = ttm_bo_move_memcpy(bo, ctx, new_mem);
> -   if (r) {
> -   goto fail;
> -   }
> +   if (r)
> +   return r;
> }
>
> +out:
> /* update statistics */
> atomic64_add((u64)bo->num_pages << PAGE_SHIFT, 
> >num_bytes_moved);
> +   radeon_bo_move_notify(bo, evict, new_mem);
> return 0;
> -fail:
> -   swap(*new_mem, bo->mem);
> -   radeon_bo_move_notify(bo, false, new_mem);
> -   swap(*new_mem, bo->mem);
> -   return r;
>  }
>
>  static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct 
> ttm_resource *mem)
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Karol Herbst

On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven  wrote:
>
> Hi Miguel,
>
> On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda
>  wrote:
> > On Wed, Nov 25, 2020 at 11:44 PM Edward Cree  wrote:
> > > To make the intent clear, you have to first be certain that you
> > >  understand the intent; otherwise by adding either a break or a
> > >  fallthrough to suppress the warning you are just destroying the
> > >  information that "the intent of this code is unknown".
> >
> > If you don't know what the intent of your own code is, then you
> > *already* have a problem in your hands.
>
> The maintainer is not necessarily the owner/author of the code, and
> thus may not know the intent of the code.
>
> > > or does it flag up code
> > >  that can be mindlessly "fixed" (in which case the warning is
> > >  worthless)?  Proponents in this thread seem to be trying to
> > >  have it both ways.
> >
> > A warning is not worthless just because you can mindlessly fix it.
> > There are many counterexamples, e.g. many
> > checkpatch/lint/lang-format/indentation warnings, functional ones like
> > the `if (a = b)` warning...
>
> BTW, you cannot mindlessly fix the latter, as you cannot know if
> "(a == b)" or "((a = b))" was intended, without understanding the code
> (and the (possibly unavailable) data sheet, and the hardware, ...).
>

to allow assignments in if statements was clearly a mistake and if you
need outside information to understand the code, your code is the
issue already.

> P.S. So far I've stayed out of this thread, as I like it if the compiler
>  flags possible mistakes.  After all I was the one fixing new
>  "may be used uninitialized" warnings thrown up by gcc-4.1, until
>  (a bit later than) support for that compiler was removed...
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect



On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote:
> On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:
>> [Why]
>> Recent changes to upstream mst code remove the callback which
>> cleared the internal state for mst. Move the missing functionality
>> that was previously called through the destroy call back for mst
>> connector
>> destroy
>>
>> Signed-off-by: Aurabindo Pillai 
>> Signed-off-by: Eryk Brol 
>> ---
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
>>   drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index b7d7ec3ba00d..d8b0f07deaf2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
>>     void dm_helpers_dp_mst_stop_top_mgr(
>>   struct dc_context *ctx,
>> -    const struct dc_link *link)
>> +    struct dc_link *link)
>>   {
>>   struct amdgpu_dm_connector *aconnector = link->priv;
>> +    uint8_t i;
>>     if (!aconnector) {
>>   DRM_ERROR("Failed to find connector for link!");
>> @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
>>   DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
>>   aconnector, aconnector->base.base.id);
>>   -    if (aconnector->mst_mgr.mst_state == true)
>> +    if (aconnector->mst_mgr.mst_state == true) {
>>   drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
>> +
>> +    for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
>> +    if (link->remote_sinks[i] == NULL)
>> +    continue;
>> +
>> +    if (link->remote_sinks[i]->sink_signal ==
>> +    SIGNAL_TYPE_DISPLAY_PORT_MST) {
>> +    dc_link_remove_remote_sink(link, link->remote_sinks[i]);
> 
> In general I think this patch looks fine, and you can have the:
> 
> Reviewed-by: Nicholas Kazlauskas 
> 
> But I think that this loop is redundant - dc_link_remove_remote_sink
> should be removing all the remote sinks. Not sure if remote_sinks can
> start at an index other than 0 though.

dc_link_remove_remote_sink() will only remove one sink. It returns as
soon as it is done removing the sink we asked for.


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: fix check order in radeon_bo_move


Ping, Dave this is another fix for the Multihop patch set.

Without it radeon is completely broken on drm-misc-next.

Thanks,
Christian.

Am 25.11.20 um 15:34 schrieb Christian König:

Reorder the code to fix checking if blitting is available.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/radeon/radeon_ttm.c | 54 +
  1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0ca381b95d3d..2b598141225f 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -216,27 +216,15 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
bool evict,
struct ttm_resource *old_mem = >mem;
int r;
  
-	if ((old_mem->mem_type == TTM_PL_SYSTEM &&

-new_mem->mem_type == TTM_PL_VRAM) ||
-   (old_mem->mem_type == TTM_PL_VRAM &&
-new_mem->mem_type == TTM_PL_SYSTEM)) {
-   hop->fpfn = 0;
-   hop->lpfn = 0;
-   hop->mem_type = TTM_PL_TT;
-   hop->flags = 0;
-   return -EMULTIHOP;
-   }
-
if (new_mem->mem_type == TTM_PL_TT) {
r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
if (r)
return r;
}
-   radeon_bo_move_notify(bo, evict, new_mem);
  
  	r = ttm_bo_wait_ctx(bo, ctx);

if (r)
-   goto fail;
+   return r;
  
  	/* Can't move a pinned BO */

rbo = container_of(bo, struct radeon_bo, tbo);
@@ -246,12 +234,12 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
bool evict,
rdev = radeon_get_rdev(bo->bdev);
if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
ttm_bo_move_null(bo, new_mem);
-   return 0;
+   goto out;
}
if (old_mem->mem_type == TTM_PL_SYSTEM &&
new_mem->mem_type == TTM_PL_TT) {
ttm_bo_move_null(bo, new_mem);
-   return 0;
+   goto out;
}
  
  	if (old_mem->mem_type == TTM_PL_TT &&

@@ -259,31 +247,37 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
bool evict,
radeon_ttm_tt_unbind(bo->bdev, bo->ttm);
ttm_resource_free(bo, >mem);
ttm_bo_assign_mem(bo, new_mem);
-   return 0;
+   goto out;
}
-   if (!rdev->ring[radeon_copy_ring_index(rdev)].ready ||
-   rdev->asic->copy.copy == NULL) {
-   /* use memcpy */
-   goto memcpy;
+   if (rdev->ring[radeon_copy_ring_index(rdev)].ready &&
+   rdev->asic->copy.copy != NULL) {
+   if ((old_mem->mem_type == TTM_PL_SYSTEM &&
+new_mem->mem_type == TTM_PL_VRAM) ||
+   (old_mem->mem_type == TTM_PL_VRAM &&
+new_mem->mem_type == TTM_PL_SYSTEM)) {
+   hop->fpfn = 0;
+   hop->lpfn = 0;
+   hop->mem_type = TTM_PL_TT;
+   hop->flags = 0;
+   return -EMULTIHOP;
+   }
+
+   r = radeon_move_blit(bo, evict, new_mem, old_mem);
+   } else {
+   r = -ENODEV;
}
  
-	r = radeon_move_blit(bo, evict, new_mem, old_mem);

if (r) {
-memcpy:
r = ttm_bo_move_memcpy(bo, ctx, new_mem);
-   if (r) {
-   goto fail;
-   }
+   if (r)
+   return r;
}
  
+out:

/* update statistics */
atomic64_add((u64)bo->num_pages << PAGE_SHIFT, >num_bytes_moved);
+   radeon_bo_move_notify(bo, evict, new_mem);
return 0;
-fail:
-   swap(*new_mem, bo->mem);
-   radeon_bo_move_notify(bo, false, new_mem);
-   swap(*new_mem, bo->mem);
-   return r;
  }
  
  static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_resource *mem)


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm/scheduler: Job timeout handler returns status

2020-11-26 Thread Andrey Grodzovsky




On 11/24/20 10:17 PM, Luben Tuikov wrote:

The job timeout handler now returns status
indicating back to the DRM layer whether the job
was successfully cancelled or whether more time
should be given to the job to complete.

Signed-off-by: Luben Tuikov 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  6 --
  include/drm/gpu_scheduler.h | 13 ++---
  2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index ff48101bab55..81b73790ecc6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -28,7 +28,7 @@
  #include "amdgpu.h"
  #include "amdgpu_trace.h"
  
-static void amdgpu_job_timedout(struct drm_sched_job *s_job)

+static int amdgpu_job_timedout(struct drm_sched_job *s_job)
  {
struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
struct amdgpu_job *job = to_amdgpu_job(s_job);
@@ -41,7 +41,7 @@ static void amdgpu_job_timedout(struct drm_sched_job *s_job)
amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) 
{
DRM_ERROR("ring %s timeout, but soft recovered\n",
  s_job->sched->name);
-   return;
+   return 0;
}
  
  	amdgpu_vm_get_task_info(ring->adev, job->pasid, );

@@ -53,10 +53,12 @@ static void amdgpu_job_timedout(struct drm_sched_job *s_job)
  
  	if (amdgpu_device_should_recover_gpu(ring->adev)) {

amdgpu_device_gpu_recover(ring->adev, job);
+   return 0;



For amdgpu specifically - not that amdgpu_device_gpu_recover returns a value 
which is 0 for successful GPU reset
meaning we reset the GPU and resubmitted to HW the job that triggered the 
timeout to HW (guilty).
It means the job is still should be considered part of pending list and so a non 
zero value
should be returned. I think only if we reset the GPU and don't submit back the 
guilty job then

it can be considered 'aborted' - but I don't think we even do this.

Andrey



} else {
drm_sched_suspend_timeout(>sched);
if (amdgpu_sriov_vf(adev))
adev->virt.tdr_debug = true;
+   return 1;
}
  }
  
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h

index 2e0c368e19f6..61f7121e1c19 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -230,10 +230,17 @@ struct drm_sched_backend_ops {
struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
  
  	/**

- * @timedout_job: Called when a job has taken too long to execute,
- * to trigger GPU recovery.
+* @timedout_job: Called when a job has taken too long to execute,
+* to trigger GPU recovery.
+*
+* Return 0, if the job has been aborted successfully and will
+* never be heard of from the device. Return non-zero if the
+* job wasn't able to be aborted, i.e. if more time should be
+* given to this job. The result is not "bool" as this
+* function is not a predicate, although its result may seem
+* as one.
 */
-   void (*timedout_job)(struct drm_sched_job *sched_job);
+   int (*timedout_job)(struct drm_sched_job *sched_job);
  
  	/**

   * @free_job: Called once the job's finished fence has been signaled

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

2020-11-26 Thread Harry Wentland


On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote:

On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:

[Why]
Recent changes to upstream mst code remove the callback which
cleared the internal state for mst. Move the missing functionality
that was previously called through the destroy call back for mst 
connector

destroy

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
  drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
  2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c

index b7d7ec3ba00d..d8b0f07deaf2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
  void dm_helpers_dp_mst_stop_top_mgr(
  struct dc_context *ctx,
-    const struct dc_link *link)
+    struct dc_link *link)
  {
  struct amdgpu_dm_connector *aconnector = link->priv;
+    uint8_t i;
  if (!aconnector) {
  DRM_ERROR("Failed to find connector for link!");
@@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
  DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
  aconnector, aconnector->base.base.id);
-    if (aconnector->mst_mgr.mst_state == true)
+    if (aconnector->mst_mgr.mst_state == true) {
  drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
+
+    for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
+    if (link->remote_sinks[i] == NULL)
+    continue;
+
+    if (link->remote_sinks[i]->sink_signal ==
+    SIGNAL_TYPE_DISPLAY_PORT_MST) {
+    dc_link_remove_remote_sink(link, link->remote_sinks[i]);


In general I think this patch looks fine, and you can have the:

Reviewed-by: Nicholas Kazlauskas 

But I think that this loop is redundant - dc_link_remove_remote_sink 
should be removing all the remote sinks. Not sure if remote_sinks can 
start at an index other than 0 though.




I'd prefer we clean this up if it's redundant. Otherwise redundant code 
stays around forever and makes future debug more cumbersome.


Harry


Regards,
Nicholas Kazlauskas


+
+    if (aconnector->dc_sink) {
+    dc_sink_release(aconnector->dc_sink);
+    aconnector->dc_sink = NULL;
+    aconnector->dc_link->cur_link_settings.lane_count 
= 0;

+    }
+    }
+    }
+    } >   }
  bool dm_helpers_dp_read_dpcd(
diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h 
b/drivers/gpu/drm/amd/display/dc/dm_helpers.h

index b2cd8491c707..07e349b1067b 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr(
  void dm_helpers_dp_mst_stop_top_mgr(
  struct dc_context *ctx,
-    const struct dc_link *link);
+    struct dc_link *link);
  /**
   * OS specific aux read callback.
   */



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7CHARRY.WENTLAND%40amd.com%7C7576866a05154b66cdb208d8921898da%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63741998164920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=sm7jvbxW%2BgEUevWgjACydnjuiE7SgtunMBUIpLT99r4%3Dreserved=0 


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

2020-11-26 Thread Kazlauskas, Nicholas


On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote:

[Why]
Recent changes to upstream mst code remove the callback which
cleared the internal state for mst. Move the missing functionality
that was previously called through the destroy call back for mst connector
destroy

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
  drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
  2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b7d7ec3ba00d..d8b0f07deaf2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
  
  void dm_helpers_dp_mst_stop_top_mgr(

struct dc_context *ctx,
-   const struct dc_link *link)
+   struct dc_link *link)
  {
struct amdgpu_dm_connector *aconnector = link->priv;
+   uint8_t i;
  
  	if (!aconnector) {

DRM_ERROR("Failed to find connector for link!");
@@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
aconnector, aconnector->base.base.id);
  
-	if (aconnector->mst_mgr.mst_state == true)

+   if (aconnector->mst_mgr.mst_state == true) {
drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
+
+   for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
+   if (link->remote_sinks[i] == NULL)
+   continue;
+
+   if (link->remote_sinks[i]->sink_signal ==
+   SIGNAL_TYPE_DISPLAY_PORT_MST) {
+   dc_link_remove_remote_sink(link, 
link->remote_sinks[i]);


In general I think this patch looks fine, and you can have the:

Reviewed-by: Nicholas Kazlauskas 

But I think that this loop is redundant - dc_link_remove_remote_sink 
should be removing all the remote sinks. Not sure if remote_sinks can 
start at an index other than 0 though.


Regards,
Nicholas Kazlauskas


+
+   if (aconnector->dc_sink) {
+   dc_sink_release(aconnector->dc_sink);
+   aconnector->dc_sink = NULL;
+   
aconnector->dc_link->cur_link_settings.lane_count = 0;
+   }
+   }
+   }
+   } >   }
  
  bool dm_helpers_dp_read_dpcd(

diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h 
b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
index b2cd8491c707..07e349b1067b 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr(
  
  void dm_helpers_dp_mst_stop_top_mgr(

struct dc_context *ctx,
-   const struct dc_link *link);
+   struct dc_link *link);
  /**
   * OS specific aux read callback.
   */



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect

[Why]
Recent changes to upstream mst code remove the callback which
cleared the internal state for mst. Move the missing functionality
that was previously called through the destroy call back for mst connector
destroy

Signed-off-by: Aurabindo Pillai 
Signed-off-by: Eryk Brol 
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +--
 drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b7d7ec3ba00d..d8b0f07deaf2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr(
 
 void dm_helpers_dp_mst_stop_top_mgr(
struct dc_context *ctx,
-   const struct dc_link *link)
+   struct dc_link *link)
 {
struct amdgpu_dm_connector *aconnector = link->priv;
+   uint8_t i;
 
if (!aconnector) {
DRM_ERROR("Failed to find connector for link!");
@@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr(
DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n",
aconnector, aconnector->base.base.id);
 
-   if (aconnector->mst_mgr.mst_state == true)
+   if (aconnector->mst_mgr.mst_state == true) {
drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false);
+
+   for (i = 0; i < MAX_SINKS_PER_LINK; i++) {
+   if (link->remote_sinks[i] == NULL)
+   continue;
+
+   if (link->remote_sinks[i]->sink_signal ==
+   SIGNAL_TYPE_DISPLAY_PORT_MST) {
+   dc_link_remove_remote_sink(link, 
link->remote_sinks[i]);
+
+   if (aconnector->dc_sink) {
+   dc_sink_release(aconnector->dc_sink);
+   aconnector->dc_sink = NULL;
+   
aconnector->dc_link->cur_link_settings.lane_count = 0;
+   }
+   }
+   }
+   }
 }
 
 bool dm_helpers_dp_read_dpcd(
diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h 
b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
index b2cd8491c707..07e349b1067b 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr(
 
 void dm_helpers_dp_mst_stop_top_mgr(
struct dc_context *ctx,
-   const struct dc_link *link);
+   struct dc_link *link);
 /**
  * OS specific aux read callback.
  */
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

Am 26.11.20 um 13:14 schrieb Thomas Zimmermann:

Am 26.11.20 um 13:08 schrieb Christian König:

[...]
Yeah, I remember a bug report about frequent page-table
modifications wrt to vram helpers. So we implemented the lazy
unmapping / vmap caching, as suggested by Christian back them. My
guess is that anything TTM-based can use a similar pattern.
Christian probably knows the corner cases.

My memory is failing me, what corner case are you talking about?

Haha! :D What I meant was that if there were corner cases, you'd know
about them. From your comment, I guess there are none.

Ah, ok :) I was wondering for a moment if I have missed something.

Cheers,
Christian.

Best regards
Thomas

Christian.

CMA seems obviously working correctly already.

For SHMEM, I'd have to figure out the reference counting of the
pages involved. My guess is that the vunmap in
drm_gem_shmem_vunmap() could be moved into the free callback, plus a
few other modifications.

Best regards
Thomas

But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel

Best regards
Thomas

That should give us at least some way forward to gradually conver
all the

drivers and helpers over to dma_resv locking.
-Daniel

The pin count is currently maintained by the vmap implementation
in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an
implicit
unpin. This prevents eviction in the damage worker. But now I
was told than
pinning is only for BOs that are controlled by userspace and
internal users
should acquire the resv lock. So vram helpers have to be fixed,
actually.

In vram helpers, unmapping does not mean eviction. The unmap
operation only
marks the BO as unmappable. The real unmap happens when the
eviction takes
place. This avoids many modifications to the page tables. IOW an
unpinned,
unmapped BO will remain in VRAM until the memory is actually
needed.

Best regards
Thomas

So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv
locking, but
the issue there is that beyond ttm, none of the helpers (and
few of the
drivers) use dma_resv. So this is a fairly big uphill battle.
Quick

interim fix seems like the right solution to me.
-Daniel

Regards,
Christian.

Best regards
Thomas

There's no recursion taking place, so I guess the reservation
lock could be
acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of
DRM client functions could do the locking.

Given how this "do the right locking" is a can of worms (and
I think

it's
worse than what you dug out already) I think the
fb_helper.lock hack is

perfectly good enough.

I'm also somewhat worried that starting to use dma_resv lock
in generic
code, while many helpers/drivers still have their
hand-rolled locking,
will make conversion over to dma_resv needlessly more
complicated.

-Daniel

Best regards
Thomas

[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394

Please note that the reservation lock you need to take
here is part of

the GEM object.

Usually we design things in the way that the code needs to
take a lock
which protects an object, then do some operations with the
object and

then release the lock again.

Having in the lock inside the operation can be done as
well, but

returning with it is kind of unusual design.

Sorry for the noob questions. I'm still trying to
understand the

implications of acquiring these locks.

Well this is the reservation lock of the GEM object we are
talking about
here. We need to take that for a couple of different
operations,

vmap/vunmap doesn't sound like a special case to me.

Regards,
Christian.

Best regards
Thomas

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

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

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

Am 26.11.20 um 13:08 schrieb Christian König:

[...]
Yeah, I remember a bug report about frequent page-table modifications
wrt to vram helpers. So we implemented the lazy unmapping / vmap
caching, as suggested by Christian back them. My guess is that
anything TTM-based can use a similar pattern. Christian probably knows
the corner cases.

My memory is failing me, what corner case are you talking about?

Haha! :D What I meant was that if there were corner cases, you'd know
about them. From your comment, I guess there are none.

Best regards
Thomas

Christian.

CMA seems obviously working correctly already.

For SHMEM, I'd have to figure out the reference counting of the pages
involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could
be moved into the free callback, plus a few other modifications.

Best regards
Thomas

But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel

Best regards
Thomas

That should give us at least some way forward to gradually conver
all the

drivers and helpers over to dma_resv locking.
-Daniel

The pin count is currently maintained by the vmap implementation
in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an
implicit
unpin. This prevents eviction in the damage worker. But now I was
told than
pinning is only for BOs that are controlled by userspace and
internal users
should acquire the resv lock. So vram helpers have to be fixed,
actually.

unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas

So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv
locking, but
the issue there is that beyond ttm, none of the helpers (and few
of the

drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel

Regards,
Christian.

Best regards
Thomas

There's no recursion taking place, so I guess the reservation
lock could be
acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of
DRM client functions could do the locking.

Given how this "do the right locking" is a can of worms (and I
think

it's
worse than what you dug out already) I think the
fb_helper.lock hack is

perfectly good enough.

I'm also somewhat worried that starting to use dma_resv lock
in generic
code, while many helpers/drivers still have their hand-rolled
locking,
will make conversion over to dma_resv needlessly more
complicated.

-Daniel

Best regards
Thomas

[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394

Please note that the reservation lock you need to take here
is part of

the GEM object.

Usually we design things in the way that the code needs to
take a lock
which protects an object, then do some operations with the
object and

then release the lock again.

Having in the lock inside the operation can be done as well,
but

returning with it is kind of unusual design.

Sorry for the noob questions. I'm still trying to
understand the

implications of acquiring these locks.

Well this is the reservation lock of the GEM object we are
talking about
here. We need to take that for a couple of different
operations,

vmap/vunmap doesn't sound like a special case to me.

Regards,
Christian.

Best regards
Thomas

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

OpenPGP_0x680DC11D530B7A23.asc
Description: application/pgp-keys

OpenPGP_signature
Description: OpenPGP digital signature
___

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed


Am 26.11.20 um 12:59 schrieb Thomas Zimmermann:

Hi

Am 26.11.20 um 12:04 schrieb Daniel Vetter:
On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann 
 wrote:


Hi

Am 25.11.20 um 17:32 schrieb Daniel Vetter:

[...]
I guess full locking is required :-/ I'm not exactly sure how to 
make this

happen with the current plethora of helpers ... I think we need an
_locked version of vmap/vunmap callbacks in drm_gem_object_funcs.


I think we might be able to get away without new callbacks.

I looked through the sources that implement and use vmap. All the
implementations are without taking resv locks. For locking, we can wrap
them in lock/unlock pairs. Having something like 
drm_gem_vmap_unlocked()

that locks and vmaps should make this easy.

In terms of implementation, only vram helpers do a pin+map in their 
vmap

code. And as I mentioned before, this is actually wrong. The pattern
dates back to when the code was still in individual drivers. It's time
to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.

Finally, there aren't that many users of vmap. A few drivers use it
while blitting framebuffers into HW buffers and ast does some permanent
mapping of the cursor BO. All this is trivial to turn into small pairs
of lock+vmap and vunmap+unlock.

That leaves generic fbdev. The shadow buffer is also trivial to fix, as
outlined during this discussion.

The code for fbdev in hardware buffers is a special case. It vmaps the
buffer during initialization and only vunmaps it during shutdown. As
this has worked so far without locking, I'd leave it as it is and put a
big comment next to is.

Hardware fbdev buffers is only required by few drivers; namely those
that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
We should consider to make the fbdev shadow buffer the default and have
drivers opt-in for the hardware buffer, if they need it.



And then document that if the callers of the _locked version wants a
permanent mapping, it also needs to pin it. Plus I guess ideally 
implement

the unlocked/permanent versions in terms of that, so that drivers only
have to implement one or the other.


For my understanding, pinning is only done in prepare_fb code. And ast
pins its cursor BOs into vram. We should document to hols vmap/vunmap
only for time and cover them with resv locks. Pinning is for cases 
where

the hardware requires buffers in a special location, but does not
protect against concurrent threat. I think those are the implicit rules
already.

I updated the radeon patchset, where all this appears to be working 
well.


Hm yeah if you want to do the full change, then that works out too.
It's just a pile of work.

But if we can finish off with an dma_resv_assert_locked in
dma_buf_vmap/vunmap, then I think that's ok. It does mean that
exporters must implement vmap caching, or performance will be
terrible. So quite some update for the dma-buf docs.


Yeah, I remember a bug report about frequent page-table modifications 
wrt to vram helpers. So we implemented the lazy unmapping / vmap 
caching, as suggested by Christian back them. My guess is that 
anything TTM-based can use a similar pattern. Christian probably knows 
the corner cases.


My memory is failing me, what corner case are you talking about?

Christian.



CMA seems obviously working correctly already.

For SHMEM, I'd have to figure out the reference counting of the pages 
involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could 
be moved into the free callback, plus a few other modifications.


Best regards
Thomas



But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel



Best regards
Thomas



That should give us at least some way forward to gradually conver 
all the

drivers and helpers over to dma_resv locking.
-Daniel

The pin count is currently maintained by the vmap implementation 
in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an 
implicit
unpin. This prevents eviction in the damage worker. But now I was 
told than
pinning is only for BOs that are controlled by userspace and 
internal users
should acquire the resv lock. So vram helpers have to be fixed, 
actually.


In vram helpers, unmapping does not mean eviction. The unmap 
operation only
marks the BO as unmappable. The real unmap happens when the 
eviction takes
place. This avoids many modifications to the page tables. IOW an 
unpinned,

unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas



So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv 
locking, but
the issue there is that beyond ttm, none of the helpers (and few 
of the

drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel



Regards,
Christian.



Best regards
Thomas




There's no recursion taking

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed


Hi

Am 26.11.20 um 12:04 schrieb Daniel Vetter:

On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann  wrote:


Hi

Am 25.11.20 um 17:32 schrieb Daniel Vetter:

[...]
I guess full locking is required :-/ I'm not exactly sure how to make this
happen with the current plethora of helpers ... I think we need an
_locked version of vmap/vunmap callbacks in drm_gem_object_funcs.


I think we might be able to get away without new callbacks.

I looked through the sources that implement and use vmap. All the
implementations are without taking resv locks. For locking, we can wrap
them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked()
that locks and vmaps should make this easy.

In terms of implementation, only vram helpers do a pin+map in their vmap
code. And as I mentioned before, this is actually wrong. The pattern
dates back to when the code was still in individual drivers. It's time
to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.

Finally, there aren't that many users of vmap. A few drivers use it
while blitting framebuffers into HW buffers and ast does some permanent
mapping of the cursor BO. All this is trivial to turn into small pairs
of lock+vmap and vunmap+unlock.

That leaves generic fbdev. The shadow buffer is also trivial to fix, as
outlined during this discussion.

The code for fbdev in hardware buffers is a special case. It vmaps the
buffer during initialization and only vunmaps it during shutdown. As
this has worked so far without locking, I'd leave it as it is and put a
big comment next to is.

Hardware fbdev buffers is only required by few drivers; namely those
that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
We should consider to make the fbdev shadow buffer the default and have
drivers opt-in for the hardware buffer, if they need it.



And then document that if the callers of the _locked version wants a
permanent mapping, it also needs to pin it. Plus I guess ideally implement
the unlocked/permanent versions in terms of that, so that drivers only
have to implement one or the other.


For my understanding, pinning is only done in prepare_fb code. And ast
pins its cursor BOs into vram. We should document to hols vmap/vunmap
only for time and cover them with resv locks. Pinning is for cases where
the hardware requires buffers in a special location, but does not
protect against concurrent threat. I think those are the implicit rules
already.

I updated the radeon patchset, where all this appears to be working well.


Hm yeah if you want to do the full change, then that works out too.
It's just a pile of work.

But if we can finish off with an dma_resv_assert_locked in
dma_buf_vmap/vunmap, then I think that's ok. It does mean that
exporters must implement vmap caching, or performance will be
terrible. So quite some update for the dma-buf docs.


Yeah, I remember a bug report about frequent page-table modifications 
wrt to vram helpers. So we implemented the lazy unmapping / vmap 
caching, as suggested by Christian back them. My guess is that anything 
TTM-based can use a similar pattern. Christian probably knows the corner 
cases.


CMA seems obviously working correctly already.

For SHMEM, I'd have to figure out the reference counting of the pages 
involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could be 
moved into the free callback, plus a few other modifications.


Best regards
Thomas



But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel



Best regards
Thomas



That should give us at least some way forward to gradually conver all the
drivers and helpers over to dma_resv locking.
-Daniel


The pin count is currently maintained by the vmap implementation in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an implicit
unpin. This prevents eviction in the damage worker. But now I was told than
pinning is only for BOs that are controlled by userspace and internal users
should acquire the resv lock. So vram helpers have to be fixed, actually.

In vram helpers, unmapping does not mean eviction. The unmap operation only
marks the BO as unmappable. The real unmap happens when the eviction takes
place. This avoids many modifications to the page tables. IOW an unpinned,
unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas



So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv locking, but
the issue there is that beyond ttm, none of the helpers (and few of the
drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel



Regards,
Christian.



Best regards
Thomas




There's no recursion taking place, so I guess the reservation
lock could be
acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of
DRM client functions could do the

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed


Hi

Am 26.11.20 um 12:28 schrieb Christian König:

Am 26.11.20 um 12:04 schrieb Daniel Vetter:
On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann 
 wrote:

Hi

Am 25.11.20 um 17:32 schrieb Daniel Vetter:

[...]
I guess full locking is required :-/ I'm not exactly sure how to 
make this

happen with the current plethora of helpers ... I think we need an
_locked version of vmap/vunmap callbacks in drm_gem_object_funcs.

I think we might be able to get away without new callbacks.

I looked through the sources that implement and use vmap. All the
implementations are without taking resv locks. For locking, we can wrap
them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked()
that locks and vmaps should make this easy.

In terms of implementation, only vram helpers do a pin+map in their vmap
code. And as I mentioned before, this is actually wrong. The pattern
dates back to when the code was still in individual drivers. It's time
to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.

Finally, there aren't that many users of vmap. A few drivers use it
while blitting framebuffers into HW buffers and ast does some permanent
mapping of the cursor BO. All this is trivial to turn into small pairs
of lock+vmap and vunmap+unlock.

That leaves generic fbdev. The shadow buffer is also trivial to fix, as
outlined during this discussion.

The code for fbdev in hardware buffers is a special case. It vmaps the
buffer during initialization and only vunmaps it during shutdown. As
this has worked so far without locking, I'd leave it as it is and put a
big comment next to is.


Please keep in mind that you only need to grab the lock if the buffer is 
not pinned otherwise.


In other words when we are scanning out from the BO it is guaranteed 
that it can't move around.


Maybe this makes the case here easier to handle.


The fbdev code is already fragile. If no shadow FB is selected, the 
hardware BO is vmapped, but never pinned; if only for the reason that 
there's no useful generic interface to do this. So we cannot lock for 
longer periods, but it's also not pinned either. This really only work 
with a few drivers that use CMA helpers, where BOs don't move.


Best regards
Thomas





Hardware fbdev buffers is only required by few drivers; namely those
that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
We should consider to make the fbdev shadow buffer the default and have
drivers opt-in for the hardware buffer, if they need it.


And then document that if the callers of the _locked version wants a
permanent mapping, it also needs to pin it. Plus I guess ideally 
implement

the unlocked/permanent versions in terms of that, so that drivers only
have to implement one or the other.

For my understanding, pinning is only done in prepare_fb code. And ast
pins its cursor BOs into vram. We should document to hols vmap/vunmap
only for time and cover them with resv locks. Pinning is for cases where
the hardware requires buffers in a special location, but does not
protect against concurrent threat. I think those are the implicit rules
already.

I updated the radeon patchset, where all this appears to be working 
well.

Hm yeah if you want to do the full change, then that works out too.
It's just a pile of work.

But if we can finish off with an dma_resv_assert_locked in
dma_buf_vmap/vunmap, then I think that's ok. It does mean that
exporters must implement vmap caching, or performance will be
terrible. So quite some update for the dma-buf docs.


That's one possibility, but I think we should keep the ability to use 
pin+vmap instead of lock+vmap.


Regards,
Christian.



But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel


Best regards
Thomas

That should give us at least some way forward to gradually conver 
all the

drivers and helpers over to dma_resv locking.
-Daniel

The pin count is currently maintained by the vmap implementation in 
vram
helpers. Calling vmap is an implicit pin; calling vunmap is an 
implicit
unpin. This prevents eviction in the damage worker. But now I was 
told than
pinning is only for BOs that are controlled by userspace and 
internal users
should acquire the resv lock. So vram helpers have to be fixed, 
actually.


In vram helpers, unmapping does not mean eviction. The unmap 
operation only
marks the BO as unmappable. The real unmap happens when the 
eviction takes
place. This avoids many modifications to the page tables. IOW an 
unpinned,

unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas


So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv 
locking, but
the issue there is that beyond ttm, none of the helpers (and few 
of the

drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed


Am 26.11.20 um 12:04 schrieb Daniel Vetter:

On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann  wrote:

Hi

Am 25.11.20 um 17:32 schrieb Daniel Vetter:

[...]
I guess full locking is required :-/ I'm not exactly sure how to make this
happen with the current plethora of helpers ... I think we need an
_locked version of vmap/vunmap callbacks in drm_gem_object_funcs.

I think we might be able to get away without new callbacks.

I looked through the sources that implement and use vmap. All the
implementations are without taking resv locks. For locking, we can wrap
them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked()
that locks and vmaps should make this easy.

In terms of implementation, only vram helpers do a pin+map in their vmap
code. And as I mentioned before, this is actually wrong. The pattern
dates back to when the code was still in individual drivers. It's time
to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.

Finally, there aren't that many users of vmap. A few drivers use it
while blitting framebuffers into HW buffers and ast does some permanent
mapping of the cursor BO. All this is trivial to turn into small pairs
of lock+vmap and vunmap+unlock.

That leaves generic fbdev. The shadow buffer is also trivial to fix, as
outlined during this discussion.

The code for fbdev in hardware buffers is a special case. It vmaps the
buffer during initialization and only vunmaps it during shutdown. As
this has worked so far without locking, I'd leave it as it is and put a
big comment next to is.


Please keep in mind that you only need to grab the lock if the buffer is 
not pinned otherwise.


In other words when we are scanning out from the BO it is guaranteed 
that it can't move around.


Maybe this makes the case here easier to handle.



Hardware fbdev buffers is only required by few drivers; namely those
that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
We should consider to make the fbdev shadow buffer the default and have
drivers opt-in for the hardware buffer, if they need it.


And then document that if the callers of the _locked version wants a
permanent mapping, it also needs to pin it. Plus I guess ideally implement
the unlocked/permanent versions in terms of that, so that drivers only
have to implement one or the other.

For my understanding, pinning is only done in prepare_fb code. And ast
pins its cursor BOs into vram. We should document to hols vmap/vunmap
only for time and cover them with resv locks. Pinning is for cases where
the hardware requires buffers in a special location, but does not
protect against concurrent threat. I think those are the implicit rules
already.

I updated the radeon patchset, where all this appears to be working well.

Hm yeah if you want to do the full change, then that works out too.
It's just a pile of work.

But if we can finish off with an dma_resv_assert_locked in
dma_buf_vmap/vunmap, then I think that's ok. It does mean that
exporters must implement vmap caching, or performance will be
terrible. So quite some update for the dma-buf docs.


That's one possibility, but I think we should keep the ability to use 
pin+vmap instead of lock+vmap.


Regards,
Christian.



But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel


Best regards
Thomas


That should give us at least some way forward to gradually conver all the
drivers and helpers over to dma_resv locking.
-Daniel


The pin count is currently maintained by the vmap implementation in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an implicit
unpin. This prevents eviction in the damage worker. But now I was told than
pinning is only for BOs that are controlled by userspace and internal users
should acquire the resv lock. So vram helpers have to be fixed, actually.

In vram helpers, unmapping does not mean eviction. The unmap operation only
marks the BO as unmappable. The real unmap happens when the eviction takes
place. This avoids many modifications to the page tables. IOW an unpinned,
unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas


So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv locking, but
the issue there is that beyond ttm, none of the helpers (and few of the
drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel


Regards,
Christian.


Best regards
Thomas


There's no recursion taking place, so I guess the reservation
lock could be
acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of
DRM client functions could do the locking.

Given how this "do the right locking" is a can of worms (and I think
it's
worse than what you dug out already) I think the fb_helper.lock hack is
perfectly good enough.

I'm also somewhat worried that

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

2020-11-26 Thread Daniel Vetter

On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann  wrote:
>
> Hi
>
> Am 25.11.20 um 17:32 schrieb Daniel Vetter:
> > [...]
> > I guess full locking is required :-/ I'm not exactly sure how to make this
> > happen with the current plethora of helpers ... I think we need an
> > _locked version of vmap/vunmap callbacks in drm_gem_object_funcs.
>
> I think we might be able to get away without new callbacks.
>
> I looked through the sources that implement and use vmap. All the
> implementations are without taking resv locks. For locking, we can wrap
> them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked()
> that locks and vmaps should make this easy.
>
> In terms of implementation, only vram helpers do a pin+map in their vmap
> code. And as I mentioned before, this is actually wrong. The pattern
> dates back to when the code was still in individual drivers. It's time
> to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.
>
> Finally, there aren't that many users of vmap. A few drivers use it
> while blitting framebuffers into HW buffers and ast does some permanent
> mapping of the cursor BO. All this is trivial to turn into small pairs
> of lock+vmap and vunmap+unlock.
>
> That leaves generic fbdev. The shadow buffer is also trivial to fix, as
> outlined during this discussion.
>
> The code for fbdev in hardware buffers is a special case. It vmaps the
> buffer during initialization and only vunmaps it during shutdown. As
> this has worked so far without locking, I'd leave it as it is and put a
> big comment next to is.
>
> Hardware fbdev buffers is only required by few drivers; namely those
> that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
> We should consider to make the fbdev shadow buffer the default and have
> drivers opt-in for the hardware buffer, if they need it.
>
> >
> > And then document that if the callers of the _locked version wants a
> > permanent mapping, it also needs to pin it. Plus I guess ideally implement
> > the unlocked/permanent versions in terms of that, so that drivers only
> > have to implement one or the other.
>
> For my understanding, pinning is only done in prepare_fb code. And ast
> pins its cursor BOs into vram. We should document to hols vmap/vunmap
> only for time and cover them with resv locks. Pinning is for cases where
> the hardware requires buffers in a special location, but does not
> protect against concurrent threat. I think those are the implicit rules
> already.
>
> I updated the radeon patchset, where all this appears to be working well.

Hm yeah if you want to do the full change, then that works out too.
It's just a pile of work.

But if we can finish off with an dma_resv_assert_locked in
dma_buf_vmap/vunmap, then I think that's ok. It does mean that
exporters must implement vmap caching, or performance will be
terrible. So quite some update for the dma-buf docs.

But if you're willing to do all that conversion of callers, then of
course I'm not stopping you. Not at all, it's great to see that kind
of maze untangled.
-Daniel

>
> Best regards
> Thomas
>
> >
> > That should give us at least some way forward to gradually conver all the
> > drivers and helpers over to dma_resv locking.
> > -Daniel
> >
> >> The pin count is currently maintained by the vmap implementation in vram
> >> helpers. Calling vmap is an implicit pin; calling vunmap is an implicit
> >> unpin. This prevents eviction in the damage worker. But now I was told than
> >> pinning is only for BOs that are controlled by userspace and internal users
> >> should acquire the resv lock. So vram helpers have to be fixed, actually.
> >>
> >> In vram helpers, unmapping does not mean eviction. The unmap operation only
> >> marks the BO as unmappable. The real unmap happens when the eviction takes
> >> place. This avoids many modifications to the page tables. IOW an unpinned,
> >> unmapped BO will remain in VRAM until the memory is actually needed.
> >>
> >> Best regards
> >> Thomas
> >>
> >>>
> >>> So I'm still not seeing how this can go boom.
> >>>
> >>> Now long term it'd be nice to cut everything over to dma_resv locking, but
> >>> the issue there is that beyond ttm, none of the helpers (and few of the
> >>> drivers) use dma_resv. So this is a fairly big uphill battle. Quick
> >>> interim fix seems like the right solution to me.
> >>> -Daniel
> >>>
> 
>  Regards,
>  Christian.
> 
> >
> > Best regards
> > Thomas
> >
> >>
> >>> There's no recursion taking place, so I guess the reservation
> >>> lock could be
> >>> acquired/release in drm_client_buffer_vmap/vunmap(), or a
> >>> separate pair of
> >>> DRM client functions could do the locking.
> >>
> >> Given how this "do the right locking" is a can of worms (and I think
> >> it's
> >> worse than what you dug out already) I think the fb_helper.lock hack is
> >> perfectly good enough.
> >>
> >> I'm also somewhat worried that

Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

Am 25.11.20 um 17:32 schrieb Daniel Vetter:

[...]
I guess full locking is required :-/ I'm not exactly sure how to make this
happen with the current plethora of helpers ... I think we need an
_locked version of vmap/vunmap callbacks in drm_gem_object_funcs.

I think we might be able to get away without new callbacks.

I looked through the sources that implement and use vmap. All the
implementations are without taking resv locks. For locking, we can wrap
them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked()
that locks and vmaps should make this easy.

In terms of implementation, only vram helpers do a pin+map in their vmap
code. And as I mentioned before, this is actually wrong. The pattern
dates back to when the code was still in individual drivers. It's time
to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead.

Finally, there aren't that many users of vmap. A few drivers use it
while blitting framebuffers into HW buffers and ast does some permanent
mapping of the cursor BO. All this is trivial to turn into small pairs

of lock+vmap and vunmap+unlock.

That leaves generic fbdev. The shadow buffer is also trivial to fix, as
outlined during this discussion.

The code for fbdev in hardware buffers is a special case. It vmaps the
buffer during initialization and only vunmaps it during shutdown. As
this has worked so far without locking, I'd leave it as it is and put a
big comment next to is.

Hardware fbdev buffers is only required by few drivers; namely those
that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work.
We should consider to make the fbdev shadow buffer the default and have
drivers opt-in for the hardware buffer, if they need it.

And then document that if the callers of the _locked version wants a
permanent mapping, it also needs to pin it. Plus I guess ideally implement
the unlocked/permanent versions in terms of that, so that drivers only
have to implement one or the other.

For my understanding, pinning is only done in prepare_fb code. And ast
pins its cursor BOs into vram. We should document to hols vmap/vunmap
only for time and cover them with resv locks. Pinning is for cases where
the hardware requires buffers in a special location, but does not
protect against concurrent threat. I think those are the implicit rules
already.

I updated the radeon patchset, where all this appears to be working well.

Best regards
Thomas

That should give us at least some way forward to gradually conver all the
drivers and helpers over to dma_resv locking.
-Daniel

The pin count is currently maintained by the vmap implementation in vram
helpers. Calling vmap is an implicit pin; calling vunmap is an implicit
unpin. This prevents eviction in the damage worker. But now I was told than
pinning is only for BOs that are controlled by userspace and internal users
should acquire the resv lock. So vram helpers have to be fixed, actually.

In vram helpers, unmapping does not mean eviction. The unmap operation only
marks the BO as unmappable. The real unmap happens when the eviction takes
place. This avoids many modifications to the page tables. IOW an unpinned,
unmapped BO will remain in VRAM until the memory is actually needed.

Best regards
Thomas

So I'm still not seeing how this can go boom.

Now long term it'd be nice to cut everything over to dma_resv locking, but
the issue there is that beyond ttm, none of the helpers (and few of the
drivers) use dma_resv. So this is a fairly big uphill battle. Quick
interim fix seems like the right solution to me.
-Daniel

Regards,
Christian.

Best regards
Thomas

There's no recursion taking place, so I guess the reservation
lock could be
acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of
DRM client functions could do the locking.

Given how this "do the right locking" is a can of worms (and I think
it's
worse than what you dug out already) I think the fb_helper.lock hack is
perfectly good enough.

I'm also somewhat worried that starting to use dma_resv lock in generic
code, while many helpers/drivers still have their hand-rolled locking,
will make conversion over to dma_resv needlessly more complicated.
-Daniel

Best regards
Thomas

[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394

Please note that the reservation lock you need to take here is part of
the GEM object.

Usually we design things in the way that the code needs to take a lock
which protects an object, then do some operations with the object and
then release the lock again.

Having in the lock inside the operation can be done as well, but
returning with it is kind of unusual design.

Sorry for the noob questions. I'm still trying to understand the
implications of acquiring these locks.

Well this is the reservation lock of the GEM object we are
talking about
here. We need to take that for a couple of

RE: [PATCH] drm/amdgpu: skip power profile switch in sriov

2020-11-26 Thread Zhao, Jiange

[AMD Public Use]

Reviewed-by: Jiange Zhao 

-Original Message-
From: Jingwen Chen  
Sent: Thursday, November 26, 2020 4:38 PM
To: amd-gfx@lists.freedesktop.org; Zhao, Jiange 
Cc: Chen, JingWen 
Subject: [PATCH] drm/amdgpu: skip power profile switch in sriov

power profile switch in vcn need to send SetWorkLoad msg to smu, which is not 
supported in sriov.

Signed-off-by: Jingwen Chen 
---
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
index 17a45baff638..8fb12afe3c96 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
@@ -1168,6 +1168,9 @@ int amdgpu_dpm_switch_power_profile(struct amdgpu_device 
*adev,  {
int ret = 0;
 
+   if (amdgpu_sriov_vf(adev))
+   return 0;
+
if (is_support_sw_smu(adev))
ret = smu_switch_power_profile(>smu, type, en);
else if (adev->powerplay.pp_funcs &&
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: skip power profile switch in sriov

2020-11-26 Thread Jingwen Chen

power profile switch in vcn need to send SetWorkLoad msg to
smu, which is not supported in sriov.

Signed-off-by: Jingwen Chen 
---
 drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
index 17a45baff638..8fb12afe3c96 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
@@ -1168,6 +1168,9 @@ int amdgpu_dpm_switch_power_profile(struct amdgpu_device 
*adev,
 {
int ret = 0;
 
+   if (amdgpu_sriov_vf(adev))
+   return 0;
+
if (is_support_sw_smu(adev))
ret = smu_switch_power_profile(>smu, type, en);
else if (adev->powerplay.pp_funcs &&
-- 
2.25.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Finn Thain

On Wed, 25 Nov 2020, Nick Desaulniers wrote:

> On Wed, Nov 25, 2020 at 1:33 PM Finn Thain  wrote:
> >
> > Or do you think that a codebase can somehow satisfy multiple checkers 
> > and their divergent interpretations of the language spec?
> 
> Have we found any cases yet that are divergent? I don't think so. 

You mean, aside from -Wimplicit-fallthrough? I'm glad you asked. How about 
-Wincompatible-pointer-types and -Wframe-larger-than?

All of the following files have been affected by divergent diagnostics 
produced by clang and gcc.

arch/arm64/include/asm/neon-intrinsics.h
arch/powerpc/xmon/Makefile
drivers/gpu/drm/i915/Makefile
drivers/gpu/drm/i915/i915_utils.h
drivers/staging/media/atomisp/pci/atomisp_subdev.c
fs/ext4/super.c
include/trace/events/qla.h
net/mac80211/rate.c
tools/lib/string.c
tools/perf/util/setup.py
tools/scripts/Makefile.include

And if I searched for 'smatch' or 'coverity' instead of 'clang' I'd 
probably find more divergence.

Here are some of the relevant commits.

0738c8b5915c7eaf1e6007b441008e8f3b460443
9c87156cce5a63735d1218f0096a65c50a7a32aa
babaab2f473817f173a2d08e410c25abf5ed0f6b
065e5e559555e2f100bc95792a8ef1b609bbe130
93f56de259376d7e4fff2b2d104082e1fa66e237
6c4798d3f08b81c2c52936b10e0fa872590c96ae
b7a313d84e853049062011d78cb04b6decd12f5c
093b75ef5995ea35d7f6bdb6c7b32a42a1999813

And before you object, "but -Wconstant-logical-operand is a clang-only 
warning! it can't be divergent with gcc!", consider that the special cases 
added to deal with clang-only warnings have to be removed when gcc catches 
up, which is more churn. Now multiply that by the number of checkers you 
care about.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Finn Thain

On Wed, 25 Nov 2020, Nick Desaulniers wrote:

> On Wed, Nov 25, 2020 at 1:33 PM Finn Thain  
> wrote:
> >
> > Or do you think that a codebase can somehow satisfy multiple checkers 
> > and their divergent interpretations of the language spec?
> 
> Have we found any cases yet that are divergent? I don't think so.

There are many implementations, so I think you are guaranteed to find more 
divergence if you look. That's because the spec is full of language like 
this: "implementations are encouraged not to emit a diagnostic" and 
"implementations are encouraged to issue a diagnostic".

Some implementations will decide to not emit (under the premise that vast 
amounts of existing code would have to get patched until the compiler goes 
quiet) whereas other implementations will decide to emit (under the 
premise that the author is doing the checking and not the janitor or the 
packager).

> It sounds to me like GCC's cases it warns for is a subset of Clang's. 
> Having additional coverage with Clang then should ensure coverage for 
> both.
> 

If that claim were true, the solution would be simple. (It's not.)

For the benefit of projects that enable -Werror and projects that 
nominated gcc as their preferred compiler, clang would simply need a flag 
to enable conformance with gcc by suppressing those additional warnings 
that clang would normally produce.

This simple solution is, of course, completely unworkable, since it would 
force clang to copy some portion of gcc's logic (rewritten under LLVM's 
unique license) and then to track future changes to that portion of gcc 
indefinitely.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Edward Cree

On 24/11/2020 21:25, Kees Cook wrote:
> I still think this isn't right -- it's a case statement that runs off
> the end without an explicit flow control determination.

Proves too much — for instance
case foo:
case bar:
thing;
break;
 doesn't require a fallthrough; after case foo:, and afaik
 no-one is suggesting it should.  Yet it, too, is "a case
 statement that runs off the end without an explicit flow
 control determination".

-ed
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Kees Cook

On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote:
> Now, what we have seems to be about 6 cases (at least what's been shown
> in this thread) where a missing break would cause potentially user
> visible issues.  That means the value of this isn't zero, but it's not
> a no-brainer massive win either.  That's why I think asking what we've
> invested vs the return isn't a useless exercise.

The number is much higher[1]. If it were 6 in the entire history of the
kernel, I would agree with you. :) Some were fixed _before_ Gustavo's
effort too, which I also count towards the idea of "this is a dangerous
weakness in C, and now we have stopped it forever."

> But the broader point I'm making is just because the compiler people
> come up with a shiny new warning doesn't necessarily mean the problem
> it's detecting is one that causes us actual problems in the code base. 
> I'd really be happier if we had a theory about what classes of CVE or
> bug we could eliminate before we embrace the next new warning.

But we did! It was long ago justified and documented[2], and even links to
the CWE[3] for it. This wasn't random joy over discovering a new warning
we could turn on, this was turning on a warning that the compiler folks
finally gave us to handle an entire class of flaws. If we need to update
the code-base to address it not a useful debate -- that was settled
already, even if you're only discovering it now. :P. This last patch
set is about finishing that work for Clang, which is correctly even
more strict than GCC.

-Kees

[1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific
numbers (about 6.5% of the patches fixed missing breaks):
v4.19:  3 of 129
v4.20:  2 of  59
v5.0:   3 of  56
v5.1:  10 of 100
v5.2:   6 of  71
v5.3:   7 of  69

And in the history of the kernel, it's been an ongoing source of
flaws:

$ l --no-merges | grep -i 'missing break' | wc -l
185

The frequency of such errors being "naturally" found was pretty
steady until the static checkers started warning, and then it was
on the rise, but the full effort flushed the rest out, and now it's
dropped to almost zero:

  1 v2.6.12
  3 v2.6.16.28
  1 v2.6.17
  1 v2.6.19
  2 v2.6.21
  1 v2.6.22
  3 v2.6.24
  3 v2.6.29
  1 v2.6.32
  1 v2.6.33
  1 v2.6.35
  4 v2.6.36
  3 v2.6.38
  2 v2.6.39
  7 v3.0
  2 v3.1
  2 v3.2
  2 v3.3
  3 v3.4
  1 v3.5
  8 v3.6
  7 v3.7
  3 v3.8
  6 v3.9
  3 v3.10
  2 v3.11
  5 v3.12
  5 v3.13
  2 v3.14
  4 v3.15
  2 v3.16
  3 v3.17
  2 v3.18
  2 v3.19
  1 v4.0
  2 v4.1
  5 v4.2
  4 v4.5
  5 v4.7
  6 v4.8
  1 v4.9
  3 v4.10
  2 v4.11
  6 v4.12
  3 v4.13
  2 v4.14
  5 v4.15
  2 v4.16
  7 v4.18
  2 v4.19
  6 v4.20
  3 v5.0
 12 v5.1
  3 v5.2
  4 v5.3
  2 v5.4
  1 v5.8

And the reason it's fully zero, is because we still have the cases we're
cleaning up right now. Even this last one from v5.8 is specifically of
the same type this series addresses:

case 4:
color_index = TrueCModeIndex;
+   break;
default:
return;
}

[2] 
https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through

All switch/case blocks must end in one of:

break;
fallthrough;
continue;
goto ;
return [expression];

[3] https://cwe.mitre.org/data/definitions/484.html

-- 
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Finn Thain

On Wed, 25 Nov 2020, Nick Desaulniers wrote:

> So developers and distributions using Clang can't have 
> -Wimplicit-fallthrough enabled because GCC is less strict (which has 
> been shown in this thread to lead to bugs)?  We'd like to have nice 
> things too, you know.
> 

Apparently the GCC developers don't want you to have "nice things" either. 
Do you think that the kernel should drop gcc in favour of clang?
Or do you think that a codebase can somehow satisfy multiple checkers and 
their divergent interpretations of the language spec?

> This is not a shiny new warning; it's already on for GCC and has existed 
> in both compilers for multiple releases.
> 

Perhaps you're referring to the compiler feature that lead to the 
ill-fated, tree-wide /* fallthrough */ patch series.

When the ink dries on the C23 language spec and the implementations figure 
out how to interpret it then sure, enforce the warning for new code -- the 
cost/benefit analysis is straight forward. However, the case for patching 
existing mature code is another story.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Edward Cree

On 25/11/2020 00:32, Miguel Ojeda wrote:
> I have said *authoring* lines of *this* kind takes a minute per line.
> Specifically: lines fixing the fallthrough warning mechanically and
> repeatedly where the compiler tells you to, and doing so full-time for
> a month.

> It is useful since it makes intent clear.
To make the intent clear, you have to first be certain that you
 understand the intent; otherwise by adding either a break or a
 fallthrough to suppress the warning you are just destroying the
 information that "the intent of this code is unknown".
Figuring out the intent of a piece of unfamiliar code takes more
 than 1 minute; just because
case foo:
thing;
case bar:
break;
 produces identical code to
case foo:
thing;
break;
case bar:
break;
 doesn't mean that *either* is correct — maybe the author meant
 to write
case foo:
return thing;
case bar:
break;
 and by inserting that break you've destroyed the marker that
 would direct someone who knew what the code was about to look
 at that point in the code and spot the problem.
Thus, you *always* have to look at more than just the immediate
 mechanical context of the code, to make a proper judgement that
 yes, this was the intent.  If you think that that sort of thing
 can be done in an *average* time of one minute, then I hope you
 stay away from code I'm responsible for!
One minute would be an optimistic target for code that, as the
 maintainer, one is already somewhat familiar with.  For code
 that you're seeing for the first time, as is usually the case
 with the people doing these mechanical fix-a-warning patches,
 it's completely unrealistic.

A warning is only useful because it makes you *think* about the
 code.  If you suppress the warning without doing that thinking,
 then you made the warning useless; and if the warning made you
 think about code that didn't *need* it, then the warning was
 useless from the start.

So make your mind up: does Clang's stricter -Wimplicit-fallthrough
 flag up code that needs thought (in which case the fixes take
 effort both to author and to review) or does it flag up code
 that can be mindlessly "fixed" (in which case the warning is
 worthless)?  Proponents in this thread seem to be trying to
 have it both ways.

-ed
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-26 Thread Miguel Ojeda

On Wed, Nov 25, 2020 at 5:24 PM Jakub Kicinski  wrote:
>
> And just to spell it out,
>
> case ENUM_VALUE1:
> bla();
> break;
> case ENUM_VALUE2:
> bla();
> default:
> break;
>
> is a fairly idiomatic way of indicating that not all values of the enum
> are expected to be handled by the switch statement.

It looks like a benign typo to me -- `ENUM_VALUE2` does not follow the
same pattern like `ENUM_VALUE1`. To me, the presence of the `default`
is what indicates (explicitly) that not everything is handled.

> Applying a real patch set and then getting a few follow ups the next day
> for trivial coding things like fallthrough missing or static missing,
> just because I didn't have the full range of compilers to check with
> before applying makes me feel pretty shitty, like I'm not doing a good
> job. YMMV.

The number of compilers, checkers, static analyzers, tests, etc. we
use keeps going up. That, indeed, means maintainers will miss more
things (unless maintainers do more work than before). But catching
bugs before they happen is *not* a bad thing.

Perhaps we could encourage more rebasing in -next (while still giving
credit to bots and testers) to avoid having many fixing commits
afterwards, but that is orthogonal.

I really don't think we should encourage the feeling that a maintainer
is doing a bad job if they don't catch everything on their reviews.
Any review is worth it. Maintainers, in the end, are just the
"guaranteed" reviewers that decide when the code looks reasonable
enough. They should definitely not feel pressured to be perfect.

Cheers,
Miguel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/6] drm/amdgpu: Don't hardcode thread name length