| Message ID | 20230519094918.1182044-2-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use different intel_hdcp_gsc_message instances | expand |
On 5/19/2023 2:49 AM, Suraj Kandpal wrote: > Add hdcp_gsc_message_in and hdcp_gsc_message_out to help > differenctiate the reply given by gsc to avoid any kind of > message corruption due message structure reuse. > hdcp_gsc_message_out will be filled in upcoming patches Generic question on the approach: for both PXP and GSC proxy, we allocate a single multi-page object and use the first half for input and the second half output. This makes things simpler because we don't need to allocate, map and then cleanup 2 separate objects. Any reason not to follow a similar approach here? Daniele > > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > Cc: Alan Previn <alan.previn.teres.alexis@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../gpu/drm/i915/display/intel_display_core.h | 3 +- > drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 41 +++++++++++++------ > 2 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h > index e36f88a39b86..ead16d341f5c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > @@ -403,7 +403,8 @@ struct intel_display { > * reused when sending message to gsc cs. > * this is only populated post Meteorlake > */ > - struct intel_hdcp_gsc_message *hdcp_message; > + struct intel_hdcp_gsc_message *hdcp_message_in; > + struct intel_hdcp_gsc_message *hdcp_message_out; > /* Mutex to protect the above hdcp component related values. */ > struct mutex comp_mutex; > } hdcp; > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > index 7e52aea6aa17..be505b2d679e 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > @@ -665,34 +665,51 @@ static int intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915, > > static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915) > { > - struct intel_hdcp_gsc_message *hdcp_message; > + struct intel_hdcp_gsc_message *hdcp_message_in, *hdcp_message_out; > int ret; > > - hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL); > + hdcp_message_in = kzalloc(sizeof(*hdcp_message_in), GFP_KERNEL); > > - if (!hdcp_message) > + if (!hdcp_message_in) > return -ENOMEM; > > + hdcp_message_out = kzalloc(sizeof(*hdcp_message_out), GFP_KERNEL); > + > + if (!hdcp_message_out) > + return -ENOMEM; > /* > * NOTE: No need to lock the comp mutex here as it is already > * going to be taken before this function called > */ > - i915->display.hdcp.hdcp_message = hdcp_message; > - ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message); > + i915->display.hdcp.hdcp_message_in = hdcp_message_in; > + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_in); > + > + if (ret) { > + drm_err(&i915->drm, "Could not initialize hdcp_message_in\n"); > + goto out; > + } > + > + i915->display.hdcp.hdcp_message_out = hdcp_message_out; > + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_out); > > if (ret) > - drm_err(&i915->drm, "Could not initialize hdcp_message\n"); > + drm_err(&i915->drm, "Could not initialize hdcp_message_out\n"); > > +out: > return ret; > } > > static void intel_hdcp_gsc_free_message(struct drm_i915_private *i915) > { > - struct intel_hdcp_gsc_message *hdcp_message = > - i915->display.hdcp.hdcp_message; > - > - i915_vma_unpin_and_release(&hdcp_message->vma, I915_VMA_RELEASE_MAP); > - kfree(hdcp_message); > + struct intel_hdcp_gsc_message *hdcp_message_in = > + i915->display.hdcp.hdcp_message_in; > + struct intel_hdcp_gsc_message *hdcp_message_out = > + i915->display.hdcp.hdcp_message_out; > + > + i915_vma_unpin_and_release(&hdcp_message_in->vma, I915_VMA_RELEASE_MAP); > + i915_vma_unpin_and_release(&hdcp_message_out->vma, I915_VMA_RELEASE_MAP); > + kfree(hdcp_message_in); > + kfree(hdcp_message_out); > } > > int intel_hdcp_gsc_init(struct drm_i915_private *i915) > @@ -782,7 +799,7 @@ ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in, > if (msg_in_len > max_msg_size || msg_out_len > max_msg_size) > return -ENOSPC; > > - hdcp_message = i915->display.hdcp.hdcp_message; > + hdcp_message = i915->display.hdcp.hdcp_message_in; > header = hdcp_message->hdcp_cmd; > addr = i915_ggtt_offset(hdcp_message->vma); >
> On 5/19/2023 2:49 AM, Suraj Kandpal wrote: > > Add hdcp_gsc_message_in and hdcp_gsc_message_out to help > > differenctiate the reply given by gsc to avoid any kind of message > > corruption due message structure reuse. > > hdcp_gsc_message_out will be filled in upcoming patches > > Generic question on the approach: for both PXP and GSC proxy, we allocate a > single multi-page object and use the first half for input and the second half > output. This makes things simpler because we don't need to allocate, map > and then cleanup 2 separate objects. Any reason not to follow a similar > approach here? > Tbh I wasn't aware PXP and GSC Proxy were taking this approach maybe I too can modify the code to use this approach, can you point me towards any reference function which I can have a look at. Regards, Suraj Kandpal > Daniele > > > > > Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > Cc: Alan Previn <alan.previn.teres.alexis@intel.com> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > .../gpu/drm/i915/display/intel_display_core.h | 3 +- > > drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 41 +++++++++++++------ > > 2 files changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h > > b/drivers/gpu/drm/i915/display/intel_display_core.h > > index e36f88a39b86..ead16d341f5c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_core.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h > > @@ -403,7 +403,8 @@ struct intel_display { > > * reused when sending message to gsc cs. > > * this is only populated post Meteorlake > > */ > > - struct intel_hdcp_gsc_message *hdcp_message; > > + struct intel_hdcp_gsc_message *hdcp_message_in; > > + struct intel_hdcp_gsc_message *hdcp_message_out; > > /* Mutex to protect the above hdcp component related > values. */ > > struct mutex comp_mutex; > > } hdcp; > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > > b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > > index 7e52aea6aa17..be505b2d679e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c > > @@ -665,34 +665,51 @@ static int > > intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915, > > > > static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915) > > { > > - struct intel_hdcp_gsc_message *hdcp_message; > > + struct intel_hdcp_gsc_message *hdcp_message_in, > *hdcp_message_out; > > int ret; > > > > - hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL); > > + hdcp_message_in = kzalloc(sizeof(*hdcp_message_in), GFP_KERNEL); > > > > - if (!hdcp_message) > > + if (!hdcp_message_in) > > return -ENOMEM; > > > > + hdcp_message_out = kzalloc(sizeof(*hdcp_message_out), > GFP_KERNEL); > > + > > + if (!hdcp_message_out) > > + return -ENOMEM; > > /* > > * NOTE: No need to lock the comp mutex here as it is already > > * going to be taken before this function called > > */ > > - i915->display.hdcp.hdcp_message = hdcp_message; > > - ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message); > > + i915->display.hdcp.hdcp_message_in = hdcp_message_in; > > + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_in); > > + > > + if (ret) { > > + drm_err(&i915->drm, "Could not initialize > hdcp_message_in\n"); > > + goto out; > > + } > > + > > + i915->display.hdcp.hdcp_message_out = hdcp_message_out; > > + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_out); > > > > if (ret) > > - drm_err(&i915->drm, "Could not initialize > hdcp_message\n"); > > + drm_err(&i915->drm, "Could not initialize > hdcp_message_out\n"); > > > > +out: > > return ret; > > } > > > > static void intel_hdcp_gsc_free_message(struct drm_i915_private *i915) > > { > > - struct intel_hdcp_gsc_message *hdcp_message = > > - i915->display.hdcp.hdcp_message; > > - > > - i915_vma_unpin_and_release(&hdcp_message->vma, > I915_VMA_RELEASE_MAP); > > - kfree(hdcp_message); > > + struct intel_hdcp_gsc_message *hdcp_message_in = > > + i915- > >display.hdcp.hdcp_message_in; > > + struct intel_hdcp_gsc_message *hdcp_message_out = > > + i915- > >display.hdcp.hdcp_message_out; > > + > > + i915_vma_unpin_and_release(&hdcp_message_in->vma, > I915_VMA_RELEASE_MAP); > > + i915_vma_unpin_and_release(&hdcp_message_out->vma, > I915_VMA_RELEASE_MAP); > > + kfree(hdcp_message_in); > > + kfree(hdcp_message_out); > > } > > > > int intel_hdcp_gsc_init(struct drm_i915_private *i915) @@ -782,7 > > +799,7 @@ ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private > *i915, u8 *msg_in, > > if (msg_in_len > max_msg_size || msg_out_len > max_msg_size) > > return -ENOSPC; > > > > - hdcp_message = i915->display.hdcp.hdcp_message; > > + hdcp_message = i915->display.hdcp.hdcp_message_in; > > header = hdcp_message->hdcp_cmd; > > addr = i915_ggtt_offset(hdcp_message->vma); > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h index e36f88a39b86..ead16d341f5c 100644 --- a/drivers/gpu/drm/i915/display/intel_display_core.h +++ b/drivers/gpu/drm/i915/display/intel_display_core.h @@ -403,7 +403,8 @@ struct intel_display { * reused when sending message to gsc cs. * this is only populated post Meteorlake */ - struct intel_hdcp_gsc_message *hdcp_message; + struct intel_hdcp_gsc_message *hdcp_message_in; + struct intel_hdcp_gsc_message *hdcp_message_out; /* Mutex to protect the above hdcp component related values. */ struct mutex comp_mutex; } hdcp; diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c index 7e52aea6aa17..be505b2d679e 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c @@ -665,34 +665,51 @@ static int intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915, static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915) { - struct intel_hdcp_gsc_message *hdcp_message; + struct intel_hdcp_gsc_message *hdcp_message_in, *hdcp_message_out; int ret; - hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL); + hdcp_message_in = kzalloc(sizeof(*hdcp_message_in), GFP_KERNEL); - if (!hdcp_message) + if (!hdcp_message_in) return -ENOMEM; + hdcp_message_out = kzalloc(sizeof(*hdcp_message_out), GFP_KERNEL); + + if (!hdcp_message_out) + return -ENOMEM; /* * NOTE: No need to lock the comp mutex here as it is already * going to be taken before this function called */ - i915->display.hdcp.hdcp_message = hdcp_message; - ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message); + i915->display.hdcp.hdcp_message_in = hdcp_message_in; + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_in); + + if (ret) { + drm_err(&i915->drm, "Could not initialize hdcp_message_in\n"); + goto out; + } + + i915->display.hdcp.hdcp_message_out = hdcp_message_out; + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message_out); if (ret) - drm_err(&i915->drm, "Could not initialize hdcp_message\n"); + drm_err(&i915->drm, "Could not initialize hdcp_message_out\n"); +out: return ret; } static void intel_hdcp_gsc_free_message(struct drm_i915_private *i915) { - struct intel_hdcp_gsc_message *hdcp_message = - i915->display.hdcp.hdcp_message; - - i915_vma_unpin_and_release(&hdcp_message->vma, I915_VMA_RELEASE_MAP); - kfree(hdcp_message); + struct intel_hdcp_gsc_message *hdcp_message_in = + i915->display.hdcp.hdcp_message_in; + struct intel_hdcp_gsc_message *hdcp_message_out = + i915->display.hdcp.hdcp_message_out; + + i915_vma_unpin_and_release(&hdcp_message_in->vma, I915_VMA_RELEASE_MAP); + i915_vma_unpin_and_release(&hdcp_message_out->vma, I915_VMA_RELEASE_MAP); + kfree(hdcp_message_in); + kfree(hdcp_message_out); } int intel_hdcp_gsc_init(struct drm_i915_private *i915) @@ -782,7 +799,7 @@ ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in, if (msg_in_len > max_msg_size || msg_out_len > max_msg_size) return -ENOSPC; - hdcp_message = i915->display.hdcp.hdcp_message; + hdcp_message = i915->display.hdcp.hdcp_message_in; header = hdcp_message->hdcp_cmd; addr = i915_ggtt_offset(hdcp_message->vma);
Add hdcp_gsc_message_in and hdcp_gsc_message_out to help differenctiate the reply given by gsc to avoid any kind of message corruption due message structure reuse. hdcp_gsc_message_out will be filled in upcoming patches Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- .../gpu/drm/i915/display/intel_display_core.h | 3 +- drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 41 +++++++++++++------ 2 files changed, 31 insertions(+), 13 deletions(-)