Quoting maitreye@codeaurora.org (2021-07-22 20:53:37)
On 2021-07-22 15:09, Stephen Boyd wrote: Thank you for the comments .
Quoting maitreye@codeaurora.org (2021-07-22 14:33:43)
Thank you Stephen.
On 2021-07-22 13:31, Stephen Boyd wrote:
Quoting maitreye (2021-07-21 16:19:40)
From: Maitreyee Rao maitreye@codeaurora.org
Add trace points across the MSM DP driver to help debug interop issues.
Changes in v4:
- Changed goto statement and used if else-if
I think drm likes to see all the changelog here to see patch evolution.
Yes, I will fix this
Signed-off-by: Maitreyee Rao maitreye@codeaurora.org
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c index be986da..8c98ab7 100644 --- a/drivers/gpu/drm/msm/dp/dp_link.c +++ b/drivers/gpu/drm/msm/dp/dp_link.c @@ -1036,43 +1036,28 @@ int dp_link_process_request(struct dp_link *dp_link)
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) { dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
return ret; }ret = dp_link_process_ds_port_status_change(link);if (!ret) {
else if (!(ret = dp_link_process_ds_port_status_change(link))){ dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
return ret; }ret = dp_link_process_link_training_request(link);if (!ret) {
else if (!(ret = dp_link_process_link_training_request(link))){ dp_link->sink_request |= DP_TEST_LINK_TRAINING;
return ret; }ret = dp_link_process_phy_test_pattern_request(link);if (!ret) {
else if (!(ret =dp_link_process_phy_test_pattern_request(link))) { dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
return ret;}ret = dp_link_process_link_status_update(link);if (!ret) {
}else if (!(ret = dp_link_process_link_status_update(link))) {The kernel coding style is to leave the brackets on the same line
if (condition) { } else if (conditon) { }See Documentation/process/coding-style.rst
Yes, I will fix this
Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe apply this patch before?
----8<---- diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c index 1195044a7a3b..408cddd90f0f 100644 --- a/drivers/gpu/drm/msm/dp/dp_link.c +++ b/drivers/gpu/drm/msm/dp/dp_link.c @@ -1027,41 +1027,22 @@ int dp_link_process_request(struct dp_link *dp_link)
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) { dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
return ret;}ret = dp_link_process_ds_port_status_change(link);if (!ret) {
} else if (!dp_link_process_ds_port_status_change(link)) { dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
return ret;}ret = dp_link_process_link_training_request(link);if (!ret) {
} else if (!dp_link_process_link_training_request(link)) { dp_link->sink_request |= DP_TEST_LINK_TRAINING;
return ret;}ret = dp_link_process_phy_test_pattern_request(link);if (!ret) {
} else if (!dp_link_process_phy_test_pattern_request(link)) { dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
return ret;}ret = dp_link_process_link_status_update(link);if (!ret) {
} else if (!dp_link_process_link_status_update(link)) { dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
return ret;}if (dp_link_is_video_pattern_requested(link)) {ret = 0;dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;}
} else {if (dp_link_is_video_pattern_requested(link))dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
if (dp_link_is_audio_pattern_requested(link)) {dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;return -EINVAL;
if (dp_link_is_audio_pattern_requested(link)) {dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;ret = -EINVAL;} } return ret;The reason I did this was to preserve the value of ret as the caller of the function checks it. Some functions return -EINVAl like in here: https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d... , so to check that it would be necessary to get the ret value.
ret is overwritten multiple times. The logic seems to be if ret is not-zero, reassign it, until we get to the end. How about this
----8<---- diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c index 1195044a7a3b..e59138566c0a 100644 --- a/drivers/gpu/drm/msm/dp/dp_link.c +++ b/drivers/gpu/drm/msm/dp/dp_link.c @@ -1027,41 +1027,27 @@ int dp_link_process_request(struct dp_link *dp_link)
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) { dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
return ret;}ret = dp_link_process_ds_port_status_change(link);if (!ret) {
} else if (!dp_link_process_ds_port_status_change(link)) { dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
return ret;}ret = dp_link_process_link_training_request(link);if (!ret) {
} else if (!dp_link_process_link_training_request(link)) { dp_link->sink_request |= DP_TEST_LINK_TRAINING;
return ret;}ret = dp_link_process_phy_test_pattern_request(link);if (!ret) {
} else if (!dp_link_process_phy_test_pattern_request(link)) { dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
return ret;}ret = dp_link_process_link_status_update(link);if (!ret) {dp_link->sink_request |= DP_LINK_STATUS_UPDATED;return ret;}if (dp_link_is_video_pattern_requested(link)) {ret = 0;dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;}if (dp_link_is_audio_pattern_requested(link)) {dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;return -EINVAL;
} else {ret = dp_link_process_link_status_update(link);if (!ret) {dp_link->sink_request |= DP_LINK_STATUS_UPDATED;} else {if (dp_link_is_video_pattern_requested(link)) {ret = 0;dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;}if (dp_link_is_audio_pattern_requested(link)) {dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;ret = -EINVAL;}} } return ret;I do agree, this will solve the issue for over writing ret variable. But I can see two potential problems with this, if we get two events at the same time or one of the other we might end up processing the two events, which won't be the expected behavior.
The code isn't written to handle this. Does it need to handle it? If so, please update it for that new feature in a different patch than this one that adds logging.
For example, if we get dp_link_process_link_status_update and it gives a non zero result and we go to the else statement, and get the event for video pattern request, we will end up processing two events.
The second problem, is currently we only have APIs that return 0 or EINVAL as return values, but if we ever in future add an API with a different return value it wont be easy to add the behavior.
Agreed. Fortunately it's just code and it can easily be rewritten in the future when it changes.
But if these issues seem like they can be ignored , then we can go ahead with your suggestions.
I think they can be ignored if we're not supposed to handle two events at the same time.