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

[Peter Rosin]

On 2019-01-10 18:48, Boris Brezillon wrote:
> On Thu, 10 Jan 2019 15:10:39 +0000
> Peter Rosin <peda@xxxxxxxxxx> wrote:
> 
>> The destination crtc rectangle is independent of source plane rotation.
>>
>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>> ---
>>  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;
>