Re: [PATCH 15/17] drm/mgag200: Remove waiting from DPMS code

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3607 bytes --]

Hi

Am 04.05.20 um 14:10 schrieb Daniel Vetter:
> On Wed, Apr 29, 2020 at 04:32:36PM +0200, Thomas Zimmermann wrote:
>> The mgag200 drivers waits for the VSYNC flag to get signalled (i.e.,
>> the page flip happens) before changing DPMS settings. This doesn't work
>> reliably if no mode has been programmed. Therefore remove the waiting
>> code. Synchronization with page flips should be done by DRM's vblank
>> handlers anyway.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> This looks a bit dangerous, hw might get angry if we drop these waits.
> 
> Generally with atomic you should never have a situation in driver code
> where you expect the display to be on, but it isn't. So this should be
> fixable by making sure we're calling this dpms function at the right spot,
> e.g. for the enable path obviously the display is always going to be off,
> and for the disable path the display is guaranteed to be on. So maybe just
> a bool enable, or split the dpms function into two.

I think this code was taken from the X11 userspace driver, which does
that same waiting.

But it's waiting even for the DPMS_ON case. If the signal generation is
disabled, why does it wait for the vsync flag? After a while the code
times out from a timeout given in HZ/jiffies. I would have expected a
value in usec. All this makes it somewhat dubious and I doubt that it's
actually correct.

If we want to keep the waiting, I agree on splitting the code into an
enable and a disable function.

The driver also busy waited during pageflips (see patch 5). That should
really be done with interrupts.

Best regards
Thomas


> 
> The old legacy helpers where a lot more fast&loose in this regard.
> -Daniel
> 
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 26 --------------------------
>>  1 file changed, 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index ee1cbe5decd71..884fc668a6dae 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -75,30 +75,6 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc)
>>  	}
>>  }
>>  
>> -static inline void mga_wait_vsync(struct mga_device *mdev)
>> -{
>> -	unsigned long timeout = jiffies + HZ/10;
>> -	unsigned int status = 0;
>> -
>> -	do {
>> -		status = RREG32(MGAREG_Status);
>> -	} while ((status & 0x08) && time_before(jiffies, timeout));
>> -	timeout = jiffies + HZ/10;
>> -	status = 0;
>> -	do {
>> -		status = RREG32(MGAREG_Status);
>> -	} while (!(status & 0x08) && time_before(jiffies, timeout));
>> -}
>> -
>> -static inline void mga_wait_busy(struct mga_device *mdev)
>> -{
>> -	unsigned long timeout = jiffies + HZ;
>> -	unsigned int status = 0;
>> -	do {
>> -		status = RREG8(MGAREG_Status + 2);
>> -	} while ((status & 0x01) && time_before(jiffies, timeout));
>> -}
>> -
>>  #define P_ARRAY_SIZE 9
>>  
>>  static int mga_g200se_set_plls(struct mga_device *mdev, long clock)
>> @@ -1435,8 +1411,6 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
>>  #endif
>>  	WREG8(MGAREG_SEQ_INDEX, 0x01);
>>  	seq1 |= RREG8(MGAREG_SEQ_DATA) & ~0x20;
>> -	mga_wait_vsync(mdev);
>> -	mga_wait_busy(mdev);
>>  	WREG8(MGAREG_SEQ_DATA, seq1);
>>  	msleep(20);
>>  	WREG8(MGAREG_CRTCEXT_INDEX, 0x01);
>> -- 
>> 2.26.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel