Re: [PATCH v3 2/3] ui: Deliver refresh rate via QemuUIInfo

[Akihiko Odaki <akihiko.odaki@gmail.com>]

On 2022/06/09 21:02, Gerd Hoffmann wrote:
> On Thu, Jun 09, 2022 at 08:45:41PM +0900, Akihiko Odaki wrote:
>> On 2022/06/09 19:28, Gerd Hoffmann wrote:
>>>> --- a/include/ui/console.h
>>>> +++ b/include/ui/console.h
>>>> @@ -139,6 +139,7 @@ typedef struct QemuUIInfo {
>>>>        int       yoff;
>>>>        uint32_t  width;
>>>>        uint32_t  height;
>>>> +    uint32_t  refresh_rate;
>>>>    } QemuUIInfo;
>>>>    /* cursor data format is 32bit RGBA */
>>>> @@ -426,7 +427,6 @@ typedef struct GraphicHwOps {
>>>>        void (*gfx_update)(void *opaque);
>>>>        bool gfx_update_async; /* if true, calls graphic_hw_update_done() */
>>>>        void (*text_update)(void *opaque, console_ch_t *text);
>>>> -    void (*update_interval)(void *opaque, uint64_t interval);
>>>>        void (*ui_info)(void *opaque, uint32_t head, QemuUIInfo *info);
>>>>        void (*gl_block)(void *opaque, bool block);
>>>>    } GraphicHwOps;
>>>
>>> So you are dropping update_interval, which isn't mentioned in the commit
>>> message at all.  Also this patch is rather big.  I'd suggest:
>>>
>>> (1) add refresh_rate
>>> (2) update users one by one
>>> (3) finally drop update_interval when no user is left.
>>>
>>> thanks,
>>>     Gerd
>>>
>>
>> I think 1 and 3 should have to be done once since refresh_rate and
>> update_interval would interfere with each other otherwise.
> 
> Well, between 1 and 3 both old and new API are active.  Shouldn't be
> much of a problem because the GraphicHwOps implementations are using
> only the one or the other.
> 
> take care,
>    Gerd
> 

The only GraphicHwOps implementation updated with this change is xenfb. 
xenfb can be switched to use refresh_rate in step 1 or 3.

Switching to use refresh_rate in step 1 would break the refresh rate 
propagation until all host displays are updated to set refresh_rate 
instead of calling update_interval.

Switching to use refresh_rate in step 3 would break the refresh rate 
propagation when a host display is updated to set refresh_rate instead 
of calling update_interval but xenfb does not use refresh_rate.

Regards,
Akihiko Odaki