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

[Philipp Zabel]

Am Freitag, den 21.10.2016, 16:49 +0800 schrieb Ying Liu:
> On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu:
> >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
> >> >> >> Does the clip thing potentially change the user's request by force?
> >> >> >> For example, the user request an unreasonable big resolution.
> >> >> >
> >> >> > The user is allowed to ask for destination coordinates extending outside
> >> >> > the crtc dimensions. This will chop off the parts that aren't visible,
> >> >> > and it will chop off the corresponding areas of the source as well.
> >> >>
> >> >> How about returning -EINVAL in this case which stands for
> >> >> an atomic check failure?
> >> >
> >> > Say the user requests to display a 640x480+0,0 source framebuffer at
> >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation
> >> > would be to see the upper right quarter of the framebuffer on the
> >> > screen, at least if the hardware was actually able to position overlays
> >> > partially offscreen.
> >> > If we can also fulfill that expectation by clipping the source rectangle
> >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
> >> > why should -EINVAL be returned?
> >>
> >> Well, IIUC, there are two kinds of clipping.
> >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
> >> 2) Clipping done by drm_plane_helper_check_state(), which potentially
> >>     changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
> >>     src_w/h and crtc_x/y/w/h, though not directly).
> >>
> >> 1) is fine, no problem.
> >> I doubt 2) is wrong as the users' original request could be changed.
> >> That's why I mentioned returning -EINVAL.
> >>
> >> Moreover, before and after applying the patch, I think the
> >> ->atomic_check behavior consistency is broken. For example,
> >> negative crtc_x or crtc_y for overlay are changed from unacceptable
> >> to potentially acceptable just because 2) may change their equivalent
> >> dst_>x/y1.
> >
> > I fail to see what's wrong with 2) as long as we can keep the observable
> > behaviour exactly the same as if the user request was unchanged.
> 
> It seems the behavior could change - negative crtc_x or crtc_y for
> overlay make ->atomic_check return -EINVAL before(overlay hw state
> machine has nothing changed), and potentially successful after(overlay
> hw state machine changes).

That in itself doesn't seem so bad. One thing we can't do though is
'position' at any negative crtc_x/y due to the fact that when clipping
the src.x1/y1 still must be even for chroma subsampled pixel formats and
the x1 still must result in scanline start addresses aligned to 8-byte
boundaries. So for 32-bit framebuffer depth negative x offsets must be
even, and for 16-bit framebuffer depth only negative x offsets that are
a multiple of 4 are possible.

regards
Philipp