[0/3,SRU,Bionic] amdgpu with mst WARNING on blanking
mbox series

Message ID 20190206192443.9649-1-dan.streetman@canonical.com
Headers show
Series
  • amdgpu with mst WARNING on blanking
Related show

Message

Dan Streetman Feb. 6, 2019, 7:24 p.m. UTC
From: Dan Streetman <ddstreet@canonical.com>

This series fixes amdgpu screen blanking when using displayport monitors
in MST configuration (i.e. displayport "daisy chaining").

In Cosmic, only the 3rd patch is needed (first 2 are already included),
and I sent that separately.

Charlene Liu (1):
  drm/amd/display: eDP sequence BL off first then DP blank.

Jerry (Fangzhi) Zuo (1):
  drm/amd/display: Fix MST dp_blank REG_WAIT timeout

Yongqiang Sun (1):
  drm/amd/display: Move wait for hpd ready out from edp power control.

 drivers/gpu/drm/amd/display/dc/core/dc_link.c | 24 ++++++++++++--
 .../drm/amd/display/dc/core/dc_link_hwss.c    | 11 +------
 .../drm/amd/display/dc/dce/dce_link_encoder.c | 17 ++--------
 .../display/dc/dce110/dce110_hw_sequencer.c   | 33 +++++++++++++------
 .../display/dc/dce110/dce110_hw_sequencer.h   |  5 +++
 .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  4 ++-
 .../gpu/drm/amd/display/dc/inc/hw_sequencer.h |  3 ++
 7 files changed, 60 insertions(+), 37 deletions(-)

Comments

Stefan Bader Feb. 7, 2019, 9:50 a.m. UTC | #1
On 06.02.19 20:24, Dan Streetman wrote:
> From: Dan Streetman <ddstreet@canonical.com>
> 
> This series fixes amdgpu screen blanking when using displayport monitors
> in MST configuration (i.e. displayport "daisy chaining").
> 
> In Cosmic, only the 3rd patch is needed (first 2 are already included),
> and I sent that separately.

For the backports (especially since they consist of some delta) you should add
some hinting:

(backported from ...
[Context adjustments [and...]]

Generally I am a bit concerned about those pre-reqs since they modify some
"core" parts and I cannot clearly say which other drivers base on this. Do those
changes require modifications to those other drivers?
At least there were no follow-up commits as far as I can tell.
> 
> Charlene Liu (1):
>   drm/amd/display: eDP sequence BL off first then DP blank.

Hunks like below (from above patch) make me doubt the upstream quality:

@@ -2309,7 +2308,6 @@ void core_link_enable_stream(
        if (pipe_ctx->stream->signal == SIGNAL_TYPE_DISPLAY_PORT_MST)
                allocate_mst_payload(pipe_ctx);

-       if (dc_is_dp_signal(pipe_ctx->stream->signal))
                core_dc->hwss.unblank_stream(pipe_ctx,
                        &pipe_ctx->stream->sink->link->cur_link_settings);
 }

Not changing the indentation level of the unblank call make it totally unclear
whether the if case before needs {} or not.

> 
> Jerry (Fangzhi) Zuo (1):
>   drm/amd/display: Fix MST dp_blank REG_WAIT timeout

Here "cherry picked" again instead of cherry-picked.

Could you follow-up with explanations about the backports (how much needed
changing), so we could fix up the commits.

-Stefan
> 
> Yongqiang Sun (1):
>   drm/amd/display: Move wait for hpd ready out from edp power control.
> 
>  drivers/gpu/drm/amd/display/dc/core/dc_link.c | 24 ++++++++++++--
>  .../drm/amd/display/dc/core/dc_link_hwss.c    | 11 +------
>  .../drm/amd/display/dc/dce/dce_link_encoder.c | 17 ++--------
>  .../display/dc/dce110/dce110_hw_sequencer.c   | 33 +++++++++++++------
>  .../display/dc/dce110/dce110_hw_sequencer.h   |  5 +++
>  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  4 ++-
>  .../gpu/drm/amd/display/dc/inc/hw_sequencer.h |  3 ++
>  7 files changed, 60 insertions(+), 37 deletions(-)
>
Dan Streetman Feb. 7, 2019, 5:54 p.m. UTC | #2
On Thu, Feb 7, 2019 at 4:50 AM Stefan Bader <stefan.bader@canonical.com> wrote:
>
> On 06.02.19 20:24, Dan Streetman wrote:
> > From: Dan Streetman <ddstreet@canonical.com>
> >
> > This series fixes amdgpu screen blanking when using displayport monitors
> > in MST configuration (i.e. displayport "daisy chaining").
> >
> > In Cosmic, only the 3rd patch is needed (first 2 are already included),
> > and I sent that separately.
>
> For the backports (especially since they consist of some delta) you should add
> some hinting:
>
> (backported from ...
> [Context adjustments [and...]]

ack, will resend with hinting

>
> Generally I am a bit concerned about those pre-reqs since they modify some
> "core" parts and I cannot clearly say which other drivers base on this. Do those
> changes require modifications to those other drivers?
> At least there were no follow-up commits as far as I can tell.
> >
> > Charlene Liu (1):
> >   drm/amd/display: eDP sequence BL off first then DP blank.
>
> Hunks like below (from above patch) make me doubt the upstream quality:
>
> @@ -2309,7 +2308,6 @@ void core_link_enable_stream(
>         if (pipe_ctx->stream->signal == SIGNAL_TYPE_DISPLAY_PORT_MST)
>                 allocate_mst_payload(pipe_ctx);
>
> -       if (dc_is_dp_signal(pipe_ctx->stream->signal))
>                 core_dc->hwss.unblank_stream(pipe_ctx,
>                         &pipe_ctx->stream->sink->link->cur_link_settings);
>  }
>
> Not changing the indentation level of the unblank call make it totally unclear
> whether the if case before needs {} or not.
>

indeed, i noticed that as well (and the build does emit a warning
basically saying 'do you really mean for this to not be inside the if
block?')

however i checked later commits and the alignment is fixed in commit
f3b72c7b00bd36773005e1bfea6b2bb558eb254f.

But I agree very much, it's sloppy and makes me question how closely
the upstream patches for the amdgpu driver were really reviewed.

> >
> > Jerry (Fangzhi) Zuo (1):
> >   drm/amd/display: Fix MST dp_blank REG_WAIT timeout
>
> Here "cherry picked" again instead of cherry-picked.

sorry, will fix in resend.

>
> Could you follow-up with explanations about the backports (how much needed
> changing), so we could fix up the commits.
>
> -Stefan
> >
> > Yongqiang Sun (1):
> >   drm/amd/display: Move wait for hpd ready out from edp power control.
> >
> >  drivers/gpu/drm/amd/display/dc/core/dc_link.c | 24 ++++++++++++--
> >  .../drm/amd/display/dc/core/dc_link_hwss.c    | 11 +------
> >  .../drm/amd/display/dc/dce/dce_link_encoder.c | 17 ++--------
> >  .../display/dc/dce110/dce110_hw_sequencer.c   | 33 +++++++++++++------
> >  .../display/dc/dce110/dce110_hw_sequencer.h   |  5 +++
> >  .../amd/display/dc/dcn10/dcn10_hw_sequencer.c |  4 ++-
> >  .../gpu/drm/amd/display/dc/inc/hw_sequencer.h |  3 ++
> >  7 files changed, 60 insertions(+), 37 deletions(-)
> >
>
>