Re: [PATCH] drm/amd/display: Enabling eDP no power sequencing with DAL feature mask
[Public]
>From: Liu, Zhan
>Sent: Monday, June 21, 2021 9:13 PM
>To: amd-gfx@lists.freedesktop.org ; Cornij,
>Nikola
>Cc: Liu, Charlene
>Subject: [PATCH] drm/amd/display: Enabling eDP no power sequencing with DAL
>feature mask
>
>[Public]
>
>[Why]
>Sometimes, DP receiver chip power-controlled externally by an
>Embedded Controller could be treated and used as eDP,
>if it drives mobile display. In this case,
>we shouldn't be doing power-sequencing, hence we can skip
>waiting for T7-ready and T9-ready."
>
>[How]
>Added a feature mask to enable eDP no power sequencing feature.
>
>To enable this, set 0x10 flag in amdgpu.dcfeaturemask on
>Linux command line.
>
>Signed-off-by: Zhan Liu
Reviewed-by: Nikola Cornij
>Change-Id: I15e8fb2979fe3ff5491ccf1ee384693d4dce787c
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++
> drivers/gpu/drm/amd/display/dc/dc.h | 1 +
> .../display/dc/dce110/dce110_hw_sequencer.c | 31 ---
> drivers/gpu/drm/amd/include/amd_shared.h | 10 +++---
> 5 files changed, 38 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>index 3de1accb060e..b588cf4398db 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>@@ -159,6 +159,7 @@ int amdgpu_smu_pptable_id = -1;
> * highest. That helps saving some idle power.
> * DISABLE_FRACTIONAL_PWM (bit 2) disabled by default
> * PSR (bit 3) disabled by default
>+ * EDP NO POWER SEQUENCING (bit 4) disabled by default
> */
> uint amdgpu_dc_feature_mask = 2;
> uint amdgpu_dc_debug_mask;
>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 c0a3119982b0..abba26c8f20a 100644
>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>@@ -1174,6 +1174,9 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>if (amdgpu_dc_feature_mask & DC_DISABLE_FRACTIONAL_PWM_MASK)
>init_data.flags.disable_fractional_pwm = true;
>
>+ if (amdgpu_dc_feature_mask & DC_EDP_NO_POWER_SEQUENCING)
>+ init_data.flags.edp_no_power_sequencing = true;
>+
>init_data.flags.power_down_display_on_boot = true;
>
>INIT_LIST_HEAD(>dm.da_list);
>diff --git a/drivers/gpu/drm/amd/display/dc/dc.h
>b/drivers/gpu/drm/amd/display/dc/dc.h
>index a70697898025..7f1d2d6f9de8 100644
>--- a/drivers/gpu/drm/amd/display/dc/dc.h
>+++ b/drivers/gpu/drm/amd/display/dc/dc.h
>@@ -297,6 +297,7 @@ struct dc_config {
>bool allow_seamless_boot_optimization;
>bool power_down_display_on_boot;
>bool edp_not_connected;
>+ bool edp_no_power_sequencing;
>bool force_enum_edp;
>bool forced_clocks;
>bool allow_lttpr_non_transparent_mode;
>diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>index 53dd305fa6b0..013d94c9506a 100644
>--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>@@ -1023,8 +1023,20 @@ void dce110_edp_backlight_control(
>/* dc_service_sleep_in_milliseconds(50); */
>/*edp 1.2*/
>panel_instance = link->panel_cntl->inst;
>- if (cntl.action == TRANSMITTER_CONTROL_BACKLIGHT_ON)
>- edp_receiver_ready_T7(link);
>+
>+ if (cntl.action == TRANSMITTER_CONTROL_BACKLIGHT_ON) {
>+ if (!link->dc->config.edp_no_power_sequencing)
>+ /*
>+* Sometimes, DP receiver chip power-controlled externally by
>an
>+* Embedded Controller could be treated and used as eDP,
>+* if it drives mobile display. In this case,
>+* we shouldn't be doing power-sequencing, hence we can skip
>+* waiting for T7-ready.
>+*/
>+ edp_receiver_ready_T7(link);
>+ else
>+ DC_LOG_DC("edp_receiver_ready_T7 skipped\n");
>+ }
>
>if (ctx->dc->ctx->dmub_srv &&
>ctx->dc->debug.dmub_command_table) {
>@@ -1049,8 +1061,19 @@ void dce110_edp_backlight_control(
>dc_link_backlight_enable_aux(link, enable);
>
>/*edp 1.2*/
>- if (cntl.action == TRANSMITTER_CONTROL_BACKLIGHT_OFF)
>- edp_add_delay_for_T9(link);
>+ if (cntl.action =
RE: [PATCH] drm/amd/display: Avoid HPD IRQ in GPU reset state
[AMD Official Use Only - Internal Distribution Only]
>
>-Original Message-
>From: Liu, Zhan
>Sent: Sunday, May 9, 2021 10:30 PM
>To: amd-gfx@lists.freedesktop.org; Liu, Zhan ; Cornij,
>Nikola
>Subject: [PATCH] drm/amd/display: Avoid HPD IRQ in GPU reset state
>
>[Why]
>If GPU is in reset state, force enabling link will cause unexpected behaviour.
>
>[How]
>Avoid handling HPD IRQ when GPU is in reset state.
>
>Signed-off-by: Zhan Liu
>Change-Id: I29d80501e44096068e98b5d5984e63822dfcef82
>---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 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 cc048c348a92..58b59152a8be 100644
>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>@@ -2769,15 +2769,15 @@ static void handle_hpd_rx_irq(void *param)
> }
> }
>
>- if (!amdgpu_in_reset(adev))
>+ if (!amdgpu_in_reset(adev)) {
> mutex_lock(>dm.dc_lock);
> #ifdef CONFIG_DRM_AMD_DC_HDCP
> result = dc_link_handle_hpd_rx_irq(dc_link, _irq_data, NULL); #else
> result = dc_link_handle_hpd_rx_irq(dc_link, NULL, NULL); #endif
>- if (!amdgpu_in_reset(adev))
> mutex_unlock(>dm.dc_lock);
>+ }
>
> out:
> if (result && !is_mst_root_connector) {
>--
>2.25.1
>
>
Reviewed-by: Nikola Cornij
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH v2 1/1] drm/i915: Use the correct max source link rate for MST
[AMD Official Use Only - Internal Distribution Only]
I'll fix the dpcd part to use kHz on Monday
My apologies as well, not only for coming up with the wrong patch in first
place, but also for missing to CC all the maintainers.
-Original Message-
From: Lyude Paul
Sent: Friday, April 30, 2021 6:41 PM
To: Cornij, Nikola ; amd-gfx@lists.freedesktop.org
Cc: Pillai, Aurabindo ; Lipski, Mikita
; ville.syrj...@linux.intel.com; koba...@canonical.com;
intel-...@lists.freedesktop.org; dri-de...@lists.freedesktop.org;
jani.nik...@linux.intel.com; bske...@redhat.com
Subject: Re: [PATCH v2 1/1] drm/i915: Use the correct max source link rate for
MST
Alright - I had Ville double check this and give their A-B on IRC (I just need
to fix the open coded link_rate_to_bw() here). Since this got broken on drm-
misc-next I'm going to go ahead and push the fix there, since I'm not going to
be around next Monday or Tuesday and I don't want to leave i915 broken in the
interim. Sorry about the noise with this Jani!
As well - I'll get to fixing the dpcd unit usage once I get back on Wednesday
(unless Nikola's already gotten to it by then) so we use KHz instead. Although
as ville has pointed out, I think we should teach the topology manager to allow
passing for the current link rate/lane count during state computation in the
long term.
On Fri, 2021-04-30 at 17:45 -0400, Nikola Cornij wrote:
> [why]
> Previously used value was not safe to provide the correct value, i.e.
> it could be 0 if not not configured, leading to no MST on this platform.
>
> [how]
> Do not use the value from BIOS, but from the structure populated at
> encoder initialization time.
>
> Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver
> Capability DPCD
> space")
> Signed-off-by: Nikola Cornij
> Reviewed-by: Lyude Paul
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bf7f8487945c..3642d7578658 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -942,7 +942,7 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
> struct intel_dp *intel_dp = _port->dp;
> enum port port = dig_port->base.port;
> int ret;
> - int bios_max_link_rate;
> + int max_source_rate = intel_dp->source_rates[intel_dp-
> >num_source_rates - 1];
>
> if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
> return 0;
> @@ -957,11 +957,11 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
>
> /* create encoders */
> intel_dp_create_fake_mst_encoders(dig_port);
> - bios_max_link_rate =
> intel_bios_dp_max_link_rate(_port->base);
> ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr,
> >drm,
>_dp->aux, 16, 3,
>(u8)dig_port->max_lanes,
> - (u8)(bios_max_link_rate /
> 27000), conn_base_id);
> + (u8)(max_source_rate /
> +27000),
> + conn_base_id);
> if (ret)
> return ret;
>
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to
check on my status. I don't bite!
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
[AMD Official Use Only - Internal Distribution Only]
Hi,
drm/radeon/ part is still WIP (i.e. I doubt it'll work as is), but drm/i915 and
drm/nouveau/ should be OK. Would it be possible to test those while I'm
figuring out drm/radeon/ settings?
I'm pretty sure the follow-up change would affect only drm/radeon/, i.e. no
modifications in other parts of the code (unless found wrong in review, of
course).
I've tested drm/amd/ and it passes - I've confirmed the link and rate values
make it all the way to drm_dp_mst_topology_mgr_set_mst().
Thanks,
Nikola
-Original Message-
From: Cornij, Nikola
Sent: Tuesday, April 27, 2021 5:29 PM
To: amd-gfx@lists.freedesktop.org
Cc: Pillai, Aurabindo ; Lipski, Mikita
; ly...@redhat.com; ville.syrj...@linux.intel.com;
koba...@canonical.com; intel-...@lists.freedesktop.org; Cornij, Nikola
Subject: [PATCH] drm/drm_mst: Use Extended Base Receiver Capability DPCD space
[why]
DP 1.4a spec madates that if DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT is set,
Extended Base Receiver Capability DPCD space must be used. Without doing that,
the three DPCD values that differ will be wrong, leading to incorrect or
limited functionality. MST link rate, for example, could have a lower value or
Synaptics quirk wouldn't work out well when Extended DPCD was not read,
resulting in no DSC for such hubs.
[how]
Modify MST topology manager to use the values from Extended DPCD where
applicable.
To prevent regression on the sources that have lower maximum link rate
capability than MAX_LINK_RATE from Extended DPCD, have the drivers supply
maximum lane count and rate at initialization time.
This also reverts the 2dcab875e763 (Revert "drm/dp_mst: Retrieve extended DPCD
caps for topology manager), brining the change back to the original commit
ad44c03208e4 (drm/dp_mst: Retrieve extended DPCD caps for topology manager).
Signed-off-by: Nikola Cornij
---
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++
.../gpu/drm/amd/display/dc/core/dc_link_dp.c | 17 ++
drivers/gpu/drm/amd/display/dc/dc_link.h | 2 ++
drivers/gpu/drm/drm_dp_mst_topology.c | 33 ---
drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-
drivers/gpu/drm/radeon/radeon_dp_mst.c| 3 ++
include/drm/drm_dp_mst_helper.h | 12 ++-
8 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index d62460b69d95..d038e3185afb 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -437,6 +437,8 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
struct amdgpu_dm_connector *aconnector,
int link_index)
{
+struct dc_link_settings max_link_enc_cap = {0};
+
aconnector->dm_dp_aux.aux.name =
kasprintf(GFP_KERNEL, "AMDGPU DM aux hw bus %d",
link_index);
@@ -450,6 +452,7 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
return;
+dc_link_dp_get_max_link_enc_cap(aconnector->dc_link,
+_link_enc_cap);
aconnector->mst_mgr.cbs = _mst_cbs;
drm_dp_mst_topology_mgr_init(
>mst_mgr,
@@ -457,6 +460,8 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
>dm_dp_aux.aux,
16,
4,
+max_link_enc_cap.lane_count,
+max_link_enc_cap.link_rate,
aconnector->connector_id);
drm_connector_attach_dp_subconnector_property(>base);
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index 3ff3d9e90983..18a0b84e9869 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
@@ -1893,6 +1893,23 @@ bool dc_link_dp_sync_lt_end(struct dc_link *link, bool
link_down)
return true;
}
+bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct
+dc_link_settings *max_link_enc_cap) {
+if (max_link_enc_cap == NULL) {
+DC_LOG_ERROR("%s: Could not return max link encoder caps", __func__);
+}
+
+if (link->link_enc->funcs->get_max_link_cap) {
+link->link_enc->funcs->get_max_link_cap(link->link_enc, max_link_enc_cap);
+return true;
+}
+
+DC_LOG_ERROR("%s: Max link encoder caps unknown", __func__);
+max_link_enc_cap->lane_count = 1;
+max_link_enc_cap->link_rate = 6;
+return false;
+}
+
static struct dc_link_settings get_max_link_cap(struct dc_link *link) {
struct dc_link_settings max_link_cap = {0}; diff --git
a/drivers/gpu/drm/amd/display/dc/dc_link.h
b/drivers/gpu/drm/amd/display/dc/dc_link.h
index 054bab45ee17..fc5622ffec3d 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_link.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
@@ -345,6 +345,8
RE: [PATCH] Revert "Revert freesync video patches temporarily"
[AMD Official Use Only - Approved for External Use]
Oh, no, I already pushed it without it :-(
My thinking was, the initial revert explained why it was reverted, and the fact
it was now being put back in (implicitly) meant the regression was no longer
reproduced with the most recent kernel.
I can put a note about it next time though.
-Original Message-
From: Wentland, Harry
Sent: Tuesday, March 16, 2021 11:52 AM
To: Cornij, Nikola ; amd-gfx@lists.freedesktop.org
Cc: Pillai, Aurabindo
Subject: Re: [PATCH] Revert "Revert freesync video patches temporarily"
On 2021-03-16 11:15 a.m., Nikola Cornij wrote:
> This reverts commit e9a777fc0c264542fbd6d51b8fe51739d09032c1
>
> Sinc this is a "revert of a revert", the end effect is that freesync
> video is back to its original state, the way it was before the first
> revert.
A brief statement describing why it was reverted and why that's no longer an
issue would be useful.
Other than that this is
Acked-by: Harry Wentland
Harry
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 +
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 369 --
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
> 4 files changed, 349 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 5da112b3feb0..428b9f2d38c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -189,6 +189,7 @@ extern int amdgpu_emu_mode;
> extern uint amdgpu_smu_memory_pool_size;
> extern int amdgpu_smu_pptable_id;
> extern uint amdgpu_dc_feature_mask;
> +extern uint amdgpu_freesync_vid_mode;
> extern uint amdgpu_dc_debug_mask;
> extern uint amdgpu_dm_abm_level;
> extern int amdgpu_backlight;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 1be42edf84d0..d6a9787e5597 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -166,6 +166,7 @@ int amdgpu_mes;
> int amdgpu_noretry = -1;
> int amdgpu_force_asic_type = -1;
> int amdgpu_tmz = -1; /* auto */
> +uint amdgpu_freesync_vid_mode;
> int amdgpu_reset_method = -1; /* auto */
> int amdgpu_num_kcq = -1;
>
> @@ -828,6 +829,17 @@ module_param_named(backlight, amdgpu_backlight, bint,
> 0444);
> MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto (default), 0 = off, 1
> = on)");
> module_param_named(tmz, amdgpu_tmz, int, 0444);
>
> +/**
> + * DOC: freesync_video (uint)
> + * Enabled the optimization to adjust front porch timing to achieve
> +seamless mode change experience
> + * when setting a freesync supported mode for which full modeset is not
> needed.
> + * The default value: 0 (off).
> + */
> +MODULE_PARM_DESC(
> + freesync_video,
> + "Enable freesync modesetting optimization feature (0 = off
> +(default), 1 = on)"); module_param_named(freesync_video,
> +amdgpu_freesync_vid_mode, uint, 0444);
> +
> /**
>* DOC: reset_method (int)
>* GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 =
> mode1, 3 = mode2, 4 = baco, 5 = pci) 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 8b464debc1ef..553e39f9538c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -212,6 +212,9 @@ static bool amdgpu_dm_psr_disable_all(struct
> amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
> /*
>* dm_vblank_get_counter
>*
> @@ -335,6 +338,17 @@ static inline bool amdgpu_dm_vrr_active(struct
> dm_crtc_state *dm_state)
> dm_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
> }
>
> +static inline bool is_dc_timing_adjust_needed(struct dm_crtc_state
> *old_state,
> + struct dm_crtc_state *new_state) {
> + if (new_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED)
> + return true;
> + else if (amdgpu_dm_vrr_active(old_state) !=
> amdgpu_dm_vrr_active(new_state))
> + return true;
> + else
> + return false;
> +}
> +
> /**
>* dm_pflip_high_irq() - Handle pageflip interrupt
>* @interrupt_params: ignored
> @@ -51
RE: Overlay issues
[AMD Official Use Only - Approved for External Use] Thanks for the suggestion, Simon. Improved the patch, now in the process of getting it reviewed internally. It'll be posted for upstream thereafter. -Original Message- From: Simon Ser Sent: Monday, February 22, 2021 5:31 PM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: Overlay issues On Monday, February 22nd, 2021 at 9:59 PM, Cornij, Nikola wrote: > [AMD Official Use Only - Approved for External Use] > > Hi Simon, > > Yes, I did have a chance to wrap this up, indeed :-) > > It turned out this and other similar setup was hitting a legit HW > limitation. I added a patch (please see attached) that'd fail this > config at validation time. The patch has been merged for upstreaming > at the beginning of February time-frame, not sure if it made it to the > public repo by now. Excellent! I don't see the patch in agd5f/drm-next yet, but it'll probably show up soon. Thanks for fixing this! One small nit: if possible, add some drm_dbg_atomic() calls explaining the error right before returning -EINVAL. This is very valuable when debugging user-space applications and trying to figure out why the kernel rejects an atomic commit. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: Overlay issues
[AMD Official Use Only - Approved for External Use] Hi Simon, Yes, I did have a chance to wrap this up, indeed :-) It turned out this and other similar setup was hitting a legit HW limitation. I added a patch (please see attached) that'd fail this config at validation time. The patch has been merged for upstreaming at the beginning of February time-frame, not sure if it made it to the public repo by now. Please let me know if you need more info on this. Regards, Nikola -Original Message- From: Simon Ser Sent: Friday, February 19, 2021 4:22 PM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: Overlay issues Hi, On Wednesday, December 23rd, 2020 at 6:48 AM, Cornij, Nikola wrote: > Another interim update: so far to me it looks like this is an issue if > there's fewer than 24 pixels left on the screen when moving the FB > outside of the left edge (e.g. with 300x300 FB size, it repros with X > = -280). When this happens, what looks like a boundary condition in > our driver is hit and destination rectangle update is skipped. > > I need to do some more digging to fully understand why is this > condition in place and how to avoid it. Did you have the chance to continue working on this? Thanks, Simon 0001-drm-amd-display-Reject-too-small-viewport-size-when-.patch Description: 0001-drm-amd-display-Reject-too-small-viewport-size-when-.patch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: Overlay issues
[AMD Public Use] Hi Simon, Another interim update: so far to me it looks like this is an issue if there's fewer than 24 pixels left on the screen when moving the FB outside of the left edge (e.g. with 300x300 FB size, it repros with X = -280). When this happens, what looks like a boundary condition in our driver is hit and destination rectangle update is skipped. I need to do some more digging to fully understand why is this condition in place and how to avoid it. Regards, Nikola -Original Message- From: Cornij, Nikola Sent: Tuesday, December 15, 2020 11:37 AM To: Simon Ser Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: Overlay issues [AMD Public Use] Yeah, but... it works with bigger FBs (300x300). So looks like some clipping somewhere works OK until a corner-case is reached. -Original Message- From: Simon Ser Sent: Tuesday, December 15, 2020 2:58 AM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: Re: Overlay issues On Tuesday, December 15th, 2020 at 5:09 AM, Cornij, Nikola wrote: > [AMD Public Use] > > Hi Simon, > > Just to keep you updated, I've reproduced issue '[1] - Overlay plane: > position not updated when CRTC_X is negative' on Ubuntu using IGT. > Seems to happen only with smaller FBs (so far tried 24x24 and I can > repro, but 300x300 is OK). > > I'll look into fixing this. Thanks for the update and for looking into this! Let me know if I can help with anything. Nicholas replied on the issue tracker that overlay planes couldn't have negative positions, so I was thinking of having a look at the SRC_Y handling (see if we can do the same for SRC_X), experimenting with FB sizes and SRC_W/H to figure out what's the overlay max size still triggering the bug. If we really can't emulate neg SRC_X we should fail atomic commits with negative SRC_X by returning EINVAL (so that user-space can fall back to not using the plane in this case). Simon ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: Overlay issues
[AMD Public Use] Yeah, but... it works with bigger FBs (300x300). So looks like some clipping somewhere works OK until a corner-case is reached. -Original Message- From: Simon Ser Sent: Tuesday, December 15, 2020 2:58 AM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: Re: Overlay issues On Tuesday, December 15th, 2020 at 5:09 AM, Cornij, Nikola wrote: > [AMD Public Use] > > Hi Simon, > > Just to keep you updated, I've reproduced issue '[1] - Overlay plane: > position not updated when CRTC_X is negative' on Ubuntu using IGT. > Seems to happen only with smaller FBs (so far tried 24x24 and I can > repro, but 300x300 is OK). > > I'll look into fixing this. Thanks for the update and for looking into this! Let me know if I can help with anything. Nicholas replied on the issue tracker that overlay planes couldn't have negative positions, so I was thinking of having a look at the SRC_Y handling (see if we can do the same for SRC_X), experimenting with FB sizes and SRC_W/H to figure out what's the overlay max size still triggering the bug. If we really can't emulate neg SRC_X we should fail atomic commits with negative SRC_X by returning EINVAL (so that user-space can fall back to not using the plane in this case). Simon ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Overlay issues
[AMD Public Use] Hi Simon, Just to keep you updated, I've reproduced issue '[1] - Overlay plane: position not updated when CRTC_X is negative' on Ubuntu using IGT. Seems to happen only with smaller FBs (so far tried 24x24 and I can repro, but 300x300 is OK). I'll look into fixing this. Regards, Nikola -Original Message- From: Cornij, Nikola Sent: Tuesday, December 8, 2020 4:11 PM To: Simon Ser Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: [PATCH 2/2] drm/amd/display: add cursor pitch check [AMD Public Use] Very good, thanks! I'll take a look at the overlay ones, then. -Original Message- From: Simon Ser Sent: Tuesday, December 8, 2020 7:00 AM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: [PATCH 2/2] drm/amd/display: add cursor pitch check Hi Nikola, On Tuesday, December 8th, 2020 at 2:36 AM, Cornij, Nikola wrote: > [AMD Public Use] > > Hi Simon, > > It looks to me I'm kinda late to the party to look at your questions > under > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Famd-gfx%2F2020-November%2F056032.htmldata=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=T5ijUId6nrrKK3R7c62wVllZ1rEhKdmi95Foijvjf5M%3Dreserved=0... > > Does the commit below and > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > s.freedesktop.org%2Farchives%2Famd-gfx%2F2020-December%2F057048.htmldata=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=6GKjptsf0S%2FWzg9CwSnp89EkaQoMxgyq9JvVcVhJIyU%3Dreserved=0 > mean the above issue is now on its way to resolution? Yes, I've figured everything out about the cursor, thanks! If you have time, another thing I need feedback about is overlay planes. I have some bug reports [1] [2] about corruption when using a small 24x24 buffer near the edges of the screen. However everything works fine with a bigger 256x256 buffer. Is there a minimum buffer size for the overlay plane? Simon [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1385data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rXhMSAySLeVbVP7Pw6vw2B0%2BnrNcKqsveeFKRYMWNhE%3Dreserved=0 [2]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1386data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=gD%2FTWuBAVM8AuEfbY7c6tp%2Byns6I6hd9SihTKrO4so0%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 2/2] drm/amd/display: add cursor pitch check
[AMD Public Use] Very good, thanks! I'll take a look at the overlay ones, then. -Original Message- From: Simon Ser Sent: Tuesday, December 8, 2020 7:00 AM To: Cornij, Nikola Cc: Alex Deucher ; Kazlauskas, Nicholas ; Deucher, Alexander ; Wentland, Harry ; amd-gfx list Subject: RE: [PATCH 2/2] drm/amd/display: add cursor pitch check Hi Nikola, On Tuesday, December 8th, 2020 at 2:36 AM, Cornij, Nikola wrote: > [AMD Public Use] > > Hi Simon, > > It looks to me I'm kinda late to the party to look at your questions > under > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Famd-gfx%2F2020-November%2F056032.htmldata=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=T5ijUId6nrrKK3R7c62wVllZ1rEhKdmi95Foijvjf5M%3Dreserved=0... > > Does the commit below and > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > s.freedesktop.org%2Farchives%2Famd-gfx%2F2020-December%2F057048.htmldata=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=6GKjptsf0S%2FWzg9CwSnp89EkaQoMxgyq9JvVcVhJIyU%3Dreserved=0 > mean the above issue is now on its way to resolution? Yes, I've figured everything out about the cursor, thanks! If you have time, another thing I need feedback about is overlay planes. I have some bug reports [1] [2] about corruption when using a small 24x24 buffer near the edges of the screen. However everything works fine with a bigger 256x256 buffer. Is there a minimum buffer size for the overlay plane? Simon [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1385data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rXhMSAySLeVbVP7Pw6vw2B0%2BnrNcKqsveeFKRYMWNhE%3Dreserved=0 [2]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1386data=04%7C01%7CNikola.Cornij%40amd.com%7C6a860bd758d044c4a3bf08d89b70d628%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637430256236209108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=gD%2FTWuBAVM8AuEfbY7c6tp%2Byns6I6hd9SihTKrO4so0%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 2/2] drm/amd/display: add cursor pitch check
[AMD Public Use]
Hi Simon,
It looks to me I'm kinda late to the party to look at your questions under
https://lists.freedesktop.org/archives/amd-gfx/2020-November/056032.html...
Does the commit below and
https://lists.freedesktop.org/archives/amd-gfx/2020-December/057048.html mean
the above issue is now on its way to resolution?
Thanks,
Nikola
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Wednesday, December 2, 2020 5:25 PM
To: Kazlauskas, Nicholas
Cc: Deucher, Alexander ; Simon Ser
; Wentland, Harry ; amd-gfx list
Subject: Re: [PATCH 2/2] drm/amd/display: add cursor pitch check
On Wed, Dec 2, 2020 at 4:33 PM Kazlauskas, Nicholas
wrote:
>
> On 2020-12-02 4:09 p.m., Simon Ser wrote:
> > Replace the width check with a pitch check, which matches DM internals.
> > Add a new check to make sure the pitch (in pixels) matches the width.
> >
> > Signed-off-by: Simon Ser
> > Cc: Alex Deucher
> > Cc: Harry Wentland
> > Cc: Nicholas Kazlauskas
>
> Series is:
>
> Reviewed-by: Nicholas Kazlauskas
Applied. Thanks!
Alex
>
> Regards,
> Nicholas Kazlauskas
>
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 19 +++
> > 1 file changed, 15 insertions(+), 4 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 9e328101187e..862a59703060 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -8988,6 +8988,7 @@ static int dm_update_plane_state(struct dc *dc,
> > struct amdgpu_crtc *new_acrtc;
> > bool needs_reset;
> > int ret = 0;
> > + unsigned int pitch;
> >
> >
> > new_plane_crtc = new_plane_state->crtc; @@ -9021,15 +9022,25
> > @@ static int dm_update_plane_state(struct dc *dc,
> > return -EINVAL;
> > }
> >
> > - switch (new_plane_state->fb->width) {
> > + /* Pitch in pixels */
> > + pitch = new_plane_state->fb->pitches[0] /
> > + new_plane_state->fb->format->cpp[0];
> > +
> > + if (new_plane_state->fb->width != pitch) {
> > + DRM_DEBUG_ATOMIC("Cursor FB width %d doesn't
> > match pitch %d",
> > + new_plane_state->fb->width,
> > + pitch);
> > + return -EINVAL;
> > + }
> > +
> > + switch (pitch) {
> > case 64:
> > case 128:
> > case 256:
> > - /* FB width is supported by cursor plane */
> > + /* FB pitch is supported by cursor
> > + plane */
> > break;
> > default:
> > - DRM_DEBUG_ATOMIC("Bad cursor FB width %d\n",
> > - new_plane_state->fb->width);
> > + DRM_DEBUG_ATOMIC("Bad cursor FB pitch %d
> > px\n",
> > + pitch);
> > return -EINVAL;
> > }
> > }
> >
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cni
> kola.cornij%40amd.com%7C0837b50ac7d6455eef1b08d897111300%7C3dd8961fe48
> 84e608e11a82d994e183d%7C0%7C0%7C637425446939520218%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C1000sdata=DVh%2FvXWkMo%2FQiuV3OclCptN1ctSWpJaPR1sND3jXvHc%3D&
> amp;reserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cnikola.cornij%40amd.com%7C0837b50ac7d6455eef1b08d897111300%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637425446939520218%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=DVh%2FvXWkMo%2FQiuV3OclCptN1ctSWpJaPR1sND3jXvHc%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/display: allow 18 bit dp output on DCN3
Reviewed-by: Nikola Cornij
-Original Message-
From: Siqueira, Rodrigo
Sent: Wednesday, October 28, 2020 6:08 PM
To: amd-gfx@lists.freedesktop.org
Cc: Cornij, Nikola ; Liu, Zhan ;
Lakha, Bhawanpreet ; Wentland, Harry
; Laktyushkin, Dmytro ;
Park, Chris
Subject: [PATCH] drm/amd/display: allow 18 bit dp output on DCN3
From: Dmytro Laktyushkin
We need this to pass dp compliance.
Signed-off-by: Dmytro Laktyushkin
Reviewed-by: Chris Park
Acked-by: Rodrigo Siqueira
---
.../gpu/drm/amd/display/dc/dcn30/dcn30_resource.c | 14 --
.../amd/display/dc/dml/dcn30/display_mode_vba_30.c | 2 +-
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
index d1ed2a99bf65..d65496917e93 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
@@ -2020,20 +2020,6 @@ static bool dcn30_internal_validate_bw(
dml_log_mode_support_params(>bw_ctx.dml);
- /* TODO: Need to check calculated vlevel why that fails validation of
below resolutions */
- if (context->res_ctx.pipe_ctx[0].stream != NULL) {
- if (context->res_ctx.pipe_ctx[0].stream->timing.h_addressable
== 640 && context->res_ctx.pipe_ctx[0].stream->timing.v_addressable == 480)
- vlevel = 0;
- if (context->res_ctx.pipe_ctx[0].stream->timing.h_addressable
== 1280 && context->res_ctx.pipe_ctx[0].stream->timing.v_addressable == 800)
- vlevel = 0;
- if (context->res_ctx.pipe_ctx[0].stream->timing.h_addressable
== 1280 && context->res_ctx.pipe_ctx[0].stream->timing.v_addressable == 768)
- vlevel = 0;
- if (context->res_ctx.pipe_ctx[0].stream->timing.h_addressable
== 1280 && context->res_ctx.pipe_ctx[0].stream->timing.v_addressable == 1024)
- vlevel = 0;
- if (context->res_ctx.pipe_ctx[0].stream->timing.h_addressable
== 2048 && context->res_ctx.pipe_ctx[0].stream->timing.v_addressable == 1536)
- vlevel = 0;
- }
-
if (vlevel == context->bw_ctx.dml.soc.num_states)
goto validate_fail;
diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_mode_vba_30.c
b/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_mode_vba_30.c
index 9e0ae18e71fa..0f668699809d 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_mode_vba_30.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_mode_vba_30.c
@@ -3628,7 +3628,7 @@ static double TruncToValidBPP(
}
}
} else {
- if (!((DSCEnable == false && (DesiredBPP == NonDSCBPP2 ||
DesiredBPP == NonDSCBPP1 || DesiredBPP == NonDSCBPP0)) ||
+ if (!((DSCEnable == false && (DesiredBPP == NonDSCBPP2 ||
DesiredBPP
+== NonDSCBPP1 || DesiredBPP == NonDSCBPP0 || DesiredBPP == 18)) ||
(DSCEnable && DesiredBPP >= MinDSCBPP &&
DesiredBPP <= MaxDSCBPP))) {
return BPP_INVALID;
} else {
--
2.29.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/dsc: Return unsigned long on compute offset
If you're going to make all of them the same, then u64, please.
This is because I'm not sure if calculations require 64-bit at some stage.
-Original Message-
From: Lipski, Mikita
Sent: November 19, 2019 10:08 AM
To: Ville Syrjälä ; Lipski, Mikita
Cc: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; Cornij,
Nikola
Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset
On 19/11/2019 09:56, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lip...@amd.com wrote:
>> From: Mikita Lipski
>>
>> We shouldn't compare int with unsigned long to find the max value and
>> since we are not expecting negative value returned from
>> compute_offset we should make this function return unsigned long so
>> we can compare the values when computing rc parameters.
>
> Why are there other unsigned longs in dsc parameter computation in the
> first place?
I believe it was initially set to be unsigned long for variable consistency,
when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters.
But now that I look at it, we can actually just set them to u32 or u64, as
nothing should exceed that.
>
>>
>> Cc: Nikola Cornij
>> Cc: Harry Wentland
>> Signed-off-by: Mikita Lipski
>> ---
>> drivers/gpu/drm/drm_dsc.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
>> index 74f3527f567d..ec40604ab6a2 100644
>> --- a/drivers/gpu/drm/drm_dsc.c
>> +++ b/drivers/gpu/drm/drm_dsc.c
>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct
>> drm_dsc_picture_parameter_set *pps_payload,
>> }
>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>>
>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int
>> pixels_per_group,
>> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int
>> pixels_per_group,
>> int groups_per_line, int grpcnt)
>> {
>> -int offset = 0;
>> -int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
>> pixels_per_group);
>> +unsigned long offset = 0;
>> +unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
>> pixels_per_group);
>>
>> if (grpcnt <= grpcnt_id)
>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group *
>> vdsc_cfg->bits_per_pixel, 16);
>> --
>> 2.17.1
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lip...@amd.com
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 1/2] drm/dsc: Update drm_dsc to reflect native 4.2.0 DSC spec
Yes
-Original Message-
From: Wentland, Harry
Sent: November 14, 2019 4:22 PM
To: Cornij, Nikola ; Lipski, Mikita
; amd-gfx@lists.freedesktop.org
Cc: Wentland, Harry ; Deucher, Alexander
; manasi.d.nav...@intel.com; David Francis
Subject: Re: [PATCH 1/2] drm/dsc: Update drm_dsc to reflect native 4.2.0 DSC
spec
On 2019-11-14 4:20 p.m., Cornij, Nikola wrote:
> This looks good, too.
>
Can we treat this as your reviewed-by for both changes?
Usually people mark these as reviewed with a
Reviewed-by: Harry Wentland
Thanks,
Harry
> -Original Message-
> From: mikita.lip...@amd.com
> Sent: November 13, 2019 2:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Wentland, Harry ; Deucher, Alexander
> ; Cornij, Nikola ;
> manasi.d.nav...@intel.com; Lipski, Mikita ; David
> Francis
> Subject: [PATCH 1/2] drm/dsc: Update drm_dsc to reflect native 4.2.0 DSC spec
>
> From: Mikita Lipski
>
> [Why]
> Some parts of the DSC spec relating to 4.2.0 were not reflected in
> drm_dsc_compute_rc_parameters, causing unexpected config failures
>
> [How]
> Add nsl_bpg_offset and rbs_min computation
>
> Signed-off-by: David Francis
> Signed-off-by: Mikita Lipski
> ---
> drivers/gpu/drm/drm_dsc.c | 72 ---
> 1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c index
> 77f4e5ae4197..79c71e3fc973 100644
> --- a/drivers/gpu/drm/drm_dsc.c
> +++ b/drivers/gpu/drm/drm_dsc.c
> @@ -245,6 +245,38 @@ void drm_dsc_pps_payload_pack(struct
> drm_dsc_picture_parameter_set *pps_payload, }
> EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>
> +static int compute_offset(struct drm_dsc_config *vdsc_cfg, int
> pixels_per_group,
> + int groups_per_line, int grpcnt)
> +{
> + int offset = 0;
> + int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
> +pixels_per_group);
> +
> + if (grpcnt <= grpcnt_id)
> + offset = DIV_ROUND_UP(grpcnt * pixels_per_group *
> vdsc_cfg->bits_per_pixel, 16);
> + else
> + offset = DIV_ROUND_UP(grpcnt_id * pixels_per_group *
> vdsc_cfg->bits_per_pixel, 16)
> + - (((grpcnt - grpcnt_id) * vdsc_cfg->slice_bpg_offset)
> >> 11);
> +
> + if (grpcnt <= groups_per_line)
> + offset += grpcnt * vdsc_cfg->first_line_bpg_offset;
> + else
> + offset += groups_per_line * vdsc_cfg->first_line_bpg_offset
> + - (((grpcnt - groups_per_line) *
> vdsc_cfg->nfl_bpg_offset) >> 11);
> +
> + if (vdsc_cfg->native_420) {
> + if (grpcnt <= groups_per_line)
> + offset -= (grpcnt * vdsc_cfg->nsl_bpg_offset) >> 11;
> + else if (grpcnt <= 2 * groups_per_line)
> + offset += (grpcnt - groups_per_line) *
> vdsc_cfg->second_line_bpg_offset
> + - ((groups_per_line * vdsc_cfg->nsl_bpg_offset)
> >> 11);
> + else
> + offset += (grpcnt - groups_per_line) *
> vdsc_cfg->second_line_bpg_offset
> + - (((grpcnt - groups_per_line) *
> vdsc_cfg->nsl_bpg_offset) >> 11);
> + }
> +
> + return offset;
> +}
> +
> /**
> * drm_dsc_compute_rc_parameters() - Write rate control
> * parameters to the dsc configuration defined in @@ -264,6 +296,7 @@ int
> drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> unsigned long hrd_delay = 0;
> unsigned long final_scale = 0;
> unsigned long rbs_min = 0;
> + unsigned long max_offset = 0;
>
> if (vdsc_cfg->native_420 || vdsc_cfg->native_422) {
> /* Number of groups used to code each line of a slice */ @@
> -342,6 +375,17 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
> *vdsc_cfg)
> return -ERANGE;
> }
>
> + if (vdsc_cfg->slice_height > 2)
> + vdsc_cfg->nsl_bpg_offset =
> DIV_ROUND_UP((vdsc_cfg->second_line_bpg_offset << 11),
> + (vdsc_cfg->slice_height
> - 1));
> + else
> + vdsc_cfg->nsl_bpg_offset = 0;
> +
> + if (vdsc_cfg->nsl_bpg_offset > 65535) {
> + DRM_DEBUG_KMS("NslBpgOffset is too large for this slice
> height\n");
> + return -ERANGE;
> + }
> +
> /* Number of groups used to code the entire slice */
> groups_total = groups_per_line * vdsc_cfg->slice_height;
>
> @@ -361,6 +405,7 @@ int drm_d
RE: [PATCH 1/2] drm/dsc: Update drm_dsc to reflect native 4.2.0 DSC spec
This looks good, too.
-Original Message-
From: mikita.lip...@amd.com
Sent: November 13, 2019 2:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Wentland, Harry ; Deucher, Alexander
; Cornij, Nikola ;
manasi.d.nav...@intel.com; Lipski, Mikita ; David
Francis
Subject: [PATCH 1/2] drm/dsc: Update drm_dsc to reflect native 4.2.0 DSC spec
From: Mikita Lipski
[Why]
Some parts of the DSC spec relating to 4.2.0 were not reflected in
drm_dsc_compute_rc_parameters, causing unexpected config failures
[How]
Add nsl_bpg_offset and rbs_min computation
Signed-off-by: David Francis
Signed-off-by: Mikita Lipski
---
drivers/gpu/drm/drm_dsc.c | 72 ---
1 file changed, 68 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c index
77f4e5ae4197..79c71e3fc973 100644
--- a/drivers/gpu/drm/drm_dsc.c
+++ b/drivers/gpu/drm/drm_dsc.c
@@ -245,6 +245,38 @@ void drm_dsc_pps_payload_pack(struct
drm_dsc_picture_parameter_set *pps_payload, }
EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
+static int compute_offset(struct drm_dsc_config *vdsc_cfg, int
pixels_per_group,
+ int groups_per_line, int grpcnt)
+{
+ int offset = 0;
+ int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
+pixels_per_group);
+
+ if (grpcnt <= grpcnt_id)
+ offset = DIV_ROUND_UP(grpcnt * pixels_per_group *
vdsc_cfg->bits_per_pixel, 16);
+ else
+ offset = DIV_ROUND_UP(grpcnt_id * pixels_per_group *
vdsc_cfg->bits_per_pixel, 16)
+ - (((grpcnt - grpcnt_id) * vdsc_cfg->slice_bpg_offset)
>> 11);
+
+ if (grpcnt <= groups_per_line)
+ offset += grpcnt * vdsc_cfg->first_line_bpg_offset;
+ else
+ offset += groups_per_line * vdsc_cfg->first_line_bpg_offset
+ - (((grpcnt - groups_per_line) *
vdsc_cfg->nfl_bpg_offset) >> 11);
+
+ if (vdsc_cfg->native_420) {
+ if (grpcnt <= groups_per_line)
+ offset -= (grpcnt * vdsc_cfg->nsl_bpg_offset) >> 11;
+ else if (grpcnt <= 2 * groups_per_line)
+ offset += (grpcnt - groups_per_line) *
vdsc_cfg->second_line_bpg_offset
+ - ((groups_per_line * vdsc_cfg->nsl_bpg_offset)
>> 11);
+ else
+ offset += (grpcnt - groups_per_line) *
vdsc_cfg->second_line_bpg_offset
+ - (((grpcnt - groups_per_line) *
vdsc_cfg->nsl_bpg_offset) >> 11);
+ }
+
+ return offset;
+}
+
/**
* drm_dsc_compute_rc_parameters() - Write rate control
* parameters to the dsc configuration defined in @@ -264,6 +296,7 @@ int
drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
unsigned long hrd_delay = 0;
unsigned long final_scale = 0;
unsigned long rbs_min = 0;
+ unsigned long max_offset = 0;
if (vdsc_cfg->native_420 || vdsc_cfg->native_422) {
/* Number of groups used to code each line of a slice */ @@
-342,6 +375,17 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
return -ERANGE;
}
+ if (vdsc_cfg->slice_height > 2)
+ vdsc_cfg->nsl_bpg_offset =
DIV_ROUND_UP((vdsc_cfg->second_line_bpg_offset << 11),
+ (vdsc_cfg->slice_height
- 1));
+ else
+ vdsc_cfg->nsl_bpg_offset = 0;
+
+ if (vdsc_cfg->nsl_bpg_offset > 65535) {
+ DRM_DEBUG_KMS("NslBpgOffset is too large for this slice
height\n");
+ return -ERANGE;
+ }
+
/* Number of groups used to code the entire slice */
groups_total = groups_per_line * vdsc_cfg->slice_height;
@@ -361,6 +405,7 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
vdsc_cfg->scale_increment_interval =
(vdsc_cfg->final_offset * (1 << 11)) /
((vdsc_cfg->nfl_bpg_offset +
+ vdsc_cfg->nsl_bpg_offset +
vdsc_cfg->slice_bpg_offset) *
(final_scale - 9));
} else {
@@ -381,10 +426,29 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
* bits/pixel (bpp) rate that is used by the encoder,
* in steps of 1/16 of a bit per pixel
*/
- rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
- DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
-vdsc_cfg->bits_per_pixel, 16) +
- groups_per_line * vdsc_cfg->first_line_bpg_offset;
+ if (vdsc_cfg->dsc_ve
RE: [PATCH 2/2] drm/dsc: Use 32-bit integers for some DSC parameter calculations
Looks good to me
-Original Message-
From: mikita.lip...@amd.com
Sent: November 13, 2019 2:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Wentland, Harry ; Deucher, Alexander
; Cornij, Nikola ;
manasi.d.nav...@intel.com; Lipski, Mikita
Subject: [PATCH 2/2] drm/dsc: Use 32-bit integers for some DSC parameter
calculations
From: Mikita Lipski
[why]
There are a few DSC PPS parameters, such as scale_increment_interval, that
could overflow 16-bit integer if non-DSC-spec-compliant configuration was used.
There is a check in the code against that, however 16-bit integer is used to
store those values, which invalidates the check, letting invalid configurations
through.
[how]
Use 32-bit integers for the affected DSC PPS parameters calculations
Signed-off-by: Nikola Cornij
Signed-off-by: Mikita Lipski
---
drivers/gpu/drm/drm_dsc.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c index
79c71e3fc973..74f3527f567d 100644
--- a/drivers/gpu/drm/drm_dsc.c
+++ b/drivers/gpu/drm/drm_dsc.c
@@ -297,6 +297,9 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
unsigned long final_scale = 0;
unsigned long rbs_min = 0;
unsigned long max_offset = 0;
+ u32 nfl_bpg_offset;
+ u32 nsl_bpg_offset;
+ u32 scale_increment_interval;
if (vdsc_cfg->native_420 || vdsc_cfg->native_422) {
/* Number of groups used to code each line of a slice */ @@
-364,28 +367,32 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
* hence we multiply by 2^11 for preserving the
* fractional part
*/
- vdsc_cfg->nfl_bpg_offset =
DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
+ nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset
<<
+11),
(vdsc_cfg->slice_height
- 1));
else
- vdsc_cfg->nfl_bpg_offset = 0;
+ nfl_bpg_offset = 0;
/* 2^16 - 1 */
- if (vdsc_cfg->nfl_bpg_offset > 65535) {
+ if (nfl_bpg_offset > 65535) {
DRM_DEBUG_KMS("NflBpgOffset is too large for this slice
height\n");
return -ERANGE;
}
+ vdsc_cfg->nfl_bpg_offset = (u16)nfl_bpg_offset;
+
if (vdsc_cfg->slice_height > 2)
- vdsc_cfg->nsl_bpg_offset =
DIV_ROUND_UP((vdsc_cfg->second_line_bpg_offset << 11),
+ nsl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->second_line_bpg_offset
<<
+11),
(vdsc_cfg->slice_height
- 1));
else
- vdsc_cfg->nsl_bpg_offset = 0;
+ nsl_bpg_offset = 0;
- if (vdsc_cfg->nsl_bpg_offset > 65535) {
+ if (nsl_bpg_offset > 65535) {
DRM_DEBUG_KMS("NslBpgOffset is too large for this slice
height\n");
return -ERANGE;
}
+ vdsc_cfg->nsl_bpg_offset = (u16)nsl_bpg_offset;
+
/* Number of groups used to code the entire slice */
groups_total = groups_per_line * vdsc_cfg->slice_height;
@@ -402,10 +409,10 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
* as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional
value,
* we need divide by 2^11 from pstDscCfg values
*/
- vdsc_cfg->scale_increment_interval =
+ scale_increment_interval =
(vdsc_cfg->final_offset * (1 << 11)) /
- ((vdsc_cfg->nfl_bpg_offset +
- vdsc_cfg->nsl_bpg_offset +
+ ((nfl_bpg_offset +
+ nsl_bpg_offset +
vdsc_cfg->slice_bpg_offset) *
(final_scale - 9));
} else {
@@ -413,14 +420,16 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config
*vdsc_cfg)
* If finalScaleValue is less than or equal to 9, a value of 0
should
* be used to disable the scale increment at the end of the
slice
*/
- vdsc_cfg->scale_increment_interval = 0;
+ scale_increment_interval = 0;
}
- if (vdsc_cfg->scale_increment_interval > 65535) {
+ if (scale_increment_interval > 65535) {
DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice
height\n");
return -ERANGE;
}
+ vdsc_cfg->scale_increment_interval = (u16)scale_increment_interval;
+
/*
* DSC spec mentions that bits_per_pixel specifies the target
* bits/pi
