Re: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in amdgpu_dm_setup_replay
[AMD Official Use Only - General]
There was a consensus to use memset instead of {0}. I remember making changes
related to that previously.
Bhawan
From: Mahfooz, Hamza
Sent: October 27, 2023 11:53 AM
To: Yuran Pereira ; airl...@gmail.com
Cc: Li, Sun peng (Leo) ; Lakha, Bhawanpreet
; Pan, Xinhui ; Siqueira,
Rodrigo ; linux-ker...@vger.kernel.org
; amd-...@lists.freedesktop.org
; dri-devel@lists.freedesktop.org
; Deucher, Alexander
; Koenig, Christian ;
linux-kernel-ment...@lists.linuxfoundation.org
Subject: Re: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in
amdgpu_dm_setup_replay
On 10/26/23 17:25, Yuran Pereira wrote:
> Since `pr_config` is not initialized after its declaration, the
> following operations with `replay_enable_option` may be performed
> when `replay_enable_option` is holding junk values which could
> possibly lead to undefined behaviour
>
> ```
> ...
> pr_config.replay_enable_option |= pr_enable_option_static_screen;
> ...
>
> if (!pr_config.replay_timing_sync_supported)
> pr_config.replay_enable_option &= ~pr_enable_option_general_ui;
> ...
> ```
>
> This patch initializes `pr_config` after its declaration to ensure that
> it doesn't contain junk data, and prevent any undefined behaviour
>
> Addresses-Coverity-ID: 1544428 ("Uninitialized scalar variable")
> Fixes: dede1fea4460 ("drm/amd/display: Add Freesync Panel DM code")
> Signed-off-by: Yuran Pereira
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> index 32d3086c4cb7..40526507f50b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> @@ -23,6 +23,7 @@
>*
>*/
>
> +#include
> #include "amdgpu_dm_replay.h"
> #include "dc.h"
> #include "dm_helpers.h"
> @@ -74,6 +75,8 @@ bool amdgpu_dm_setup_replay(struct dc_link *link, struct
> amdgpu_dm_connector *ac
>struct replay_config pr_config;
I would prefer setting pr_config = {0};
>union replay_debug_flags *debug_flags = NULL;
>
> + memset(_config, 0, sizeof(pr_config));
> +
>// For eDP, if Replay is supported, return true to skip checks
>if (link->replay_settings.config.replay_supported)
>return true;
--
Hamza
Re: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in amdgpu_dm_setup_replay
[AMD Official Use Only - General]
Thanks,
Reviewed-by: Bhawanpreet Lakha
From: Yuran Pereira
Sent: October 26, 2023 5:25 PM
To: airl...@gmail.com
Cc: Yuran Pereira ; Wentland, Harry
; Li, Sun peng (Leo) ; Siqueira,
Rodrigo ; Deucher, Alexander
; Koenig, Christian ; Pan,
Xinhui ; dan...@ffwll.ch ; Lakha,
Bhawanpreet ; amd-...@lists.freedesktop.org
; dri-devel@lists.freedesktop.org
; linux-ker...@vger.kernel.org
; linux-kernel-ment...@lists.linuxfoundation.org
Subject: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in
amdgpu_dm_setup_replay
Since `pr_config` is not initialized after its declaration, the
following operations with `replay_enable_option` may be performed
when `replay_enable_option` is holding junk values which could
possibly lead to undefined behaviour
```
...
pr_config.replay_enable_option |= pr_enable_option_static_screen;
...
if (!pr_config.replay_timing_sync_supported)
pr_config.replay_enable_option &= ~pr_enable_option_general_ui;
...
```
This patch initializes `pr_config` after its declaration to ensure that
it doesn't contain junk data, and prevent any undefined behaviour
Addresses-Coverity-ID: 1544428 ("Uninitialized scalar variable")
Fixes: dede1fea4460 ("drm/amd/display: Add Freesync Panel DM code")
Signed-off-by: Yuran Pereira
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
index 32d3086c4cb7..40526507f50b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
@@ -23,6 +23,7 @@
*
*/
+#include
#include "amdgpu_dm_replay.h"
#include "dc.h"
#include "dm_helpers.h"
@@ -74,6 +75,8 @@ bool amdgpu_dm_setup_replay(struct dc_link *link, struct
amdgpu_dm_connector *ac
struct replay_config pr_config;
union replay_debug_flags *debug_flags = NULL;
+ memset(_config, 0, sizeof(pr_config));
+
// For eDP, if Replay is supported, return true to skip checks
if (link->replay_settings.config.replay_supported)
return true;
--
2.25.1
Re: [PATCH] drm: Update MST First Link Slot Information Based on Encoding Format
[AMD Official Use Only]
Hi Lyude,
Jerry is busy these few weeks so I will be taking a look at this. I added the
start/total slots to the mst_state and am updating them in atomic_check.
Also ignore the V2 "* Remove get_mst_link_encoding_cap" I got a bit lost in
trying to figure out the patch layout that was sent before.
Thanks,
Bhawan
From: Bhawanpreet Lakha
Sent: October 12, 2021 5:58 PM
To: Zuo, Jerry ; dri-devel@lists.freedesktop.org
; ly...@redhat.com
Cc: Wentland, Harry ; Lin, Wayne ;
Kazlauskas, Nicholas ; Lakha, Bhawanpreet
Subject: [PATCH] drm: Update MST First Link Slot Information Based on Encoding
Format
8b/10b encoding format requires to reserve the first slot for
recording metadata. Real data transmission starts from the second slot,
with a total of available 63 slots available.
In 128b/132b encoding format, metadata is transmitted separately
in LLCP packet before MTP. Real data transmission starts from
the first slot, with a total of 64 slots available.
v2:
* Remove get_mst_link_encoding_cap
* Move total/start slots to mst_state, and copy it to mst_mgr in
atomic_check
Signed-off-by: Fangzhi Zuo
Signed-off-by: Bhawanpreet Lakha
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++
drivers/gpu/drm/drm_dp_mst_topology.c | 35 +++
include/drm/drm_dp_mst_helper.h | 13 +++
3 files changed, 69 insertions(+), 7 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 5020f2d36fe1..4ad50eb0091a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10612,6 +10612,8 @@ static int amdgpu_dm_atomic_check(struct drm_device
*dev,
#if defined(CONFIG_DRM_AMD_DC_DCN)
struct dsc_mst_fairness_vars vars[MAX_PIPES];
#endif
+ struct drm_dp_mst_topology_state *mst_state;
+ struct drm_dp_mst_topology_mgr *mgr;
trace_amdgpu_dm_atomic_check_begin(state);
@@ -10819,6 +10821,32 @@ static int amdgpu_dm_atomic_check(struct drm_device
*dev,
lock_and_validation_needed = true;
}
+#if defined(CONFIG_DRM_AMD_DC_DCN)
+ for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+ struct amdgpu_dm_connector *aconnector;
+ struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
+ u8 link_coding_cap;
+
+ if (!mgr->mst_state )
+ continue;
+
+ drm_connector_list_iter_begin(dev, );
+ drm_for_each_connector_iter(connector, ) {
+ int id = connector->index;
+
+ if (id == mst_state->mgr->conn_base_id) {
+ aconnector = to_amdgpu_dm_connector(connector);
+ link_coding_cap =
dc_link_dp_mst_decide_link_encoding_format(aconnector->dc_link);
+ drm_dp_mst_update_coding_cap(mst_state,
link_coding_cap);
+
+ break;
+ }
+ }
+ drm_connector_list_iter_end();
+
+ }
+#endif
/**
* Streams and planes are reset when there are changes that affect
* bandwidth. Anything that affects bandwidth needs to go through
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index ad0795afc21c..fb5c47c4cb2e 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
drm_dp_mst_topology_mgr *mgr)
struct drm_dp_payload req_payload;
struct drm_dp_mst_port *port;
int i, j;
- int cur_slots = 1;
+ int cur_slots = mgr->start_slot;
bool skip;
mutex_lock(>payload_lock);
@@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr
*mgr,
num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
/* max. time slots - one slot for MTP header */
- if (num_slots > 63)
+ if (num_slots > mgr->total_avail_slots)
return -ENOSPC;
return num_slots;
}
@@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
drm_dp_mst_topology_mgr *mgr,
int ret;
/* max. time slots - one slot for MTP header */
- if (slots > 63)
+ if (slots > mgr->total_avail_slots)
return -ENOSPC;
vcpi->pbn = pbn;
@@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
+void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state *mst_state,
uint8_t link_coding_cap)
+{
+ if (link_coding_cap == DP_CAP_ANSI_128B132B) {
+ mst_state->total_a
Re: [PATCH] drm/amd/display: Fix off by one in hdmi_14_process_transaction()
[AMD Official Use Only - Internal Distribution Only]
Thanks
Reviewed-by: Bhawanpreet Lakha
From: Dan Carpenter
Sent: March 2, 2021 6:15 AM
To: Wentland, Harry ; Lakha, Bhawanpreet
Cc: Li, Sun peng (Leo) ; Deucher, Alexander
; Koenig, Christian ;
David Airlie ; Daniel Vetter ; Dan Carpenter
; Lakha, Bhawanpreet ;
Siqueira, Rodrigo ; Liu, Wenjing
; amd-...@lists.freedesktop.org
; dri-devel@lists.freedesktop.org
; kernel-janit...@vger.kernel.org
Subject: [PATCH] drm/amd/display: Fix off by one in
hdmi_14_process_transaction()
The hdcp_i2c_offsets[] array did not have an entry for
HDCP_MESSAGE_ID_WRITE_CONTENT_STREAM_TYPE so it led to an off by one
read overflow. I added an entry and copied the 0x0 value for the offset
from similar code in drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c.
I also declared several of these arrays as having HDCP_MESSAGE_ID_MAX
entries. This doesn't change the code, but it's just a belt and
suspenders approach to try future proof the code.
Fixes: 4c283fdac08a ("drm/amd/display: Add HDCP module")
Signed-off-by: Dan Carpenter
---
>From static analysis, as mentioned in the commit message the offset
is basically an educated guess.
I reported this bug on Apr 16, 2020 but I guess we lost take of it.
drivers/gpu/drm/amd/display/dc/hdcp/hdcp_msg.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/hdcp/hdcp_msg.c
b/drivers/gpu/drm/amd/display/dc/hdcp/hdcp_msg.c
index 5e384a8a83dc..51855a2624cf 100644
--- a/drivers/gpu/drm/amd/display/dc/hdcp/hdcp_msg.c
+++ b/drivers/gpu/drm/amd/display/dc/hdcp/hdcp_msg.c
@@ -39,7 +39,7 @@
#define HDCP14_KSV_SIZE 5
#define HDCP14_MAX_KSV_FIFO_SIZE 127*HDCP14_KSV_SIZE
-static const bool hdcp_cmd_is_read[] = {
+static const bool hdcp_cmd_is_read[HDCP_MESSAGE_ID_MAX] = {
[HDCP_MESSAGE_ID_READ_BKSV] = true,
[HDCP_MESSAGE_ID_READ_RI_R0] = true,
[HDCP_MESSAGE_ID_READ_PJ] = true,
@@ -75,7 +75,7 @@ static const bool hdcp_cmd_is_read[] = {
[HDCP_MESSAGE_ID_WRITE_CONTENT_STREAM_TYPE] = false
};
-static const uint8_t hdcp_i2c_offsets[] = {
+static const uint8_t hdcp_i2c_offsets[HDCP_MESSAGE_ID_MAX] = {
[HDCP_MESSAGE_ID_READ_BKSV] = 0x0,
[HDCP_MESSAGE_ID_READ_RI_R0] = 0x8,
[HDCP_MESSAGE_ID_READ_PJ] = 0xA,
@@ -106,7 +106,8 @@ static const uint8_t hdcp_i2c_offsets[] = {
[HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_SEND_ACK] = 0x60,
[HDCP_MESSAGE_ID_WRITE_REPEATER_AUTH_STREAM_MANAGE] = 0x60,
[HDCP_MESSAGE_ID_READ_REPEATER_AUTH_STREAM_READY] = 0x80,
- [HDCP_MESSAGE_ID_READ_RXSTATUS] = 0x70
+ [HDCP_MESSAGE_ID_READ_RXSTATUS] = 0x70,
+ [HDCP_MESSAGE_ID_WRITE_CONTENT_STREAM_TYPE] = 0x0,
};
struct protection_properties {
@@ -184,7 +185,7 @@ static const struct protection_properties
hdmi_14_protection = {
.process_transaction = hdmi_14_process_transaction
};
-static const uint32_t hdcp_dpcd_addrs[] = {
+static const uint32_t hdcp_dpcd_addrs[HDCP_MESSAGE_ID_MAX] = {
[HDCP_MESSAGE_ID_READ_BKSV] = 0x68000,
[HDCP_MESSAGE_ID_READ_RI_R0] = 0x68005,
[HDCP_MESSAGE_ID_READ_PJ] = 0x,
--
2.30.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH -next] drm/amd/display: tweak the kerneldoc for active_vblank_irq_count
[AMD Official Use Only - Internal Distribution Only]
Thanks,
Reviewed-by: Bhawanpreet Lakha
From: Lukas Bulwahn
Sent: January 11, 2021 3:46 AM
To: Lakha, Bhawanpreet ; Kazlauskas, Nicholas
; Deucher, Alexander ;
Koenig, Christian ; amd-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org ;
linux-ker...@vger.kernel.org ;
linux-...@vger.kernel.org ;
kernel-janit...@vger.kernel.org ; Lukas
Bulwahn
Subject: [PATCH -next] drm/amd/display: tweak the kerneldoc for
active_vblank_irq_count
Commit 71338cb4a7c2 ("drm/amd/display: enable idle optimizations for linux
(MALL stutter)") adds active_vblank_irq_count to amdgpu_display_manager
in ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h.
The kerneldoc is incorrectly formatted, and make htmldocs warns:
./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:
340: warning: Incorrect use of kernel-doc format: *
@active_vblank_irq_count
379: warning: Function parameter or member 'active_vblank_irq_count' not
described in 'amdgpu_display_manager'
Tweak the kerneldoc for active_vblank_irq_count.
Signed-off-by: Lukas Bulwahn
---
applies on amdgpu's -next and next-20210111
Bhawanpreet, Nick, please review and ack.
Alex, Christian, please pick on top of the commit above.
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f084e2fc9569..5ee1b766884e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -337,7 +337,7 @@ struct amdgpu_display_manager {
const struct gpu_info_soc_bounding_box_v1_0 *soc_bounding_box;
/**
-* @active_vblank_irq_count
+* @active_vblank_irq_count:
*
* number of currently active vblank irqs
*/
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null
[AMD Official Use Only - Internal Distribution Only]
I took a closer look at this and there seems to be an issue in the function.
Crtc being null is a valid case here. The sequence that leads to this is,
unplug -> disable crtc/release vcpi slots then hotplug. The issue is that
pos->port is not guaranteed to be released in
drm_dp_atomic_release_vcpi_slots() so list_for_each_entry(pos,
_state->vcpis, next) {} might still iterate thought it.
So, when a hotplug is done we still loop through the old port which has port! =
null, crtc = null, and vpci = 0. I didn't find anything that I can use to
remove the port from the list. So, a potential solution to this would be to add
a check for vpci = 0 and skip that port.
Thoughts/Suggestions?
Bhawan
From: amd-gfx on behalf of Lakha,
Bhawanpreet
Sent: August 14, 2020 2:52 PM
To: Ruhl, Michael J ; Lipski, Mikita
; Kazlauskas, Nicholas ;
Deucher, Alexander
Cc: amd-...@lists.freedesktop.org ;
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null
pos->port->connector?
This is checking the crtc not the connector. The crtc can be null if its
disabled.
Since it is happening after a unplug->hotplug, I guess we are missing something
in the disable sequence and the old connector is still in the list.
Bhawan
>>-Original Message-
>>From: dri-devel On Behalf Of
>>Bhawanpreet Lakha
>>Sent: Friday, August 14, 2020 1:02 PM
>>To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com;
>>alexander.deuc...@amd.com
>>Cc: Bhawanpreet Lakha ; dri-
>>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
>>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null
>>
>>[Why]
>>In certain cases the crtc can be NULL and returning -EINVAL causes
>>atomic check to fail when it shouln't. This leads to valid
>>configurations failing because atomic check fails.
>
>So is this a bug fix or an exception case, or an expected possibility?
>
>From my reading of the function comments, it is not clear that
>pos->port->connector
>might be NULL for some reason.
>A better explanation of why this would occur would make this a much more
>useful commit message.
>
>My reading is that you ran into this issue an are masking it with this fix.
>
>Rather than this is a real possibility and this is the correct fix.
>
>Mike
>
>>[How]
>>Don't early return if crtc is null
>>
>>Signed-off-by: Bhawanpreet Lakha
>>---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>>b/drivers/gpu/drm/drm_dp_mst_topology.c
>>index 70c4b7afed12..bc90a1485699 100644
>>--- a/drivers/gpu/drm/drm_dp_mst_topology.c
>>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct
>>drm_atomic_state *state, struct drm
>>
>>crtc = conn_state->crtc;
>>
>>- if (WARN_ON(!crtc))
>>- return -EINVAL;
>>+ if (!crtc)
>>+ continue;
>>
>>if (!drm_dp_mst_dsc_aux_for_port(pos->port))
>>continue;
>>--
>>2.17.1
>>
>>___
>>dri-devel mailing list
>>dri-devel@lists.freedesktop.org
>>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-develdata=02%7C01%7CBhawanpreet.Lakha%40amd.com%7C0f5d55c551644fef3df908d840787b3e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330233520819407sdata=5N%2BBb0Qp3bd5zANfxovb%2BrVLAGnbP1sjyw3EeCHXj2w%3Dreserved=0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/dp_mst: Don't return error code when crtc is null
[AMD Official Use Only - Internal Distribution Only] pos->port->connector? This is checking the crtc not the connector. The crtc can be null if its disabled. Since it is happening after a unplug->hotplug, I guess we are missing something in the disable sequence and the old connector is still in the list. Bhawan >>-Original Message- >>From: dri-devel On Behalf Of >>Bhawanpreet Lakha >>Sent: Friday, August 14, 2020 1:02 PM >>To: mikita.lip...@amd.com; nicholas.kazlaus...@amd.com; >>alexander.deuc...@amd.com >>Cc: Bhawanpreet Lakha ; dri- >>de...@lists.freedesktop.org; amd-...@lists.freedesktop.org >>Subject: [PATCH] drm/dp_mst: Don't return error code when crtc is null >> >>[Why] >>In certain cases the crtc can be NULL and returning -EINVAL causes >>atomic check to fail when it shouln't. This leads to valid >>configurations failing because atomic check fails. > >So is this a bug fix or an exception case, or an expected possibility? > >From my reading of the function comments, it is not clear that >pos->port->connector >might be NULL for some reason. >A better explanation of why this would occur would make this a much more >useful commit message. > >My reading is that you ran into this issue an are masking it with this fix. > >Rather than this is a real possibility and this is the correct fix. > >Mike > >>[How] >>Don't early return if crtc is null >> >>Signed-off-by: Bhawanpreet Lakha >>--- >> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >>diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >>b/drivers/gpu/drm/drm_dp_mst_topology.c >>index 70c4b7afed12..bc90a1485699 100644 >>--- a/drivers/gpu/drm/drm_dp_mst_topology.c >>+++ b/drivers/gpu/drm/drm_dp_mst_topology.c >>@@ -5037,8 +5037,8 @@ int drm_dp_mst_add_affected_dsc_crtcs(struct >>drm_atomic_state *state, struct drm >> >>crtc = conn_state->crtc; >> >>- if (WARN_ON(!crtc)) >>- return -EINVAL; >>+ if (!crtc) >>+ continue; >> >>if (!drm_dp_mst_dsc_aux_for_port(pos->port)) >>continue; >>-- >>2.17.1 >> >>___ >>dri-devel mailing list >>dri-devel@lists.freedesktop.org >>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-develdata=02%7C01%7CBhawanpreet.Lakha%40amd.com%7C0f5d55c551644fef3df908d840787b3e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330233520819407sdata=5N%2BBb0Qp3bd5zANfxovb%2BrVLAGnbP1sjyw3EeCHXj2w%3Dreserved=0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm/amd/display: Add HDCP module - static analysis bug report
Hi Daniel,
I have the patches prepared but they needed some testing before I send them
(code needed a slight refactor to use the drm_hdcp.h), I should be able to send
the patches this week.
Thanks,
Bhawan
On 2019-11-04 10:23 a.m., Wentland, Harry wrote:
> On 2019-11-04 5:53 a.m., Daniel Vetter wrote:
>> On Wed, Oct 9, 2019 at 10:58 PM Daniel Vetter wrote:
>>> On Wed, Oct 9, 2019 at 10:46 PM Lakha, Bhawanpreet
>>> wrote:
>>>> I misunderstood and was talking about the ksv validation specifically
>>>> (usage of drm_hdcp_check_ksvs_revoked()).
>>> Hm for that specifically I think you want to do both, i.e. both
>>> consult your psp, but also check for revoked ksvs with the core
>>> helper. At least on some platforms only the core helper might have the
>>> updated revoke list.
>>>
> I think it's an either/or. Either we use an HDCP implementation that's
> fully running in x86 kernel space (still not sure how that's compliant)
> or we fully rely on our PSP FW to do what it's designed to do. I don't
> think it makes sense to mix and match here.
>
>>>> For the defines I will create patches to use drm_hdcp where it is usable.
>>> Thanks a lot. Ime once we have shared definitions it's much easier to
>>> also share some helpers, where it makes sense.
>>>
>>> Aside I think the hdcp code could also use a bit of demidlayering. At
>>> least I'm not understanding why you add a 2nd abstraction layer for
>>> i2c/dpcd, dm_helper already has that. That seems like one abstraction
>>> layer too much.
>> I haven't seen anything fly by or in the latest pull request ... you
>> folks still working on this or more put on the "maybe, probably never"
>> pile?
>>
> Following up with Bhawan.
>
> Harry
>
>> -Daniel
>>
>>
>>> -Daniel
>>>
>>>>
>>>> Bhawan
>>>>
>>>> On 2019-10-09 2:43 p.m., Daniel Vetter wrote:
>>>>> On Wed, Oct 9, 2019 at 8:23 PM Lakha, Bhawanpreet
>>>>> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The reason we don't use drm_hdcp is because our policy is to do hdcp
>>>>>> verification using PSP/HW (onboard secure processor).
>>>>> i915 also uses hw to auth, we still use the parts from drm_hdcp ...
>>>>> Did you actually look at what's in there? It's essentially just shared
>>>>> defines and data structures from the standard, plus a few minimal
>>>>> helpers to en/decode some bits. Just from a quick read the entire
>>>>> patch very much looks like midlayer everywhere design that we
>>>>> discussed back when DC landed ...
>>>>> -Daniel
>>>>>
>>>>>> Bhawan
>>>>>>
>>>>>> On 2019-10-09 12:32 p.m., Daniel Vetter wrote:
>>>>>>> On Thu, Oct 03, 2019 at 11:08:03PM +0100, Colin Ian King wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Static analysis with Coverity has detected a potential issue with
>>>>>>>> function validate_bksv in
>>>>>>>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c with recent
>>>>>>>> commit:
>>>>>>>>
>>>>>>>> commit ed9d8e2bcb003ec94658cafe9b1bb3960e2139ec
>>>>>>>> Author: Bhawanpreet Lakha
>>>>>>>> Date: Tue Aug 6 17:52:01 2019 -0400
>>>>>>>>
>>>>>>>>drm/amd/display: Add HDCP module
>>>>>>> I think the real question here is ... why is this not using drm_hdcp?
>>>>>>> -Daniel
>>>>>>>
>>>>>>>> The analysis is as follows:
>>>>>>>>
>>>>>>>> 28 static inline enum mod_hdcp_status validate_bksv(struct
>>>>>>>> mod_hdcp *hdcp)
>>>>>>>> 29 {
>>>>>>>>
>>>>>>>> CID 89852 (#1 of 1): Out-of-bounds read (OVERRUN)
>>>>>>>>
>>>>>>>> 1. overrun-local:
>>>>>>>> Overrunning array of 5 bytes at byte offset 7 by dereferencing pointer
>>>>>>>> (uint64_t *)hdcp->auth.msg.hdcp1.bksv.
>>>>>>>>
>>>>>>>> 30uint64_t n = *(uint64_t *)hdcp->auth.msg.hdcp1.bksv;
>>>>>>>> 31uint8_t count = 0;
>>>>>&g
Re: [PATCH] drm/amdgpu/display: fix build error casused by CONFIG_DRM_AMD_DC_DCN2_1
Reviewed-by: Bhawanpreet Lakha
On 2019-10-15 12:51 p.m., Hersen Wu wrote:
> when CONFIG_DRM_AMD_DC_DCN2_1 is not enable in .config,
> there is build error. struct dpm_clocks shoud not be
> guarded.
>
> Signed-off-by: Hersen Wu
> ---
> drivers/gpu/drm/amd/display/dc/dm_pp_smu.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
> b/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
> index 24d65dbbd749..ef7df9ef6d7e 100644
> --- a/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
> +++ b/drivers/gpu/drm/amd/display/dc/dm_pp_smu.h
> @@ -249,8 +249,6 @@ struct pp_smu_funcs_nv {
> };
> #endif
>
> -#if defined(CONFIG_DRM_AMD_DC_DCN2_1)
> -
> #define PP_SMU_NUM_SOCCLK_DPM_LEVELS 8
> #define PP_SMU_NUM_DCFCLK_DPM_LEVELS 8
> #define PP_SMU_NUM_FCLK_DPM_LEVELS4
> @@ -288,7 +286,6 @@ struct pp_smu_funcs_rn {
> enum pp_smu_status (*get_dpm_clock_table) (struct pp_smu *pp,
> struct dpm_clocks *clock_table);
> };
> -#endif
>
> struct pp_smu_funcs {
> struct pp_smu ctx;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu/display: hook renoir dc to pplib funcs
Reviewed-by: Bhawanpreet Lakha
On 2019-10-15 11:04 a.m., Hersen Wu wrote:
> enable dc get dmp clock table and set dcn watermarks
> via pplib.
>
> Signed-off-by: Hersen Wu
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c | 93 +++
> drivers/gpu/drm/amd/display/dc/dm_pp_smu.h| 2 +-
> 2 files changed, 94 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
> index 95564b8de3ce..7add40dea9b7 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
> @@ -891,6 +891,90 @@ enum pp_smu_status pp_nv_get_uclk_dpm_states(struct
> pp_smu *pp,
> return PP_SMU_RESULT_FAIL;
> }
>
> +#ifdef CONFIG_DRM_AMD_DC_DCN2_1
> +enum pp_smu_status pp_rn_get_dpm_clock_table(
> + struct pp_smu *pp, struct dpm_clocks *clock_table)
> +{
> + const struct dc_context *ctx = pp->dm;
> + struct amdgpu_device *adev = ctx->driver_context;
> + struct smu_context *smu = >smu;
> +
> + if (!smu->ppt_funcs)
> + return PP_SMU_RESULT_UNSUPPORTED;
> +
> + if (!smu->ppt_funcs->get_dpm_clock_table)
> + return PP_SMU_RESULT_UNSUPPORTED;
> +
> + if (!smu->ppt_funcs->get_dpm_clock_table(smu, clock_table))
> + return PP_SMU_RESULT_OK;
> +
> + return PP_SMU_RESULT_FAIL;
> +}
> +
> +enum pp_smu_status pp_rn_set_wm_ranges(struct pp_smu *pp,
> + struct pp_smu_wm_range_sets *ranges)
> +{
> + const struct dc_context *ctx = pp->dm;
> + struct amdgpu_device *adev = ctx->driver_context;
> + struct smu_context *smu = >smu;
> + struct dm_pp_wm_sets_with_clock_ranges_soc15 wm_with_clock_ranges;
> + struct dm_pp_clock_range_for_dmif_wm_set_soc15 *wm_dce_clocks =
> + wm_with_clock_ranges.wm_dmif_clocks_ranges;
> + struct dm_pp_clock_range_for_mcif_wm_set_soc15 *wm_soc_clocks =
> + wm_with_clock_ranges.wm_mcif_clocks_ranges;
> + int32_t i;
> +
> + if (!smu->funcs)
> + return PP_SMU_RESULT_UNSUPPORTED;
> +
> + wm_with_clock_ranges.num_wm_dmif_sets = ranges->num_reader_wm_sets;
> + wm_with_clock_ranges.num_wm_mcif_sets = ranges->num_writer_wm_sets;
> +
> + for (i = 0; i < wm_with_clock_ranges.num_wm_dmif_sets; i++) {
> + if (ranges->reader_wm_sets[i].wm_inst > 3)
> + wm_dce_clocks[i].wm_set_id = WM_SET_A;
> + else
> + wm_dce_clocks[i].wm_set_id =
> + ranges->reader_wm_sets[i].wm_inst;
> +
> + wm_dce_clocks[i].wm_min_dcfclk_clk_in_khz =
> + ranges->reader_wm_sets[i].min_drain_clk_mhz;
> +
> + wm_dce_clocks[i].wm_max_dcfclk_clk_in_khz =
> + ranges->reader_wm_sets[i].max_drain_clk_mhz;
> +
> + wm_dce_clocks[i].wm_min_mem_clk_in_khz =
> + ranges->reader_wm_sets[i].min_fill_clk_mhz;
> +
> + wm_dce_clocks[i].wm_max_mem_clk_in_khz =
> + ranges->reader_wm_sets[i].max_fill_clk_mhz;
> + }
> +
> + for (i = 0; i < wm_with_clock_ranges.num_wm_mcif_sets; i++) {
> + if (ranges->writer_wm_sets[i].wm_inst > 3)
> + wm_soc_clocks[i].wm_set_id = WM_SET_A;
> + else
> + wm_soc_clocks[i].wm_set_id =
> + ranges->writer_wm_sets[i].wm_inst;
> + wm_soc_clocks[i].wm_min_socclk_clk_in_khz =
> + ranges->writer_wm_sets[i].min_fill_clk_mhz;
> +
> + wm_soc_clocks[i].wm_max_socclk_clk_in_khz =
> + ranges->writer_wm_sets[i].max_fill_clk_mhz;
> +
> + wm_soc_clocks[i].wm_min_mem_clk_in_khz =
> + ranges->writer_wm_sets[i].min_drain_clk_mhz;
> +
> + wm_soc_clocks[i].wm_max_mem_clk_in_khz =
> + ranges->writer_wm_sets[i].max_drain_clk_mhz;
> + }
> +
> + smu_set_watermarks_for_clock_ranges(>smu, _with_clock_ranges);
> +
> + return PP_SMU_RESULT_OK;
> +}
> +#endif
> +
> void dm_pp_get_funcs(
> struct dc_context *ctx,
> struct pp_smu_funcs *funcs)
> @@ -935,6 +1019,15 @@ void dm_pp_get_funcs(
> funcs->nv_funcs.set_pstate_handshake_support =
> pp_nv_set_pstate_handshake_support;
> break;
> #endif
> +
> +#ifdef CONFIG_DRM_AMD_DC_DCN2_1
> + case DCN_VERSION_2_1:
> + funcs->ctx.ver = PP_SMU_VER_RN;
> + funcs->rn_funcs.pp_smu.dm = ctx;
> + funcs->rn_funcs.set_wm_ranges = pp_rn_set_wm_ranges;
> + funcs->rn_funcs.get_dpm_clock_table = pp_rn_get_dpm_clock_table;
> + break;
> +#endif
> default:
> DRM_ERROR("smu version is not supported !\n");
> break;
> diff --git
Re: drm/amd/display: Add HDCP module - static analysis bug report
I misunderstood and was talking about the ksv validation specifically
(usage of drm_hdcp_check_ksvs_revoked()).
For the defines I will create patches to use drm_hdcp where it is usable.
Bhawan
On 2019-10-09 2:43 p.m., Daniel Vetter wrote:
> On Wed, Oct 9, 2019 at 8:23 PM Lakha, Bhawanpreet
> wrote:
>> Hi,
>>
>> The reason we don't use drm_hdcp is because our policy is to do hdcp
>> verification using PSP/HW (onboard secure processor).
> i915 also uses hw to auth, we still use the parts from drm_hdcp ...
> Did you actually look at what's in there? It's essentially just shared
> defines and data structures from the standard, plus a few minimal
> helpers to en/decode some bits. Just from a quick read the entire
> patch very much looks like midlayer everywhere design that we
> discussed back when DC landed ...
> -Daniel
>
>> Bhawan
>>
>> On 2019-10-09 12:32 p.m., Daniel Vetter wrote:
>>> On Thu, Oct 03, 2019 at 11:08:03PM +0100, Colin Ian King wrote:
>>>> Hi,
>>>>
>>>> Static analysis with Coverity has detected a potential issue with
>>>> function validate_bksv in
>>>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c with recent
>>>> commit:
>>>>
>>>> commit ed9d8e2bcb003ec94658cafe9b1bb3960e2139ec
>>>> Author: Bhawanpreet Lakha
>>>> Date: Tue Aug 6 17:52:01 2019 -0400
>>>>
>>>> drm/amd/display: Add HDCP module
>>> I think the real question here is ... why is this not using drm_hdcp?
>>> -Daniel
>>>
>>>> The analysis is as follows:
>>>>
>>>>28 static inline enum mod_hdcp_status validate_bksv(struct mod_hdcp
>>>> *hdcp)
>>>>29 {
>>>>
>>>> CID 89852 (#1 of 1): Out-of-bounds read (OVERRUN)
>>>>
>>>> 1. overrun-local:
>>>> Overrunning array of 5 bytes at byte offset 7 by dereferencing pointer
>>>> (uint64_t *)hdcp->auth.msg.hdcp1.bksv.
>>>>
>>>>30uint64_t n = *(uint64_t *)hdcp->auth.msg.hdcp1.bksv;
>>>>31uint8_t count = 0;
>>>>32
>>>>33while (n) {
>>>>34count++;
>>>>35n &= (n - 1);
>>>>36}
>>>>
>>>> hdcp->auth.msg.hdcp1.bksv is an array of 5 uint8_t as defined in
>>>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h as follows:
>>>>
>>>> struct mod_hdcp_message_hdcp1 {
>>>> uint8_t an[8];
>>>> uint8_t aksv[5];
>>>> uint8_t ainfo;
>>>> uint8_t bksv[5];
>>>> uint16_tr0p;
>>>> uint8_t bcaps;
>>>> uint16_tbstatus;
>>>> uint8_t ksvlist[635];
>>>> uint16_tksvlist_size;
>>>> uint8_t vp[20];
>>>>
>>>> uint16_tbinfo_dp;
>>>> };
>>>>
>>>> variable n is going to contain the contains of r0p and bcaps. I'm not
>>>> sure if that is intentional. If not, then the count is going to be
>>>> incorrect if these are non-zero.
>>>>
>>>> Colin
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm/amd/display: Add HDCP module - static analysis bug report
Hi,
The reason we don't use drm_hdcp is because our policy is to do hdcp
verification using PSP/HW (onboard secure processor).
Bhawan
On 2019-10-09 12:32 p.m., Daniel Vetter wrote:
> On Thu, Oct 03, 2019 at 11:08:03PM +0100, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis with Coverity has detected a potential issue with
>> function validate_bksv in
>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c with recent
>> commit:
>>
>> commit ed9d8e2bcb003ec94658cafe9b1bb3960e2139ec
>> Author: Bhawanpreet Lakha
>> Date: Tue Aug 6 17:52:01 2019 -0400
>>
>> drm/amd/display: Add HDCP module
> I think the real question here is ... why is this not using drm_hdcp?
> -Daniel
>
>>
>> The analysis is as follows:
>>
>> 28 static inline enum mod_hdcp_status validate_bksv(struct mod_hdcp *hdcp)
>> 29 {
>>
>> CID 89852 (#1 of 1): Out-of-bounds read (OVERRUN)
>>
>> 1. overrun-local:
>> Overrunning array of 5 bytes at byte offset 7 by dereferencing pointer
>> (uint64_t *)hdcp->auth.msg.hdcp1.bksv.
>>
>> 30uint64_t n = *(uint64_t *)hdcp->auth.msg.hdcp1.bksv;
>> 31uint8_t count = 0;
>> 32
>> 33while (n) {
>> 34count++;
>> 35n &= (n - 1);
>> 36}
>>
>> hdcp->auth.msg.hdcp1.bksv is an array of 5 uint8_t as defined in
>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h as follows:
>>
>> struct mod_hdcp_message_hdcp1 {
>> uint8_t an[8];
>> uint8_t aksv[5];
>> uint8_t ainfo;
>> uint8_t bksv[5];
>> uint16_tr0p;
>> uint8_t bcaps;
>> uint16_tbstatus;
>> uint8_t ksvlist[635];
>> uint16_tksvlist_size;
>> uint8_t vp[20];
>>
>> uint16_tbinfo_dp;
>> };
>>
>> variable n is going to contain the contains of r0p and bcaps. I'm not
>> sure if that is intentional. If not, then the count is going to be
>> incorrect if these are non-zero.
>>
>> Colin
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
HDCP Content Type Interface
Hi all, This is regarding the recent hdcp content type patch that was merged into drm-misc. (https://patchwork.freedesktop.org/patch/320958/?series=57233=11) There are displays on the market that advertise HDCP 2.2 support and will pass authentication and encryption but will then show a corrupted/blue/black screen (the driver cannot detect this). These displays work with HDCP 1.4 without any issues. Due to the large number of HDCP-supporting devices on the market we might not be able to catch them with a blacklist. From the user modes perspective, HDCP1.4 and HDCP2.2 Type0 are the same thing. Meaning that this interface doesn't allow us to force the hdcp version. Due to the problems mentioned above we might want to expose the ability for a user to force an HDCP downgrade to a certain level (e.g. 1.4) in case they experience problems. What are your thoughts? and what would be a good way to deal with it? Thanks, Bhawan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
