[Freedreno] [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging
maitreye at codeaurora.org
maitreye at codeaurora.org
Fri Jul 23 03:53:37 UTC 2021
On 2021-07-22 15:09, Stephen Boyd wrote:
Thank you for the comments .
> Quoting maitreye at 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 at 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 at 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/dp/dp_link.c#L972
>> , 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. 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. But if these issues seem like they can be ignored ,
then we can go ahead with your suggestions.
More information about the Freedreno
mailing list