Re: [Xen-devel] [PATCH v4 01/15] drm: Initialize struct drm_crtc_state.no_vblank from device settings

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi Emil

Am 27.01.20 um 19:12 schrieb Emil Velikov:
> Hi Thomas,
> 
> On Thu, 23 Jan 2020 at 09:21, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> 
>> @@ -174,12 +174,22 @@ struct drm_crtc_state {
>>          * @no_vblank:
>>          *
>>          * Reflects the ability of a CRTC to send VBLANK events. This state
>> -        * usually depends on the pipeline configuration, and the main usuage
>> -        * is CRTCs feeding a writeback connector operating in oneshot mode.
>> -        * In this case the VBLANK event is only generated when a job is 
>> queued
>> -        * to the writeback connector, and we want the core to fake VBLANK
>> -        * events when this part of the pipeline hasn't changed but others 
>> had
>> -        * or when the CRTC and connectors are being disabled.
>> +        * usually depends on the pipeline configuration. If set to true, DRM
>> +        * atomic helpers will sendout a fake VBLANK event during display
>> +        * updates.
>> +        *
>> +        * One usage is for drivers and/or hardware without support for 
>> VBLANK
>> +        * interrupts. Such drivers typically do not initialize vblanking
>> +        * (i.e., call drm_vblank_init() wit the number of CRTCs). For CRTCs
>> +        * without initialized vblanking, the field is initialized to true 
>> and
>> +        * a VBLANK event will be send out on each update of the display
>> +        * pipeline.
>> +        *
>> +        * Another usage is CRTCs feeding a writeback connector operating in
>> +        * oneshot mode. In this case the VBLANK event is only generated when
>> +        * a job is queued to the writeback connector, and we want the core
>> +        * to fake VBLANK events when this part of the pipeline hasn't 
>> changed
>> +        * but others had or when the CRTC and connectors are being disabled.
>>          *
> 
> Perhaps it's just me, yet the following ideas would make the topic
> significantly easier and clearer.
> 
>  - adding explicit "fake" when talking about drm/atomic _helpers_
> generating/sending a VBLANK event.
> For example, in 15/15 the commit message says "fake", while inline
> comment omits it... Leading to the confusion pointed out.

No problem on being more precise here. I'll update the docs accordingly.

> 
> - s/no_vblank/fake_vblank/g or s/no_vblank/no_hw_vblank/g
> Simple and concise. With slight inclination towards the former wording :-)

I'd prefer to not change the field's name. The current name 'no_vblank'
indicates state and lets helpers decide what to do with it. The name
'fake_vblank' indicates an instruction to the helpers, telling them what
to do. It does neither seem to fit into drm_crtc_state, nor into the
overall concept.

Best regards
Thomas

> 
> If you and Daniel agree with the rename, then the first sentence of
> the description should probably be tweaked.
> 
> HTH
> Emil
> 

-- 
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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel