diff mbox series

[2/4] drm/atmel-hlcdc: do not swap w/h of the crtc when a plane is rotated

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

Commit Message

Peter Rosin Jan. 10, 2019, 3:10 p.m. UTC
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(-)

Comments

Peter Rosin Jan. 11, 2019, 1:29 p.m. UTC | #1
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;
>
Nicolas Ferre Jan. 11, 2019, 2:14 p.m. UTC | #2
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 mbox series

Patch

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;