RE: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[Frederic Konrad]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: 16 May 2022 10:57
> To: Frederic Konrad <fkonrad@xilinx.com>
> Cc: qemu-devel@nongnu.org; alistair@alistair23.me;
> edgar.iglesias@gmail.com; qemu-arm@nongnu.org; Sai Pavan Boddu
> <saipava@xilinx.com>; Edgar Iglesias <edgari@xilinx.com>;
> fkonrad@amd.com; Sai Pavan Boddu <saipava@xilinx.com>; Edgar Iglesias
> <edgari@xilinx.com>
> Subject: Re: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal
> 
> On Tue, 3 May 2022 at 16:27, <frederic.konrad@xilinx.com> wrote:
> >
> > From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> >
> > Add a periodic timer which raises vblank at a frequency of 30Hz.
> >
> > Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Changes by fkonrad:
> >   - Switched to transaction-based ptimer API.
> >   - Added the DP_INT_VBLNK_START macro.
> > Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> > ---
> 
> 
> > @@ -1309,6 +1323,10 @@ static void xlnx_dp_realize(DeviceState *dev,
> Error **errp)
> >                                             &as);
> >      AUD_set_volume_out(s->amixer_output_stream, 0, 255, 255);
> >      xlnx_dp_audio_activate(s);
> > +    s->vblank = ptimer_init(vblank_hit, s, PTIMER_POLICY_DEFAULT);
> > +    ptimer_transaction_begin(s->vblank);
> > +    ptimer_set_freq(s->vblank, 30);
> > +    ptimer_transaction_commit(s->vblank);
> 
> The ptimer documentation (in include/hw/ptimer.h) says
>  * The default ptimer policy retains backward compatibility with the legacy
>  * timers. Custom policies are adjusting the default one. Consider providing
>  * a correct policy for your timer.
> 
> and goes on to describe various weird behaviours of the default
> policy. You almost certainly don't want to use PTIMER_POLICY_DEFAULT
> for a new timer -- instead figure out the behaviour you actually
> want and specify the appropriate flags.

Hi Peter,

Thanks for your feedback.

Yes, I think, I can just use CONTINUOUS_TRIGGER and NO_IMMEDIATE_TRIGGER
instead of forcing the decrementer / reload value to 1.  Would that be cleaner?

Thanks,
Fred

> 
> thanks
> -- PMM