| Message ID | 20201115210815.5272-22-sean.z.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/27] drm/i915/pxp: Introduce Intel PXP component | expand |
Quoting Huang, Sean Z (2020-11-15 23:08:10) > Implement the intel_pxp_gem_object_status() to allow ring0 i915 > display querying the current PXP session state. In the design, > ring0 display should not perform protection flip on the protected > buffers if there is no PXP session alive. No users for this code? Dead code should not be included. If this is only to be used by following patches, it should only be included at that point. It's better to introduce the code in the same patch that uses it, to make review easier. Regards, Joonas > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 44d17ae27b94..05fe143675b1 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -334,3 +334,11 @@ void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir) > end: > return; > } > + > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata) > +{ > + if (i915->pxp.r0ctx && i915->pxp.r0ctx->flag_display_hm_surface_keys) > + return true; > + else > + return false; > +} > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index c0119ccdab08..eb0e548ce434 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -111,4 +111,6 @@ int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915); > int intel_pxp_init(struct drm_i915_private *i915); > void intel_pxp_uninit(struct drm_i915_private *i915); > > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata); > + > #endif > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Joonas, Thanks for your feedback! intel_pxp_gem_object_status() was used in the following commit "[27/27] drm/i915/pxp: Add plane decryption support", which belongs to the i915 display component instead of PXP. So personally I prefer not to merge this code commit with "[27/27] drm/i915/pxp: Add plane decryption support", and prevent one single code commit from having multiple i915 components change (display and PXP) at the same time. Best regards, Sean -----Original Message----- From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Sent: Monday, November 16, 2020 2:50 AM To: Huang, Sean Z <sean.z.huang@intel.com>; Intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 22/27] drm/i915/pxp: Expose session state for display protection flip Quoting Huang, Sean Z (2020-11-15 23:08:10) > Implement the intel_pxp_gem_object_status() to allow ring0 i915 > display querying the current PXP session state. In the design, > ring0 display should not perform protection flip on the protected > buffers if there is no PXP session alive. No users for this code? Dead code should not be included. If this is only to be used by following patches, it should only be included at that point. It's better to introduce the code in the same patch that uses it, to make review easier. Regards, Joonas > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c > b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 44d17ae27b94..05fe143675b1 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -334,3 +334,11 @@ void intel_pxp_irq_handler(struct intel_gt *gt, > u16 iir) > end: > return; > } > + > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 > +gem_object_metadata) { > + if (i915->pxp.r0ctx && i915->pxp.r0ctx->flag_display_hm_surface_keys) > + return true; > + else > + return false; > +} > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h > b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index c0119ccdab08..eb0e548ce434 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -111,4 +111,6 @@ int > i915_pxp_global_terminate_complete_callback(struct drm_i915_private > *i915); int intel_pxp_init(struct drm_i915_private *i915); void > intel_pxp_uninit(struct drm_i915_private *i915); > > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 > +gem_object_metadata); > + > #endif > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2020-11-16 at 02:38:10 +0530, Huang, Sean Z wrote: > Implement the intel_pxp_gem_object_status() to allow ring0 i915 > display querying the current PXP session state. In the design, > ring0 display should not perform protection flip on the protected > buffers if there is no PXP session alive. > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 44d17ae27b94..05fe143675b1 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -334,3 +334,11 @@ void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir) > end: > return; > } > + > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata) Currently this api used by Patch 27 of this series, uses gem object user flag (obj->user_flags) to pass as gem_object_metadata but it is unused ? why do we need this gem_object_metadata ? Thanks, Anshuman Gupta. > +{ > + if (i915->pxp.r0ctx && i915->pxp.r0ctx->flag_display_hm_surface_keys) > + return true; > + else > + return false; > +} > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index c0119ccdab08..eb0e548ce434 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -111,4 +111,6 @@ int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915); > int intel_pxp_init(struct drm_i915_private *i915); > void intel_pxp_uninit(struct drm_i915_private *i915); > > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata); > + > #endif > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Anshuman, Quoted "Currently this api used by Patch 27 of this series, uses gem object user flag (obj->user_flags) to pass as gem_object_metadata but it is unused ? why do we need this gem_object_metadata ?" Yes, you are correct, the argument gem_object_metadata of intel_pxp_gem_object_status() isn't required anymore. This is because the original design is to allow display driver passing the object metadata, so PXP can check if the object is still valid base on the metadata. But in the current design, we don't need this gem_object_metadata and we should consider to remove it. I will provide an update revision removing this argument, thanks for the feedback! Best regards, Sean -----Original Message----- From: Anshuman Gupta <anshuman.gupta@intel.com> Sent: Tuesday, November 17, 2020 12:39 AM To: Huang, Sean Z <sean.z.huang@intel.com> Cc: Intel-gfx@lists.freedesktop.org; Bommu, Krishnaiah <krishnaiah.bommu@intel.com> Subject: Re: [Intel-gfx] [PATCH 22/27] drm/i915/pxp: Expose session state for display protection flip On 2020-11-16 at 02:38:10 +0530, Huang, Sean Z wrote: > Implement the intel_pxp_gem_object_status() to allow ring0 i915 > display querying the current PXP session state. In the design, > ring0 display should not perform protection flip on the protected > buffers if there is no PXP session alive. > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c > b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 44d17ae27b94..05fe143675b1 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -334,3 +334,11 @@ void intel_pxp_irq_handler(struct intel_gt *gt, > u16 iir) > end: > return; > } > + > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 > +gem_object_metadata) Currently this api used by Patch 27 of this series, uses gem object user flag (obj->user_flags) to pass as gem_object_metadata but it is unused ? why do we need this gem_object_metadata ? Thanks, Anshuman Gupta. > +{ > + if (i915->pxp.r0ctx && i915->pxp.r0ctx->flag_display_hm_surface_keys) > + return true; > + else > + return false; > +} > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h > b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index c0119ccdab08..eb0e548ce434 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -111,4 +111,6 @@ int > i915_pxp_global_terminate_complete_callback(struct drm_i915_private > *i915); int intel_pxp_init(struct drm_i915_private *i915); void > intel_pxp_uninit(struct drm_i915_private *i915); > > +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 > +gem_object_metadata); > + > #endif > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index 44d17ae27b94..05fe143675b1 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -334,3 +334,11 @@ void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir) end: return; } + +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata) +{ + if (i915->pxp.r0ctx && i915->pxp.r0ctx->flag_display_hm_surface_keys) + return true; + else + return false; +} diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h index c0119ccdab08..eb0e548ce434 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h @@ -111,4 +111,6 @@ int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915); int intel_pxp_init(struct drm_i915_private *i915); void intel_pxp_uninit(struct drm_i915_private *i915); +bool intel_pxp_gem_object_status(struct drm_i915_private *i915, u64 gem_object_metadata); + #endif
Implement the intel_pxp_gem_object_status() to allow ring0 i915 display querying the current PXP session state. In the design, ring0 display should not perform protection flip on the protected buffers if there is no PXP session alive. Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> --- drivers/gpu/drm/i915/pxp/intel_pxp.c | 8 ++++++++ drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ 2 files changed, 10 insertions(+)