| Message ID | 20180629111722.20299-3-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >
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! / > --------------- > ¯\_(ツ)_/¯
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.
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.
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
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); }; /**
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(-)