| Message ID | 20190110151020.30468-3-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atmel-hlcdc: fix plane clipping/rotation issues | expand |
On Thu, 10 Jan 2019 15:10:39 +0000 Peter Rosin <peda@axentia.se> wrote: > The destination crtc rectangle is independent of source plane rotation. > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > index ea8fc0deb814..d6f93f029020 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c > @@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, > * Swap width and size in case of 90 or 270 degrees rotation > */ > if (drm_rotation_90_or_270(state->base.rotation)) { > - tmp = state->crtc_w; > - state->crtc_w = state->crtc_h; > - state->crtc_h = tmp; Again, I guess I assumed ->crtc_h/w were the width and height before rotation when I initially added rotation support. This change might break users too. > tmp = state->src_w; > state->src_w = state->src_h; > state->src_h = tmp;
On 2019-01-10 18:48, Boris Brezillon wrote: > On Thu, 10 Jan 2019 15:10:39 +0000 > Peter Rosin <peda@axentia.se> wrote: > >> The destination crtc rectangle is independent of source plane rotation. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> index ea8fc0deb814..d6f93f029020 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >> @@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, >> * Swap width and size in case of 90 or 270 degrees rotation >> */ >> if (drm_rotation_90_or_270(state->base.rotation)) { >> - tmp = state->crtc_w; >> - state->crtc_w = state->crtc_h; >> - state->crtc_h = tmp; > > Again, I guess I assumed ->crtc_h/w were the width and height before > rotation when I initially added rotation support. And I thought so too, possibly since I have only been doing drm-stuff with this driver, but I also suspect that the incompleteness of the libdrm modetest program is to blame. I don't think it's possible to specify individual src and dst rectangles with it, and that seems rather limiting when dealing with rotated planes. I can easily see why someone using modetest thinks the crtc rect should be rotated by the driver... > This change might break users too. Right you are, and the same impossible scenario. Fix things to do the right thing and risk breaking users, or don't and preserve the buggy non-portable issues of the driver making it unusable for others. I don't care either way, because rotating planes with this stride- method is practically useless here. It simply requires to much memory bandwidth. I might work ok for smaller panels with lower pixel clock frequencies though? I think the LCDC might read the same data more than once when data is not in the "natural" order? (no, I do not need an answer to this question, and I do not have time to dig in this area at the moment...) However, if you can't do both patch 1 and 2 (because users regress), then patch 3 is no good either. The reason is that drm_atomic_helper_check_plane_state assumes the rotational properties fixed by patch 1 and 2, and the behavior is "odd" if you have that wrong. Cheers, Peter >> tmp = state->src_w; >> state->src_w = state->src_h; >> state->src_h = tmp; >
On 11/01/2019 at 14:29, Peter Rosin wrote: > On 2019-01-10 18:48, Boris Brezillon wrote: >> On Thu, 10 Jan 2019 15:10:39 +0000 >> Peter Rosin <peda@axentia.se> wrote: >> >>> The destination crtc rectangle is independent of source plane rotation. >>> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>> index ea8fc0deb814..d6f93f029020 100644 >>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c >>> @@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, >>> * Swap width and size in case of 90 or 270 degrees rotation >>> */ >>> if (drm_rotation_90_or_270(state->base.rotation)) { >>> - tmp = state->crtc_w; >>> - state->crtc_w = state->crtc_h; >>> - state->crtc_h = tmp; >> >> Again, I guess I assumed ->crtc_h/w were the width and height before >> rotation when I initially added rotation support. > > And I thought so too, possibly since I have only been doing drm-stuff > with this driver, but I also suspect that the incompleteness of the > libdrm modetest program is to blame. I don't think it's possible to > specify individual src and dst rectangles with it, and that seems > rather limiting when dealing with rotated planes. I can easily see > why someone using modetest thinks the crtc rect should be rotated by > the driver... > >> This change might break users too. > > Right you are, and the same impossible scenario. Fix things to do the > right thing and risk breaking users, or don't and preserve the buggy > non-portable issues of the driver making it unusable for others. I understand that we are the only ones to be different here. My colleague Josh also helped me grasp the implications of this issue. I would say that we mustn't be different. So please consider fixing this. Some users might have started something with rotations but we'll make sure to help them with the issue encountered and our additional DRM libraries (like libplanes) can be fixed easily to make this change transparent. > I don't care either way, because rotating planes with this stride- > method is practically useless here. It simply requires to much > memory bandwidth. I might work ok for smaller panels with lower > pixel clock frequencies though? Rotation works for our use cases. > I think the LCDC might read the same > data more than once when data is not in the "natural" order? (no, > I do not need an answer to this question, and I do not have time to > dig in this area at the moment...) > > However, if you can't do both patch 1 and 2 (because users regress), > then patch 3 is no good either. The reason is that > drm_atomic_helper_check_plane_state assumes the rotational > properties fixed by patch 1 and 2, and the behavior is "odd" if you > have that wrong. Thanks for continuing the discussion Boris and thanks to Peter for this work. You have my opinion: please go-on with the fix. Best regards, Nicolas >>> tmp = state->src_w; >>> state->src_w = state->src_h; >>> state->src_h = tmp; >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index ea8fc0deb814..d6f93f029020 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -642,9 +642,6 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p, * Swap width and size in case of 90 or 270 degrees rotation */ if (drm_rotation_90_or_270(state->base.rotation)) { - tmp = state->crtc_w; - state->crtc_w = state->crtc_h; - state->crtc_h = tmp; tmp = state->src_w; state->src_w = state->src_h; state->src_h = tmp;
The destination crtc rectangle is independent of source plane rotation. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 --- 1 file changed, 3 deletions(-)