[PATCH v2 0/8] drm/vc4: Add support for the transposer IP

[PATCH v2 0/8] drm/vc4: Add support for the transposer IP

[Boris Brezillon]

Hello,

This is the second version of this series adding writeback support
to the VC4 display engine.

This version is based on drm-misc-next and include a bunch of
modifications to core that I had to add to make it work on VC4.

Feel free to comment on those modifications.

On the driver side, no much has changed except I modified a bit
the implementation to adjust the latest revision of the writeback
interface.

Regards,

Boris

Boris Brezillon (8):
  drm/atomic: Avoid connector to writeback_connector casts
  drm/connector: Pass a drm_connector_state to ->atomic_commit()
  drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()
  drm/crtc: Add a generic infrastructure to fake VBLANK events
  drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic
    commit_tail() helpers
  drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path
  drm/vc4: Add support for the transposer block
  ARM: dts: bcm283x: Add Transposer block

 .../devicetree/bindings/display/brcm,bcm-vc4.txt   |   6 +
 arch/arm/boot/dts/bcm283x.dtsi                     |   6 +
 drivers/gpu/drm/drm_atomic.c                       |   4 +-
 drivers/gpu/drm/drm_atomic_helper.c                |  46 +-
 drivers/gpu/drm/vc4/Makefile                       |   1 +
 drivers/gpu/drm/vc4/vc4_crtc.c                     | 139 +++++-
 drivers/gpu/drm/vc4/vc4_debugfs.c                  |   1 +
 drivers/gpu/drm/vc4/vc4_drv.c                      |   1 +
 drivers/gpu/drm/vc4/vc4_drv.h                      |   7 +
 drivers/gpu/drm/vc4/vc4_kms.c                      |  11 +-
 drivers/gpu/drm/vc4/vc4_regs.h                     |   1 +
 drivers/gpu/drm/vc4/vc4_txp.c                      | 487 +++++++++++++++++++++
 include/drm/drm_atomic_helper.h                    |   1 +
 include/drm/drm_crtc.h                             |  15 +
 include/drm/drm_modeset_helper_vtables.h           |   4 +-
 include/drm/drm_writeback.h                        |   6 +
 16 files changed, 700 insertions(+), 36 deletions(-)
 create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c

-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/8] drm/atomic: Avoid connector to writeback_connector casts

[Boris Brezillon]

Use container_of() instead of type casting so that it keeps working
even if base is moved inside the drm_writeback_connector struct.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_atomic.c | 4 +++-
 include/drm/drm_writeback.h  | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 178842380f75..6ea20d5dee66 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2423,6 +2423,7 @@ static int prepare_signaling(struct drm_device *dev,
 	}
 
 	for_each_new_connector_in_state(state, conn, conn_state, i) {
+		struct drm_writeback_connector *wb_conn;
 		struct drm_writeback_job *job;
 		struct drm_out_fence_state *f;
 		struct dma_fence *fence;
@@ -2446,7 +2447,8 @@ static int prepare_signaling(struct drm_device *dev,
 		f[*num_fences].out_fence_ptr = fence_ptr;
 		*fence_state = f;
 
-		fence = drm_writeback_get_out_fence((struct drm_writeback_connector *)conn);
+		wb_conn = drm_connector_to_writeback(conn);
+		fence = drm_writeback_get_out_fence(wb_conn);
 		if (!fence)
 			return -ENOMEM;
 
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index a10fe556dfd4..23df9d463003 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -110,6 +110,12 @@ struct drm_writeback_job {
 	struct dma_fence *out_fence;
 };
 
+static inline struct drm_writeback_connector *
+drm_connector_to_writeback(struct drm_connector *connector)
+{
+	return container_of(connector, struct drm_writeback_connector, base);
+}
+
 int drm_writeback_connector_init(struct drm_device *dev,
 				 struct drm_writeback_connector *wb_connector,
 				 const struct drm_connector_funcs *con_funcs,
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/8] drm/atomic: Avoid connector to writeback_connector casts

[Daniel Vetter]

On Fri, Jun 29, 2018 at 01:17:14PM +0200, Boris Brezillon wrote:
> Use container_of() instead of type casting so that it keeps working
> even if base is moved inside the drm_writeback_connector struct.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>  include/drm/drm_writeback.h  | 6 ++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 178842380f75..6ea20d5dee66 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2423,6 +2423,7 @@ static int prepare_signaling(struct drm_device *dev,
>  	}
>  
>  	for_each_new_connector_in_state(state, conn, conn_state, i) {
> +		struct drm_writeback_connector *wb_conn;
>  		struct drm_writeback_job *job;
>  		struct drm_out_fence_state *f;
>  		struct dma_fence *fence;
> @@ -2446,7 +2447,8 @@ static int prepare_signaling(struct drm_device *dev,
>  		f[*num_fences].out_fence_ptr = fence_ptr;
>  		*fence_state = f;
>  
> -		fence = drm_writeback_get_out_fence((struct drm_writeback_connector *)conn);
> +		wb_conn = drm_connector_to_writeback(conn);
> +		fence = drm_writeback_get_out_fence(wb_conn);
>  		if (!fence)
>  			return -ENOMEM;
>  
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index a10fe556dfd4..23df9d463003 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -110,6 +110,12 @@ struct drm_writeback_job {
>  	struct dma_fence *out_fence;
>  };
>  
> +static inline struct drm_writeback_connector *
> +drm_connector_to_writeback(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct drm_writeback_connector, base);
> +}
> +
>  int drm_writeback_connector_init(struct drm_device *dev,
>  				 struct drm_writeback_connector *wb_connector,
>  				 const struct drm_connector_funcs *con_funcs,
> -- 
> 2.14.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Boris Brezillon]

Other atomic hooks are passed state objects, let's change this one to
be consistent.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
 include/drm/drm_modeset_helper_vtables.h | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 17baf5057132..69063bcf2334 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 
 		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
 			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
-			funcs->atomic_commit(connector, new_conn_state->writeback_job);
+			funcs->atomic_commit(connector, new_conn_state);
 		}
 	}
 }
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 3b289773297c..fb841f44949c 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -980,11 +980,13 @@ struct drm_connector_helper_funcs {
 	 *
 	 * This hook is to be used by drivers implementing writeback connectors
 	 * that need a point when to commit the writeback job to the hardware.
+	 * The writeback_job to commit is available in
+	 * &drm_connector_state.writeback_job.
 	 *
 	 * This callback is used by the atomic modeset helpers.
 	 */
 	void (*atomic_commit)(struct drm_connector *connector,
-			      struct drm_writeback_job *writeback_job);
+			      struct drm_connector_state *state);
 };
 
 /**
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Liviu Dudau]

On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
> Other atomic hooks are passed state objects, let's change this one to
> be consistent.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
>  include/drm/drm_modeset_helper_vtables.h | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 17baf5057132..69063bcf2334 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>  
>  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
>  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> +			funcs->atomic_commit(connector, new_conn_state);
>  		}
>  	}
>  }
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 3b289773297c..fb841f44949c 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs {
>  	 *
>  	 * This hook is to be used by drivers implementing writeback connectors
>  	 * that need a point when to commit the writeback job to the hardware.
> +	 * The writeback_job to commit is available in
> +	 * &drm_connector_state.writeback_job.
>  	 *
>  	 * This callback is used by the atomic modeset helpers.
>  	 */
>  	void (*atomic_commit)(struct drm_connector *connector,
> -			      struct drm_writeback_job *writeback_job);
> +			      struct drm_connector_state *state);
>  };
>  
>  /**
> -- 
> 2.14.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Liviu Dudau]

On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
> Other atomic hooks are passed state objects, let's change this one to
> be consistent.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
>  include/drm/drm_modeset_helper_vtables.h | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 17baf5057132..69063bcf2334 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>  
>  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
>  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> +			funcs->atomic_commit(connector, new_conn_state);

Forgot to add: I think it is worth adding a check here that the hook has
been implemented by the driver, AFAIK it is not a mandatory hook, even
for writeback enabled drivers.

Let me know what you plan to do with this patch as I will have to update
the malidp patches as well, otherwise linux-next is going to flag me.

Best regards,
Liviu

>  		}
>  	}
>  }
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 3b289773297c..fb841f44949c 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs {
>  	 *
>  	 * This hook is to be used by drivers implementing writeback connectors
>  	 * that need a point when to commit the writeback job to the hardware.
> +	 * The writeback_job to commit is available in
> +	 * &drm_connector_state.writeback_job.
>  	 *
>  	 * This callback is used by the atomic modeset helpers.
>  	 */
>  	void (*atomic_commit)(struct drm_connector *connector,
> -			      struct drm_writeback_job *writeback_job);
> +			      struct drm_connector_state *state);
>  };
>  
>  /**
> -- 
> 2.14.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Daniel Vetter]

On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
> On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:
> > Other atomic hooks are passed state objects, let's change this one to
> > be consistent.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
> >  include/drm/drm_modeset_helper_vtables.h | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 17baf5057132..69063bcf2334 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> >  
> >  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
> >  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> > -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> > +			funcs->atomic_commit(connector, new_conn_state);
> 
> Forgot to add: I think it is worth adding a check here that the hook has
> been implemented by the driver, AFAIK it is not a mandatory hook, even
> for writeback enabled drivers.

Either way this should be documented in the hook (atm it says nothing
about whether it's mandatory/optional and for whom).
> 
> Let me know what you plan to do with this patch as I will have to update
> the malidp patches as well, otherwise linux-next is going to flag me.

Probably simplest if you send a pull to Dave right away with the current
malidp code, then we backmerge that into drm-misc-next, and Boris rebases.
There's no point in hanging onto pull requests until right before the
feature freeze, just makes things more complicated.

Patch itself lgtm.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> Best regards,
> Liviu
> 
> >  		}
> >  	}
> >  }
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 3b289773297c..fb841f44949c 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -980,11 +980,13 @@ struct drm_connector_helper_funcs {
> >  	 *
> >  	 * This hook is to be used by drivers implementing writeback connectors
> >  	 * that need a point when to commit the writeback job to the hardware.
> > +	 * The writeback_job to commit is available in
> > +	 * &drm_connector_state.writeback_job.
> >  	 *
> >  	 * This callback is used by the atomic modeset helpers.
> >  	 */
> >  	void (*atomic_commit)(struct drm_connector *connector,
> > -			      struct drm_writeback_job *writeback_job);
> > +			      struct drm_connector_state *state);
> >  };
> >  
> >  /**
> > -- 
> > 2.14.1
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Boris Brezillon]

On Mon, 2 Jul 2018 09:51:46 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
> > On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:  
> > > Other atomic hooks are passed state objects, let's change this one to
> > > be consistent.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
> > >  include/drm/drm_modeset_helper_vtables.h | 4 +++-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 17baf5057132..69063bcf2334 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> > >  
> > >  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
> > >  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> > > -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> > > +			funcs->atomic_commit(connector, new_conn_state);  
> > 
> > Forgot to add: I think it is worth adding a check here that the hook has
> > been implemented by the driver, AFAIK it is not a mandatory hook, even
> > for writeback enabled drivers.  

I'm just curious, from where do you queue the writeback job if you don't
have a connector->atomic_commit() hook implemented? AFAICT, the
encoder->enable() method is only called when the encoder is being
enabled, and not every time you update the FB_ID prop of the writeback
connector. Am I missing something?

> 
> Either way this should be documented in the hook (atm it says nothing
> about whether it's mandatory/optional and for whom).

I'm fine making this hook optional. I'll update the code and the doc in
a separate commit.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Liviu Dudau]

On Mon, Jul 02, 2018 at 11:49:11AM +0200, Boris Brezillon wrote:
> On Mon, 2 Jul 2018 09:51:46 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:
> > > On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:  
> > > > Other atomic hooks are passed state objects, let's change this one to
> > > > be consistent.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
> > > >  include/drm/drm_modeset_helper_vtables.h | 4 +++-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 17baf5057132..69063bcf2334 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> > > >  
> > > >  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
> > > >  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> > > > -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> > > > +			funcs->atomic_commit(connector, new_conn_state);  
> > > 
> > > Forgot to add: I think it is worth adding a check here that the hook has
> > > been implemented by the driver, AFAIK it is not a mandatory hook, even
> > > for writeback enabled drivers.  
> 
> I'm just curious, from where do you queue the writeback job if you don't
> have a connector->atomic_commit() hook implemented? AFAICT, the
> encoder->enable() method is only called when the encoder is being
> enabled, and not every time you update the FB_ID prop of the writeback
> connector. Am I missing something?

In malidp_drv.c:malidp_atomic_commit_tail() we call
malidp_mw_atomic_commit() after drm_atomic_helper_commit_planes(drm,
state, DRM_PLANE_COMMIT_ACTIVE_ONLY).

malidp_mw_atomic_commit() then checks the writeback job and if it has an
fb associated with it then it calls drm_writeback_queue_job() before
enabling the memwrite hardware bits.

You can see it all in action here:

https://github.com/ARM-software/linux/commit/f7a88ca52530958703bcfc30adc302841ac89989


Best regards,
Liviu

> 
> > 
> > Either way this should be documented in the hook (atm it says nothing
> > about whether it's mandatory/optional and for whom).
> 
> I'm fine making this hook optional. I'll update the code and the doc in
> a separate commit.

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()

[Boris Brezillon]

On Mon, 2 Jul 2018 12:14:20 +0100
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Mon, Jul 02, 2018 at 11:49:11AM +0200, Boris Brezillon wrote:
> > On Mon, 2 Jul 2018 09:51:46 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Fri, Jun 29, 2018 at 12:37:10PM +0100, Liviu Dudau wrote:  
> > > > On Fri, Jun 29, 2018 at 01:17:15PM +0200, Boris Brezillon wrote:    
> > > > > Other atomic hooks are passed state objects, let's change this one to
> > > > > be consistent.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_atomic_helper.c      | 2 +-
> > > > >  include/drm/drm_modeset_helper_vtables.h | 4 +++-
> > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index 17baf5057132..69063bcf2334 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -1187,7 +1187,7 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> > > > >  
> > > > >  		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
> > > > >  			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
> > > > > -			funcs->atomic_commit(connector, new_conn_state->writeback_job);
> > > > > +			funcs->atomic_commit(connector, new_conn_state);    
> > > > 
> > > > Forgot to add: I think it is worth adding a check here that the hook has
> > > > been implemented by the driver, AFAIK it is not a mandatory hook, even
> > > > for writeback enabled drivers.    
> > 
> > I'm just curious, from where do you queue the writeback job if you don't
> > have a connector->atomic_commit() hook implemented? AFAICT, the
> > encoder->enable() method is only called when the encoder is being
> > enabled, and not every time you update the FB_ID prop of the writeback
> > connector. Am I missing something?  
> 
> In malidp_drv.c:malidp_atomic_commit_tail() we call
> malidp_mw_atomic_commit() after drm_atomic_helper_commit_planes(drm,
> state, DRM_PLANE_COMMIT_ACTIVE_ONLY).
> 
> malidp_mw_atomic_commit() then checks the writeback job and if it has an
> fb associated with it then it calls drm_writeback_queue_job() before
> enabling the memwrite hardware bits.

Okay. That's a good reason to make it optional.

Thanks,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/8] drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()

[Boris Brezillon]

drm_atomic_helper_wait_for_vblanks() assumes the CRTC will continuously
generate VBLANK events and the vblank counter will keep increasing.
While this work for a regular pipeline, it doesn't when you have the
CRTC is feeding the transposer block, because this block works in
oneshot mode, and, by the time we reach
drm_atomic_helper_wait_for_vblanks() the only VBLANK event might have
already been sent and the VBLANK counter will stay unchanged, thus
triggering a timeout.

Luckily, we can replace the drm_atomic_helper_wait_for_vblanks() call
by drm_atomic_helper_wait_for_flip_done() because the only thing we
want to check when calling drm_atomic_helper_wait_for_vblanks() from
vc4_atomic_complete_commit() is that new FBs are in use and the old
ones can be safely released.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 8a411e5f8776..91239b0a4fa0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -153,18 +153,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
-	/* Make sure that drm_atomic_helper_wait_for_vblanks()
-	 * actually waits for vblank.  If we're doing a full atomic
-	 * modeset (as opposed to a vc4_update_plane() short circuit),
-	 * then we need to wait for scanout to be done with our
-	 * display lists before we free it and potentially reallocate
-	 * and overwrite the dlist memory with a new modeset.
-	 */
-	state->legacy_cursor_update = false;
-
 	drm_atomic_helper_commit_hw_done(state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	drm_atomic_helper_wait_for_flip_done(dev, state);
 
 	drm_atomic_helper_cleanup_planes(dev, state);
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/8] drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 970 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> drm_atomic_helper_wait_for_vblanks() assumes the CRTC will continuously
> generate VBLANK events and the vblank counter will keep increasing.
> While this work for a regular pipeline, it doesn't when you have the
> CRTC is feeding the transposer block, because this block works in
> oneshot mode, and, by the time we reach
> drm_atomic_helper_wait_for_vblanks() the only VBLANK event might have
> already been sent and the VBLANK counter will stay unchanged, thus
> triggering a timeout.
>
> Luckily, we can replace the drm_atomic_helper_wait_for_vblanks() call
> by drm_atomic_helper_wait_for_flip_done() because the only thing we
> want to check when calling drm_atomic_helper_wait_for_vblanks() from
> vc4_atomic_complete_commit() is that new FBs are in use and the old
> ones can be safely released.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Boris Brezillon]

In some cases CRTCs are active but are not able to generating events, at
least not at every frame at it's expected to.
This is typically the case when the CRTC is feeding a writeback connector
that has no job queued. In this situation the CRTC is usually stopped
until a new job is queued, and this can lead to timeouts when part of
the pipeline is updated but no new jobs are queued to the active
writeback connector.

In order to solve that, we add a ->no_vblank flag to drm_crtc_state
and ask the CRTC drivers to set it to true when they know they're not
able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
helper can then be used to fake VBLANKs at commit time.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  1 +
 include/drm/drm_crtc.h              | 15 ++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 69063bcf2334..ca586993c2a2 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
+/**
+ * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
+ * @old_state: atomic state object with old state structures
+ *
+ * This function walks all CRTCs and fake VBLANK events on those with
+ * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
+ * The primary use of this function is writeback connectors working in oneshot
+ * mode and faking VBLANK events. In this case they only fake the VBLANK event
+ * when a job is queued, and any change to the pipeline that does not touch the
+ * connector is leading to timeouts when calling
+ * drm_atomic_helper_wait_for_vblanks() or
+ * drm_atomic_helper_wait_for_flip_done().
+ *
+ * This is part of the atomic helper support for nonblocking commits, see
+ * drm_atomic_helper_setup_commit() for an overview.
+ */
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
+{
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
+
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
+				      new_crtc_state, i) {
+		unsigned long flags;
+
+		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)
+			continue;
+
+		spin_lock_irqsave(&old_state->dev->event_lock, flags);
+		if (new_crtc_state->event) {
+			drm_crtc_send_vblank_event(crtc,
+						   new_crtc_state->event);
+			new_crtc_state->event = NULL;
+		}
+		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
+	}
+}
+EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
+
 /**
  * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
  * @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 26aaba58d6ce..99e2a5297c69 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 				   bool nonblock);
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
 void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
 void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..7435dc66c08a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
  * @zpos_changed: zpos values of planes on this crtc have been updated
  * @color_mgmt_changed: color management properties have changed (degamma or
  *	gamma LUT or CSC matrix)
+ * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
+ *	usually depends on the pipeline configuration, and the main usuage is
+ *	CRTCs feeding a writeback connector operating in oneshot mode. In this
+ *	case the VBLANK event is only generated when a job is queued to the
+ *	writeback connector, and we want the core to fake VBLANK events when
+ *	this part of the pipeline hasn't changed but others had or when the
+ *	CRTC and connectors are disabled.
+ *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
+ *	current state, the CRTC driver is then responsible for updating this
+ *	field when needed.
+ *	Note that, even when no_blank is set to true, the CRTC driver can still
+ *	steal the &drm_crtc_state.event object and send the event on its own.
+ *	That's usually what happens when a job is queued to the writeback
+ *	connector.
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
@@ -118,6 +132,7 @@ struct drm_crtc_state {
 	bool connectors_changed : 1;
 	bool zpos_changed : 1;
 	bool color_mgmt_changed : 1;
+	bool no_vblank : 1;
 
 	/* attached planes bitmask:
 	 * WARNING: transitional helpers do not maintain plane_mask so
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Liviu Dudau]

On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
> In some cases CRTCs are active but are not able to generating events, at
> least not at every frame at it's expected to.
> This is typically the case when the CRTC is feeding a writeback connector
> that has no job queued. In this situation the CRTC is usually stopped
> until a new job is queued, and this can lead to timeouts when part of
> the pipeline is updated but no new jobs are queued to the active
> writeback connector.
> 
> In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> and ask the CRTC drivers to set it to true when they know they're not
> able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> helper can then be used to fake VBLANKs at commit time.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  1 +
>  include/drm/drm_crtc.h              | 15 ++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 69063bcf2334..ca586993c2a2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
> +/**
> + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> + * @old_state: atomic state object with old state structures
> + *
> + * This function walks all CRTCs and fake VBLANK events on those with
> + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> + * The primary use of this function is writeback connectors working in oneshot
> + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> + * when a job is queued, and any change to the pipeline that does not touch the
> + * connector is leading to timeouts when calling
> + * drm_atomic_helper_wait_for_vblanks() or
> + * drm_atomic_helper_wait_for_flip_done().
> + *
> + * This is part of the atomic helper support for nonblocking commits, see
> + * drm_atomic_helper_setup_commit() for an overview.
> + */
> +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> +				      new_crtc_state, i) {
> +		unsigned long flags;
> +
> +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)
> +			continue;
> +
> +		spin_lock_irqsave(&old_state->dev->event_lock, flags);
> +		if (new_crtc_state->event) {
> +			drm_crtc_send_vblank_event(crtc,
> +						   new_crtc_state->event);
> +			new_crtc_state->event = NULL;
> +		}
> +		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
> +
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
>   * @old_state: atomic state object with old state structures
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 26aaba58d6ce..99e2a5297c69 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  				   bool nonblock);
>  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
> +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..7435dc66c08a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
>   * @zpos_changed: zpos values of planes on this crtc have been updated
>   * @color_mgmt_changed: color management properties have changed (degamma or
>   *	gamma LUT or CSC matrix)
> + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
> + *	usually depends on the pipeline configuration, and the main usuage is
> + *	CRTCs feeding a writeback connector operating in oneshot mode. In this
> + *	case the VBLANK event is only generated when a job is queued to the
> + *	writeback connector, and we want the core to fake VBLANK events when
> + *	this part of the pipeline hasn't changed but others had or when the
> + *	CRTC and connectors are disabled.
> + *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
> + *	current state, the CRTC driver is then responsible for updating this
> + *	field when needed.
> + *	Note that, even when no_blank is set to true, the CRTC driver can still
> + *	steal the &drm_crtc_state.event object and send the event on its own.
> + *	That's usually what happens when a job is queued to the writeback
> + *	connector.
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
>   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> @@ -118,6 +132,7 @@ struct drm_crtc_state {
>  	bool connectors_changed : 1;
>  	bool zpos_changed : 1;
>  	bool color_mgmt_changed : 1;
> +	bool no_vblank : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> -- 
> 2.14.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Daniel Vetter]

On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
> In some cases CRTCs are active but are not able to generating events, at
> least not at every frame at it's expected to.
> This is typically the case when the CRTC is feeding a writeback connector
> that has no job queued. In this situation the CRTC is usually stopped
> until a new job is queued, and this can lead to timeouts when part of
> the pipeline is updated but no new jobs are queued to the active
> writeback connector.
> 
> In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> and ask the CRTC drivers to set it to true when they know they're not
> able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> helper can then be used to fake VBLANKs at commit time.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  1 +
>  include/drm/drm_crtc.h              | 15 ++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 69063bcf2334..ca586993c2a2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
> +/**
> + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> + * @old_state: atomic state object with old state structures
> + *
> + * This function walks all CRTCs and fake VBLANK events on those with
> + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> + * The primary use of this function is writeback connectors working in oneshot
> + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> + * when a job is queued, and any change to the pipeline that does not touch the
> + * connector is leading to timeouts when calling
> + * drm_atomic_helper_wait_for_vblanks() or
> + * drm_atomic_helper_wait_for_flip_done().
> + *
> + * This is part of the atomic helper support for nonblocking commits, see
> + * drm_atomic_helper_setup_commit() for an overview.
> + */
> +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> +{
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
> +
> +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> +				      new_crtc_state, i) {
> +		unsigned long flags;
> +
> +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)

Uh, this essentially makes it impossible to reset no_vblank. For control
flow state bits we only check the new state for it (see e.g. the various
*_changed or plane_bits or whatever).

> +			continue;
> +
> +		spin_lock_irqsave(&old_state->dev->event_lock, flags);
> +		if (new_crtc_state->event) {
> +			drm_crtc_send_vblank_event(crtc,
> +						   new_crtc_state->event);
> +			new_crtc_state->event = NULL;
> +		}
> +		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
> +
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
>   * @old_state: atomic state object with old state structures
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 26aaba58d6ce..99e2a5297c69 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  				   bool nonblock);
>  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
> +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..7435dc66c08a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
>   * @zpos_changed: zpos values of planes on this crtc have been updated
>   * @color_mgmt_changed: color management properties have changed (degamma or
>   *	gamma LUT or CSC matrix)
> + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
> + *	usually depends on the pipeline configuration, and the main usuage is
> + *	CRTCs feeding a writeback connector operating in oneshot mode. In this
> + *	case the VBLANK event is only generated when a job is queued to the
> + *	writeback connector, and we want the core to fake VBLANK events when
> + *	this part of the pipeline hasn't changed but others had or when the
> + *	CRTC and connectors are disabled.

s/are disabled/are getting disabled/

You'll never get a request for an event when it's already off.

> + *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
> + *	current state, the CRTC driver is then responsible for updating this
> + *	field when needed.

Not parsing the above ... probably missing a "... will not reset ..."

> + *	Note that, even when no_blank is set to true, the CRTC driver can still
> + *	steal the &drm_crtc_state.event object and send the event on its own.
> + *	That's usually what happens when a job is queued to the writeback
> + *	connector.

The last sentence is confusing imo. Just drop it?

Please use the inline comment style for struct members, and then also
polish the formatting a bit (e.g. paragraph breaks, which are only
possible with the inline style).

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
>   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> @@ -118,6 +132,7 @@ struct drm_crtc_state {
>  	bool connectors_changed : 1;
>  	bool zpos_changed : 1;
>  	bool color_mgmt_changed : 1;
> +	bool no_vblank : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> -- 
> 2.14.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Boris Brezillon]

On Mon, 2 Jul 2018 10:02:52 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
> > In some cases CRTCs are active but are not able to generating events, at
> > least not at every frame at it's expected to.
> > This is typically the case when the CRTC is feeding a writeback connector
> > that has no job queued. In this situation the CRTC is usually stopped
> > until a new job is queued, and this can lead to timeouts when part of
> > the pipeline is updated but no new jobs are queued to the active
> > writeback connector.
> > 
> > In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> > and ask the CRTC drivers to set it to true when they know they're not
> > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> > helper can then be used to fake VBLANKs at commit time.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  1 +
> >  include/drm/drm_crtc.h              | 15 ++++++++++++++
> >  3 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 69063bcf2334..ca586993c2a2 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> >  
> > +/**
> > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> > + * @old_state: atomic state object with old state structures
> > + *
> > + * This function walks all CRTCs and fake VBLANK events on those with
> > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> > + * The primary use of this function is writeback connectors working in oneshot
> > + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> > + * when a job is queued, and any change to the pipeline that does not touch the
> > + * connector is leading to timeouts when calling
> > + * drm_atomic_helper_wait_for_vblanks() or
> > + * drm_atomic_helper_wait_for_flip_done().
> > + *
> > + * This is part of the atomic helper support for nonblocking commits, see
> > + * drm_atomic_helper_setup_commit() for an overview.
> > + */
> > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> > +{
> > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> > +				      new_crtc_state, i) {
> > +		unsigned long flags;
> > +
> > +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)  
> 
> Uh, this essentially makes it impossible to reset no_vblank.

I don't want ->no_vblank to be reset by the core. It's up to the CRTC
driver to clear/set it when something changes in the pipeline.

> For control
> flow state bits we only check the new state for it (see e.g. the various
> *_changed or plane_bits or whatever).

I tried with !new_crtc_state->no_vblank only, but then it does not
handle the case where the CRTC and connector are being disabled, and I
end up with a timeout.

> 
> > +			continue;
> > +
> > +		spin_lock_irqsave(&old_state->dev->event_lock, flags);
> > +		if (new_crtc_state->event) {
> > +			drm_crtc_send_vblank_event(crtc,
> > +						   new_crtc_state->event);
> > +			new_crtc_state->event = NULL;
> > +		}
> > +		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
> > +	}
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
> > +
> >  /**
> >   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> >   * @old_state: atomic state object with old state structures
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 26aaba58d6ce..99e2a5297c69 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >  				   bool nonblock);
> >  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
> > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
> >  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
> >  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
> >  
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 23eddbccab10..7435dc66c08a 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
> >   * @zpos_changed: zpos values of planes on this crtc have been updated
> >   * @color_mgmt_changed: color management properties have changed (degamma or
> >   *	gamma LUT or CSC matrix)
> > + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
> > + *	usually depends on the pipeline configuration, and the main usuage is
> > + *	CRTCs feeding a writeback connector operating in oneshot mode. In this
> > + *	case the VBLANK event is only generated when a job is queued to the
> > + *	writeback connector, and we want the core to fake VBLANK events when
> > + *	this part of the pipeline hasn't changed but others had or when the
> > + *	CRTC and connectors are disabled.  
> 
> s/are disabled/are getting disabled/
> 
> You'll never get a request for an event when it's already off.

That's true.

> 
> > + *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
> > + *	current state, the CRTC driver is then responsible for updating this
> > + *	field when needed.  
> 
> Not parsing the above ... probably missing a "... will not reset ..."

Exactly. Will add the missing words.

> 
> > + *	Note that, even when no_blank is set to true, the CRTC driver can still
> > + *	steal the &drm_crtc_state.event object and send the event on its own.
> > + *	That's usually what happens when a job is queued to the writeback
> > + *	connector.  
> 
> The last sentence is confusing imo. Just drop it?

Yes, I know, but it's also important to state that the ->no_blank +
event == NULL is a valid combination, and just means that the driver
decided to generate the event (that happens when a new WB job is
queued).

> 
> Please use the inline comment style for struct members, and then also
> polish the formatting a bit (e.g. paragraph breaks, which are only
> possible with the inline style).

I considered that, but the other fields were already documented in the
single block above the struct, so I thought keeping things consistent
was better. Should I just add this field doc inline and keep the other
ones where they are, or should I add a patch moving all docs inline?

> 
> With the nits addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> >   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
> >   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> > @@ -118,6 +132,7 @@ struct drm_crtc_state {
> >  	bool connectors_changed : 1;
> >  	bool zpos_changed : 1;
> >  	bool color_mgmt_changed : 1;
> > +	bool no_vblank : 1;
> >  
> >  	/* attached planes bitmask:
> >  	 * WARNING: transitional helpers do not maintain plane_mask so
> > -- 
> > 2.14.1
> >   
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Daniel Vetter]

On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
> On Mon, 2 Jul 2018 10:02:52 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
> > > In some cases CRTCs are active but are not able to generating events, at
> > > least not at every frame at it's expected to.
> > > This is typically the case when the CRTC is feeding a writeback connector
> > > that has no job queued. In this situation the CRTC is usually stopped
> > > until a new job is queued, and this can lead to timeouts when part of
> > > the pipeline is updated but no new jobs are queued to the active
> > > writeback connector.
> > > 
> > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> > > and ask the CRTC drivers to set it to true when they know they're not
> > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> > > helper can then be used to fake VBLANKs at commit time.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_atomic_helper.h     |  1 +
> > >  include/drm/drm_crtc.h              | 15 ++++++++++++++
> > >  3 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 69063bcf2334..ca586993c2a2 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> > >  
> > > +/**
> > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> > > + * @old_state: atomic state object with old state structures
> > > + *
> > > + * This function walks all CRTCs and fake VBLANK events on those with
> > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> > > + * The primary use of this function is writeback connectors working in oneshot
> > > + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> > > + * when a job is queued, and any change to the pipeline that does not touch the
> > > + * connector is leading to timeouts when calling
> > > + * drm_atomic_helper_wait_for_vblanks() or
> > > + * drm_atomic_helper_wait_for_flip_done().
> > > + *
> > > + * This is part of the atomic helper support for nonblocking commits, see
> > > + * drm_atomic_helper_setup_commit() for an overview.
> > > + */
> > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> > > +{
> > > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> > > +				      new_crtc_state, i) {
> > > +		unsigned long flags;
> > > +
> > > +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)  
> > 
> > Uh, this essentially makes it impossible to reset no_vblank.
> 
> I don't want ->no_vblank to be reset by the core. It's up to the CRTC
> driver to clear/set it when something changes in the pipeline.
> 
> > For control
> > flow state bits we only check the new state for it (see e.g. the various
> > *_changed or plane_bits or whatever).
> 
> I tried with !new_crtc_state->no_vblank only, but then it does not
> handle the case where the CRTC and connector are being disabled, and I
> end up with a timeout.

Why that? You should have a new_crtc_state even when you disable the crtc,
and you can set the ->no_vblank on that one too. With your current code
it's essentially impossible to reset ->no_vblank, since it will only have
effect for the _next_ atomic update, not the one you're doing right now.
That doesn't make sense.

E.g. try to do the same logic of checking both old&new state for e.g.
needs_modeset and watch how all the logic totally falls to pieces.
-Daniel

> 
> > 
> > > +			continue;
> > > +
> > > +		spin_lock_irqsave(&old_state->dev->event_lock, flags);
> > > +		if (new_crtc_state->event) {
> > > +			drm_crtc_send_vblank_event(crtc,
> > > +						   new_crtc_state->event);
> > > +			new_crtc_state->event = NULL;
> > > +		}
> > > +		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
> > > +
> > >  /**
> > >   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> > >   * @old_state: atomic state object with old state structures
> > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > > index 26aaba58d6ce..99e2a5297c69 100644
> > > --- a/include/drm/drm_atomic_helper.h
> > > +++ b/include/drm/drm_atomic_helper.h
> > > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > >  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> > >  				   bool nonblock);
> > >  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
> > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
> > >  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
> > >  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
> > >  
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 23eddbccab10..7435dc66c08a 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
> > >   * @zpos_changed: zpos values of planes on this crtc have been updated
> > >   * @color_mgmt_changed: color management properties have changed (degamma or
> > >   *	gamma LUT or CSC matrix)
> > > + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
> > > + *	usually depends on the pipeline configuration, and the main usuage is
> > > + *	CRTCs feeding a writeback connector operating in oneshot mode. In this
> > > + *	case the VBLANK event is only generated when a job is queued to the
> > > + *	writeback connector, and we want the core to fake VBLANK events when
> > > + *	this part of the pipeline hasn't changed but others had or when the
> > > + *	CRTC and connectors are disabled.  
> > 
> > s/are disabled/are getting disabled/
> > 
> > You'll never get a request for an event when it's already off.
> 
> That's true.
> 
> > 
> > > + *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
> > > + *	current state, the CRTC driver is then responsible for updating this
> > > + *	field when needed.  
> > 
> > Not parsing the above ... probably missing a "... will not reset ..."
> 
> Exactly. Will add the missing words.
> 
> > 
> > > + *	Note that, even when no_blank is set to true, the CRTC driver can still
> > > + *	steal the &drm_crtc_state.event object and send the event on its own.
> > > + *	That's usually what happens when a job is queued to the writeback
> > > + *	connector.  
> > 
> > The last sentence is confusing imo. Just drop it?
> 
> Yes, I know, but it's also important to state that the ->no_blank +
> event == NULL is a valid combination, and just means that the driver
> decided to generate the event (that happens when a new WB job is
> queued).
> 
> > 
> > Please use the inline comment style for struct members, and then also
> > polish the formatting a bit (e.g. paragraph breaks, which are only
> > possible with the inline style).
> 
> I considered that, but the other fields were already documented in the
> single block above the struct, so I thought keeping things consistent
> was better. Should I just add this field doc inline and keep the other
> ones where they are, or should I add a patch moving all docs inline?
> 
> > 
> > With the nits addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > >   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> > >   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
> > >   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> > > @@ -118,6 +132,7 @@ struct drm_crtc_state {
> > >  	bool connectors_changed : 1;
> > >  	bool zpos_changed : 1;
> > >  	bool color_mgmt_changed : 1;
> > > +	bool no_vblank : 1;
> > >  
> > >  	/* attached planes bitmask:
> > >  	 * WARNING: transitional helpers do not maintain plane_mask so
> > > -- 
> > > 2.14.1
> > >   
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Boris Brezillon]

On Mon, 2 Jul 2018 10:37:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
> > On Mon, 2 Jul 2018 10:02:52 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:  
> > > > In some cases CRTCs are active but are not able to generating events, at
> > > > least not at every frame at it's expected to.
> > > > This is typically the case when the CRTC is feeding a writeback connector
> > > > that has no job queued. In this situation the CRTC is usually stopped
> > > > until a new job is queued, and this can lead to timeouts when part of
> > > > the pipeline is updated but no new jobs are queued to the active
> > > > writeback connector.
> > > > 
> > > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> > > > and ask the CRTC drivers to set it to true when they know they're not
> > > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> > > > helper can then be used to fake VBLANKs at commit time.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_atomic_helper.h     |  1 +
> > > >  include/drm/drm_crtc.h              | 15 ++++++++++++++
> > > >  3 files changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 69063bcf2334..ca586993c2a2 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> > > >  
> > > > +/**
> > > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> > > > + * @old_state: atomic state object with old state structures
> > > > + *
> > > > + * This function walks all CRTCs and fake VBLANK events on those with
> > > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> > > > + * The primary use of this function is writeback connectors working in oneshot
> > > > + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> > > > + * when a job is queued, and any change to the pipeline that does not touch the
> > > > + * connector is leading to timeouts when calling
> > > > + * drm_atomic_helper_wait_for_vblanks() or
> > > > + * drm_atomic_helper_wait_for_flip_done().
> > > > + *
> > > > + * This is part of the atomic helper support for nonblocking commits, see
> > > > + * drm_atomic_helper_setup_commit() for an overview.
> > > > + */
> > > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> > > > +{
> > > > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > > > +	struct drm_crtc *crtc;
> > > > +	int i;
> > > > +
> > > > +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> > > > +				      new_crtc_state, i) {
> > > > +		unsigned long flags;
> > > > +
> > > > +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)    
> > > 
> > > Uh, this essentially makes it impossible to reset no_vblank.  
> > 
> > I don't want ->no_vblank to be reset by the core. It's up to the CRTC
> > driver to clear/set it when something changes in the pipeline.
> >   
> > > For control
> > > flow state bits we only check the new state for it (see e.g. the various
> > > *_changed or plane_bits or whatever).  
> > 
> > I tried with !new_crtc_state->no_vblank only, but then it does not
> > handle the case where the CRTC and connector are being disabled, and I
> > end up with a timeout.  
> 
> Why that? You should have a new_crtc_state even when you disable the crtc,
> and you can set the ->no_vblank on that one too.

Hm, right. I'll have to check why I didn't have ->no_blank set to true
when I disable the CRTC. Probably a problem in vc4_crtc.c.


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Daniel Vetter]

Forgot to address your other comments.

On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote:
> On Mon, 2 Jul 2018 10:02:52 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote:
> > > In some cases CRTCs are active but are not able to generating events, at
> > > least not at every frame at it's expected to.
> > > This is typically the case when the CRTC is feeding a writeback connector
> > > that has no job queued. In this situation the CRTC is usually stopped
> > > until a new job is queued, and this can lead to timeouts when part of
> > > the pipeline is updated but no new jobs are queued to the active
> > > writeback connector.
> > > 
> > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state
> > > and ask the CRTC drivers to set it to true when they know they're not
> > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank()
> > > helper can then be used to fake VBLANKs at commit time.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 40 +++++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_atomic_helper.h     |  1 +
> > >  include/drm/drm_crtc.h              | 15 ++++++++++++++
> > >  3 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 69063bcf2334..ca586993c2a2 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> > >  
> > > +/**
> > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
> > > + * @old_state: atomic state object with old state structures
> > > + *
> > > + * This function walks all CRTCs and fake VBLANK events on those with
> > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
> > > + * The primary use of this function is writeback connectors working in oneshot
> > > + * mode and faking VBLANK events. In this case they only fake the VBLANK event
> > > + * when a job is queued, and any change to the pipeline that does not touch the
> > > + * connector is leading to timeouts when calling
> > > + * drm_atomic_helper_wait_for_vblanks() or
> > > + * drm_atomic_helper_wait_for_flip_done().
> > > + *
> > > + * This is part of the atomic helper support for nonblocking commits, see
> > > + * drm_atomic_helper_setup_commit() for an overview.
> > > + */
> > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
> > > +{
> > > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > > +
> > > +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> > > +				      new_crtc_state, i) {
> > > +		unsigned long flags;
> > > +
> > > +		if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank)  
> > 
> > Uh, this essentially makes it impossible to reset no_vblank.
> 
> I don't want ->no_vblank to be reset by the core. It's up to the CRTC
> driver to clear/set it when something changes in the pipeline.
> 
> > For control
> > flow state bits we only check the new state for it (see e.g. the various
> > *_changed or plane_bits or whatever).
> 
> I tried with !new_crtc_state->no_vblank only, but then it does not
> handle the case where the CRTC and connector are being disabled, and I
> end up with a timeout.
> 
> > 
> > > +			continue;
> > > +
> > > +		spin_lock_irqsave(&old_state->dev->event_lock, flags);
> > > +		if (new_crtc_state->event) {
> > > +			drm_crtc_send_vblank_event(crtc,
> > > +						   new_crtc_state->event);
> > > +			new_crtc_state->event = NULL;
> > > +		}
> > > +		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
> > > +
> > >  /**
> > >   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> > >   * @old_state: atomic state object with old state structures
> > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > > index 26aaba58d6ce..99e2a5297c69 100644
> > > --- a/include/drm/drm_atomic_helper.h
> > > +++ b/include/drm/drm_atomic_helper.h
> > > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > >  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> > >  				   bool nonblock);
> > >  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state);
> > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state);
> > >  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state);
> > >  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state);
> > >  
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 23eddbccab10..7435dc66c08a 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs;
> > >   * @zpos_changed: zpos values of planes on this crtc have been updated
> > >   * @color_mgmt_changed: color management properties have changed (degamma or
> > >   *	gamma LUT or CSC matrix)
> > > + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state
> > > + *	usually depends on the pipeline configuration, and the main usuage is
> > > + *	CRTCs feeding a writeback connector operating in oneshot mode. In this
> > > + *	case the VBLANK event is only generated when a job is queued to the
> > > + *	writeback connector, and we want the core to fake VBLANK events when
> > > + *	this part of the pipeline hasn't changed but others had or when the
> > > + *	CRTC and connectors are disabled.  
> > 
> > s/are disabled/are getting disabled/
> > 
> > You'll never get a request for an event when it's already off.
> 
> That's true.
> 
> > 
> > > + *	__drm_atomic_helper_crtc_duplicate_state() will the value from the
> > > + *	current state, the CRTC driver is then responsible for updating this
> > > + *	field when needed.  
> > 
> > Not parsing the above ... probably missing a "... will not reset ..."
> 
> Exactly. Will add the missing words.
> 
> > 
> > > + *	Note that, even when no_blank is set to true, the CRTC driver can still
> > > + *	steal the &drm_crtc_state.event object and send the event on its own.
> > > + *	That's usually what happens when a job is queued to the writeback
> > > + *	connector.  
> > 
> > The last sentence is confusing imo. Just drop it?
> 
> Yes, I know, but it's also important to state that the ->no_blank +
> event == NULL is a valid combination, and just means that the driver
> decided to generate the event (that happens when a new WB job is
> queued).

Then make it more explicit, as-is I had no idea what you meant exactly.
What about
> 
> > 
> > Please use the inline comment style for struct members, and then also
> > polish the formatting a bit (e.g. paragraph breaks, which are only
> > possible with the inline style).
> 
> I considered that, but the other fields were already documented in the
> single block above the struct, so I thought keeping things consistent
> was better. Should I just add this field doc inline and keep the other
> ones where they are, or should I add a patch moving all docs inline?

There's _lots_ of inline comments already, and all new ones should be
inline. The only reason I haven't done the conversion to all of them is
that it would be a nice opportunity to update/clean up the comments
(often there's a lot more to say than what's captured in the single line),
which is why it didn't happen yet. Just moving the comments without
updating them seems less useful imo.

I'd just do the inline comment for this, that's what everyone else is
doing too.
-Daniel

> 
> > 
> > With the nits addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > >   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> > >   * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
> > >   * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
> > > @@ -118,6 +132,7 @@ struct drm_crtc_state {
> > >  	bool connectors_changed : 1;
> > >  	bool zpos_changed : 1;
> > >  	bool color_mgmt_changed : 1;
> > > +	bool no_vblank : 1;
> > >  
> > >  	/* attached planes bitmask:
> > >  	 * WARNING: transitional helpers do not maintain plane_mask so
> > > -- 
> > > 2.14.1
> > >   
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events

[Boris Brezillon]

On Mon, 2 Jul 2018 10:40:54 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> > >   
> > > > + *	Note that, even when no_blank is set to true, the CRTC driver can still
> > > > + *	steal the &drm_crtc_state.event object and send the event on its own.
> > > > + *	That's usually what happens when a job is queued to the writeback
> > > > + *	connector.    
> > > 
> > > The last sentence is confusing imo. Just drop it?  
> > 
> > Yes, I know, but it's also important to state that the ->no_blank +
> > event == NULL is a valid combination, and just means that the driver
> > decided to generate the event (that happens when a new WB job is
> > queued).  
> 
> Then make it more explicit, as-is I had no idea what you meant exactly.
> What about

Your suggestion is missing :P.

> >   
> > > 
> > > Please use the inline comment style for struct members, and then also
> > > polish the formatting a bit (e.g. paragraph breaks, which are only
> > > possible with the inline style).  
> > 
> > I considered that, but the other fields were already documented in the
> > single block above the struct, so I thought keeping things consistent
> > was better. Should I just add this field doc inline and keep the other
> > ones where they are, or should I add a patch moving all docs inline?  
> 
> There's _lots_ of inline comments already, and all new ones should be
> inline. The only reason I haven't done the conversion to all of them is
> that it would be a nice opportunity to update/clean up the comments
> (often there's a lot more to say than what's captured in the single line),
> which is why it didn't happen yet. Just moving the comments without
> updating them seems less useful imo.
> 
> I'd just do the inline comment for this, that's what everyone else is
> doing too.

Okay, will do.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Boris Brezillon]

Now that we have a way to fake VBLANK events when requested by the CRTC
hook it up to the generic commit_tail() helpers.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ca586993c2a2..1a088462bc42 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
+	drm_atomic_helper_fake_vblank(old_state);
+
 	drm_atomic_helper_commit_hw_done(old_state);
 
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
@@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 
+	drm_atomic_helper_fake_vblank(old_state);
+
 	drm_atomic_helper_commit_hw_done(old_state);
 
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Liviu Dudau]

On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
> Now that we have a way to fake VBLANK events when requested by the CRTC
> hook it up to the generic commit_tail() helpers.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ca586993c2a2..1a088462bc42 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> +	drm_atomic_helper_fake_vblank(old_state);
> +
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>  
> +	drm_atomic_helper_fake_vblank(old_state);
> +
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> -- 
> 2.14.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Daniel Vetter]

On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
> Now that we have a way to fake VBLANK events when requested by the CRTC
> hook it up to the generic commit_tail() helpers.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

I really don't like this. We've had epic amounts of bugs with atomic
drivers failing to send out vblank events when they should, and I added a
bunch of debug checks to make sure that doesn't happen anymore.

Now there's no way anymore for drivers to spot this until they have
misrenderings on wayland and no idea why. Imo this should only be used by
specific drivers, with a comment why exactly they need it.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ca586993c2a2..1a088462bc42 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
> +	drm_atomic_helper_fake_vblank(old_state);
> +
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>  
> +	drm_atomic_helper_fake_vblank(old_state);
> +
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> -- 
> 2.14.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Daniel Vetter]

On Mon, Jul 02, 2018 at 09:54:32AM +0200, Daniel Vetter wrote:
> On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
> > Now that we have a way to fake VBLANK events when requested by the CRTC
> > hook it up to the generic commit_tail() helpers.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> I really don't like this. We've had epic amounts of bugs with atomic
> drivers failing to send out vblank events when they should, and I added a
> bunch of debug checks to make sure that doesn't happen anymore.
> 
> Now there's no way anymore for drivers to spot this until they have
> misrenderings on wayland and no idea why. Imo this should only be used by
> specific drivers, with a comment why exactly they need it.

Meh I retract, you have the special no_vblank state flag to control this.
Looks all good to me.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ca586993c2a2..1a088462bc42 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> >  
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  
> > +	drm_atomic_helper_fake_vblank(old_state);
> > +
> >  	drm_atomic_helper_commit_hw_done(old_state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >  
> > +	drm_atomic_helper_fake_vblank(old_state);
> > +
> >  	drm_atomic_helper_commit_hw_done(old_state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > -- 
> > 2.14.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Boris Brezillon]

On Mon, 2 Jul 2018 09:57:30 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jul 02, 2018 at 09:54:32AM +0200, Daniel Vetter wrote:
> > On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:  
> > > Now that we have a way to fake VBLANK events when requested by the CRTC
> > > hook it up to the generic commit_tail() helpers.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> > 
> > I really don't like this. We've had epic amounts of bugs with atomic
> > drivers failing to send out vblank events when they should, and I added a
> > bunch of debug checks to make sure that doesn't happen anymore.
> > 
> > Now there's no way anymore for drivers to spot this until they have
> > misrenderings on wayland and no idea why. Imo this should only be used by
> > specific drivers, with a comment why exactly they need it.  
> 
> Meh I retract, you have the special no_vblank state flag to control this.
> Looks all good to me.

Hehe, just replied to your other email :-).

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > -Daniel
> >   
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index ca586993c2a2..1a088462bc42 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> > >  
> > >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > >  
> > > +	drm_atomic_helper_fake_vblank(old_state);
> > > +
> > >  	drm_atomic_helper_commit_hw_done(old_state);
> > >  
> > >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > > @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> > >  	drm_atomic_helper_commit_planes(dev, old_state,
> > >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > >  
> > > +	drm_atomic_helper_fake_vblank(old_state);
> > > +
> > >  	drm_atomic_helper_commit_hw_done(old_state);
> > >  
> > >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > > -- 
> > > 2.14.1
> > >   
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch  
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic commit_tail() helpers

[Boris Brezillon]

On Mon, 2 Jul 2018 09:54:32 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Jun 29, 2018 at 01:17:18PM +0200, Boris Brezillon wrote:
> > Now that we have a way to fake VBLANK events when requested by the CRTC
> > hook it up to the generic commit_tail() helpers.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> 
> I really don't like this. We've had epic amounts of bugs with atomic
> drivers failing to send out vblank events when they should, and I added a
> bunch of debug checks to make sure that doesn't happen anymore.
> 
> Now there's no way anymore for drivers to spot this until they have
> misrenderings on wayland and no idea why. Imo this should only be used by
> specific drivers, with a comment why exactly they need it.

Hm, that's not entirely true. Drivers have to explicitly set
->no_vblank to true for drm_atomic_helper_fake_vblank() to actually do
something, and I seriously hope drivers won't set that field randomly.
So, on all existing drivers but VC4 after the TXP patch, the behavior
will be unchanged, and the core will spot misbehaving drivers just like
before.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ca586993c2a2..1a088462bc42 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1448,6 +1448,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> >  
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  
> > +	drm_atomic_helper_fake_vblank(old_state);
> > +
> >  	drm_atomic_helper_commit_hw_done(old_state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > @@ -1477,6 +1479,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >  
> > +	drm_atomic_helper_fake_vblank(old_state);
> > +
> >  	drm_atomic_helper_commit_hw_done(old_state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > -- 
> > 2.14.1
> >   
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/8] drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path

[Boris Brezillon]

Mimic what is done in drm_atomic_commit_tail() and call
drm_atomic_helper_fake_vblank() so that VBLANK events are faked
when the drm_crtc_state.no_vblank is true. Will be needed when we'll
add support for the transposer block.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 91239b0a4fa0..ca5aa7fba769 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -153,6 +153,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
+	drm_atomic_helper_fake_vblank(state);
+
 	drm_atomic_helper_commit_hw_done(state);
 
 	drm_atomic_helper_wait_for_flip_done(dev, state);
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/8] drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 334 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> Mimic what is done in drm_atomic_commit_tail() and call
> drm_atomic_helper_fake_vblank() so that VBLANK events are faked
> when the drm_crtc_state.no_vblank is true. Will be needed when we'll
> add support for the transposer block.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 7/8] drm/vc4: Add support for the transposer block

[Boris Brezillon]

From: Boris Brezillon <boris.brezillon@free-electrons.com>

The transposer block is providing support for mem-to-mem composition,
which is exposed as a drm_writeback connector in DRM.

Add a driver to support this feature.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/display/brcm,bcm-vc4.txt   |   6 +
 drivers/gpu/drm/vc4/Makefile                       |   1 +
 drivers/gpu/drm/vc4/vc4_crtc.c                     | 139 +++++-
 drivers/gpu/drm/vc4/vc4_debugfs.c                  |   1 +
 drivers/gpu/drm/vc4/vc4_drv.c                      |   1 +
 drivers/gpu/drm/vc4/vc4_drv.h                      |   7 +
 drivers/gpu/drm/vc4/vc4_regs.h                     |   1 +
 drivers/gpu/drm/vc4/vc4_txp.c                      | 487 +++++++++++++++++++++
 8 files changed, 619 insertions(+), 24 deletions(-)
 create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c

diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index 284e2b14cfbe..26649b4c4dd8 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -74,6 +74,12 @@ Required properties for DSI:
 		The 3 clocks output from the DSI analog PHY: dsi[01]_byte,
 		dsi[01]_ddr2, and dsi[01]_ddr
 
+Required properties for the TXP (writeback) block:
+- compatible:	Should be "brcm,bcm2835-txp"
+- reg:		Physical base address and length of the TXP block's registers
+- interrupts:	The interrupt number
+		  See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+
 [1] Documentation/devicetree/bindings/media/video-interfaces.txt
 
 Example:
diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile
index 4a3a868235f8..b303703bc7f3 100644
--- a/drivers/gpu/drm/vc4/Makefile
+++ b/drivers/gpu/drm/vc4/Makefile
@@ -19,6 +19,7 @@ vc4-y := \
 	vc4_plane.o \
 	vc4_render_cl.o \
 	vc4_trace_points.o \
+	vc4_txp.o \
 	vc4_v3d.o \
 	vc4_validate.o \
 	vc4_validate_shaders.o
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index dcadf793ee80..477dadc8d6db 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -46,6 +46,8 @@ struct vc4_crtc_state {
 	struct drm_crtc_state base;
 	/* Dlist area for this CRTC configuration. */
 	struct drm_mm_node mm;
+	bool feed_txp;
+	bool txp_armed;
 };
 
 static inline struct vc4_crtc_state *
@@ -324,10 +326,8 @@ static struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc)
 	return NULL;
 }
 
-static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
+static void vc4_crtc_config_pv(struct drm_crtc *crtc)
 {
-	struct drm_device *dev = crtc->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc);
 	struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
 	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
@@ -338,12 +338,6 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	bool is_dsi = (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 ||
 		       vc4_encoder->type == VC4_ENCODER_TYPE_DSI1);
 	u32 format = is_dsi ? PV_CONTROL_FORMAT_DSIV_24 : PV_CONTROL_FORMAT_24;
-	bool debug_dump_regs = false;
-
-	if (debug_dump_regs) {
-		DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));
-		vc4_crtc_dump_regs(vc4_crtc);
-	}
 
 	/* Reset the PV fifo. */
 	CRTC_WRITE(PV_CONTROL, 0);
@@ -419,6 +413,52 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
 				 PV_CONTROL_CLK_SELECT) |
 		   PV_CONTROL_FIFO_CLR |
 		   PV_CONTROL_EN);
+}
+
+static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
+	bool debug_dump_regs = false;
+
+	if (debug_dump_regs) {
+		DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));
+		vc4_crtc_dump_regs(vc4_crtc);
+	}
+
+	if (vc4_crtc->channel == 2) {
+		u32 dispctrl;
+		u32 dsp3_mux;
+
+		/*
+		 * SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to
+		 * FIFO X'.
+		 * SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.
+		 *
+		 * DSP3 is connected to FIFO2 unless the transposer is
+		 * enabled. In this case, FIFO 2 is directly accessed by the
+		 * TXP IP, and we need to prevent disable the
+		 * FIFO2 -> pixelvalve1 route.
+		 */
+		if (vc4_state->feed_txp)
+			dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
+		else
+			dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
+
+		/* Reconfigure the DSP3 mux if required. */
+		dispctrl = HVS_READ(SCALER_DISPCTRL);
+		if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {
+			dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
+			HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);
+		}
+	}
+
+	if (!vc4_state->feed_txp)
+		vc4_crtc_config_pv(crtc);
 
 	HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
 		  SCALER_DISPBKGND_AUTOHS |
@@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
 	}
 }
 
+void vc4_crtc_txp_armed(struct drm_crtc_state *state)
+{
+	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
+
+	vc4_state->txp_armed = true;
+}
+
 static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
 		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
 
 		spin_lock_irqsave(&dev->event_lock, flags);
-		vc4_crtc->event = crtc->state->event;
-		crtc->state->event = NULL;
+
+		if (!vc4_state->feed_txp || vc4_state->txp_armed) {
+			vc4_crtc->event = crtc->state->event;
+			crtc->state->event = NULL;
+		}
 
 		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
 			  vc4_state->mm.start);
@@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
-	struct drm_crtc_state *state = crtc->state;
-	struct drm_display_mode *mode = &state->adjusted_mode;
+	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 
 	require_hvs_enabled(dev);
 
@@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	/* Turn on the scaler, which will wait for vstart to start
 	 * compositing.
+	 * When feeding the transposer, we should operate in oneshot
+	 * mode.
 	 */
 	HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel),
 		  VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) |
 		  VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) |
-		  SCALER_DISPCTRLX_ENABLE);
+		  SCALER_DISPCTRLX_ENABLE |
+		  (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
 
-	/* Turn on the pixel valve, which will emit the vstart signal. */
-	CRTC_WRITE(PV_V_CONTROL,
-		   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
+	/* When feeding the composer block the pixelvalve is unneeded and
+	 * should not be enabled.
+	 */
+	if (!vc4_state->feed_txp)
+		CRTC_WRITE(PV_V_CONTROL,
+			   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
 }
 
 static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
@@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	struct drm_plane *plane;
 	unsigned long flags;
 	const struct drm_plane_state *plane_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *conn_state;
 	u32 dlist_count = 0;
-	int ret;
+	int ret, i;
 
 	/* The pixelvalve can only feed one encoder (and encoders are
 	 * 1:1 with connectors.)
@@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	state->no_vblank = false;
+	for_each_new_connector_in_state(state->state, conn, conn_state, i) {
+		if (conn_state->crtc != crtc)
+			continue;
+
+		/* The writeback connector is implemented using the transposer
+		 * block which is directly taking its data from the HVS FIFO.
+		 */
+		if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
+			state->no_vblank = true;
+			vc4_state->feed_txp = true;
+		}
+
+		break;
+	}
+
 	return 0;
 }
 
@@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 	if (vc4_crtc->event &&
-	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
+	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) ||
+	     vc4_state->feed_txp)) {
 		drm_crtc_send_vblank_event(crtc, vc4_crtc->event);
 		vc4_crtc->event = NULL;
 		drm_crtc_vblank_put(crtc);
@@ -721,6 +796,13 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
 
+void vc4_crtc_handle_vblank(struct vc4_crtc *crtc)
+{
+	crtc->t_vblank = ktime_get();
+	drm_crtc_handle_vblank(&crtc->base);
+	vc4_crtc_handle_page_flip(crtc);
+}
+
 static irqreturn_t vc4_crtc_irq_handler(int irq, void *data)
 {
 	struct vc4_crtc *vc4_crtc = data;
@@ -728,10 +810,8 @@ static irqreturn_t vc4_crtc_irq_handler(int irq, void *data)
 	irqreturn_t ret = IRQ_NONE;
 
 	if (stat & PV_INT_VFP_START) {
-		vc4_crtc->t_vblank = ktime_get();
 		CRTC_WRITE(PV_INTSTAT, PV_INT_VFP_START);
-		drm_crtc_handle_vblank(&vc4_crtc->base);
-		vc4_crtc_handle_page_flip(vc4_crtc);
+		vc4_crtc_handle_vblank(vc4_crtc);
 		ret = IRQ_HANDLED;
 	}
 
@@ -884,12 +964,15 @@ static int vc4_page_flip(struct drm_crtc *crtc,
 
 static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc)
 {
-	struct vc4_crtc_state *vc4_state;
+	struct vc4_crtc_state *vc4_state, *old_vc4_state;
 
 	vc4_state = kzalloc(sizeof(*vc4_state), GFP_KERNEL);
 	if (!vc4_state)
 		return NULL;
 
+	old_vc4_state = to_vc4_crtc_state(crtc->state);
+	vc4_state->feed_txp = old_vc4_state->feed_txp;
+
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &vc4_state->base);
 	return &vc4_state->base;
 }
@@ -987,9 +1070,17 @@ static void vc4_set_crtc_possible_masks(struct drm_device *drm,
 	struct drm_encoder *encoder;
 
 	drm_for_each_encoder(encoder, drm) {
-		struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
+		struct vc4_encoder *vc4_encoder;
 		int i;
 
+		/* HVS FIFO2 can feed the TXP IP. */
+		if (crtc_data->hvs_channel == 2 &&
+		    encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL) {
+			encoder->possible_crtcs |= drm_crtc_mask(crtc);
+			continue;
+		}
+
+		vc4_encoder = to_vc4_encoder(encoder);
 		for (i = 0; i < ARRAY_SIZE(crtc_data->encoder_types); i++) {
 			if (vc4_encoder->type == encoder_types[i]) {
 				vc4_encoder->clock_select = i;
diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index 5db06bdb5f27..7a0003de71ab 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -21,6 +21,7 @@ static const struct drm_info_list vc4_debugfs_list[] = {
 	{"dsi1_regs", vc4_dsi_debugfs_regs, 0, (void *)(uintptr_t)1},
 	{"hdmi_regs", vc4_hdmi_debugfs_regs, 0},
 	{"vec_regs", vc4_vec_debugfs_regs, 0},
+	{"txp_regs", vc4_txp_debugfs_regs, 0},
 	{"hvs_regs", vc4_hvs_debugfs_regs, 0},
 	{"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0},
 	{"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1},
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 466d0a27b415..e42fd5ec41cc 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -344,6 +344,7 @@ static struct platform_driver *const component_drivers[] = {
 	&vc4_vec_driver,
 	&vc4_dpi_driver,
 	&vc4_dsi_driver,
+	&vc4_txp_driver,
 	&vc4_hvs_driver,
 	&vc4_crtc_driver,
 	&vc4_v3d_driver,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index eace76c621a1..bd6ef1f31822 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -73,6 +73,7 @@ struct vc4_dev {
 	struct vc4_dpi *dpi;
 	struct vc4_dsi *dsi1;
 	struct vc4_vec *vec;
+	struct vc4_txp *txp;
 
 	struct vc4_hang_state *hang_state;
 
@@ -698,6 +699,8 @@ bool vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
 			     bool in_vblank_irq, int *vpos, int *hpos,
 			     ktime_t *stime, ktime_t *etime,
 			     const struct drm_display_mode *mode);
+void vc4_crtc_handle_vblank(struct vc4_crtc *crtc);
+void vc4_crtc_txp_armed(struct drm_crtc_state *state);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
@@ -745,6 +748,10 @@ int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused);
 extern struct platform_driver vc4_vec_driver;
 int vc4_vec_debugfs_regs(struct seq_file *m, void *unused);
 
+/* vc4_txp.c */
+extern struct platform_driver vc4_txp_driver;
+int vc4_txp_debugfs_regs(struct seq_file *m, void *unused);
+
 /* vc4_irq.c */
 irqreturn_t vc4_irq(int irq, void *arg);
 void vc4_irq_preinstall(struct drm_device *dev);
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index d6864fa4bd14..744a689751f0 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -330,6 +330,7 @@
 #define SCALER_DISPCTRL0                        0x00000040
 # define SCALER_DISPCTRLX_ENABLE		BIT(31)
 # define SCALER_DISPCTRLX_RESET			BIT(30)
+
 /* Generates a single frame when VSTART is seen and stops at the last
  * pixel read from the FIFO.
  */
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
new file mode 100644
index 000000000000..e352d1916fc4
--- /dev/null
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright © 2018 Broadcom
+ *
+ * Authors:
+ *	Eric Anholt <eric@anholt.net>
+ *	Boris Brezillon <boris.brezillon@bootlin.com>
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_writeback.h>
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+
+#include "vc4_drv.h"
+#include "vc4_regs.h"
+
+/* Base address of the output.  Raster formats must be 4-byte aligned,
+ * T and LT must be 16-byte aligned or maybe utile-aligned (docs are
+ * inconsistent, but probably utile).
+ */
+#define TXP_DST_PTR		0x00
+
+/* Pitch in bytes for raster images, 16-byte aligned.  For tiled, it's
+ * the width in tiles.
+ */
+#define TXP_DST_PITCH		0x04
+/* For T-tiled imgaes, DST_PITCH should be the number of tiles wide,
+ * shifted up.
+ */
+# define TXP_T_TILE_WIDTH_SHIFT		7
+/* For LT-tiled images, DST_PITCH should be the number of utiles wide,
+ * shifted up.
+ */
+# define TXP_LT_TILE_WIDTH_SHIFT	4
+
+/* Pre-rotation width/height of the image.  Must match HVS config.
+ *
+ * If TFORMAT and 32-bit, limit is 1920 for 32-bit and 3840 to 16-bit
+ * and width/height must be tile or utile-aligned as appropriate.  If
+ * transposing (rotating), width is limited to 1920.
+ *
+ * Height is limited to various numbers between 4088 and 4095.  I'd
+ * just use 4088 to be safe.
+ */
+#define TXP_DIM			0x08
+# define TXP_HEIGHT_SHIFT		16
+# define TXP_HEIGHT_MASK		GENMASK(31, 16)
+# define TXP_WIDTH_SHIFT		0
+# define TXP_WIDTH_MASK			GENMASK(15, 0)
+
+#define TXP_DST_CTRL		0x0c
+/* These bits are set to 0x54 */
+#define TXP_PILOT_SHIFT			24
+#define TXP_PILOT_MASK			GENMASK(31, 24)
+/* Bits 22-23 are set to 0x01 */
+#define TXP_VERSION_SHIFT		22
+#define TXP_VERSION_MASK		GENMASK(23, 22)
+
+/* Powers down the internal memory. */
+# define TXP_POWERDOWN			BIT(21)
+
+/* Enables storing the alpha component in 8888/4444, instead of
+ * filling with ~ALPHA_INVERT.
+ */
+# define TXP_ALPHA_ENABLE		BIT(20)
+
+/* 4 bits, each enables stores for a channel in each set of 4 bytes.
+ * Set to 0xf for normal operation.
+ */
+# define TXP_BYTE_ENABLE_SHIFT		16
+# define TXP_BYTE_ENABLE_MASK		GENMASK(19, 16)
+
+/* Debug: Generate VSTART again at EOF. */
+# define TXP_VSTART_AT_EOF		BIT(15)
+
+/* Debug: Terminate the current frame immediately.  Stops AXI
+ * writes.
+ */
+# define TXP_ABORT			BIT(14)
+
+# define TXP_DITHER			BIT(13)
+
+/* Inverts alpha if TXP_ALPHA_ENABLE, chooses fill value for
+ * !TXP_ALPHA_ENABLE.
+ */
+# define TXP_ALPHA_INVERT		BIT(12)
+
+/* Note: I've listed the channels here in high bit (in byte 3/2/1) to
+ * low bit (in byte 0) order.
+ */
+# define TXP_FORMAT_SHIFT		8
+# define TXP_FORMAT_MASK		GENMASK(11, 8)
+# define TXP_FORMAT_ABGR4444		0
+# define TXP_FORMAT_ARGB4444		1
+# define TXP_FORMAT_BGRA4444		2
+# define TXP_FORMAT_RGBA4444		3
+# define TXP_FORMAT_BGR565		6
+# define TXP_FORMAT_RGB565		7
+/* 888s are non-rotated, raster-only */
+# define TXP_FORMAT_BGR888		8
+# define TXP_FORMAT_RGB888		9
+# define TXP_FORMAT_ABGR8888		12
+# define TXP_FORMAT_ARGB8888		13
+# define TXP_FORMAT_BGRA8888		14
+# define TXP_FORMAT_RGBA8888		15
+
+/* If TFORMAT is set, generates LT instead of T format. */
+# define TXP_LINEAR_UTILE		BIT(7)
+
+/* Rotate output by 90 degrees. */
+# define TXP_TRANSPOSE			BIT(6)
+
+/* Generate a tiled format for V3D. */
+# define TXP_TFORMAT			BIT(5)
+
+/* Generates some undefined test mode output. */
+# define TXP_TEST_MODE			BIT(4)
+
+/* Request odd field from HVS. */
+# define TXP_FIELD			BIT(3)
+
+/* Raise interrupt when idle. */
+# define TXP_EI				BIT(2)
+
+/* Set when generating a frame, clears when idle. */
+# define TXP_BUSY			BIT(1)
+
+/* Starts a frame.  Self-clearing. */
+# define TXP_GO				BIT(0)
+
+/* Number of lines received and committed to memory. */
+#define TXP_PROGRESS		0x10
+
+#define TXP_READ(offset) readl(txp->regs + (offset))
+#define TXP_WRITE(offset, val) writel(val, txp->regs + (offset))
+
+struct vc4_txp {
+	struct platform_device *pdev;
+
+	struct drm_writeback_connector connector;
+
+	void __iomem *regs;
+};
+
+static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
+{
+	return container_of(encoder, struct vc4_txp, connector.encoder);
+}
+
+static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
+{
+	return container_of(conn, struct vc4_txp, connector.base);
+}
+
+#define TXP_REG(reg) { reg, #reg }
+static const struct {
+	u32 reg;
+	const char *name;
+} txp_regs[] = {
+	TXP_REG(TXP_DST_PTR),
+	TXP_REG(TXP_DST_PITCH),
+	TXP_REG(TXP_DIM),
+	TXP_REG(TXP_DST_CTRL),
+	TXP_REG(TXP_PROGRESS),
+};
+
+#ifdef CONFIG_DEBUG_FS
+int vc4_txp_debugfs_regs(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *)m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_txp *txp = vc4->txp;
+	int i;
+
+	if (!txp)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(txp_regs); i++) {
+		seq_printf(m, "%s (0x%04x): 0x%08x\n",
+			   txp_regs[i].name, txp_regs[i].reg,
+			   TXP_READ(txp_regs[i].reg));
+	}
+
+	return 0;
+}
+#endif
+
+static int vc4_txp_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				    dev->mode_config.max_height);
+}
+
+static enum drm_mode_status
+vc4_txp_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	int w = mode->hdisplay, h = mode->vdisplay;
+
+	if (w < mode_config->min_width || w > mode_config->max_width)
+		return MODE_BAD_HVALUE;
+
+	if (h < mode_config->min_height || h > mode_config->max_height)
+		return MODE_BAD_VVALUE;
+
+	return MODE_OK;
+}
+
+static const u32 vc4_txp_formats[] = {
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_RGBX8888,
+	DRM_FORMAT_BGRX8888,
+	DRM_FORMAT_RGBA8888,
+	DRM_FORMAT_BGRA8888,
+};
+
+static int vc4_txp_connector_atomic_check(struct drm_connector *conn,
+					struct drm_connector_state *conn_state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_gem_cma_object *gem;
+	struct drm_framebuffer *fb;
+	int i;
+
+	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
+		return 0;
+
+	crtc_state = drm_atomic_get_new_crtc_state(conn_state->state,
+						   conn_state->crtc);
+
+	fb = conn_state->writeback_job->fb;
+	if (fb->width != crtc_state->mode.hdisplay ||
+	    fb->height != crtc_state->mode.vdisplay) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+			      fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(vc4_txp_formats); i++) {
+		if (fb->format->format == vc4_txp_formats[i])
+			break;
+	}
+
+	if (i == ARRAY_SIZE(vc4_txp_formats))
+		return -EINVAL;
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+
+	/* Pitch must be aligned on 16 bytes. */
+	if (fb->pitches[0] & GENMASK(3, 0))
+		return -EINVAL;
+
+	vc4_crtc_txp_armed(crtc_state);
+
+	return 0;
+}
+
+static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
+					struct drm_connector_state *conn_state)
+{
+	struct vc4_txp *txp = connector_to_vc4_txp(conn);
+	struct drm_gem_cma_object *gem;
+	struct drm_display_mode *mode;
+	struct drm_framebuffer *fb;
+	u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
+
+	if (WARN_ON(!conn_state->writeback_job ||
+		    !conn_state->writeback_job->fb))
+		return;
+
+	mode = &conn_state->crtc->state->adjusted_mode;
+	fb = conn_state->writeback_job->fb;
+
+	switch (fb->format->format) {
+	case DRM_FORMAT_ARGB8888:
+		ctrl |= TXP_ALPHA_ENABLE;
+	case DRM_FORMAT_XRGB8888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_ARGB8888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+
+	case DRM_FORMAT_ABGR8888:
+		ctrl |= TXP_ALPHA_ENABLE;
+	case DRM_FORMAT_XBGR8888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_ABGR8888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+
+	case DRM_FORMAT_RGBA8888:
+		ctrl |= TXP_ALPHA_ENABLE;
+	case DRM_FORMAT_RGBX8888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGBA8888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+
+	case DRM_FORMAT_BGRA8888:
+		ctrl |= TXP_ALPHA_ENABLE;
+	case DRM_FORMAT_BGRX8888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGRA8888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+
+	case DRM_FORMAT_BGR888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_BGR888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+
+	case DRM_FORMAT_RGB888:
+		ctrl |= VC4_SET_FIELD(TXP_FORMAT_RGB888, TXP_FORMAT) |
+			VC4_SET_FIELD(0xf, TXP_BYTE_ENABLE);
+		break;
+	default:
+		WARN_ON(1);
+		return;
+	}
+
+	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]);
+	TXP_WRITE(TXP_DST_PITCH, fb->pitches[0]);
+	TXP_WRITE(TXP_DIM,
+		  VC4_SET_FIELD(mode->hdisplay, TXP_WIDTH) |
+		  VC4_SET_FIELD(mode->vdisplay, TXP_HEIGHT));
+
+	TXP_WRITE(TXP_DST_CTRL, ctrl);
+
+	drm_writeback_queue_job(&txp->connector, conn_state->writeback_job);
+}
+
+static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = {
+	.get_modes = vc4_txp_connector_get_modes,
+	.mode_valid = vc4_txp_connector_mode_valid,
+	.atomic_check = vc4_txp_connector_atomic_check,
+	.atomic_commit = vc4_txp_connector_atomic_commit,
+};
+
+static enum drm_connector_status
+vc4_txp_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static void vc4_txp_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs vc4_txp_connector_funcs = {
+	.detect = vc4_txp_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = vc4_txp_connector_destroy,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static void vc4_txp_encoder_disable(struct drm_encoder *encoder)
+{
+	struct vc4_txp *txp = encoder_to_vc4_txp(encoder);
+
+	if (TXP_READ(TXP_DST_CTRL) & TXP_BUSY) {
+		TXP_WRITE(TXP_DST_CTRL, TXP_ABORT);
+
+		while (TXP_READ(TXP_DST_CTRL) & TXP_BUSY)
+			;
+	}
+
+	TXP_WRITE(TXP_DST_CTRL, TXP_POWERDOWN);
+}
+
+static const struct drm_encoder_helper_funcs vc4_txp_encoder_helper_funcs = {
+	.disable = vc4_txp_encoder_disable,
+};
+
+static irqreturn_t vc4_txp_interrupt(int irq, void *data)
+{
+	struct vc4_txp *txp = data;
+
+	TXP_WRITE(TXP_DST_CTRL, TXP_READ(TXP_DST_CTRL) & ~TXP_EI);
+	vc4_crtc_handle_vblank(to_vc4_crtc(txp->connector.base.state->crtc));
+	drm_writeback_signal_completion(&txp->connector, 0);
+
+	return IRQ_HANDLED;
+}
+
+static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm = dev_get_drvdata(master);
+	struct vc4_dev *vc4 = to_vc4_dev(drm);
+	struct vc4_txp *txp;
+	int ret, irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL);
+	if (!txp)
+		return -ENOMEM;
+
+	txp->pdev = pdev;
+
+	txp->regs = vc4_ioremap_regs(pdev, 0);
+	if (IS_ERR(txp->regs))
+		return PTR_ERR(txp->regs);
+
+	drm_connector_helper_add(&txp->connector.base,
+				 &vc4_txp_connector_helper_funcs);
+	ret = drm_writeback_connector_init(drm, &txp->connector,
+					   &vc4_txp_connector_funcs,
+					   &vc4_txp_encoder_helper_funcs,
+					   vc4_txp_formats,
+					   ARRAY_SIZE(vc4_txp_formats));
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
+			       dev_name(dev), txp);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, txp);
+	vc4->txp = txp;
+
+	return 0;
+}
+
+static void vc4_txp_unbind(struct device *dev, struct device *master,
+			   void *data)
+{
+	struct drm_device *drm = dev_get_drvdata(master);
+	struct vc4_dev *vc4 = to_vc4_dev(drm);
+	struct vc4_txp *txp = dev_get_drvdata(dev);
+
+	vc4_txp_connector_destroy(&txp->connector.base);
+
+	vc4->txp = NULL;
+}
+
+static const struct component_ops vc4_txp_ops = {
+	.bind   = vc4_txp_bind,
+	.unbind = vc4_txp_unbind,
+};
+
+static int vc4_txp_probe(struct platform_device *pdev)
+{
+	return component_add(&pdev->dev, &vc4_txp_ops);
+}
+
+static int vc4_txp_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &vc4_txp_ops);
+	return 0;
+}
+
+static const struct of_device_id vc4_txp_dt_match[] = {
+	{ .compatible = "brcm,bcm2835-txp" },
+	{ /* sentinel */ },
+};
+
+struct platform_driver vc4_txp_driver = {
+	.probe = vc4_txp_probe,
+	.remove = vc4_txp_remove,
+	.driver = {
+		.name = "vc4_txp",
+		.of_match_table = vc4_txp_dt_match,
+	},
+};
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/8] drm/vc4: Add support for the transposer block

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 7545 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> From: Boris Brezillon <boris.brezillon@free-electrons.com>
>
> The transposer block is providing support for mem-to-mem composition,
> which is exposed as a drm_writeback connector in DRM.
>
> Add a driver to support this feature.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> +static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> +	bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
> +	bool debug_dump_regs = false;
> +
> +	if (debug_dump_regs) {
> +		DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));
> +		vc4_crtc_dump_regs(vc4_crtc);
> +	}
> +
> +	if (vc4_crtc->channel == 2) {
> +		u32 dispctrl;
> +		u32 dsp3_mux;
> +
> +		/*
> +		 * SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to
> +		 * FIFO X'.
> +		 * SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.
> +		 *
> +		 * DSP3 is connected to FIFO2 unless the transposer is
> +		 * enabled. In this case, FIFO 2 is directly accessed by the
> +		 * TXP IP, and we need to prevent disable the

s/prevent //

> +		 * FIFO2 -> pixelvalve1 route.
> +		 */
> +		if (vc4_state->feed_txp)
> +			dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
> +		else
> +			dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
> +
> +		/* Reconfigure the DSP3 mux if required. */
> +		dispctrl = HVS_READ(SCALER_DISPCTRL);
> +		if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {
> +			dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> +			HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);
> +		}

