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

From: Boris Brezillon <boris.brezillon@bootlin.com>
To: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 2/8] drm/connector: Pass a drm_connector_state to ->atomic_commit()
Date: Mon, 2 Jul 2018 13:58:16 +0200	[thread overview]
Message-ID: <20180702135816.1d092bfa@bbrezillon> (raw)
In-Reply-To: <20180702111420.GB15340@e110455-lin.cambridge.arm.com>

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