| Message ID | 20190206192443.9649-1-dan.streetman@canonical.com |
|---|---|
| Headers | show |
| Series | amdgpu with mst WARNING on blanking | expand |
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(-) >
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(-) > > > >
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(-)