Re: [Intel-gfx] [PATCH 9/9] drm/i915/doc: Add kernel doc for i915_sched_engine

2021-06-04 Thread Matthew Brost
On Fri, Jun 04, 2021 at 12:20:36PM -0500, Jason Ekstrand wrote:
> On Thu, Jun 3, 2021 at 4:09 PM Matthew Brost  wrote:
> >
> > Signed-off-by: Matthew Brost 
> > ---
> >  Documentation/gpu/i915.rst  |  6 
> >  drivers/gpu/drm/i915/i915_scheduler_types.h | 37 ++---
> >  2 files changed, 38 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > index 42ce0196930a..8f4f5471a05b 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -425,6 +425,12 @@ User Batchbuffer Execution
> >  .. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > :doc: User command execution
> >
> > +Scheduling
> > +--
> > +.. kernel-doc:: drivers/gpu/drm/i915/i915_scheduler_types.h
> > +   :functions: i915_sched_engine
> > +
> > +
> >  Logical Rings, Logical Ring Contexts and Execlists
> >  --
> >
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h 
> > b/drivers/gpu/drm/i915/i915_scheduler_types.h
> > index 9d79514450de..e3da7517853f 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler_types.h
> > +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
> > @@ -91,7 +91,21 @@ struct i915_dependency {
> > &(rq__)->sched.signalers_list, \
> > signal_link)
> >
> > +/**
> > + * sturct i915_sched_engine - scheduler engine
> > + *
> > + * A schedule engine represents a submission queue with different priority
> > + * bands. It contains all the common state (relative to the backend) to 
> > queue,
> > + * track, and submit a request.
> > + *
> > + * This object at the moment is quite i915 specific but will transition 
> > into a
> > + * container for the drm_gpu_scheduler plus a few other variables once the 
> > i915
> > + * is integrated with the DRM scheduler.
> > + */
> 
> Why is this its own patch?  The above comment would be nice to have in
> patch 1/9 and the kref could be documented there too.
> 

I thought it make the review easier? Either way, can squash this patch into the
previous 8 as needed in next rev.

Matt

> >  struct i915_sched_engine {
> > +   /**
> > +* @ref: reference count of schedule engine object
> > +*/
> > struct kref ref;
> >
> > /**
> > @@ -100,11 +114,18 @@ struct i915_sched_engine {
> >  */
> > spinlock_t lock;
> >
> > +   /**
> > +* @requests: list of requests inflight on this schedule engine
> > +*/
> > struct list_head requests;
> > -   struct list_head hold; /* ready requests, but on hold */
> >
> > /**
> > -* @tasklet: softirq tasklet for bottom handler
> > +* @hold: list of requests on hold.
> > +*/
> > +   struct list_head hold;
> > +
> > +   /**
> > +* @tasklet: softirq tasklet for submission
> >  */
> > struct tasklet_struct tasklet;
> >
> > @@ -137,14 +158,20 @@ struct i915_sched_engine {
> >  */
> > bool no_priolist;
> >
> > -   /* Back pointer to engine */
> > +   /**
> > +* @engine: back pointer to engine
> > +*/
> > struct intel_engine_cs *engine;
> >
> > -   /* Kick backend */
> > +   /**
> > +* @kick_backed: kick back after a request's priority has changed
> > +*/
> > void(*kick_backend)(const struct i915_request *rq,
> > int prio);
> >
> > -   /*
> > +   /**
> > +* @schedule: schedule function to adjust priority of request
> > +*
> >  * Call when the priority on a request has changed and it and its
> >  * dependencies may need rescheduling. Note the request itself may
> >  * not be ready to run!
> > --
> > 2.28.0
> >
> > ___
> > Intel-gfx mailing list
> > intel-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 9/9] drm/i915/doc: Add kernel doc for i915_sched_engine

2021-06-04 Thread Jason Ekstrand
On Thu, Jun 3, 2021 at 4:09 PM Matthew Brost  wrote:
>
> Signed-off-by: Matthew Brost 
> ---
>  Documentation/gpu/i915.rst  |  6 
>  drivers/gpu/drm/i915/i915_scheduler_types.h | 37 ++---
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index 42ce0196930a..8f4f5471a05b 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -425,6 +425,12 @@ User Batchbuffer Execution
>  .. kernel-doc:: drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> :doc: User command execution
>
> +Scheduling
> +--
> +.. kernel-doc:: drivers/gpu/drm/i915/i915_scheduler_types.h
> +   :functions: i915_sched_engine
> +
> +
>  Logical Rings, Logical Ring Contexts and Execlists
>  --
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h 
> b/drivers/gpu/drm/i915/i915_scheduler_types.h
> index 9d79514450de..e3da7517853f 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler_types.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
> @@ -91,7 +91,21 @@ struct i915_dependency {
> &(rq__)->sched.signalers_list, \
> signal_link)
>
> +/**
> + * sturct i915_sched_engine - scheduler engine
> + *
> + * A schedule engine represents a submission queue with different priority
> + * bands. It contains all the common state (relative to the backend) to 
> queue,
> + * track, and submit a request.
> + *
> + * This object at the moment is quite i915 specific but will transition into 
> a
> + * container for the drm_gpu_scheduler plus a few other variables once the 
> i915
> + * is integrated with the DRM scheduler.
> + */

Why is this its own patch?  The above comment would be nice to have in
patch 1/9 and the kref could be documented there too.

>  struct i915_sched_engine {
> +   /**
> +* @ref: reference count of schedule engine object
> +*/
> struct kref ref;
>
> /**
> @@ -100,11 +114,18 @@ struct i915_sched_engine {
>  */
> spinlock_t lock;
>
> +   /**
> +* @requests: list of requests inflight on this schedule engine
> +*/
> struct list_head requests;
> -   struct list_head hold; /* ready requests, but on hold */
>
> /**
> -* @tasklet: softirq tasklet for bottom handler
> +* @hold: list of requests on hold.
> +*/
> +   struct list_head hold;
> +
> +   /**
> +* @tasklet: softirq tasklet for submission
>  */
> struct tasklet_struct tasklet;
>
> @@ -137,14 +158,20 @@ struct i915_sched_engine {
>  */
> bool no_priolist;
>
> -   /* Back pointer to engine */
> +   /**
> +* @engine: back pointer to engine
> +*/
> struct intel_engine_cs *engine;
>
> -   /* Kick backend */
> +   /**
> +* @kick_backed: kick back after a request's priority has changed
> +*/
> void(*kick_backend)(const struct i915_request *rq,
> int prio);
>
> -   /*
> +   /**
> +* @schedule: schedule function to adjust priority of request
> +*
>  * Call when the priority on a request has changed and it and its
>  * dependencies may need rescheduling. Note the request itself may
>  * not be ready to run!
> --
> 2.28.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx