Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Michal Wajdeczko]

On Tue, May 02, 2017 at 09:37:45AM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 02/05/17 05:39, Michal Wajdeczko wrote:
> > Prepare for alternate GuC notification mechanism.
> > 
> > Signed-off-by: Michal Wajdeczko 
> > Cc: Joonas Lahtinen 
> > Cc: Daniele Ceraolo Spurio 
> > ---
> >  drivers/gpu/drm/i915/intel_uc.c | 10 +-
> >  drivers/gpu/drm/i915/intel_uc.h |  7 +++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c 
> > b/drivers/gpu/drm/i915/intel_uc.c
> > index 7fd75ca..72f49e6 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private 
> > *dev_priv)
> > i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> >  }
> > 
> > +static void guc_write_irq_trigger(struct intel_guc *guc)
> > +{
> > +   struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > +
> > +   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +}
> > +
> >  void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  {
> > struct intel_guc *guc = _priv->guc;
> > 
> > mutex_init(>send_mutex);
> > guc->send = intel_guc_send_nop;
> > +   guc->notify = guc_write_irq_trigger;
> >  }
> > 
> >  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> > @@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const 
> > u32 *action, u32 len)
> > 
> > POSTING_READ(SOFT_SCRATCH(i - 1));
> > 
> > -   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +   intel_guc_notify(guc);
> > 
> > /*
> >  * No GuC command should ever take longer than 10ms.
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h 
> > b/drivers/gpu/drm/i915/intel_uc.h
> > index 1e0eecd..097289b 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -210,6 +210,9 @@ struct intel_guc {
> > 
> > /* GuC's FW specific send function */
> > int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> > +
> > +   /* GuC's FW specific notify function */
> > +   void (*notify)(struct intel_guc *guc);
> >  };
> > 
> >  struct intel_huc {
> > @@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc 
> > *guc, const u32 *action, u32 l
> >  {
> > return guc->send(guc, action, len);
> >  }
> > +static inline void intel_guc_notify(struct intel_guc *guc)
> > +{
> > +   guc->notify(guc);
> > +}
> > 
> 
> personal preference: I would use guc->notify directly instead of adding
> intel_guc_notify(). intel_guc_send() made more sense because a function with
> that name already existed and by keeping it we minimized the change, but I
> don't see such benefit with intel_guc_notify() and calling the function
> pointer directly feels more in sync with what we do elsewhere in the driver.
> 

Note that thanks to the compiler, resulting code is the same, but by keeping
intel_guc_notify() we are more flexible than when using hardcoded guc->notify.
Note that the same reason why we added intel_guc_send() "minimize the change"
can also be valid when in the future we may want to switch from vfun pointer
to other implementation that then can be hidden in that tiny inline that you
just wanted to kill.

-Michal 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Daniele Ceraolo Spurio]

On 02/05/17 05:39, Michal Wajdeczko wrote:

Prepare for alternate GuC notification mechanism.

Signed-off-by: Michal Wajdeczko 
Cc: Joonas Lahtinen 
Cc: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/intel_uc.c | 10 +-
 drivers/gpu/drm/i915/intel_uc.h |  7 +++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7fd75ca..72f49e6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private 
*dev_priv)
i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
 }

+static void guc_write_irq_trigger(struct intel_guc *guc)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
struct intel_guc *guc = _priv->guc;

mutex_init(>send_mutex);
guc->send = intel_guc_send_nop;
+   guc->notify = guc_write_irq_trigger;
 }

 static void fetch_uc_fw(struct drm_i915_private *dev_priv,
@@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 
*action, u32 len)

POSTING_READ(SOFT_SCRATCH(i - 1));

-   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+   intel_guc_notify(guc);

/*
 * No GuC command should ever take longer than 10ms.
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 1e0eecd..097289b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -210,6 +210,9 @@ struct intel_guc {

/* GuC's FW specific send function */
int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
+
+   /* GuC's FW specific notify function */
+   void (*notify)(struct intel_guc *guc);
 };

 struct intel_huc {
@@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, 
const u32 *action, u32 l
 {
return guc->send(guc, action, len);
 }
+static inline void intel_guc_notify(struct intel_guc *guc)
+{
+   guc->notify(guc);
+}



personal preference: I would use guc->notify directly instead of adding 
intel_guc_notify(). intel_guc_send() made more sense because a function 
with that name already existed and by keeping it we minimized the 
change, but I don't see such benefit with intel_guc_notify() and calling 
the function pointer directly feels more in sync with what we do 
elsewhere in the driver.


No strong feelings anyway, so:

Reviewed-by: Daniele Ceraolo Spurio 

Regards,
Daniele


 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Michal Wajdeczko]

Prepare for alternate GuC notification mechanism.

Signed-off-by: Michal Wajdeczko 
Cc: Joonas Lahtinen 
Cc: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/intel_uc.c | 10 +-
 drivers/gpu/drm/i915/intel_uc.h |  7 +++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7fd75ca..72f49e6 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private 
*dev_priv)
i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
 }
 
+static void guc_write_irq_trigger(struct intel_guc *guc)
+{
+   struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+}
+
 void intel_uc_init_early(struct drm_i915_private *dev_priv)
 {
struct intel_guc *guc = _priv->guc;
 
mutex_init(>send_mutex);
guc->send = intel_guc_send_nop;
+   guc->notify = guc_write_irq_trigger;
 }
 
 static void fetch_uc_fw(struct drm_i915_private *dev_priv,
@@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 
*action, u32 len)
 
POSTING_READ(SOFT_SCRATCH(i - 1));
 
-   I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
+   intel_guc_notify(guc);
 
/*
 * No GuC command should ever take longer than 10ms.
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 1e0eecd..097289b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -210,6 +210,9 @@ struct intel_guc {
 
/* GuC's FW specific send function */
int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
+
+   /* GuC's FW specific notify function */
+   void (*notify)(struct intel_guc *guc);
 };
 
 struct intel_huc {
@@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, 
const u32 *action, u32 l
 {
return guc->send(guc, action, len);
 }
+static inline void intel_guc_notify(struct intel_guc *guc)
+{
+   guc->notify(guc);
+}
 
 /* intel_guc_loader.c */
 int intel_guc_select_fw(struct intel_guc *guc);
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx