| Message ID | 20191001161721.13793-14-mikita.lipski@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSC MST support for AMDGPU | expand |
Ok, let's stop and slow down for a minute here since I've repeated myself a few times, and I'd like to figure out what's actually happening here and whether I'm not being clear enough with my explanations, or if there's just some misunderstanding here. I'll start of by trying my best to explain my hesistance in accepting these patches. To be clear, MST with DSC is a big thing in terms of impact. There's a lot of things to consider: * What should the default DSC policy for MST be? Should we keep it on by default so that we don't need to trigger a large number of PBN recalculations and enable DSC on a per-case basis, or are there legitimate reasons to keep DSC off by default (such as potential issues with lossiness down the line). * We have other drivers in the tree that are planning on implementing MST DSC quite soon. Chances are they'll be implementing helpers to do this so this can be supported across the DRM tree, and chances are we'll just have to rewrite and remove most of this code in that case once they do. * amdgpu already has a _lot_ of code that doesn't need to, and shouldn't be there. This ranges from various DC specific helpers that are essentially the same as the helpers that we already implement in the rest of DRM, to misuses of various kernel APIs, and a complete lack of any kind of code style (at least the last time that I checked) in or out of DC. This makes the driver more difficult for people who don't work at AMD but are well accustomed to working on the rest of the kernel, e.g. myself and others, and it's a problem that seriously needs to be solved. Adding more redundant DP helpers that will inevitably get removed is just making the problem worse. With all of this being said: I've been asking the same questions about this patch throughout the whole patch series so far (since it got passed off to you from David's internship ending): * Can we keep DSC enabled by default, so that these patches aren't needed? * If we can't, can we move this into common code so that other drivers can use it? The problem is I haven't received any responses to these questions, other then being told by David a month or two ago that it would be expedient to just accept the patches and move on. Honestly, if discussion had been had about the changes I requested, I would have given my r-b a long time ago. In fact-I really want to give it now! There's multiple patches in this series that would be very useful for other things that are being worked on at the moment, such as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really think this needs to be discussed first, and I'm worried that just going ahead and giving my R-B before they have been discussed will further encourage rushing changes like this in the future and continually building more tech debt for ourselves. Please note as well: if anything I've asked for is confusing, or you don't understand what I'm asking or looking for I am more then willing to help explain things and help out as best as I can. I understand that a lot of the developers working on DRM at AMD may have more experience in Windows and Mac land and as a result, trying to get used to the way that we do things in the Linux kernel can be a confusing endeavor. I'm more then happy to help out with this wherever I can, all you need to do is ask. Asking a few questions about something you aren't sure you understand can save both of us a lot of time, and avoid having to go through this many patch respins. In the mean time, I'd be willing to look at what patches from this series that have already been reviewed which could be pushed to drm-misc or friends in the mean time to speed things up a bit. On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@amd.com wrote: > From: Mikita Lipski <mikita.lipski@amd.com> > > Since for DSC MST connector's PBN is claculated differently > due to compression, we have to recalculate both PBN and > VCPI slots for that connector. > > This patch proposes to use similair logic as in > dm_encoder_helper_atomic_chek, but since we do not know which > connectors will have DSC enabled we have to recalculate PBN only > after that's determined, which is done in > compute_mst_dsc_configs_for_state. > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 +++++++++++++++++-- > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 -- > 2 files changed, 59 insertions(+), 11 deletions(-) > > 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 81e30918f9ec..7501ce2233ed 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct > drm_encoder *encoder) > > } > > +static int convert_dc_color_depth_into_bpc (enum dc_color_depth > display_color_depth) > +{ > + switch (display_color_depth) { > + case COLOR_DEPTH_666: > + return 6; > + case COLOR_DEPTH_888: > + return 8; > + case COLOR_DEPTH_101010: > + return 10; > + case COLOR_DEPTH_121212: > + return 12; > + case COLOR_DEPTH_141414: > + return 14; > + case COLOR_DEPTH_161616: > + return 16; > + default: > + break; > + } > + return 0; > +} > + > static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > struct drm_crtc_state *crtc_state, > struct drm_connector_state > *conn_state) > @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs > amdgpu_dm_encoder_helper_funcs = { > .atomic_check = dm_encoder_helper_atomic_check > }; > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, > + struct dc_state *dc_state) > +{ > + struct dc_stream_state *stream; > + struct amdgpu_dm_connector *aconnector; > + int i, clock = 0, bpp = 0; > + > + for (i = 0; i < dc_state->stream_count; i++) { > + stream = dc_state->streams[i]; > + aconnector = (struct amdgpu_dm_connector *)stream- > >dm_stream_context; > + > + if (stream && stream->timing.flags.DSC == 1) { > + bpp = convert_dc_color_depth_into_bpc(stream- > >timing.display_color_depth)* 3; > + clock = stream->timing.pix_clk_100hz / 10; > + > + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp, > true); > + > + aconnector->vcpi_slots = > drm_dp_atomic_find_vcpi_slots(state, > + &aconnector- > >mst_port->mst_mgr, > + aconnector->port, > + aconnector->pbn); > + if (aconnector->vcpi_slots < 0) > + return aconnector->vcpi_slots; > + } > + } > + return 0; > +} > +#endif > + > static void dm_drm_plane_reset(struct drm_plane *plane) > { > struct dm_plane_state *amdgpu_state = NULL; > @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct drm_device > *dev, > if (ret) > goto fail; > > - /* Perform validation of MST topology in the state*/ > - ret = drm_dp_mst_atomic_check(state); > - if (ret) > - goto fail; > - > if (state->legacy_cursor_update) { > /* > * This is a fast cursor update coming from the plane update > @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct drm_device > *dev, > #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > if (!compute_mst_dsc_configs_for_state(dm_state->context)) > goto fail; > + > + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state- > >context); > + if (ret) > + goto fail; > #endif > if (dc_validate_global_state(dc, dm_state->context, false) != > DC_OK) { > ret = -EINVAL; > @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct drm_device > *dev, > dc_retain_state(old_dm_state->context); > } > } > + /* Perform validation of MST topology in the state*/ > + ret = drm_dp_mst_atomic_check(state); > + if (ret) > + goto fail; > > /* Store the overall update type for use later in atomic check. */ > for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > 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 bd694499e3de..3e44e2da2d0d 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 > @@ -201,12 +201,6 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( > mst_port = aconnector->port; > > if (enable) { > - > - /* Convert kilobits per second / 64 (for 64 timeslots) to pbn > (54/64 megabytes per second) */ > - pbn_per_timeslot = dc_link_bandwidth_kbps( > - stream->link, dc_link_get_link_cap(stream- > >link)) / (8 * 1000 * 54); > - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, > pbn_per_timeslot); > - > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, > aconnector->pbn, > aconnector->vcpi_slots);
... yikes I need to apologize because I was going through my email and I realized you _did_ respond to me earlier regarding some of these questions, it just appears the reply fell through the cracks and somehow I didn't notice it until just now. Sorry about that! I'm still not sure I understand why this is a future enhancement? Everything we need to implement these helpers should already be here, it's just a matter of keeping track who has dsc enabled where in the mst atomic state On Mon, 2019-10-07 at 17:52 -0400, Lyude Paul wrote: > Ok, let's stop and slow down for a minute here since I've repeated myself a > few times, and I'd like to figure out what's actually happening here and > whether I'm not being clear enough with my explanations, or if there's just > some misunderstanding here. > > I'll start of by trying my best to explain my hesistance in accepting these > patches. To be clear, MST with DSC is a big thing in terms of impact. > There's > a lot of things to consider: > * What should the default DSC policy for MST be? Should we keep it on by > default so that we don't need to trigger a large number of PBN > recalculations and enable DSC on a per-case basis, or are there > legitimate > reasons to keep DSC off by default (such as potential issues with > lossiness > down the line). > * We have other drivers in the tree that are planning on implementing MST > DSC > quite soon. Chances are they'll be implementing helpers to do this so > this > can be supported across the DRM tree, and chances are we'll just have to > rewrite and remove most of this code in that case once they do. > * amdgpu already has a _lot_ of code that doesn't need to, and shouldn't be > there. This ranges from various DC specific helpers that are essentially > the same as the helpers that we already implement in the rest of DRM, to > misuses of various kernel APIs, and a complete lack of any kind of code > style (at least the last time that I checked) in or out of DC. This makes > the driver more difficult for people who don't work at AMD but are well > accustomed to working on the rest of the kernel, e.g. myself and others, > and it's a problem that seriously needs to be solved. Adding more > redundant > DP helpers that will inevitably get removed is just making the problem > worse. > > With all of this being said: I've been asking the same questions about this > patch throughout the whole patch series so far (since it got passed off to > you > from David's internship ending): > > * Can we keep DSC enabled by default, so that these patches aren't needed? > * If we can't, can we move this into common code so that other drivers can > use it? > The problem is I haven't received any responses to these questions, other > then > being told by David a month or two ago that it would be expedient to just > accept the patches and move on. Honestly, if discussion had been had about > the > changes I requested, I would have given my r-b a long time ago. In fact-I > really want to give it now! There's multiple patches in this series that > would > be very useful for other things that are being worked on at the moment, such > as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really think > this needs to be discussed first, and I'm worried that just going ahead and > giving my R-B before they have been discussed will further encourage rushing > changes like this in the future and continually building more tech debt for > ourselves. > > Please note as well: if anything I've asked for is confusing, or you don't > understand what I'm asking or looking for I am more then willing to help > explain things and help out as best as I can. I understand that a lot of the > developers working on DRM at AMD may have more experience in Windows and Mac > land and as a result, trying to get used to the way that we do things in the > Linux kernel can be a confusing endeavor. I'm more then happy to help out > with > this wherever I can, all you need to do is ask. Asking a few questions about > something you aren't sure you understand can save both of us a lot of time, > and avoid having to go through this many patch respins. > > In the mean time, I'd be willing to look at what patches from this series > that > have already been reviewed which could be pushed to drm-misc or friends in > the > mean time to speed things up a bit. > > On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@amd.com wrote: > > From: Mikita Lipski <mikita.lipski@amd.com> > > > > Since for DSC MST connector's PBN is claculated differently > > due to compression, we have to recalculate both PBN and > > VCPI slots for that connector. > > > > This patch proposes to use similair logic as in > > dm_encoder_helper_atomic_chek, but since we do not know which > > connectors will have DSC enabled we have to recalculate PBN only > > after that's determined, which is done in > > compute_mst_dsc_configs_for_state. > > > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > > Cc: Harry Wentland <harry.wentland@amd.com> > > Cc: Lyude Paul <lyude@redhat.com> > > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > > --- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 +++++++++++++++++-- > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 -- > > 2 files changed, 59 insertions(+), 11 deletions(-) > > > > 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 81e30918f9ec..7501ce2233ed 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct > > drm_encoder *encoder) > > > > } > > > > +static int convert_dc_color_depth_into_bpc (enum dc_color_depth > > display_color_depth) > > +{ > > + switch (display_color_depth) { > > + case COLOR_DEPTH_666: > > + return 6; > > + case COLOR_DEPTH_888: > > + return 8; > > + case COLOR_DEPTH_101010: > > + return 10; > > + case COLOR_DEPTH_121212: > > + return 12; > > + case COLOR_DEPTH_141414: > > + return 14; > > + case COLOR_DEPTH_161616: > > + return 16; > > + default: > > + break; > > + } > > + return 0; > > +} > > + > > static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > > struct drm_crtc_state *crtc_state, > > struct drm_connector_state > > *conn_state) > > @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs > > amdgpu_dm_encoder_helper_funcs = { > > .atomic_check = dm_encoder_helper_atomic_check > > }; > > > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state > > *state, > > + struct dc_state *dc_state) > > +{ > > + struct dc_stream_state *stream; > > + struct amdgpu_dm_connector *aconnector; > > + int i, clock = 0, bpp = 0; > > + > > + for (i = 0; i < dc_state->stream_count; i++) { > > + stream = dc_state->streams[i]; > > + aconnector = (struct amdgpu_dm_connector *)stream- > > > dm_stream_context; > > + > > + if (stream && stream->timing.flags.DSC == 1) { > > + bpp = convert_dc_color_depth_into_bpc(stream- > > > timing.display_color_depth)* 3; > > + clock = stream->timing.pix_clk_100hz / 10; > > + > > + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp, > > true); > > + > > + aconnector->vcpi_slots = > > drm_dp_atomic_find_vcpi_slots(state, > > + &aconnector- > > > mst_port->mst_mgr, > > + aconnector->port, > > + aconnector->pbn); > > + if (aconnector->vcpi_slots < 0) > > + return aconnector->vcpi_slots; > > + } > > + } > > + return 0; > > +} > > +#endif > > + > > static void dm_drm_plane_reset(struct drm_plane *plane) > > { > > struct dm_plane_state *amdgpu_state = NULL; > > @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct drm_device > > *dev, > > if (ret) > > goto fail; > > > > - /* Perform validation of MST topology in the state*/ > > - ret = drm_dp_mst_atomic_check(state); > > - if (ret) > > - goto fail; > > - > > if (state->legacy_cursor_update) { > > /* > > * This is a fast cursor update coming from the plane update > > @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct drm_device > > *dev, > > #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > if (!compute_mst_dsc_configs_for_state(dm_state->context)) > > goto fail; > > + > > + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state- > > > context); > > + if (ret) > > + goto fail; > > #endif > > if (dc_validate_global_state(dc, dm_state->context, false) != > > DC_OK) { > > ret = -EINVAL; > > @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct drm_device > > *dev, > > dc_retain_state(old_dm_state->context); > > } > > } > > + /* Perform validation of MST topology in the state*/ > > + ret = drm_dp_mst_atomic_check(state); > > + if (ret) > > + goto fail; > > > > /* Store the overall update type for use later in atomic check. */ > > for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > > 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 bd694499e3de..3e44e2da2d0d 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 > > @@ -201,12 +201,6 @@ bool > > dm_helpers_dp_mst_write_payload_allocation_table( > > mst_port = aconnector->port; > > > > if (enable) { > > - > > - /* Convert kilobits per second / 64 (for 64 timeslots) to pbn > > (54/64 megabytes per second) */ > > - pbn_per_timeslot = dc_link_bandwidth_kbps( > > - stream->link, dc_link_get_link_cap(stream- > > > link)) / (8 * 1000 * 54); > > - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, > > pbn_per_timeslot); > > - > > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, > > aconnector->pbn, > > aconnector->vcpi_slots);
On Tue, 2019-10-08 at 12:24 -0400, Lyude Paul wrote: > ... > yikes > I need to apologize because I was going through my email and I realized you > _did_ respond to me earlier regarding some of these questions, it just > appears > the reply fell through the cracks and somehow I didn't notice it until just > now. Sorry about that! Referring to this response, jfyi: https://lists.freedesktop.org/archives/amd-gfx/2019-October/040683.html (sorry again for replying before reading that!) > > I'm still not sure I understand why this is a future enhancement? Everything > we need to implement these helpers should already be here, it's just a > matter > of keeping track who has dsc enabled where in the mst atomic state > > On Mon, 2019-10-07 at 17:52 -0400, Lyude Paul wrote: > > Ok, let's stop and slow down for a minute here since I've repeated myself > > a > > few times, and I'd like to figure out what's actually happening here and > > whether I'm not being clear enough with my explanations, or if there's > > just > > some misunderstanding here. > > > > I'll start of by trying my best to explain my hesistance in accepting > > these > > patches. To be clear, MST with DSC is a big thing in terms of impact. > > There's > > a lot of things to consider: > > * What should the default DSC policy for MST be? Should we keep it on by > > default so that we don't need to trigger a large number of PBN > > recalculations and enable DSC on a per-case basis, or are there > > legitimate > > reasons to keep DSC off by default (such as potential issues with > > lossiness > > down the line). > > * We have other drivers in the tree that are planning on implementing MST > > DSC > > quite soon. Chances are they'll be implementing helpers to do this so > > this > > can be supported across the DRM tree, and chances are we'll just have > > to > > rewrite and remove most of this code in that case once they do. > > * amdgpu already has a _lot_ of code that doesn't need to, and shouldn't > > be > > there. This ranges from various DC specific helpers that are > > essentially > > the same as the helpers that we already implement in the rest of DRM, > > to > > misuses of various kernel APIs, and a complete lack of any kind of code > > style (at least the last time that I checked) in or out of DC. This > > makes > > the driver more difficult for people who don't work at AMD but are well > > accustomed to working on the rest of the kernel, e.g. myself and > > others, > > and it's a problem that seriously needs to be solved. Adding more > > redundant > > DP helpers that will inevitably get removed is just making the problem > > worse. > > > > With all of this being said: I've been asking the same questions about > > this > > patch throughout the whole patch series so far (since it got passed off to > > you > > from David's internship ending): > > > > * Can we keep DSC enabled by default, so that these patches aren't > > needed? > > * If we can't, can we move this into common code so that other drivers > > can > > use it? > > The problem is I haven't received any responses to these questions, other > > then > > being told by David a month or two ago that it would be expedient to just > > accept the patches and move on. Honestly, if discussion had been had about > > the > > changes I requested, I would have given my r-b a long time ago. In fact-I > > really want to give it now! There's multiple patches in this series that > > would > > be very useful for other things that are being worked on at the moment, > > such > > as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really think > > this needs to be discussed first, and I'm worried that just going ahead > > and > > giving my R-B before they have been discussed will further encourage > > rushing > > changes like this in the future and continually building more tech debt > > for > > ourselves. > > > > Please note as well: if anything I've asked for is confusing, or you don't > > understand what I'm asking or looking for I am more then willing to help > > explain things and help out as best as I can. I understand that a lot of > > the > > developers working on DRM at AMD may have more experience in Windows and > > Mac > > land and as a result, trying to get used to the way that we do things in > > the > > Linux kernel can be a confusing endeavor. I'm more then happy to help out > > with > > this wherever I can, all you need to do is ask. Asking a few questions > > about > > something you aren't sure you understand can save both of us a lot of > > time, > > and avoid having to go through this many patch respins. > > > > In the mean time, I'd be willing to look at what patches from this series > > that > > have already been reviewed which could be pushed to drm-misc or friends in > > the > > mean time to speed things up a bit. > > > > On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@amd.com wrote: > > > From: Mikita Lipski <mikita.lipski@amd.com> > > > > > > Since for DSC MST connector's PBN is claculated differently > > > due to compression, we have to recalculate both PBN and > > > VCPI slots for that connector. > > > > > > This patch proposes to use similair logic as in > > > dm_encoder_helper_atomic_chek, but since we do not know which > > > connectors will have DSC enabled we have to recalculate PBN only > > > after that's determined, which is done in > > > compute_mst_dsc_configs_for_state. > > > > > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > > > Cc: Harry Wentland <harry.wentland@amd.com> > > > Cc: Lyude Paul <lyude@redhat.com> > > > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > > > --- > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 +++++++++++++++++-- > > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 -- > > > 2 files changed, 59 insertions(+), 11 deletions(-) > > > > > > 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 81e30918f9ec..7501ce2233ed 100644 > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct > > > drm_encoder *encoder) > > > > > > } > > > > > > +static int convert_dc_color_depth_into_bpc (enum dc_color_depth > > > display_color_depth) > > > +{ > > > + switch (display_color_depth) { > > > + case COLOR_DEPTH_666: > > > + return 6; > > > + case COLOR_DEPTH_888: > > > + return 8; > > > + case COLOR_DEPTH_101010: > > > + return 10; > > > + case COLOR_DEPTH_121212: > > > + return 12; > > > + case COLOR_DEPTH_141414: > > > + return 14; > > > + case COLOR_DEPTH_161616: > > > + return 16; > > > + default: > > > + break; > > > + } > > > + return 0; > > > +} > > > + > > > static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, > > > struct drm_crtc_state *crtc_state, > > > struct drm_connector_state > > > *conn_state) > > > @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs > > > amdgpu_dm_encoder_helper_funcs = { > > > .atomic_check = dm_encoder_helper_atomic_check > > > }; > > > > > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > > +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state > > > *state, > > > + struct dc_state *dc_state) > > > +{ > > > + struct dc_stream_state *stream; > > > + struct amdgpu_dm_connector *aconnector; > > > + int i, clock = 0, bpp = 0; > > > + > > > + for (i = 0; i < dc_state->stream_count; i++) { > > > + stream = dc_state->streams[i]; > > > + aconnector = (struct amdgpu_dm_connector *)stream- > > > > dm_stream_context; > > > + > > > + if (stream && stream->timing.flags.DSC == 1) { > > > + bpp = convert_dc_color_depth_into_bpc(stream- > > > > timing.display_color_depth)* 3; > > > + clock = stream->timing.pix_clk_100hz / 10; > > > + > > > + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp, > > > true); > > > + > > > + aconnector->vcpi_slots = > > > drm_dp_atomic_find_vcpi_slots(state, > > > + &aconnector- > > > > mst_port->mst_mgr, > > > + aconnector->port, > > > + aconnector->pbn); > > > + if (aconnector->vcpi_slots < 0) > > > + return aconnector->vcpi_slots; > > > + } > > > + } > > > + return 0; > > > +} > > > +#endif > > > + > > > static void dm_drm_plane_reset(struct drm_plane *plane) > > > { > > > struct dm_plane_state *amdgpu_state = NULL; > > > @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct > > > drm_device > > > *dev, > > > if (ret) > > > goto fail; > > > > > > - /* Perform validation of MST topology in the state*/ > > > - ret = drm_dp_mst_atomic_check(state); > > > - if (ret) > > > - goto fail; > > > - > > > if (state->legacy_cursor_update) { > > > /* > > > * This is a fast cursor update coming from the plane update > > > @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct > > > drm_device > > > *dev, > > > #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > > if (!compute_mst_dsc_configs_for_state(dm_state->context)) > > > goto fail; > > > + > > > + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state- > > > > context); > > > + if (ret) > > > + goto fail; > > > #endif > > > if (dc_validate_global_state(dc, dm_state->context, false) != > > > DC_OK) { > > > ret = -EINVAL; > > > @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct > > > drm_device > > > *dev, > > > dc_retain_state(old_dm_state->context); > > > } > > > } > > > + /* Perform validation of MST topology in the state*/ > > > + ret = drm_dp_mst_atomic_check(state); > > > + if (ret) > > > + goto fail; > > > > > > /* Store the overall update type for use later in atomic check. */ > > > for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > > > 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 bd694499e3de..3e44e2da2d0d 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 > > > @@ -201,12 +201,6 @@ bool > > > dm_helpers_dp_mst_write_payload_allocation_table( > > > mst_port = aconnector->port; > > > > > > if (enable) { > > > - > > > - /* Convert kilobits per second / 64 (for 64 timeslots) to pbn > > > (54/64 megabytes per second) */ > > > - pbn_per_timeslot = dc_link_bandwidth_kbps( > > > - stream->link, dc_link_get_link_cap(stream- > > > > link)) / (8 * 1000 * 54); > > > - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, > > > pbn_per_timeslot); > > > - > > > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, > > > aconnector->pbn, > > > aconnector->vcpi_slots);
On 08.10.2019 12:24, Lyude Paul wrote: > ... > yikes > I need to apologize because I was going through my email and I realized you > _did_ respond to me earlier regarding some of these questions, it just appears > the reply fell through the cracks and somehow I didn't notice it until just > now. Sorry about that! > No worries, the patches got messy and are easily lost in here. I'll clean up on the next batch so it will be clearer what's happening > I'm still not sure I understand why this is a future enhancement? Everything > we need to implement these helpers should already be here, it's just a matter > of keeping track who has dsc enabled where in the mst atomic state As I've mentioned earlier our policy for DSC is to keep it disabled if not needed, because it is a lossy compression. Beside the fact that we don't want imperfect image on the screen, enabling DSC would also increase our power consumption. If we need it - we use the greedy algorithm inside compute_mst_dsc_configs_for_link to only enable DSC on streams that can't fit into allowed bandwidth without compression. I'm not exactly sure how it can be moved as helper to DRM. We keep track on which stream has DSC enabled by raising DSC flag in dc_stream_state flags structure. That it why pbn recalculation was moved to a separate function so only streams that have DSC flag raised get VCPI recalculated. I guess if we would have dsc_desired flag on drm_connnector_state and it would then perform recalculation of PBN and VCPI for the connector. Would that be something applicable as a DRM helper? > On Mon, 2019-10-07 at 17:52 -0400, Lyude Paul wrote: >> Ok, let's stop and slow down for a minute here since I've repeated myself a >> few times, and I'd like to figure out what's actually happening here and >> whether I'm not being clear enough with my explanations, or if there's just >> some misunderstanding here. >> >> I'll start of by trying my best to explain my hesistance in accepting these >> patches. To be clear, MST with DSC is a big thing in terms of impact. >> There's >> a lot of things to consider: >> * What should the default DSC policy for MST be? Should we keep it on by >> default so that we don't need to trigger a large number of PBN >> recalculations and enable DSC on a per-case basis, or are there >> legitimate >> reasons to keep DSC off by default (such as potential issues with >> lossiness >> down the line). >> * We have other drivers in the tree that are planning on implementing MST >> DSC >> quite soon. Chances are they'll be implementing helpers to do this so >> this >> can be supported across the DRM tree, and chances are we'll just have to >> rewrite and remove most of this code in that case once they do. >> * amdgpu already has a _lot_ of code that doesn't need to, and shouldn't be >> there. This ranges from various DC specific helpers that are essentially >> the same as the helpers that we already implement in the rest of DRM, to >> misuses of various kernel APIs, and a complete lack of any kind of code >> style (at least the last time that I checked) in or out of DC. This makes >> the driver more difficult for people who don't work at AMD but are well >> accustomed to working on the rest of the kernel, e.g. myself and others, >> and it's a problem that seriously needs to be solved. Adding more >> redundant >> DP helpers that will inevitably get removed is just making the problem >> worse. >> >> With all of this being said: I've been asking the same questions about this >> patch throughout the whole patch series so far (since it got passed off to >> you >> from David's internship ending): >> >> * Can we keep DSC enabled by default, so that these patches aren't needed? >> * If we can't, can we move this into common code so that other drivers can >> use it? >> The problem is I haven't received any responses to these questions, other >> then >> being told by David a month or two ago that it would be expedient to just >> accept the patches and move on. Honestly, if discussion had been had about >> the >> changes I requested, I would have given my r-b a long time ago. In fact-I >> really want to give it now! There's multiple patches in this series that >> would >> be very useful for other things that are being worked on at the moment, such >> as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really think >> this needs to be discussed first, and I'm worried that just going ahead and >> giving my R-B before they have been discussed will further encourage rushing >> changes like this in the future and continually building more tech debt for >> ourselves. >> >> Please note as well: if anything I've asked for is confusing, or you don't >> understand what I'm asking or looking for I am more then willing to help >> explain things and help out as best as I can. I understand that a lot of the >> developers working on DRM at AMD may have more experience in Windows and Mac >> land and as a result, trying to get used to the way that we do things in the >> Linux kernel can be a confusing endeavor. I'm more then happy to help out >> with >> this wherever I can, all you need to do is ask. Asking a few questions about >> something you aren't sure you understand can save both of us a lot of time, >> and avoid having to go through this many patch respins. >> >> In the mean time, I'd be willing to look at what patches from this series >> that >> have already been reviewed which could be pushed to drm-misc or friends in >> the >> mean time to speed things up a bit. >> >> On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@amd.com wrote: >>> From: Mikita Lipski <mikita.lipski@amd.com> >>> >>> Since for DSC MST connector's PBN is claculated differently >>> due to compression, we have to recalculate both PBN and >>> VCPI slots for that connector. >>> >>> This patch proposes to use similair logic as in >>> dm_encoder_helper_atomic_chek, but since we do not know which >>> connectors will have DSC enabled we have to recalculate PBN only >>> after that's determined, which is done in >>> compute_mst_dsc_configs_for_state. >>> >>> Cc: Jerry Zuo <Jerry.Zuo@amd.com> >>> Cc: Harry Wentland <harry.wentland@amd.com> >>> Cc: Lyude Paul <lyude@redhat.com> >>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> >>> --- >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 +++++++++++++++++-- >>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 -- >>> 2 files changed, 59 insertions(+), 11 deletions(-) >>> >>> 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 81e30918f9ec..7501ce2233ed 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct >>> drm_encoder *encoder) >>> >>> } >>> >>> +static int convert_dc_color_depth_into_bpc (enum dc_color_depth >>> display_color_depth) >>> +{ >>> + switch (display_color_depth) { >>> + case COLOR_DEPTH_666: >>> + return 6; >>> + case COLOR_DEPTH_888: >>> + return 8; >>> + case COLOR_DEPTH_101010: >>> + return 10; >>> + case COLOR_DEPTH_121212: >>> + return 12; >>> + case COLOR_DEPTH_141414: >>> + return 14; >>> + case COLOR_DEPTH_161616: >>> + return 16; >>> + default: >>> + break; >>> + } >>> + return 0; >>> +} >>> + >>> static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, >>> struct drm_crtc_state *crtc_state, >>> struct drm_connector_state >>> *conn_state) >>> @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs >>> amdgpu_dm_encoder_helper_funcs = { >>> .atomic_check = dm_encoder_helper_atomic_check >>> }; >>> >>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT >>> +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state >>> *state, >>> + struct dc_state *dc_state) >>> +{ >>> + struct dc_stream_state *stream; >>> + struct amdgpu_dm_connector *aconnector; >>> + int i, clock = 0, bpp = 0; >>> + >>> + for (i = 0; i < dc_state->stream_count; i++) { >>> + stream = dc_state->streams[i]; >>> + aconnector = (struct amdgpu_dm_connector *)stream- >>>> dm_stream_context; >>> + >>> + if (stream && stream->timing.flags.DSC == 1) { >>> + bpp = convert_dc_color_depth_into_bpc(stream- >>>> timing.display_color_depth)* 3; >>> + clock = stream->timing.pix_clk_100hz / 10; >>> + >>> + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp, >>> true); >>> + >>> + aconnector->vcpi_slots = >>> drm_dp_atomic_find_vcpi_slots(state, >>> + &aconnector- >>>> mst_port->mst_mgr, >>> + aconnector->port, >>> + aconnector->pbn); >>> + if (aconnector->vcpi_slots < 0) >>> + return aconnector->vcpi_slots; >>> + } >>> + } >>> + return 0; >>> +} >>> +#endif >>> + >>> static void dm_drm_plane_reset(struct drm_plane *plane) >>> { >>> struct dm_plane_state *amdgpu_state = NULL; >>> @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct drm_device >>> *dev, >>> if (ret) >>> goto fail; >>> >>> - /* Perform validation of MST topology in the state*/ >>> - ret = drm_dp_mst_atomic_check(state); >>> - if (ret) >>> - goto fail; >>> - >>> if (state->legacy_cursor_update) { >>> /* >>> * This is a fast cursor update coming from the plane update >>> @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct drm_device >>> *dev, >>> #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT >>> if (!compute_mst_dsc_configs_for_state(dm_state->context)) >>> goto fail; >>> + >>> + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state- >>>> context); >>> + if (ret) >>> + goto fail; >>> #endif >>> if (dc_validate_global_state(dc, dm_state->context, false) != >>> DC_OK) { >>> ret = -EINVAL; >>> @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct drm_device >>> *dev, >>> dc_retain_state(old_dm_state->context); >>> } >>> } >>> + /* Perform validation of MST topology in the state*/ >>> + ret = drm_dp_mst_atomic_check(state); >>> + if (ret) >>> + goto fail; >>> >>> /* Store the overall update type for use later in atomic check. */ >>> for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { >>> 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 bd694499e3de..3e44e2da2d0d 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 >>> @@ -201,12 +201,6 @@ bool >>> dm_helpers_dp_mst_write_payload_allocation_table( >>> mst_port = aconnector->port; >>> >>> if (enable) { >>> - >>> - /* Convert kilobits per second / 64 (for 64 timeslots) to pbn >>> (54/64 megabytes per second) */ >>> - pbn_per_timeslot = dc_link_bandwidth_kbps( >>> - stream->link, dc_link_get_link_cap(stream- >>>> link)) / (8 * 1000 * 54); >>> - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, >>> pbn_per_timeslot); >>> - >>> ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, >>> aconnector->pbn, >>> aconnector->vcpi_slots);
On Tue, 2019-10-08 at 21:26 +0000, Mikita Lipski wrote: > > On 08.10.2019 12:24, Lyude Paul wrote: > > ... > > yikes > > I need to apologize because I was going through my email and I realized > > you > > _did_ respond to me earlier regarding some of these questions, it just > > appears > > the reply fell through the cracks and somehow I didn't notice it until > > just > > now. Sorry about that! > > > No worries, the patches got messy and are easily lost in here. I'll > clean up on the next batch so it will be clearer what's happening > > > I'm still not sure I understand why this is a future enhancement? > > Everything > > we need to implement these helpers should already be here, it's just a > > matter > > of keeping track who has dsc enabled where in the mst atomic state > > As I've mentioned earlier our policy for DSC is to keep it disabled if > not needed, because it is a lossy compression. Beside the fact that we > don't want imperfect image on the screen, enabling DSC would also > increase our power consumption. If we need it - we use the greedy > algorithm inside compute_mst_dsc_configs_for_link to only enable DSC on > streams that can't fit into allowed bandwidth without compression. > > I'm not exactly sure how it can be moved as helper to DRM. > We keep track on which stream has DSC enabled by raising DSC flag in > dc_stream_state flags structure. > That it why pbn recalculation was moved to a separate function so only > streams that have DSC flag raised get VCPI recalculated. > > I guess if we would have dsc_desired flag on drm_connnector_state and it > would then perform recalculation of PBN and VCPI for the connector. > Would that be something applicable as a DRM helper? This would probably be a better fit for drm_dp_mst_topology_mgr's atomic state (take a look at the atomic VCPI helpers), specifically in struct drm_dp_mst_vcpi (maybe we should rename it to struct drm_dp_mst_port_bw while we're at it) and store DSC enablement information there. Then we can write a helper to go through all of the enabled ports on the topology and add the affected ones into the atomic state and set ->mode_changed on all of them. > > > > On Mon, 2019-10-07 at 17:52 -0400, Lyude Paul wrote: > > > Ok, let's stop and slow down for a minute here since I've repeated > > > myself a > > > few times, and I'd like to figure out what's actually happening here and > > > whether I'm not being clear enough with my explanations, or if there's > > > just > > > some misunderstanding here. > > > > > > I'll start of by trying my best to explain my hesistance in accepting > > > these > > > patches. To be clear, MST with DSC is a big thing in terms of impact. > > > There's > > > a lot of things to consider: > > > * What should the default DSC policy for MST be? Should we keep it on > > > by > > > default so that we don't need to trigger a large number of PBN > > > recalculations and enable DSC on a per-case basis, or are there > > > legitimate > > > reasons to keep DSC off by default (such as potential issues with > > > lossiness > > > down the line). > > > * We have other drivers in the tree that are planning on implementing > > > MST > > > DSC > > > quite soon. Chances are they'll be implementing helpers to do this > > > so > > > this > > > can be supported across the DRM tree, and chances are we'll just > > > have to > > > rewrite and remove most of this code in that case once they do. > > > * amdgpu already has a _lot_ of code that doesn't need to, and > > > shouldn't be > > > there. This ranges from various DC specific helpers that are > > > essentially > > > the same as the helpers that we already implement in the rest of > > > DRM, to > > > misuses of various kernel APIs, and a complete lack of any kind of > > > code > > > style (at least the last time that I checked) in or out of DC. This > > > makes > > > the driver more difficult for people who don't work at AMD but are > > > well > > > accustomed to working on the rest of the kernel, e.g. myself and > > > others, > > > and it's a problem that seriously needs to be solved. Adding more > > > redundant > > > DP helpers that will inevitably get removed is just making the > > > problem > > > worse. > > > > > > With all of this being said: I've been asking the same questions about > > > this > > > patch throughout the whole patch series so far (since it got passed off > > > to > > > you > > > from David's internship ending): > > > > > > * Can we keep DSC enabled by default, so that these patches aren't > > > needed? > > > * If we can't, can we move this into common code so that other drivers > > > can > > > use it? > > > The problem is I haven't received any responses to these questions, > > > other > > > then > > > being told by David a month or two ago that it would be expedient to > > > just > > > accept the patches and move on. Honestly, if discussion had been had > > > about > > > the > > > changes I requested, I would have given my r-b a long time ago. In fact- > > > I > > > really want to give it now! There's multiple patches in this series that > > > would > > > be very useful for other things that are being worked on at the moment, > > > such > > > as the changes to drm_dp_dpcd_read/drm_dp_dpcd_write(). But I really > > > think > > > this needs to be discussed first, and I'm worried that just going ahead > > > and > > > giving my R-B before they have been discussed will further encourage > > > rushing > > > changes like this in the future and continually building more tech debt > > > for > > > ourselves. > > > > > > Please note as well: if anything I've asked for is confusing, or you > > > don't > > > understand what I'm asking or looking for I am more then willing to help > > > explain things and help out as best as I can. I understand that a lot of > > > the > > > developers working on DRM at AMD may have more experience in Windows and > > > Mac > > > land and as a result, trying to get used to the way that we do things in > > > the > > > Linux kernel can be a confusing endeavor. I'm more then happy to help > > > out > > > with > > > this wherever I can, all you need to do is ask. Asking a few questions > > > about > > > something you aren't sure you understand can save both of us a lot of > > > time, > > > and avoid having to go through this many patch respins. > > > > > > In the mean time, I'd be willing to look at what patches from this > > > series > > > that > > > have already been reviewed which could be pushed to drm-misc or friends > > > in > > > the > > > mean time to speed things up a bit. > > > > > > On Tue, 2019-10-01 at 12:17 -0400, mikita.lipski@amd.com wrote: > > > > From: Mikita Lipski <mikita.lipski@amd.com> > > > > > > > > Since for DSC MST connector's PBN is claculated differently > > > > due to compression, we have to recalculate both PBN and > > > > VCPI slots for that connector. > > > > > > > > This patch proposes to use similair logic as in > > > > dm_encoder_helper_atomic_chek, but since we do not know which > > > > connectors will have DSC enabled we have to recalculate PBN only > > > > after that's determined, which is done in > > > > compute_mst_dsc_configs_for_state. > > > > > > > > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > > > > Cc: Harry Wentland <harry.wentland@amd.com> > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Signed-off-by: Mikita Lipski <mikita.lipski@amd.com> > > > > --- > > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 64 > > > > +++++++++++++++++-- > > > > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 -- > > > > 2 files changed, 59 insertions(+), 11 deletions(-) > > > > > > > > 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 81e30918f9ec..7501ce2233ed 100644 > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct > > > > drm_encoder *encoder) > > > > > > > > } > > > > > > > > +static int convert_dc_color_depth_into_bpc (enum dc_color_depth > > > > display_color_depth) > > > > +{ > > > > + switch (display_color_depth) { > > > > + case COLOR_DEPTH_666: > > > > + return 6; > > > > + case COLOR_DEPTH_888: > > > > + return 8; > > > > + case COLOR_DEPTH_101010: > > > > + return 10; > > > > + case COLOR_DEPTH_121212: > > > > + return 12; > > > > + case COLOR_DEPTH_141414: > > > > + return 14; > > > > + case COLOR_DEPTH_161616: > > > > + return 16; > > > > + default: > > > > + break; > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > static int dm_encoder_helper_atomic_check(struct drm_encoder > > > > *encoder, > > > > struct drm_crtc_state > > > > *crtc_state, > > > > struct drm_connector_state > > > > *conn_state) > > > > @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs > > > > amdgpu_dm_encoder_helper_funcs = { > > > > .atomic_check = dm_encoder_helper_atomic_check > > > > }; > > > > > > > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > > > +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state > > > > *state, > > > > + struct dc_state *dc_state) > > > > +{ > > > > + struct dc_stream_state *stream; > > > > + struct amdgpu_dm_connector *aconnector; > > > > + int i, clock = 0, bpp = 0; > > > > + > > > > + for (i = 0; i < dc_state->stream_count; i++) { > > > > + stream = dc_state->streams[i]; > > > > + aconnector = (struct amdgpu_dm_connector *)stream- > > > > > dm_stream_context; > > > > + > > > > + if (stream && stream->timing.flags.DSC == 1) { > > > > + bpp = convert_dc_color_depth_into_bpc(stream- > > > > > timing.display_color_depth)* 3; > > > > + clock = stream->timing.pix_clk_100hz / 10; > > > > + > > > > + aconnector->pbn = drm_dp_calc_pbn_mode(clock, > > > > bpp, > > > > true); > > > > + > > > > + aconnector->vcpi_slots = > > > > drm_dp_atomic_find_vcpi_slots(state, > > > > + &aconnector- > > > > > mst_port->mst_mgr, > > > > + aconnector- > > > > >port, > > > > + aconnector- > > > > >pbn); > > > > + if (aconnector->vcpi_slots < 0) > > > > + return aconnector->vcpi_slots; > > > > + } > > > > + } > > > > + return 0; > > > > +} > > > > +#endif > > > > + > > > > static void dm_drm_plane_reset(struct drm_plane *plane) > > > > { > > > > struct dm_plane_state *amdgpu_state = NULL; > > > > @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct > > > > drm_device > > > > *dev, > > > > if (ret) > > > > goto fail; > > > > > > > > - /* Perform validation of MST topology in the state*/ > > > > - ret = drm_dp_mst_atomic_check(state); > > > > - if (ret) > > > > - goto fail; > > > > - > > > > if (state->legacy_cursor_update) { > > > > /* > > > > * This is a fast cursor update coming from the plane > > > > update > > > > @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct > > > > drm_device > > > > *dev, > > > > #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > > > > if (!compute_mst_dsc_configs_for_state(dm_state- > > > > >context)) > > > > goto fail; > > > > + > > > > + ret = dm_update_mst_vcpi_slots_for_dsc(state, > > > > dm_state- > > > > > context); > > > > + if (ret) > > > > + goto fail; > > > > #endif > > > > if (dc_validate_global_state(dc, dm_state->context, > > > > false) != > > > > DC_OK) { > > > > ret = -EINVAL; > > > > @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct > > > > drm_device > > > > *dev, > > > > dc_retain_state(old_dm_state- > > > > >context); > > > > } > > > > } > > > > + /* Perform validation of MST topology in the state*/ > > > > + ret = drm_dp_mst_atomic_check(state); > > > > + if (ret) > > > > + goto fail; > > > > > > > > /* Store the overall update type for use later in atomic > > > > check. */ > > > > for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > > > > 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 bd694499e3de..3e44e2da2d0d 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 > > > > @@ -201,12 +201,6 @@ bool > > > > dm_helpers_dp_mst_write_payload_allocation_table( > > > > mst_port = aconnector->port; > > > > > > > > if (enable) { > > > > - > > > > - /* Convert kilobits per second / 64 (for 64 timeslots) > > > > to pbn > > > > (54/64 megabytes per second) */ > > > > - pbn_per_timeslot = dc_link_bandwidth_kbps( > > > > - stream->link, > > > > dc_link_get_link_cap(stream- > > > > > link)) / (8 * 1000 * 54); > > > > - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, > > > > pbn_per_timeslot); > > > > - > > > > ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, > > > > aconnector->pbn, > > > > aconnector- > > > > >vcpi_slots); > > -- > Thanks, > Mikita Lipski > Software Engineer, AMD > mikita.lipski@amd.com
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 81e30918f9ec..7501ce2233ed 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4569,6 +4569,27 @@ static void dm_encoder_helper_disable(struct drm_encoder *encoder) } +static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_depth) +{ + switch (display_color_depth) { + case COLOR_DEPTH_666: + return 6; + case COLOR_DEPTH_888: + return 8; + case COLOR_DEPTH_101010: + return 10; + case COLOR_DEPTH_121212: + return 12; + case COLOR_DEPTH_141414: + return 14; + case COLOR_DEPTH_161616: + return 16; + default: + break; + } + return 0; +} + static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) @@ -4616,6 +4637,36 @@ const struct drm_encoder_helper_funcs amdgpu_dm_encoder_helper_funcs = { .atomic_check = dm_encoder_helper_atomic_check }; +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT +static int dm_update_mst_vcpi_slots_for_dsc(struct drm_atomic_state *state, + struct dc_state *dc_state) +{ + struct dc_stream_state *stream; + struct amdgpu_dm_connector *aconnector; + int i, clock = 0, bpp = 0; + + for (i = 0; i < dc_state->stream_count; i++) { + stream = dc_state->streams[i]; + aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context; + + if (stream && stream->timing.flags.DSC == 1) { + bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth)* 3; + clock = stream->timing.pix_clk_100hz / 10; + + aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp, true); + + aconnector->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state, + &aconnector->mst_port->mst_mgr, + aconnector->port, + aconnector->pbn); + if (aconnector->vcpi_slots < 0) + return aconnector->vcpi_slots; + } + } + return 0; +} +#endif + static void dm_drm_plane_reset(struct drm_plane *plane) { struct dm_plane_state *amdgpu_state = NULL; @@ -7629,11 +7680,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, if (ret) goto fail; - /* Perform validation of MST topology in the state*/ - ret = drm_dp_mst_atomic_check(state); - if (ret) - goto fail; - if (state->legacy_cursor_update) { /* * This is a fast cursor update coming from the plane update @@ -7705,6 +7751,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, #ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT if (!compute_mst_dsc_configs_for_state(dm_state->context)) goto fail; + + ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state->context); + if (ret) + goto fail; #endif if (dc_validate_global_state(dc, dm_state->context, false) != DC_OK) { ret = -EINVAL; @@ -7734,6 +7784,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, dc_retain_state(old_dm_state->context); } } + /* Perform validation of MST topology in the state*/ + ret = drm_dp_mst_atomic_check(state); + if (ret) + goto fail; /* Store the overall update type for use later in atomic check. */ for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { 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 bd694499e3de..3e44e2da2d0d 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 @@ -201,12 +201,6 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( mst_port = aconnector->port; if (enable) { - - /* Convert kilobits per second / 64 (for 64 timeslots) to pbn (54/64 megabytes per second) */ - pbn_per_timeslot = dc_link_bandwidth_kbps( - stream->link, dc_link_get_link_cap(stream->link)) / (8 * 1000 * 54); - aconnector->vcpi_slots = DIV_ROUND_UP(aconnector->pbn, pbn_per_timeslot); - ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, aconnector->pbn, aconnector->vcpi_slots);