Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta
On 2020-03-31 2:08 p.m., Lakha, Bhawanpreet wrote: [AMD Official Use Only - Internal Distribution Only] mod_hdcp_hdcp2_get_link_encryption_status() isn't being used, should probably remove it in a followup patch I still think it's better if you add it to the function for consistency. Feel free to fix that bit up before you merge if you want, either way this patch is: Reiviewed-by: Nicholas Kazlauskas Regards, Nicholas Kazlauskas *From:* Kazlauskas, Nicholas *Sent:* March 31, 2020 2:03 PM *To:* Alex Deucher ; Lakha, Bhawanpreet *Cc:* Deucher, Alexander ; amd-gfx list ; Zhang, Hawking *Subject:* Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta On 2020-03-31 1:37 p.m., Alex Deucher wrote: On Mon, Mar 30, 2020 at 6:36 PM Bhawanpreet Lakha wrote: [Why] The buffer used when calling psp is a shared buffer. If we have multiple calls at the same time we can overwrite the buffer. [How] Add mutex to guard the shared buffer. Signed-off-by: Bhawanpreet Lakha Acked-by: Alex Deucher One comment inline: --- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 2 + .../drm/amd/display/modules/hdcp/hdcp_psp.c | 420 +++--- 3 files changed, 257 insertions(+), 167 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c index dbaeffc4431e..9d587bc27663 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c @@ -888,6 +888,7 @@ static int psp_hdcp_load(struct psp_context *psp) if (!ret) { psp->hdcp_context.hdcp_initialized = true; psp->hdcp_context.session_id = cmd->resp.session_id; + mutex_init(&psp->hdcp_context.mutex); } kfree(cmd); @@ -1033,6 +1034,7 @@ static int psp_dtm_load(struct psp_context *psp) if (!ret) { psp->dtm_context.dtm_initialized = true; psp->dtm_context.session_id = cmd->resp.session_id; + mutex_init(&psp->dtm_context.mutex); } kfree(cmd); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h index 297435c0c7c1..6a717fd5efc7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h @@ -161,6 +161,7 @@ struct psp_hdcp_context { struct amdgpu_bo *hdcp_shared_bo; uint64_t hdcp_shared_mc_addr; void *hdcp_shared_buf; + struct mutex mutex; }; struct psp_dtm_context { @@ -169,6 +170,7 @@ struct psp_dtm_context { struct amdgpu_bo *dtm_shared_bo; uint64_t dtm_shared_mc_addr; void *dtm_shared_buf; + struct mutex mutex; }; #define MEM_TRAIN_SYSTEM_SIGNATURE 0x54534942 diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c index c2929815c3ee..aa147e171557 100644 --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c @@ -51,12 +51,15 @@ enum mod_hdcp_status mod_hdcp_remove_display_from_topology( struct ta_dtm_shared_memory *dtm_cmd; struct mod_hdcp_display *display = get_active_display_at_index(hdcp, index); + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; dtm_cmd = (struct ta_dtm_shared_memory *)psp->dtm_context.dtm_shared_buf; if (!display || !is_display_added(display)) return MOD_HDCP_STATUS_DISPLAY_NOT_FOUND; + mutex_lock(&psp->dtm_context.mutex); + memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)); dtm_cmd->cmd_id = TA_DTM_COMMAND__TOPOLOGY_UPDATE_V2; @@ -66,14 +69,15 @@ enum mod_hdcp_status mod_hdcp_remove_display_from_topology( psp_dtm_invoke(psp, dtm_cmd->cmd_id); - if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) - return MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; + if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) { + status = MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; + } else { + display->state = MOD_HDCP_DISPLAY_ACTIVE; + HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); + } - display->state = MOD_HDCP_DISPLAY_ACTIVE; - HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); - - return MOD_HDCP_STATUS_SUCCESS; - + mutex_unlock(&psp->dtm_context.mutex); + return status; } enum mod_hdcp_status mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, uint8_t in
Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta
[AMD Official Use Only - Internal Distribution Only] mod_hdcp_hdcp2_get_link_encryption_status() isn't being used, should probably remove it in a followup patch From: Kazlauskas, Nicholas Sent: March 31, 2020 2:03 PM To: Alex Deucher ; Lakha, Bhawanpreet Cc: Deucher, Alexander ; amd-gfx list ; Zhang, Hawking Subject: Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta On 2020-03-31 1:37 p.m., Alex Deucher wrote: > On Mon, Mar 30, 2020 at 6:36 PM Bhawanpreet Lakha > wrote: >> >> [Why] >> The buffer used when calling psp is a shared buffer. If we have multiple >> calls >> at the same time we can overwrite the buffer. >> >> [How] >> Add mutex to guard the shared buffer. >> >> Signed-off-by: Bhawanpreet Lakha > > Acked-by: Alex Deucher One comment inline: > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 + >> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 2 + >> .../drm/amd/display/modules/hdcp/hdcp_psp.c | 420 +++--- >> 3 files changed, 257 insertions(+), 167 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c >> index dbaeffc4431e..9d587bc27663 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c >> @@ -888,6 +888,7 @@ static int psp_hdcp_load(struct psp_context *psp) >> if (!ret) { >> psp->hdcp_context.hdcp_initialized = true; >> psp->hdcp_context.session_id = cmd->resp.session_id; >> + mutex_init(&psp->hdcp_context.mutex); >> } >> >> kfree(cmd); >> @@ -1033,6 +1034,7 @@ static int psp_dtm_load(struct psp_context *psp) >> if (!ret) { >> psp->dtm_context.dtm_initialized = true; >> psp->dtm_context.session_id = cmd->resp.session_id; >> + mutex_init(&psp->dtm_context.mutex); >> } >> >> kfree(cmd); >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h >> index 297435c0c7c1..6a717fd5efc7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h >> @@ -161,6 +161,7 @@ struct psp_hdcp_context { >> struct amdgpu_bo*hdcp_shared_bo; >> uint64_thdcp_shared_mc_addr; >> void*hdcp_shared_buf; >> + struct mutexmutex; >> }; >> >> struct psp_dtm_context { >> @@ -169,6 +170,7 @@ struct psp_dtm_context { >> struct amdgpu_bo*dtm_shared_bo; >> uint64_tdtm_shared_mc_addr; >> void*dtm_shared_buf; >> + struct mutexmutex; >> }; >> >> #define MEM_TRAIN_SYSTEM_SIGNATURE 0x54534942 >> diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c >> b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c >> index c2929815c3ee..aa147e171557 100644 >> --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c >> +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c >> @@ -51,12 +51,15 @@ enum mod_hdcp_status >> mod_hdcp_remove_display_from_topology( >> struct ta_dtm_shared_memory *dtm_cmd; >> struct mod_hdcp_display *display = >> get_active_display_at_index(hdcp, index); >> + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; >> >> dtm_cmd = (struct ta_dtm_shared_memory >> *)psp->dtm_context.dtm_shared_buf; >> >> if (!display || !is_display_added(display)) >> return MOD_HDCP_STATUS_DISPLAY_NOT_FOUND; >> >> + mutex_lock(&psp->dtm_context.mutex); >> + >> memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)); >> >> dtm_cmd->cmd_id = TA_DTM_COMMAND__TOPOLOGY_UPDATE_V2; >> @@ -66,14 +69,15 @@ enum mod_hdcp_status >> mod_hdcp_remove_display_from_topology( >> >> psp_dtm_invoke(psp, dtm_cmd->cmd_id); >> >> - if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) >> - return MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; >> + if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) { >> + status = MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; >> + } else { >> + display->state = MOD_HDCP_DISPLAY_ACTIVE; >>
Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta
On 2020-03-31 1:37 p.m., Alex Deucher wrote: On Mon, Mar 30, 2020 at 6:36 PM Bhawanpreet Lakha wrote: [Why] The buffer used when calling psp is a shared buffer. If we have multiple calls at the same time we can overwrite the buffer. [How] Add mutex to guard the shared buffer. Signed-off-by: Bhawanpreet Lakha Acked-by: Alex Deucher One comment inline: --- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 2 + .../drm/amd/display/modules/hdcp/hdcp_psp.c | 420 +++--- 3 files changed, 257 insertions(+), 167 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c index dbaeffc4431e..9d587bc27663 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c @@ -888,6 +888,7 @@ static int psp_hdcp_load(struct psp_context *psp) if (!ret) { psp->hdcp_context.hdcp_initialized = true; psp->hdcp_context.session_id = cmd->resp.session_id; + mutex_init(&psp->hdcp_context.mutex); } kfree(cmd); @@ -1033,6 +1034,7 @@ static int psp_dtm_load(struct psp_context *psp) if (!ret) { psp->dtm_context.dtm_initialized = true; psp->dtm_context.session_id = cmd->resp.session_id; + mutex_init(&psp->dtm_context.mutex); } kfree(cmd); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h index 297435c0c7c1..6a717fd5efc7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h @@ -161,6 +161,7 @@ struct psp_hdcp_context { struct amdgpu_bo*hdcp_shared_bo; uint64_thdcp_shared_mc_addr; void*hdcp_shared_buf; + struct mutexmutex; }; struct psp_dtm_context { @@ -169,6 +170,7 @@ struct psp_dtm_context { struct amdgpu_bo*dtm_shared_bo; uint64_tdtm_shared_mc_addr; void*dtm_shared_buf; + struct mutexmutex; }; #define MEM_TRAIN_SYSTEM_SIGNATURE 0x54534942 diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c index c2929815c3ee..aa147e171557 100644 --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c @@ -51,12 +51,15 @@ enum mod_hdcp_status mod_hdcp_remove_display_from_topology( struct ta_dtm_shared_memory *dtm_cmd; struct mod_hdcp_display *display = get_active_display_at_index(hdcp, index); + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; dtm_cmd = (struct ta_dtm_shared_memory *)psp->dtm_context.dtm_shared_buf; if (!display || !is_display_added(display)) return MOD_HDCP_STATUS_DISPLAY_NOT_FOUND; + mutex_lock(&psp->dtm_context.mutex); + memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)); dtm_cmd->cmd_id = TA_DTM_COMMAND__TOPOLOGY_UPDATE_V2; @@ -66,14 +69,15 @@ enum mod_hdcp_status mod_hdcp_remove_display_from_topology( psp_dtm_invoke(psp, dtm_cmd->cmd_id); - if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) - return MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; + if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) { + status = MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; + } else { + display->state = MOD_HDCP_DISPLAY_ACTIVE; + HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); + } - display->state = MOD_HDCP_DISPLAY_ACTIVE; - HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); - - return MOD_HDCP_STATUS_SUCCESS; - + mutex_unlock(&psp->dtm_context.mutex); + return status; } enum mod_hdcp_status mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, uint8_t index) @@ -83,6 +87,7 @@ enum mod_hdcp_status mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, struct mod_hdcp_display *display = get_active_display_at_index(hdcp, index); struct mod_hdcp_link *link = &hdcp->connection.link; + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; if (!psp->dtm_context.dtm_initialized) { DRM_ERROR("Failed to add display topology, DTM TA is not initialized."); @@ -94,6 +99,7 @@ enum mod_hdcp_status mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, dtm_cmd = (struct ta_dtm_shared_memory *)psp->dtm_context.dtm_shared_buf; + mutex_lock(&psp->dtm_context.mutex); memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)); dtm_cmd->cmd_id = TA_DTM_COMMAND__TOPOLOGY_UPDATE_V2; @@ -113,13 +119,15 @@ enum mod_hdcp_status
Re: [PATCH] drm/amd/display: Guard calls to hdcp_ta and dtm_ta
On Mon, Mar 30, 2020 at 6:36 PM Bhawanpreet Lakha wrote: > > [Why] > The buffer used when calling psp is a shared buffer. If we have multiple calls > at the same time we can overwrite the buffer. > > [How] > Add mutex to guard the shared buffer. > > Signed-off-by: Bhawanpreet Lakha Acked-by: Alex Deucher > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 + > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 2 + > .../drm/amd/display/modules/hdcp/hdcp_psp.c | 420 +++--- > 3 files changed, 257 insertions(+), 167 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > index dbaeffc4431e..9d587bc27663 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > @@ -888,6 +888,7 @@ static int psp_hdcp_load(struct psp_context *psp) > if (!ret) { > psp->hdcp_context.hdcp_initialized = true; > psp->hdcp_context.session_id = cmd->resp.session_id; > + mutex_init(&psp->hdcp_context.mutex); > } > > kfree(cmd); > @@ -1033,6 +1034,7 @@ static int psp_dtm_load(struct psp_context *psp) > if (!ret) { > psp->dtm_context.dtm_initialized = true; > psp->dtm_context.session_id = cmd->resp.session_id; > + mutex_init(&psp->dtm_context.mutex); > } > > kfree(cmd); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h > index 297435c0c7c1..6a717fd5efc7 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h > @@ -161,6 +161,7 @@ struct psp_hdcp_context { > struct amdgpu_bo*hdcp_shared_bo; > uint64_thdcp_shared_mc_addr; > void*hdcp_shared_buf; > + struct mutexmutex; > }; > > struct psp_dtm_context { > @@ -169,6 +170,7 @@ struct psp_dtm_context { > struct amdgpu_bo*dtm_shared_bo; > uint64_tdtm_shared_mc_addr; > void*dtm_shared_buf; > + struct mutexmutex; > }; > > #define MEM_TRAIN_SYSTEM_SIGNATURE 0x54534942 > diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c > b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c > index c2929815c3ee..aa147e171557 100644 > --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c > +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp_psp.c > @@ -51,12 +51,15 @@ enum mod_hdcp_status > mod_hdcp_remove_display_from_topology( > struct ta_dtm_shared_memory *dtm_cmd; > struct mod_hdcp_display *display = > get_active_display_at_index(hdcp, index); > + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; > > dtm_cmd = (struct ta_dtm_shared_memory > *)psp->dtm_context.dtm_shared_buf; > > if (!display || !is_display_added(display)) > return MOD_HDCP_STATUS_DISPLAY_NOT_FOUND; > > + mutex_lock(&psp->dtm_context.mutex); > + > memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)); > > dtm_cmd->cmd_id = TA_DTM_COMMAND__TOPOLOGY_UPDATE_V2; > @@ -66,14 +69,15 @@ enum mod_hdcp_status > mod_hdcp_remove_display_from_topology( > > psp_dtm_invoke(psp, dtm_cmd->cmd_id); > > - if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) > - return MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; > + if (dtm_cmd->dtm_status != TA_DTM_STATUS__SUCCESS) { > + status = MOD_HDCP_STATUS_UPDATE_TOPOLOGY_FAILURE; > + } else { > + display->state = MOD_HDCP_DISPLAY_ACTIVE; > + HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); > + } > > - display->state = MOD_HDCP_DISPLAY_ACTIVE; > - HDCP_TOP_REMOVE_DISPLAY_TRACE(hdcp, display->index); > - > - return MOD_HDCP_STATUS_SUCCESS; > - > + mutex_unlock(&psp->dtm_context.mutex); > + return status; > } > enum mod_hdcp_status mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, > uint8_t index) > @@ -83,6 +87,7 @@ enum mod_hdcp_status > mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, > struct mod_hdcp_display *display = > get_active_display_at_index(hdcp, index); > struct mod_hdcp_link *link = &hdcp->connection.link; > + enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS; > > if (!psp->dtm_context.dtm_initialized) { > DRM_ERROR("Failed to add display topology, DTM TA is not > initialized."); > @@ -94,6 +99,7 @@ enum mod_hdcp_status > mod_hdcp_add_display_to_topology(struct mod_hdcp *hdcp, > > dtm_cmd = (struct ta_dtm_shared_memory > *)psp->dtm_context.dtm_shared_buf; > > + mutex_lock(&psp->dtm_context.mutex); > memset(dtm_cmd, 0, sizeof(struct ta_dtm_shared_memory)