Re: [Intel-gfx] [PATCH 03/16] drm/atomic: Move __drm_atomic_helper_disable_plane/set_config()
Den 27.03.2019 14.55, skrev Daniel Vetter:
> On Wed, Mar 27, 2019 at 2:42 PM Noralf Trønnes wrote:
>>
>>
>>
>> Den 26.03.2019 21.48, skrev Daniel Vetter:
>>> On Tue, Mar 26, 2019 at 06:55:33PM +0100, Noralf Trønnes wrote:
Prepare for moving drm_fb_helper modesetting code to drm_client.
drm_client will be linked to drm.ko, so move
__drm_atomic_helper_disable_plane() and __drm_atomic_helper_set_config()
out of drm_kms_helper.ko.
While at it, fix two checkpatch complaints:
- WARNING: Block comments use a trailing */ on a separate line
- CHECK: Alignment should match open parenthesis
Signed-off-by: Noralf Trønnes
>>>
>>> Triggers a bit my midlayer ocd, but I can't come up with a better idea
>>> either.
>>>
>>> Reviewed-by: Daniel Vetter
>>>
>>
>> The Intel CI informed me that this patch didn't apply..
>>
>> Turns out there's a patch in drm-next by Sean:
>>
>> drm: Merge __drm_atomic_helper_disable_all() into
>> drm_atomic_helper_disable_all()
>>
>> https://cgit.freedesktop.org/drm/drm/commit?id=37406a60fac7de336dec331a8707a94462ac5a5e
>>
>> Should I move the whole function with the kerneldoc to drm_atomic.c, or
>> should I keep the doc and function in drm_atomic_helper.c and just
>> recreate __drm_atomic_helper_disable_all() and call that ?
>
> So my longterm goal with all these compat helpers is to move them into
> the core anyway, and make them at least the default, if not only the
> only accepted choice for atomic drivers. We've already done that for
> the dpms hook. We can't make it the enforced default yet for planes
> because of cursors, but I think we could just move both of these into
> the core (disable_plane and update_plane), make it the default option,
> and remove all driver users that just set it as the default hook.
>
> Would need to be renamed to something like drm_atomic_default_* ofc.
>
> Not sure why you're talking about disable_all here though, since that
> doesn't seem to be touched in this patch at all?
Sorry my bad, I was just confused. Looking at it again, I see that I can
just rebase on top of that patch.
Noralf.
> -Daniel
>
>>
>> Noralf.
>>
---
drivers/gpu/drm/drm_atomic.c| 168
drivers/gpu/drm/drm_atomic_helper.c | 164 ---
drivers/gpu/drm/drm_crtc_internal.h | 5 +
include/drm/drm_atomic_helper.h | 4 -
4 files changed, 173 insertions(+), 168 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5eb40130fafb..c3a9ffbf2310 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1130,6 +1130,174 @@ int drm_atomic_nonblocking_commit(struct
drm_atomic_state *state)
}
EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
+/* just used from drm-client and atomic-helper: */
+int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
+ struct drm_plane_state *plane_state)
+{
+int ret;
+
+ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+if (ret != 0)
+return ret;
+
+drm_atomic_set_fb_for_plane(plane_state, NULL);
+plane_state->crtc_x = 0;
+plane_state->crtc_y = 0;
+plane_state->crtc_w = 0;
+plane_state->crtc_h = 0;
+plane_state->src_x = 0;
+plane_state->src_y = 0;
+plane_state->src_w = 0;
+plane_state->src_h = 0;
+
+return 0;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_disable_plane);
+
+static int update_output_state(struct drm_atomic_state *state,
+ struct drm_mode_set *set)
+{
+struct drm_device *dev = set->crtc->dev;
+struct drm_crtc *crtc;
+struct drm_crtc_state *new_crtc_state;
+struct drm_connector *connector;
+struct drm_connector_state *new_conn_state;
+int ret, i;
+
+ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
+ state->acquire_ctx);
+if (ret)
+return ret;
+
+/* First disable all connectors on the target crtc. */
+ret = drm_atomic_add_affected_connectors(state, set->crtc);
+if (ret)
+return ret;
+
+for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+if (new_conn_state->crtc == set->crtc) {
+ret =
drm_atomic_set_crtc_for_connector(new_conn_state,
+NULL);
+if (ret)
+return ret;
+
+/* Make sure legacy setCrtc always re-trains */
+new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
+}
+}
+
>>
Re: [Intel-gfx] [PATCH 03/16] drm/atomic: Move __drm_atomic_helper_disable_plane/set_config()
On Wed, Mar 27, 2019 at 2:42 PM Noralf Trønnes wrote:
>
>
>
> Den 26.03.2019 21.48, skrev Daniel Vetter:
> > On Tue, Mar 26, 2019 at 06:55:33PM +0100, Noralf Trønnes wrote:
> >> Prepare for moving drm_fb_helper modesetting code to drm_client.
> >> drm_client will be linked to drm.ko, so move
> >> __drm_atomic_helper_disable_plane() and __drm_atomic_helper_set_config()
> >> out of drm_kms_helper.ko.
> >>
> >> While at it, fix two checkpatch complaints:
> >> - WARNING: Block comments use a trailing */ on a separate line
> >> - CHECK: Alignment should match open parenthesis
> >>
> >> Signed-off-by: Noralf Trønnes
> >
> > Triggers a bit my midlayer ocd, but I can't come up with a better idea
> > either.
> >
> > Reviewed-by: Daniel Vetter
> >
>
> The Intel CI informed me that this patch didn't apply..
>
> Turns out there's a patch in drm-next by Sean:
>
> drm: Merge __drm_atomic_helper_disable_all() into
> drm_atomic_helper_disable_all()
>
> https://cgit.freedesktop.org/drm/drm/commit?id=37406a60fac7de336dec331a8707a94462ac5a5e
>
> Should I move the whole function with the kerneldoc to drm_atomic.c, or
> should I keep the doc and function in drm_atomic_helper.c and just
> recreate __drm_atomic_helper_disable_all() and call that ?
So my longterm goal with all these compat helpers is to move them into
the core anyway, and make them at least the default, if not only the
only accepted choice for atomic drivers. We've already done that for
the dpms hook. We can't make it the enforced default yet for planes
because of cursors, but I think we could just move both of these into
the core (disable_plane and update_plane), make it the default option,
and remove all driver users that just set it as the default hook.
Would need to be renamed to something like drm_atomic_default_* ofc.
Not sure why you're talking about disable_all here though, since that
doesn't seem to be touched in this patch at all?
-Daniel
>
> Noralf.
>
> >> ---
> >> drivers/gpu/drm/drm_atomic.c| 168
> >> drivers/gpu/drm/drm_atomic_helper.c | 164 ---
> >> drivers/gpu/drm/drm_crtc_internal.h | 5 +
> >> include/drm/drm_atomic_helper.h | 4 -
> >> 4 files changed, 173 insertions(+), 168 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 5eb40130fafb..c3a9ffbf2310 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -1130,6 +1130,174 @@ int drm_atomic_nonblocking_commit(struct
> >> drm_atomic_state *state)
> >> }
> >> EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >>
> >> +/* just used from drm-client and atomic-helper: */
> >> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> +int ret;
> >> +
> >> +ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> >> +if (ret != 0)
> >> +return ret;
> >> +
> >> +drm_atomic_set_fb_for_plane(plane_state, NULL);
> >> +plane_state->crtc_x = 0;
> >> +plane_state->crtc_y = 0;
> >> +plane_state->crtc_w = 0;
> >> +plane_state->crtc_h = 0;
> >> +plane_state->src_x = 0;
> >> +plane_state->src_y = 0;
> >> +plane_state->src_w = 0;
> >> +plane_state->src_h = 0;
> >> +
> >> +return 0;
> >> +}
> >> +EXPORT_SYMBOL(__drm_atomic_helper_disable_plane);
> >> +
> >> +static int update_output_state(struct drm_atomic_state *state,
> >> + struct drm_mode_set *set)
> >> +{
> >> +struct drm_device *dev = set->crtc->dev;
> >> +struct drm_crtc *crtc;
> >> +struct drm_crtc_state *new_crtc_state;
> >> +struct drm_connector *connector;
> >> +struct drm_connector_state *new_conn_state;
> >> +int ret, i;
> >> +
> >> +ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> >> + state->acquire_ctx);
> >> +if (ret)
> >> +return ret;
> >> +
> >> +/* First disable all connectors on the target crtc. */
> >> +ret = drm_atomic_add_affected_connectors(state, set->crtc);
> >> +if (ret)
> >> +return ret;
> >> +
> >> +for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >> +if (new_conn_state->crtc == set->crtc) {
> >> +ret =
> >> drm_atomic_set_crtc_for_connector(new_conn_state,
> >> +NULL);
> >> +if (ret)
> >> +return ret;
> >> +
> >> +/* Make sure legacy setCrtc always re-trains */
> >> +new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
> >> +}
> >> +}
> >> +
> >> +/* Then set all connectors from set->connectors on the target crtc */
> >> +for (i = 0; i < set->num_connectors; i++) {
> >> +new_conn_state = drm_atomic_get_connector_state(state,
> >> +
Re: [Intel-gfx] [PATCH 03/16] drm/atomic: Move __drm_atomic_helper_disable_plane/set_config()
Den 26.03.2019 21.48, skrev Daniel Vetter:
> On Tue, Mar 26, 2019 at 06:55:33PM +0100, Noralf Trønnes wrote:
>> Prepare for moving drm_fb_helper modesetting code to drm_client.
>> drm_client will be linked to drm.ko, so move
>> __drm_atomic_helper_disable_plane() and __drm_atomic_helper_set_config()
>> out of drm_kms_helper.ko.
>>
>> While at it, fix two checkpatch complaints:
>> - WARNING: Block comments use a trailing */ on a separate line
>> - CHECK: Alignment should match open parenthesis
>>
>> Signed-off-by: Noralf Trønnes
>
> Triggers a bit my midlayer ocd, but I can't come up with a better idea
> either.
>
> Reviewed-by: Daniel Vetter
>
The Intel CI informed me that this patch didn't apply.
Turns out there's a patch in drm-next by Sean:
drm: Merge __drm_atomic_helper_disable_all() into
drm_atomic_helper_disable_all()
https://cgit.freedesktop.org/drm/drm/commit?id=37406a60fac7de336dec331a8707a94462ac5a5e
Should I move the whole function with the kerneldoc to drm_atomic.c, or
should I keep the doc and function in drm_atomic_helper.c and just
recreate __drm_atomic_helper_disable_all() and call that ?
Noralf.
>> ---
>> drivers/gpu/drm/drm_atomic.c| 168
>> drivers/gpu/drm/drm_atomic_helper.c | 164 ---
>> drivers/gpu/drm/drm_crtc_internal.h | 5 +
>> include/drm/drm_atomic_helper.h | 4 -
>> 4 files changed, 173 insertions(+), 168 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 5eb40130fafb..c3a9ffbf2310 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1130,6 +1130,174 @@ int drm_atomic_nonblocking_commit(struct
>> drm_atomic_state *state)
>> }
>> EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>>
>> +/* just used from drm-client and atomic-helper: */
>> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
>> + struct drm_plane_state *plane_state)
>> +{
>> +int ret;
>> +
>> +ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
>> +if (ret != 0)
>> +return ret;
>> +
>> +drm_atomic_set_fb_for_plane(plane_state, NULL);
>> +plane_state->crtc_x = 0;
>> +plane_state->crtc_y = 0;
>> +plane_state->crtc_w = 0;
>> +plane_state->crtc_h = 0;
>> +plane_state->src_x = 0;
>> +plane_state->src_y = 0;
>> +plane_state->src_w = 0;
>> +plane_state->src_h = 0;
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(__drm_atomic_helper_disable_plane);
>> +
>> +static int update_output_state(struct drm_atomic_state *state,
>> + struct drm_mode_set *set)
>> +{
>> +struct drm_device *dev = set->crtc->dev;
>> +struct drm_crtc *crtc;
>> +struct drm_crtc_state *new_crtc_state;
>> +struct drm_connector *connector;
>> +struct drm_connector_state *new_conn_state;
>> +int ret, i;
>> +
>> +ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
>> + state->acquire_ctx);
>> +if (ret)
>> +return ret;
>> +
>> +/* First disable all connectors on the target crtc. */
>> +ret = drm_atomic_add_affected_connectors(state, set->crtc);
>> +if (ret)
>> +return ret;
>> +
>> +for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>> +if (new_conn_state->crtc == set->crtc) {
>> +ret = drm_atomic_set_crtc_for_connector(new_conn_state,
>> +NULL);
>> +if (ret)
>> +return ret;
>> +
>> +/* Make sure legacy setCrtc always re-trains */
>> +new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
>> +}
>> +}
>> +
>> +/* Then set all connectors from set->connectors on the target crtc */
>> +for (i = 0; i < set->num_connectors; i++) {
>> +new_conn_state = drm_atomic_get_connector_state(state,
>> +
>> set->connectors[i]);
>> +if (IS_ERR(new_conn_state))
>> +return PTR_ERR(new_conn_state);
>> +
>> +ret = drm_atomic_set_crtc_for_connector(new_conn_state,
>> +set->crtc);
>> +if (ret)
>> +return ret;
>> +}
>> +
>> +for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> +/*
>> + * Don't update ->enable for the CRTC in the set_config request,
>> + * since a mismatch would indicate a bug in the upper layers.
>> + * The actual modeset code later on will catch any
>> + * inconsistencies here.
>> + */
>> +if (crtc == set->crtc)
>> +continue;
>> +
>> +if (!new_crtc_state->connector_mask) {
>> +ret = drm_atomic_set_mod
Re: [Intel-gfx] [PATCH 03/16] drm/atomic: Move __drm_atomic_helper_disable_plane/set_config()
On Tue, Mar 26, 2019 at 06:55:33PM +0100, Noralf Trønnes wrote:
> Prepare for moving drm_fb_helper modesetting code to drm_client.
> drm_client will be linked to drm.ko, so move
> __drm_atomic_helper_disable_plane() and __drm_atomic_helper_set_config()
> out of drm_kms_helper.ko.
>
> While at it, fix two checkpatch complaints:
> - WARNING: Block comments use a trailing */ on a separate line
> - CHECK: Alignment should match open parenthesis
>
> Signed-off-by: Noralf Trønnes
Triggers a bit my midlayer ocd, but I can't come up with a better idea
either.
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic.c| 168
> drivers/gpu/drm/drm_atomic_helper.c | 164 ---
> drivers/gpu/drm/drm_crtc_internal.h | 5 +
> include/drm/drm_atomic_helper.h | 4 -
> 4 files changed, 173 insertions(+), 168 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5eb40130fafb..c3a9ffbf2310 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1130,6 +1130,174 @@ int drm_atomic_nonblocking_commit(struct
> drm_atomic_state *state)
> }
> EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>
> +/* just used from drm-client and atomic-helper: */
> +int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
> + struct drm_plane_state *plane_state)
> +{
> + int ret;
> +
> + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + if (ret != 0)
> + return ret;
> +
> + drm_atomic_set_fb_for_plane(plane_state, NULL);
> + plane_state->crtc_x = 0;
> + plane_state->crtc_y = 0;
> + plane_state->crtc_w = 0;
> + plane_state->crtc_h = 0;
> + plane_state->src_x = 0;
> + plane_state->src_y = 0;
> + plane_state->src_w = 0;
> + plane_state->src_h = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(__drm_atomic_helper_disable_plane);
> +
> +static int update_output_state(struct drm_atomic_state *state,
> +struct drm_mode_set *set)
> +{
> + struct drm_device *dev = set->crtc->dev;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> + int ret, i;
> +
> + ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> +state->acquire_ctx);
> + if (ret)
> + return ret;
> +
> + /* First disable all connectors on the target crtc. */
> + ret = drm_atomic_add_affected_connectors(state, set->crtc);
> + if (ret)
> + return ret;
> +
> + for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> + if (new_conn_state->crtc == set->crtc) {
> + ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> + NULL);
> + if (ret)
> + return ret;
> +
> + /* Make sure legacy setCrtc always re-trains */
> + new_conn_state->link_status = DRM_LINK_STATUS_GOOD;
> + }
> + }
> +
> + /* Then set all connectors from set->connectors on the target crtc */
> + for (i = 0; i < set->num_connectors; i++) {
> + new_conn_state = drm_atomic_get_connector_state(state,
> +
> set->connectors[i]);
> + if (IS_ERR(new_conn_state))
> + return PTR_ERR(new_conn_state);
> +
> + ret = drm_atomic_set_crtc_for_connector(new_conn_state,
> + set->crtc);
> + if (ret)
> + return ret;
> + }
> +
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> + /*
> + * Don't update ->enable for the CRTC in the set_config request,
> + * since a mismatch would indicate a bug in the upper layers.
> + * The actual modeset code later on will catch any
> + * inconsistencies here.
> + */
> + if (crtc == set->crtc)
> + continue;
> +
> + if (!new_crtc_state->connector_mask) {
> + ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state,
> + NULL);
> + if (ret < 0)
> + return ret;
> +
> + new_crtc_state->active = false;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* just used from drm-client and atomic-helper: */
> +int __drm_atomic_helper_set_config(struct drm_mode_set *set,
> +struct drm_atomic_state *state)
> +{
> + struct drm_crtc_state *crtc_state;
> + struct drm_plane_state *primary_state;
> +
