Re: [PATCH v4] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel <p.zabel@pengutronix.de>]

On Tue, 2017-02-07 at 13:19 +0200, Ville Syrjälä wrote:
> On Tue, Feb 07, 2017 at 09:51:50AM +0100, Philipp Zabel wrote:
> > On Fri, 2017-02-03 at 15:20 +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 03, 2017 at 10:31:39AM +0100, Philipp Zabel wrote:
> > > > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> > > > bounds. This checks for full plane coverage and scaling already, so
> > > > we can drop some custom checks. Use the clipped coordinates everywhere.
> > > > 
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > > Changes since v3:
> > > >  - Disallow target frames that start offscreen (state->crtc_x/y < 0), to avoid
> > > >    confusing userspace: due to the necessary line start address alignment, we
> > > >    could only support very specific negative values for crtc_x/y, depending on
> > > >    the pixel format.
> > > 
> > > That's really no different to the user specifying non-zero src
> > > coordinates. So I'm wondering what's the point of special casing
> > > crtc coordinates this way because you'll need to check the src
> > > coordinates anyway.
> > 
> > User expectations.
> > 
> > For the src_x/y / src.x1/y1 source coordinates we have format specific
> > alignment requirements due to limitations of the DMA unit, there is no
> > way around it.
> > For the crtc_x/y plane coordinates there are no alignment requirements
> > at all, the partial plane can be positioned at any integer x/y position.
> > If we allow to simulate a partially offscreen plane at negative
> > positions by clipping and setting src.x1/y1, suddenly the crtc_x/y have
> > alignment requirements, but only if the values are negative.
> > I assume that would rather confuse any user space application that tries
> > to reason about which crtc_x/y values are valid, so it's probably better
> > to disallow it altogether.
> 
> I don't really agree. Userspace has to be prepared for anything
> to fail really. Whether it fails every time or sometimes is not that
> much different IMO. Given the simple return value userspace can't
> really draw any real conclusions about the reason for the failure.

So you say we should allow negative crtc_x, even if the valid range with
something on the screen ist [ - plane width ... crtc width ]
with the [- plane width ... 0] range in steps of 2, 4, or 8 pixels
depending on the pixel format and the [0 ... crtc width] range in steps
of 1 pixels?

> A related question is how strict we should really be. In the past we've
> been very lax in fudging the coordinates to make something appear on the
> screen. Probably I went a bit too far in the laxness actually, and with
> atomic we want to tighten things up a little. Just no one has managed to
> really answer how tight we should make things. Is it OK to round things
> to pixel boundaries perhaps?

In my opinion: yes.

> Maybe even macropixel boundaries?

Maybe even memory burst size / line start address boundaries?

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel