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@xxxxxxx>
>
> 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:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
