Re: [PATCH] drm/mgag200: sleep instead of busy wait for BMC

[Jacob Keller <jacob.e.keller@intel.com>]

On 1/29/2026 10:47 AM, Jocelyn Falempe wrote:
> On 29/01/2026 18:35, Jacob Keller wrote:
>> On 1/29/2026 12:15 AM, Thomas Zimmermann wrote:
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/ 
>>>> drm/ mgag200/mgag200_bmc.c
>>>> index a689c71ff165..599b710bab9b 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
>>>> @@ -1,6 +1,7 @@
>>>>   // SPDX-License-Identifier: GPL-2.0-only
>>>>   #include <linux/delay.h>
>>>> +#include <linux/iopoll.h>
>>>>   #include <drm/drm_atomic_helper.h>
>>>>   #include <drm/drm_edid.h>
>>>> @@ -12,7 +13,7 @@
>>>>   void mgag200_bmc_stop_scanout(struct mga_device *mdev)
>>>>   {
>>>>       u8 tmp;
>>>> -    int iter_max;
>>>> +    int ret;
>>>>       /*
>>>>        * 1 - The first step is to inform the BMC of an upcoming mode
>>>> @@ -44,28 +45,20 @@ void mgag200_bmc_stop_scanout(struct mga_device 
>>>> *mdev)
>>>>        * 3a- The third step is to verify if there is an active scan.
>>>>        * We are waiting for a 0 on remhsyncsts <XSPAREREG<0>).
>>>>        */
>>>
>>> Either these comments or the original test seems incorrect.
>>>
>>> The test below is supposed to detect whether the BMC is scanning out 
>>> from the framebuffer. While it reads a horizontal scanline the bit 
>>> should be 0. That's what the test is for, but it gets the condition 
>>> wrong.
>>>
>>>> -    iter_max = 300;
>>>> -    while (!(tmp & 0x1) && iter_max) {
>>>> -        WREG8(DAC_INDEX, MGA1064_SPAREREG);
>>>> -        tmp = RREG8(DAC_DATA);
>>>> -        udelay(1000);
>>>> -        iter_max--;
>>>> -    }
>>>> +    ret = read_poll_timeout(RREG_DAC, tmp, !(tmp & 0x1),
>>>> +                1000, 300000, false,
>>>> +                MGA1064_SPAREREG);
>>>
>>> The original while loop ran as long as "!(tmp & 0x1)".  And now the 
>>> test stops if "!(tmp & 0x1)" AFAICT.  This (accidentally?) fixes the 
>>> test and makes the comment correct.
>>>
>>>
>>>> +    if (ret == -ETIMEDOUT)
>>>> +        return;
>>>>       /*
>>>>        * 3b- This step occurs only if the remove is actually
>>>
>>> Since you're at it, maybe fix this comment to say
>>>
>>> '... only if the remote BMC is ...'
>>>
>>>>        * scanning. We are waiting for the end of the frame which is
>>>>        * a 1 on remvsyncsts (XSPAREREG<1>)
>>>>        */
>>>> -    if (iter_max) {
>>>> -        iter_max = 300;
>>>> -        while ((tmp & 0x2) && iter_max) {
>>>> -            WREG8(DAC_INDEX, MGA1064_SPAREREG);
>>>> -            tmp = RREG8(DAC_DATA);
>>>> -            udelay(1000);
>>>> -            iter_max--;
>>>> -        }
>>>> -    }
>>>> +    (void)read_poll_timeout(RREG_DAC, tmp, (tmp & 0x2),
>>>> +                1000, 300000, false,
>>>> +                MGA1064_SPAREREG);
>>>
>>> Again, the comment and original code disagree and the original test 
>>> condition appears to be inverted. It whats to test of the BMC has 
>>> finished scanning out the final frame. The bit should turn 1. Instead 
>>> it tests if the bit is already 1, which is likely true. Hence that's 
>>> probably where your 300 msec delays comes from.
>>>
>>> Best regards
>>> Thomas
>>>
>> @Dave or @Jocelyn, any chance one of you could help me figure out 
>> whether Thomas is correct here? It does seem likely that the 
>> conditions were originally inverted and thus forcing a wait for 
>> 300msec every time regardless. That does match my experience... But I 
>> don't have (and web searches failed to find) any relevant datasheets...
> 
> I will give it a try tomorrow, on my test machine, and check what this 
> register value is in this case.
> Regarding documentation, I've only seen the original documentation for 
> the Matrox AGP card from 1999, but I never seen one with the BMC registers.
> 
>  From what I understand this code is only there to wait enough time. As
> mgag200_bmc_stop_scanout() is only called on hotplug, we could even 
> replace that part with a msleep(300);
> 

If Thomas is correct, (and the comment was correct but not the 
implementation), then I suspect we'll actually be able to wait 
significantly less time than 300 msec.

Thanks,
Jake