| Message ID | 20180125142630.22034-2-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 25, 2018 at 04:26:25PM +0200, Peter Ujfalusi wrote: > Instead of drivers duplicating the drm_atomic_helper_check() code to be > able to normalize the zpos they can use the normalize_zpos flag to let the > drm core to do it. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ > include/drm/drm_mode_config.h | 8 ++++++++ > include/drm/drm_plane.h | 4 ++-- > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ab4032167094..0f6a4949e6dc 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); > * functions depend upon an updated adjusted_mode.clock to e.g. properly compute > * watermarks. > * > + * Note that zpos normalization will add all enable planes to the state which > + * might not desired for some drivers. > + * For example enable/disable of a cursor plane which have fixed zpos value > + * would trigger all other enabled planes to be forced to the state change. > + * > * RETURNS: > * Zero for success or -errno > */ > @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, > if (ret) > return ret; > > + if (dev->mode_config.normalize_zpos) { > + ret = drm_atomic_normalize_zpos(dev, state); > + if (ret) > + return ret; > + } I think we originally had this in drm_atomic_helper_check_planes(). Looking through some of the drivers it looks like we could maybe kill a few more LOC by putting it there. > + > ret = drm_atomic_helper_check_planes(dev, state); > if (ret) > return ret; > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 2cb6f02df64a..987ab63ae037 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -792,6 +792,14 @@ struct drm_mode_config { > /* cursor size */ > uint32_t cursor_width, cursor_height; > > + /** > + * @normalize_zpos: > + * > + * If true the drm core will call drm_atomic_normalize_zpos() as part of > + * atomic mode checking from drm_atomic_helper_check() > + */ > + bool normalize_zpos; > + Can we pack it next to some other bools to try and keep the struct size down? > /** > * @suspend_state: > * > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 8185e3468a23..2c0adb124e0f 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -50,8 +50,8 @@ struct drm_modeset_acquire_ctx; > * plane with a lower ID. > * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1 > * where N is the number of active planes for given crtc. Note that > - * the driver must call drm_atomic_normalize_zpos() to update this before > - * it can be trusted. > + * the driver must set drm_mode_config.normalize_zpos or call > + * drm_atomic_normalize_zpos() to update this before it can be trusted. > * @src: clipped source coordinates of the plane (in 16.16) > * @dst: clipped destination coordinates of the plane > * @state: backpointer to global drm_atomic_state > -- > Peter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Thu, Jan 25, 2018 at 04:40:48PM +0200, Ville Syrjälä wrote: > On Thu, Jan 25, 2018 at 04:26:25PM +0200, Peter Ujfalusi wrote: > > Instead of drivers duplicating the drm_atomic_helper_check() code to be > > able to normalize the zpos they can use the normalize_zpos flag to let the > > drm core to do it. > > > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ > > include/drm/drm_mode_config.h | 8 ++++++++ > > include/drm/drm_plane.h | 4 ++-- > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index ab4032167094..0f6a4949e6dc 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); > > * functions depend upon an updated adjusted_mode.clock to e.g. properly compute > > * watermarks. > > * > > + * Note that zpos normalization will add all enable planes to the state which > > + * might not desired for some drivers. > > + * For example enable/disable of a cursor plane which have fixed zpos value > > + * would trigger all other enabled planes to be forced to the state change. > > + * > > * RETURNS: > > * Zero for success or -errno > > */ > > @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, > > if (ret) > > return ret; > > > > + if (dev->mode_config.normalize_zpos) { > > + ret = drm_atomic_normalize_zpos(dev, state); > > + if (ret) > > + return ret; > > + } > > I think we originally had this in drm_atomic_helper_check_planes(). > Looking through some of the drivers it looks like we could maybe > kill a few more LOC by putting it there. Actually, I guess it's fine as is. I though the "async" flip thing I saw in some of the drivers wasn't in the atomic helper. But it is there. That actually makes me slightly worried whether it's safe to just blindly replace the hand rolled stuff w/o "async" with drm_atomic_helper_check(). The commit messages should perhaps justify that somehow.
On 2018-01-25 16:40, Ville Syrjälä wrote: > On Thu, Jan 25, 2018 at 04:26:25PM +0200, Peter Ujfalusi wrote: >> Instead of drivers duplicating the drm_atomic_helper_check() code to be >> able to normalize the zpos they can use the normalize_zpos flag to let the >> drm core to do it. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ >> include/drm/drm_mode_config.h | 8 ++++++++ >> include/drm/drm_plane.h | 4 ++-- >> 3 files changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index ab4032167094..0f6a4949e6dc 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); >> * functions depend upon an updated adjusted_mode.clock to e.g. properly compute >> * watermarks. >> * >> + * Note that zpos normalization will add all enable planes to the state which >> + * might not desired for some drivers. >> + * For example enable/disable of a cursor plane which have fixed zpos value >> + * would trigger all other enabled planes to be forced to the state change. >> + * >> * RETURNS: >> * Zero for success or -errno >> */ >> @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, >> if (ret) >> return ret; >> >> + if (dev->mode_config.normalize_zpos) { >> + ret = drm_atomic_normalize_zpos(dev, state); >> + if (ret) >> + return ret; >> + } > > I think we originally had this in drm_atomic_helper_check_planes(). > Looking through some of the drivers it looks like we could maybe > kill a few more LOC by putting it there. Yes, it was. Then removed and the function was duplicated in several drivers. With this change it is moved back, but kept as optional to avoid issues with drivers do not using the drm_atomic_normalize_zpos() > >> + >> ret = drm_atomic_helper_check_planes(dev, state); >> if (ret) >> return ret; >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >> index 2cb6f02df64a..987ab63ae037 100644 >> --- a/include/drm/drm_mode_config.h >> +++ b/include/drm/drm_mode_config.h >> @@ -792,6 +792,14 @@ struct drm_mode_config { >> /* cursor size */ >> uint32_t cursor_width, cursor_height; >> >> + /** >> + * @normalize_zpos: >> + * >> + * If true the drm core will call drm_atomic_normalize_zpos() as part of >> + * atomic mode checking from drm_atomic_helper_check() >> + */ >> + bool normalize_zpos; >> + > > Can we pack it next to some other bools to try and keep the struct > size down? Sure, I'll move it. > >> /** >> * @suspend_state: >> * >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 8185e3468a23..2c0adb124e0f 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -50,8 +50,8 @@ struct drm_modeset_acquire_ctx; >> * plane with a lower ID. >> * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1 >> * where N is the number of active planes for given crtc. Note that >> - * the driver must call drm_atomic_normalize_zpos() to update this before >> - * it can be trusted. >> + * the driver must set drm_mode_config.normalize_zpos or call >> + * drm_atomic_normalize_zpos() to update this before it can be trusted. >> * @src: clipped source coordinates of the plane (in 16.16) >> * @dst: clipped destination coordinates of the plane >> * @state: backpointer to global drm_atomic_state >> -- >> Peter >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ville, On 2018-01-25 16:51, Ville Syrjälä wrote: > On Thu, Jan 25, 2018 at 04:40:48PM +0200, Ville Syrjälä wrote: >> On Thu, Jan 25, 2018 at 04:26:25PM +0200, Peter Ujfalusi wrote: >>> Instead of drivers duplicating the drm_atomic_helper_check() code to be >>> able to normalize the zpos they can use the normalize_zpos flag to let the >>> drm core to do it. >>> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ >>> include/drm/drm_mode_config.h | 8 ++++++++ >>> include/drm/drm_plane.h | 4 ++-- >>> 3 files changed, 21 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index ab4032167094..0f6a4949e6dc 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); >>> * functions depend upon an updated adjusted_mode.clock to e.g. properly compute >>> * watermarks. >>> * >>> + * Note that zpos normalization will add all enable planes to the state which >>> + * might not desired for some drivers. >>> + * For example enable/disable of a cursor plane which have fixed zpos value >>> + * would trigger all other enabled planes to be forced to the state change. >>> + * >>> * RETURNS: >>> * Zero for success or -errno >>> */ >>> @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, >>> if (ret) >>> return ret; >>> >>> + if (dev->mode_config.normalize_zpos) { >>> + ret = drm_atomic_normalize_zpos(dev, state); >>> + if (ret) >>> + return ret; >>> + } >> >> I think we originally had this in drm_atomic_helper_check_planes(). >> Looking through some of the drivers it looks like we could maybe >> kill a few more LOC by putting it there. > > Actually, I guess it's fine as is. I though the "async" flip thing I > saw in some of the drivers wasn't in the atomic helper. But it is > there. > > That actually makes me slightly worried whether it's safe to just > blindly replace the hand rolled stuff w/o "async" with > drm_atomic_helper_check(). The commit messages should perhaps > justify that somehow. I only changed 'hand rolled' stuff in the drivers where the local .atomic_check implementation is the same as the drm_atomic_helper_check() or in case of rcar-du, where I removed the drm_atomic_helper_check() part from the custom callback and let it call the function itself. I'm not sure if I understand the problem. This series does the following in essence: drm_atomic_helper_check(...) { /* does A */ } driver_hand_rolled_atomic_helper_check(...) { /* does A */ } - .atomic_check = driver_hand_rolled_atomic_helper_check, + .atomic_check = drm_atomic_helper_check, I'm most likely missing something, but not sure what. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Fri, Jan 26, 2018 at 11:29:03AM +0200, Peter Ujfalusi wrote: > Ville, > > On 2018-01-25 16:51, Ville Syrjälä wrote: > > On Thu, Jan 25, 2018 at 04:40:48PM +0200, Ville Syrjälä wrote: > >> On Thu, Jan 25, 2018 at 04:26:25PM +0200, Peter Ujfalusi wrote: > >>> Instead of drivers duplicating the drm_atomic_helper_check() code to be > >>> able to normalize the zpos they can use the normalize_zpos flag to let the > >>> drm core to do it. > >>> > >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>> --- > >>> drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ > >>> include/drm/drm_mode_config.h | 8 ++++++++ > >>> include/drm/drm_plane.h | 4 ++-- > >>> 3 files changed, 21 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>> index ab4032167094..0f6a4949e6dc 100644 > >>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>> @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); > >>> * functions depend upon an updated adjusted_mode.clock to e.g. properly compute > >>> * watermarks. > >>> * > >>> + * Note that zpos normalization will add all enable planes to the state which > >>> + * might not desired for some drivers. > >>> + * For example enable/disable of a cursor plane which have fixed zpos value > >>> + * would trigger all other enabled planes to be forced to the state change. > >>> + * > >>> * RETURNS: > >>> * Zero for success or -errno > >>> */ > >>> @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, > >>> if (ret) > >>> return ret; > >>> > >>> + if (dev->mode_config.normalize_zpos) { > >>> + ret = drm_atomic_normalize_zpos(dev, state); > >>> + if (ret) > >>> + return ret; > >>> + } > >> > >> I think we originally had this in drm_atomic_helper_check_planes(). > >> Looking through some of the drivers it looks like we could maybe > >> kill a few more LOC by putting it there. > > > > Actually, I guess it's fine as is. I though the "async" flip thing I > > saw in some of the drivers wasn't in the atomic helper. But it is > > there. > > > > That actually makes me slightly worried whether it's safe to just > > blindly replace the hand rolled stuff w/o "async" with > > drm_atomic_helper_check(). The commit messages should perhaps > > justify that somehow. > > I only changed 'hand rolled' stuff in the drivers where the local > .atomic_check implementation is the same as the > drm_atomic_helper_check() or in case of rcar-du, where I removed the > drm_atomic_helper_check() part from the custom callback and let it call > the function itself. > > I'm not sure if I understand the problem. This series does the following > in essence: > > drm_atomic_helper_check(...) > { > /* does A */ > } > > driver_hand_rolled_atomic_helper_check(...) > { > /* does A */ > } > > - .atomic_check = driver_hand_rolled_atomic_helper_check, > + .atomic_check = drm_atomic_helper_check, > > I'm most likely missing something, but not sure what. The if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(drm, state); part. The helper has it, as does tegra, but sti does not. Would be nice to have something in the comment message documenting why it's safe to add it to sti and other drivers that didn't already have it.
Ville, On 2018-01-26 18:43, Ville Syrjälä wrote: >> I'm not sure if I understand the problem. This series does the following >> in essence: >> >> drm_atomic_helper_check(...) >> { >> /* does A */ >> } >> >> driver_hand_rolled_atomic_helper_check(...) >> { >> /* does A */ >> } >> >> - .atomic_check = driver_hand_rolled_atomic_helper_check, >> + .atomic_check = drm_atomic_helper_check, >> >> I'm most likely missing something, but not sure what. > > The > > if (state->legacy_cursor_update) > state->async_update = !drm_atomic_helper_async_check(drm, state); > > part. Yes, you are right. this part is missing from the hand rolled atomic_check function in the drivers I'm touching in this series. The reason for that is that they all got their atomic_check function copied from the core before the legacy_cursor_update check and async check is added. Drivers just 'left behind' by not getting the same change. Which might or might not cause issues for them, but it is still better to use common code as much as possible to have consistent way of working among the DRM drivers. > The helper has it, as does tegra, but sti does not. Would be nice to > have something in the comment message documenting why it's safe to add > it to sti and other drivers that didn't already have it. I purposefully created separate patches for the drivers so the change can be reverted easier. I can add to the commit message for the drivers something like: Note: the drm_atomic_helper_check() now includes if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(drm, state); which was added after the driver moved away from using it (38d868e41c4b9250d5a115c049dc2d48f4909581 drm: Don't force all planes to be added to the state due to zpos) - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ab4032167094..0f6a4949e6dc 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); * functions depend upon an updated adjusted_mode.clock to e.g. properly compute * watermarks. * + * Note that zpos normalization will add all enable planes to the state which + * might not desired for some drivers. + * For example enable/disable of a cursor plane which have fixed zpos value + * would trigger all other enabled planes to be forced to the state change. + * * RETURNS: * Zero for success or -errno */ @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret; + if (dev->mode_config.normalize_zpos) { + ret = drm_atomic_normalize_zpos(dev, state); + if (ret) + return ret; + } + ret = drm_atomic_helper_check_planes(dev, state); if (ret) return ret; diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 2cb6f02df64a..987ab63ae037 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -792,6 +792,14 @@ struct drm_mode_config { /* cursor size */ uint32_t cursor_width, cursor_height; + /** + * @normalize_zpos: + * + * If true the drm core will call drm_atomic_normalize_zpos() as part of + * atomic mode checking from drm_atomic_helper_check() + */ + bool normalize_zpos; + /** * @suspend_state: * diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..2c0adb124e0f 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -50,8 +50,8 @@ struct drm_modeset_acquire_ctx; * plane with a lower ID. * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1 * where N is the number of active planes for given crtc. Note that - * the driver must call drm_atomic_normalize_zpos() to update this before - * it can be trusted. + * the driver must set drm_mode_config.normalize_zpos or call + * drm_atomic_normalize_zpos() to update this before it can be trusted. * @src: clipped source coordinates of the plane (in 16.16) * @dst: clipped destination coordinates of the plane * @state: backpointer to global drm_atomic_state
Instead of drivers duplicating the drm_atomic_helper_check() code to be able to normalize the zpos they can use the normalize_zpos flag to let the drm core to do it. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ include/drm/drm_mode_config.h | 8 ++++++++ include/drm/drm_plane.h | 4 ++-- 3 files changed, 21 insertions(+), 2 deletions(-)