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

[Ying Liu]

On Mon, Oct 24, 2016 at 7:50 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
> 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.

I think the alignment requirements from HW can be guaranteed by
proper ->atomic_check implementation.
The main concern about this patch is still the clipping 2) itself.
Besides the negative crtc_x/y example, another one is that
'modetest -P 24:1280x800' may run successfully on the i.MX6Q
SabreSD board with the 1024x768 LVDS primary display.
IMO, this may confuse the userspace.
Note that this test case cannot pass atomic check before
applying this patch.

Regards,
Liu Ying

>
> regards
> Philipp
>