On Tue, Apr 29, 2025 at 01:16:13PM +0000, Gote, Nitin R wrote:
> > Hi Nitin,
> >
> > [...]
> >
> > > > > + if (wait_for_atomic((!stop_ring(engine) == 0), 20)) {
> > > > > drm_err(&engine->i915->drm,
> > > > > "failed to set %s head to zero "
> > > > > "ctl %08x head %08x tail %08x start
> > > > > %08x\n",
> > > >
> > > > I am wondering if xcs_resume() calling stop_ring() too would benefit
> > > > from having this timeout on hand as well. That would require moving
> > > > wait_for_atomic((!stop_ring(engine) == 0), 20) along with your
> > > > comment to a separate wrapper function.
> > > > What do you think?
> > >
> > > In xcs_resume(), there is no need for a timeout for stop_ring(), as we
> > > have not
> > encountered any issues/errors in xcs_resume().
> > > So, I think, currently there is no need for a separate wrapper function.
> > In that case, I do not have any more concerns:
> > Reviewed-by: Krzysztof Karas <[email protected]>
> >
> Thanks for the review Krzysztof.
>
> > Best Regards,
> > Krzysztof
>
> Hi Rodrigo/Jani,
> May I ask you to push this change?
I just pushed the patch, but I'm wondering if a simple
ENGINE_POSTING_READ(engine, RING_HEAD);
ENGINE_POSTING_READ(engine, RING_TAIL);
before the return inside the stop_ring()
wouldn't be enough to accomplish the same here...
>
> Thanks,
> Nitin
>
>
>
