From: Karthik B S <karthik.b.s@intel.com>
To: "Daniel Vetter" <daniel@ffwll.ch>, "Michel Dänzer" <michel@daenzer.net>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
intel-gfx <intel-gfx@lists.freedesktop.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Daniel Vetter <daniel.vetter@intel.com>,
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>
Subject: Re: [Intel-gfx] [PATCH v5 1/5] drm/i915: Add enable/disable flip done and flip done handler
Date: Wed, 5 Aug 2020 19:23:28 +0530 [thread overview]
Message-ID: <e124de77-bd32-d14d-fdce-51d8fa4cee1a@intel.com> (raw)
In-Reply-To: <CAKMK7uFzvuT81J38Y_4NnZx3xQhJ4Ha4dVyDy+pwCRF8gbuRVw@mail.gmail.com>
On 7/28/2020 3:04 AM, Daniel Vetter wrote:
> On Mon, Jul 27, 2020 at 2:27 PM Michel Dänzer <michel@daenzer.net> wrote:
>>
>> On 2020-07-25 1:26 a.m., Paulo Zanoni wrote:
>>> Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index 1fa67700d8f4..95953b393941 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -697,14 +697,24 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
>>>> return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
>>>> }
>>>>
>>>> +static u32 g4x_get_flip_counter(struct drm_crtc *crtc)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>>> + enum pipe pipe = to_intel_crtc(crtc)->pipe;
>>>> +
>>>> + return I915_READ(PIPE_FLIPCOUNT_G4X(pipe));
>>>> +}
>>>> +
>>>> u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
>>>> {
>>>> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>>>> enum pipe pipe = to_intel_crtc(crtc)->pipe;
>>>>
>>>> + if (crtc->state->async_flip)
>>>> + return g4x_get_flip_counter(crtc);
>>>> +
>>>> return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
>>>
>>> I don't understand the intention behind this, can you please clarify?
>>> This goes back to my reply of the cover letter. It seems that here
>>> we're going to alternate between two different counters in our vblank
>>> count. So if user space alternates between sometimes using async flips
>>> and sometimes using normal flip it's going to get some very weird
>>> deltas, isn't it? At least this is what I remember from when I played
>>> with these registers: FLIPCOUNT drifts away from FRMCOUNT when we start
>>> using async flips.
>>
>> This definitely looks wrong. The counter value returned by the
>> get_vblank_counter hook is supposed to increment when a vertical blank
>> period occurs; page flips are not supposed to affect this in any way.
>
> Also you just flat out can't access crtc->state from interrupt
> context. Anything you need in there needs to be protected by the right
> irq-type spin_lock, updates correctly synchronized against both the
> interrupt handler and atomic updates, and data copied over, not
> pointers. Otherwise just crash&burn.
Thanks for the review.
I will be removing this change in the next revision based on the
feedback received, but I will keep this in mind whenever I'll have to
access something from the interrupt context.
Thanks,
Karthik.B.S
> -Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-08-05 13:53 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-20 11:31 [Intel-gfx] [PATCH v5 0/5] Asynchronous flip implementation for i915 Karthik B S
2020-07-20 11:31 ` [Intel-gfx] [PATCH v5 1/5] drm/i915: Add enable/disable flip done and flip done handler Karthik B S
2020-07-24 23:26 ` Paulo Zanoni
2020-07-27 12:27 ` Michel Dänzer
2020-07-27 21:34 ` Daniel Vetter
2020-08-05 13:53 ` Karthik B S [this message]
2020-08-05 13:46 ` Karthik B S
2020-08-05 13:43 ` Karthik B S
2020-07-20 11:31 ` [Intel-gfx] [PATCH v5 2/5] drm/i915: Add support for async flips in I915 Karthik B S
2020-07-24 23:26 ` Paulo Zanoni
2020-07-28 7:37 ` Karthik B S
2020-07-20 11:31 ` [Intel-gfx] [PATCH v5 3/5] drm/i915: Add checks specific to async flips Karthik B S
2020-07-20 11:31 ` [Intel-gfx] [PATCH v5 4/5] drm/i915: Do not call drm_crtc_arm_vblank_event in " Karthik B S
2020-07-20 11:31 ` [Intel-gfx] [PATCH v5 5/5] drm/i915: Enable async flips in i915 Karthik B S
2020-07-20 13:04 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for Asynchronous flip implementation for i915 (rev5) Patchwork
2020-07-20 13:25 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-07-20 15:48 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-07-23 8:44 ` Karthik B S
2020-07-23 14:04 ` Vudum, Lakshminarayana
2020-07-23 12:14 ` Patchwork
2020-07-23 13:45 ` [Intel-gfx] ✓ Fi.CI.IGT: success " Patchwork
2020-07-24 23:26 ` [Intel-gfx] [PATCH v5 0/5] Asynchronous flip implementation for i915 Paulo Zanoni
2020-07-29 7:23 ` Kulkarni, Vandita
2020-07-29 7:33 ` Michel Dänzer
2020-08-04 5:49 ` Kulkarni, Vandita
2020-08-04 6:06 ` Karthik B S
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e124de77-bd32-d14d-fdce-51d8fa4cee1a@intel.com \
--to=karthik.b.s@intel.com \
--cc=daniel.vetter@intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=michel@daenzer.net \
--cc=nicholas.kazlauskas@amd.com \
--cc=paulo.r.zanoni@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).