Re: [PATCH v3 1/6] drm: Add drm_mode_config->normalize_zpos boolean

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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 Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel