Re: [PATCH 0/4] drm/atmel-hlcdc: fix plane clipping/rotation issues

[Boris Brezillon]

On Thu, 31 Jan 2019 13:13:22 +0000
Peter Rosin <peda@xxxxxxxxxx> wrote:

> On 2019-01-27 09:27, Boris Brezillon wrote:
> > On Thu, 10 Jan 2019 15:10:28 +0000
> > Peter Rosin <peda@xxxxxxxxxx> wrote:
> >   
> >> Hi!
> >>
> >> I found an unfortunate issue while recoding plane handling to use
> >> drm_atomic_helper_check_plane_state(). The driver rotates clockwise,
> >> which is not correct. I simply fixed it (patch 1/4), but maybe that
> >> will cause regressions for unsuspecting users who simply assumed
> >> that the clockwise rotation was correct? I don't know what to do
> >> about that? Adding an option to get the old broken behavior seems
> >> useless, wouldn't it be just as easy to just fix whatever app to
> >> rotate the other way instead of adding an option somewhere?
> >>
> >> I have only tested this series on sama5d3, but I did check the docs
> >> for various other chips (sama5d2, sama5d4, sam9n12, sam9g15, sam9g35
> >> and sam9x35) supported by the driver (relevant to patch 4/4).
> >>
> >> Cheers,
> >> Peter
> >>
> >> Peter Rosin (4):
> >>   drm/atmel-hlcdc: rotate planes counterclockwise
> >>   drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated
> >>   drm/atmel-hlcdc: fix clipping of planes  
> > 
> > Queued patches 1-3 to drm-misc-next.  
> 
> Great, thanks.
> 
> >>   drm/atmel-hlcdc: do not immediately disable planes, wait for next
> >>     frame  
> > 
> > Still waiting for Nicolas feedback on this one.  
> 
> [Adding back Nicolas, he seems to have gone missing from the list
> recipients.]
> 
> I have done some testing of that patch and for me it's a definite
> improvement. The test I did was removing a white plane from a white
> background. Without the patch, the driver will output black where
> the plane was for the current frame (since the driver does that
> disc-area thing for the largest hidden part of the background).
> With the patch, I get no visual glitches when removing a plane.
> 
> I use a plane to scroll a text, and if you know what to look for,
> the black rectangle that flickers by as the plane with the scrolling
> text is removed is little bit disturbing. Not a significant problem,
> and maybe only geeks notice it, but still...
> 
> Just wanted to say that the resulting "black hole" mentioned in the
> other thread really does exist and that the patch may make sense
> beyond the fact that it removes usage of undocumented features.
> 
> I have not seen any bad side effects fro the patch, but admittedly
> my testing was very limited and I did not try to remove the plane
> while doing other stuff with the driver. So, there might still be
> reasons for removing planes immediately...

Since everything is now synchronized on vsync events thanks to the
atomic modeset infra (including plane/crtc disable requests), I think
the problem I was trying to fix at the time no longer exists (might
re-appear if we start supporting async plane disable requests which is
anyway not supported by the core). So I think I'll just apply your
patch.

Thanks,

Boris