This is fine, but you could also skip the matching mux check here -- the
read is the expensive part.

> +	}
> +
> +	if (!vc4_state->feed_txp)
> +		vc4_crtc_config_pv(crtc);
>  
>  	HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
>  		  SCALER_DISPBKGND_AUTOHS |
> @@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
>  	}
>  }
>  
> +void vc4_crtc_txp_armed(struct drm_crtc_state *state)
> +{
> +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
> +
> +	vc4_state->txp_armed = true;
> +}
> +
>  static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
>  		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>  
>  		spin_lock_irqsave(&dev->event_lock, flags);
> -		vc4_crtc->event = crtc->state->event;
> -		crtc->state->event = NULL;
> +
> +		if (!vc4_state->feed_txp || vc4_state->txp_armed) {
> +			vc4_crtc->event = crtc->state->event;
> +			crtc->state->event = NULL;
> +		}
>  
>  		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
>  			  vc4_state->mm.start);
> @@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> -	struct drm_crtc_state *state = crtc->state;
> -	struct drm_display_mode *mode = &state->adjusted_mode;
> +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>  
>  	require_hvs_enabled(dev);
>  
> @@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	/* Turn on the scaler, which will wait for vstart to start
>  	 * compositing.
> +	 * When feeding the transposer, we should operate in oneshot
> +	 * mode.
>  	 */
>  	HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel),
>  		  VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) |
>  		  VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) |
> -		  SCALER_DISPCTRLX_ENABLE);
> +		  SCALER_DISPCTRLX_ENABLE |
> +		  (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
>  
> -	/* Turn on the pixel valve, which will emit the vstart signal. */
> -	CRTC_WRITE(PV_V_CONTROL,
> -		   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
> +	/* When feeding the composer block the pixelvalve is unneeded and

"transposer block"? composer block made me think HVS.

> +	 * should not be enabled.
> +	 */
> +	if (!vc4_state->feed_txp)
> +		CRTC_WRITE(PV_V_CONTROL,
> +			   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
>  }
>  
>  static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
> @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct drm_plane *plane;
>  	unsigned long flags;
>  	const struct drm_plane_state *plane_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *conn_state;
>  	u32 dlist_count = 0;
> -	int ret;
> +	int ret, i;
>  
>  	/* The pixelvalve can only feed one encoder (and encoders are
>  	 * 1:1 with connectors.)
> @@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
>  	if (ret)
>  		return ret;
>  
> +	state->no_vblank = false;
> +	for_each_new_connector_in_state(state->state, conn, conn_state, i) {
> +		if (conn_state->crtc != crtc)
> +			continue;
> +
> +		/* The writeback connector is implemented using the transposer
> +		 * block which is directly taking its data from the HVS FIFO.
> +		 */
> +		if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
> +			state->no_vblank = true;
> +			vc4_state->feed_txp = true;
> +		}
> +
> +		break;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  	if (vc4_crtc->event &&
> -	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
> +	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) ||
> +	     vc4_state->feed_txp)) {

Can vc4_crtc->event even be set if vc4_state->feed_txp?

> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index d6864fa4bd14..744a689751f0 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -330,6 +330,7 @@
>  #define SCALER_DISPCTRL0                        0x00000040
>  # define SCALER_DISPCTRLX_ENABLE		BIT(31)
>  # define SCALER_DISPCTRLX_RESET			BIT(30)
> +
>  /* Generates a single frame when VSTART is seen and stops at the last
>   * pixel read from the FIFO.
>   */

stray hunk?

> +static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct vc4_txp *txp = connector_to_vc4_txp(conn);
> +	struct drm_gem_cma_object *gem;
> +	struct drm_display_mode *mode;
> +	struct drm_framebuffer *fb;
> +	u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
> +
> +	if (WARN_ON(!conn_state->writeback_job ||
> +		    !conn_state->writeback_job->fb))
> +		return;
> +
> +	mode = &conn_state->crtc->state->adjusted_mode;
> +	fb = conn_state->writeback_job->fb;
> +
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_ARGB8888:
> +		ctrl |= TXP_ALPHA_ENABLE;

Optional suggestion: Have the txp_formats[] table be a struct with these
register values in it.

All my feedback seems really minor, so with whatever components you like
integrated, feel free to add:

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/8] drm/vc4: Add support for the transposer block

[Boris Brezillon]

Hi Eric,

On Fri, 29 Jun 2018 13:35:04 -0700
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> >
> > The transposer block is providing support for mem-to-mem composition,
> > which is exposed as a drm_writeback connector in DRM.
> >
> > Add a driver to support this feature.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>  
> 
> > +static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> > +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> > +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> > +	bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
> > +	bool debug_dump_regs = false;
> > +
> > +	if (debug_dump_regs) {
> > +		DRM_INFO("CRTC %d regs before:\n", drm_crtc_index(crtc));
> > +		vc4_crtc_dump_regs(vc4_crtc);
> > +	}
> > +
> > +	if (vc4_crtc->channel == 2) {
> > +		u32 dispctrl;
> > +		u32 dsp3_mux;
> > +
> > +		/*
> > +		 * SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3 to
> > +		 * FIFO X'.
> > +		 * SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.
> > +		 *
> > +		 * DSP3 is connected to FIFO2 unless the transposer is
> > +		 * enabled. In this case, FIFO 2 is directly accessed by the
> > +		 * TXP IP, and we need to prevent disable the  
> 
> s/prevent //
> 
> > +		 * FIFO2 -> pixelvalve1 route.
> > +		 */
> > +		if (vc4_state->feed_txp)
> > +			dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
> > +		else
> > +			dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
> > +
> > +		/* Reconfigure the DSP3 mux if required. */
> > +		dispctrl = HVS_READ(SCALER_DISPCTRL);
> > +		if ((dispctrl & SCALER_DISPCTRL_DSP3_MUX_MASK) != dsp3_mux) {
> > +			dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> > +			HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);
> > +		}  
> 
> This is fine, but you could also skip the matching mux check here -- the
> read is the expensive part.
> 
> > +	}
> > +
> > +	if (!vc4_state->feed_txp)
> > +		vc4_crtc_config_pv(crtc);
> >  
> >  	HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
> >  		  SCALER_DISPBKGND_AUTOHS |
> > @@ -499,6 +539,13 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
> >  	}
> >  }
> >  
> > +void vc4_crtc_txp_armed(struct drm_crtc_state *state)
> > +{
> > +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
> > +
> > +	vc4_state->txp_armed = true;
> > +}
> > +
> >  static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -514,8 +561,11 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
> >  		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> >  
> >  		spin_lock_irqsave(&dev->event_lock, flags);
> > -		vc4_crtc->event = crtc->state->event;
> > -		crtc->state->event = NULL;
> > +
> > +		if (!vc4_state->feed_txp || vc4_state->txp_armed) {
> > +			vc4_crtc->event = crtc->state->event;
> > +			crtc->state->event = NULL;
> > +		}
> >  
> >  		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
> >  			  vc4_state->mm.start);
> > @@ -533,8 +583,8 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >  	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> > -	struct drm_crtc_state *state = crtc->state;
> > -	struct drm_display_mode *mode = &state->adjusted_mode;
> > +	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> > +	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> >  
> >  	require_hvs_enabled(dev);
> >  
> > @@ -546,15 +596,21 @@ static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
> >  
> >  	/* Turn on the scaler, which will wait for vstart to start
> >  	 * compositing.
> > +	 * When feeding the transposer, we should operate in oneshot
> > +	 * mode.
> >  	 */
> >  	HVS_WRITE(SCALER_DISPCTRLX(vc4_crtc->channel),
> >  		  VC4_SET_FIELD(mode->hdisplay, SCALER_DISPCTRLX_WIDTH) |
> >  		  VC4_SET_FIELD(mode->vdisplay, SCALER_DISPCTRLX_HEIGHT) |
> > -		  SCALER_DISPCTRLX_ENABLE);
> > +		  SCALER_DISPCTRLX_ENABLE |
> > +		  (vc4_state->feed_txp ? SCALER_DISPCTRLX_ONESHOT : 0));
> >  
> > -	/* Turn on the pixel valve, which will emit the vstart signal. */
> > -	CRTC_WRITE(PV_V_CONTROL,
> > -		   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
> > +	/* When feeding the composer block the pixelvalve is unneeded and  
> 
> "transposer block"? composer block made me think HVS.
> 
> > +	 * should not be enabled.
> > +	 */
> > +	if (!vc4_state->feed_txp)
> > +		CRTC_WRITE(PV_V_CONTROL,
> > +			   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
> >  }
> >  
> >  static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
> > @@ -579,8 +635,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
> >  	struct drm_plane *plane;
> >  	unsigned long flags;
> >  	const struct drm_plane_state *plane_state;
> > +	struct drm_connector *conn;
> > +	struct drm_connector_state *conn_state;
> >  	u32 dlist_count = 0;
> > -	int ret;
> > +	int ret, i;
> >  
> >  	/* The pixelvalve can only feed one encoder (and encoders are
> >  	 * 1:1 with connectors.)
> > @@ -600,6 +658,22 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
> >  	if (ret)
> >  		return ret;
> >  
> > +	state->no_vblank = false;
> > +	for_each_new_connector_in_state(state->state, conn, conn_state, i) {
> > +		if (conn_state->crtc != crtc)
> > +			continue;
> > +
> > +		/* The writeback connector is implemented using the transposer
> > +		 * block which is directly taking its data from the HVS FIFO.
> > +		 */
> > +		if (conn->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
> > +			state->no_vblank = true;
> > +			vc4_state->feed_txp = true;
> > +		}
> > +
> > +		break;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -713,7 +787,8 @@ static void vc4_crtc_handle_page_flip(struct vc4_crtc *vc4_crtc)
> >  
> >  	spin_lock_irqsave(&dev->event_lock, flags);
> >  	if (vc4_crtc->event &&
> > -	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)))) {
> > +	    (vc4_state->mm.start == HVS_READ(SCALER_DISPLACTX(chan)) ||
> > +	     vc4_state->feed_txp)) {  
> 
> Can vc4_crtc->event even be set if vc4_state->feed_txp?
> 
> > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> > index d6864fa4bd14..744a689751f0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_regs.h
> > +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> > @@ -330,6 +330,7 @@
> >  #define SCALER_DISPCTRL0                        0x00000040
> >  # define SCALER_DISPCTRLX_ENABLE		BIT(31)
> >  # define SCALER_DISPCTRLX_RESET			BIT(30)
> > +
> >  /* Generates a single frame when VSTART is seen and stops at the last
> >   * pixel read from the FIFO.
> >   */  
> 
> stray hunk?
> 
> > +static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
> > +					struct drm_connector_state *conn_state)
> > +{
> > +	struct vc4_txp *txp = connector_to_vc4_txp(conn);
> > +	struct drm_gem_cma_object *gem;
> > +	struct drm_display_mode *mode;
> > +	struct drm_framebuffer *fb;
> > +	u32 ctrl = TXP_GO | TXP_VSTART_AT_EOF | TXP_EI;
> > +
> > +	if (WARN_ON(!conn_state->writeback_job ||
> > +		    !conn_state->writeback_job->fb))
> > +		return;
> > +
> > +	mode = &conn_state->crtc->state->adjusted_mode;
> > +	fb = conn_state->writeback_job->fb;
> > +
> > +	switch (fb->format->format) {
> > +	case DRM_FORMAT_ARGB8888:
> > +		ctrl |= TXP_ALPHA_ENABLE;  
> 
> Optional suggestion: Have the txp_formats[] table be a struct with these
> register values in it.
> 
> All my feedback seems really minor, so with whatever components you like
> integrated, feel free to add:

Will fix all the things you pointed in your review.

Thanks,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 8/8] ARM: dts: bcm283x: Add Transposer block

[Boris Brezillon]

From: Boris Brezillon <boris.brezillon@free-electrons.com>

The transposer block is allowing one to write the result of the VC4
composition back to memory instead of displaying it on a screen.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index ac00e730f898..740870898b1e 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -66,6 +66,12 @@
 			clock-frequency = <1000000>;
 		};
 
+		txp@7e004000 {
+			compatible = "brcm,bcm2835-txp";
+			reg = <0x7e004000 0x20>;
+			interrupts = <1 11>;
+		};
+
 		dma: dma@7e007000 {
 			compatible = "brcm,bcm2835-dma";
 			reg = <0x7e007000 0xf00>;
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 8/8] ARM: dts: bcm283x: Add Transposer block

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 372 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> From: Boris Brezillon <boris.brezillon@free-electrons.com>
>
> The transposer block is allowing one to write the result of the VC4
> composition back to memory instead of displaying it on a screen.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Reviewed-by: Eric Anholt <eric@anholt.net>


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/8] drm/vc4: Add support for the transposer IP

[Boris Brezillon]

Hello,

This is the second version of this series adding writeback support
to the VC4 display engine.

This version is based on drm-misc-next and include a bunch of
modifications to core that I had to add to make it work on VC4.

Feel free to comment on those modifications.

On the driver side, no much has changed except I modified a bit
the implementation to adjust the latest revision of the writeback
interface.

Regards,

Boris

Boris Brezillon (8):
  drm/atomic: Avoid connector to writeback_connector casts
  drm/connector: Pass a drm_connector_state to ->atomic_commit()
  drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()
  drm/crtc: Add a generic infrastructure to fake VBLANK events
  drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic
    commit_tail() helpers
  drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path
  drm/vc4: Add support for the transposer block
  ARM: dts: bcm283x: Add Transposer block

 .../devicetree/bindings/display/brcm,bcm-vc4.txt   |   6 +
 arch/arm/boot/dts/bcm283x.dtsi                     |   6 +
 drivers/gpu/drm/drm_atomic.c                       |   4 +-
 drivers/gpu/drm/drm_atomic_helper.c                |  46 +-
 drivers/gpu/drm/vc4/Makefile                       |   1 +
 drivers/gpu/drm/vc4/vc4_crtc.c                     | 139 +++++-
 drivers/gpu/drm/vc4/vc4_debugfs.c                  |   1 +
 drivers/gpu/drm/vc4/vc4_drv.c                      |   1 +
 drivers/gpu/drm/vc4/vc4_drv.h                      |   7 +
 drivers/gpu/drm/vc4/vc4_kms.c                      |  11 +-
 drivers/gpu/drm/vc4/vc4_regs.h                     |   1 +
 drivers/gpu/drm/vc4/vc4_txp.c                      | 487 +++++++++++++++++++++
 include/drm/drm_atomic_helper.h                    |   1 +
 include/drm/drm_crtc.h                             |  15 +
 include/drm/drm_modeset_helper_vtables.h           |   4 +-
 include/drm/drm_writeback.h                        |   6 +
 16 files changed, 700 insertions(+), 36 deletions(-)
 create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c

-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/8] drm/vc4: Add support for the transposer IP

[Liviu Dudau]

On Fri, Jun 29, 2018 at 01:17:22PM +0200, Boris Brezillon wrote:
> Hello,
> 
> This is the second version of this series adding writeback support
> to the VC4 display engine.
> 
> This version is based on drm-misc-next and include a bunch of
> modifications to core that I had to add to make it work on VC4.
> 
> Feel free to comment on those modifications.
> 
> On the driver side, no much has changed except I modified a bit
> the implementation to adjust the latest revision of the writeback
> interface.

This is a duplicate of the cover letter.

I've reviewed / checked patches 1-5 and 8, so you can add my Reviewed-by
tag from me if you want.

Best regards,
Liviu

> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (8):
>   drm/atomic: Avoid connector to writeback_connector casts
>   drm/connector: Pass a drm_connector_state to ->atomic_commit()
>   drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()
>   drm/crtc: Add a generic infrastructure to fake VBLANK events
>   drm/atomic: Call drm_atomic_helper_fake_vblank() from the generic
>     commit_tail() helpers
>   drm/vc4: Call drm_atomic_helper_fake_vblank() in the commit path
>   drm/vc4: Add support for the transposer block
>   ARM: dts: bcm283x: Add Transposer block
> 
>  .../devicetree/bindings/display/brcm,bcm-vc4.txt   |   6 +
>  arch/arm/boot/dts/bcm283x.dtsi                     |   6 +
>  drivers/gpu/drm/drm_atomic.c                       |   4 +-
>  drivers/gpu/drm/drm_atomic_helper.c                |  46 +-
>  drivers/gpu/drm/vc4/Makefile                       |   1 +
>  drivers/gpu/drm/vc4/vc4_crtc.c                     | 139 +++++-
>  drivers/gpu/drm/vc4/vc4_debugfs.c                  |   1 +
>  drivers/gpu/drm/vc4/vc4_drv.c                      |   1 +
>  drivers/gpu/drm/vc4/vc4_drv.h                      |   7 +
>  drivers/gpu/drm/vc4/vc4_kms.c                      |  11 +-
>  drivers/gpu/drm/vc4/vc4_regs.h                     |   1 +
>  drivers/gpu/drm/vc4/vc4_txp.c                      | 487 +++++++++++++++++++++
>  include/drm/drm_atomic_helper.h                    |   1 +
>  include/drm/drm_crtc.h                             |  15 +
>  include/drm/drm_modeset_helper_vtables.h           |   4 +-
>  include/drm/drm_writeback.h                        |   6 +
>  16 files changed, 700 insertions(+), 36 deletions(-)
>  create mode 100644 drivers/gpu/drm/vc4/vc4_txp.c
> 
> -- 
> 2.14.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/8] drm/vc4: Add support for the transposer IP

[Boris Brezillon]

On Fri, 29 Jun 2018 12:40:45 +0100
Liviu Dudau <Liviu.Dudau@arm.com> wrote:

> On Fri, Jun 29, 2018 at 01:17:22PM +0200, Boris Brezillon wrote:
> > Hello,
> > 
> > This is the second version of this series adding writeback support
> > to the VC4 display engine.
> > 
> > This version is based on drm-misc-next and include a bunch of
> > modifications to core that I had to add to make it work on VC4.
> > 
> > Feel free to comment on those modifications.
> > 
> > On the driver side, no much has changed except I modified a bit
> > the implementation to adjust the latest revision of the writeback
> > interface.  
> 
> This is a duplicate of the cover letter.

Yep, I forgot to clean the directory I used to generate my patches :-/

> 
> I've reviewed / checked patches 1-5 and 8, so you can add my Reviewed-by
> tag from me if you want.

Thanks for your review.

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel