From: Karthik B S <karthik.b.s@intel.com>
To: Paulo Zanoni <paulo.r.zanoni@intel.com>, intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org, daniel.vetter@intel.com,
harry.wentland@amd.com, nicholas.kazlauskas@amd.com
Subject: Re: [Intel-gfx] [PATCH v5 2/5] drm/i915: Add support for async flips in I915
Date: Tue, 28 Jul 2020 13:07:51 +0530 [thread overview]
Message-ID: <dbf8d949-4d4b-30eb-90c5-7f476ada2286@intel.com> (raw)
In-Reply-To: <9a77507657f9f25a651edb39756c83f454f06631.camel@intel.com>
On 7/25/2020 4:56 AM, Paulo Zanoni wrote:
> Em seg, 2020-07-20 às 17:01 +0530, Karthik B S escreveu:
>> Set the Async Address Update Enable bit in plane ctl
>> when async flip is requested.
>>
>> v2: -Move the Async flip enablement to individual patch (Paulo)
>>
>> v3: -Rebased.
>>
>> v4: -Add separate plane hook for async flip case (Ville)
>>
>> v5: -Rebased.
>>
>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_display.c | 6 +++++
>> drivers/gpu/drm/i915/display/intel_sprite.c | 25 ++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> 3 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index b8ff032195d9..4773f39e5924 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -4766,6 +4766,12 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> u32 plane_ctl;
>>
>> + /* During Async flip, no other updates are allowed */
>
> My understanding is that this function is fully setting the right bits
> based on the chosen config (instead of doing read-modify-write), and
> the checks for "other updates" were done before. So the logic
> implemented here of early returning doesn't make sense.
>
Thanks for the review.
Yes the check for other updates are done before.
So I could either do read-modify-write and return early, or,
keep the existing code flow as is, since the are checks already present.
I will keep the existing flow and remove the early return in the next
revision.
>
>> + if (crtc_state->uapi.async_flip) {
>> + plane_ctl |= PLANE_CTL_ASYNC_FLIP;
>
> I wonder why gcc does not complain we're ORing with an unitialized
> value.
Will initialize the plane_ctl variable to zero.
>
>
>> + return plane_ctl;
>> + }
>> +
>> plane_ctl = PLANE_CTL_ENABLE;
>
> It seems to be the return above means we'll never even try to enable
> the plane, we're only relying on the fact that plane_ctl is not zero
> initialize so maybe bit 31 is already set.
>
Since we only allow async flips on planes that are already enabled,
I assumed this would not be needed. Also, other than bit 9 (async
address update enable), this register is double buffered and cannot be
updated asynchronously.
>
>>
>> if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>> index c26ca029fc0a..3747482e8fa3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>> @@ -603,6 +603,24 @@ icl_program_input_csc(struct intel_plane *plane,
>> PLANE_INPUT_CSC_POSTOFF(pipe, plane_id, 2), 0x0);
>> }
>>
>> +static void
>> +skl_program_async_surface_address(struct drm_i915_private *dev_priv,
>> + const struct intel_plane_state *plane_state,
>> + enum pipe pipe, enum plane_id plane_id,
>> + u32 surf_addr)
>> +{
>> + unsigned long irqflags;
>> + u32 plane_ctl = plane_state->ctl;
>> +
>> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +
>> + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
>> + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
>> + intel_plane_ggtt_offset(plane_state) + surf_addr);
>> +
>> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +}
>> +
>> static void
>> skl_program_plane(struct intel_plane *plane,
>> const struct intel_crtc_state *crtc_state,
>> @@ -631,6 +649,13 @@ skl_program_plane(struct intel_plane *plane,
>> u32 keymsk, keymax;
>> u32 plane_ctl = plane_state->ctl;
>>
>> + /* During Async flip, no other updates are allowed */
>> + if (crtc_state->uapi.async_flip) {
>> + skl_program_async_surface_address(dev_priv, plane_state,
>> + pipe, plane_id, surf_addr);
>> + return;
>> + }
>
>
> I'd vote for us to keep the "don't rewrite registers that shouldn't
> change" part on its own commit, since it's just an optimization. It
> could even go at the end of the series. But perhaps this is simple
> enough and not needed.
>
>
Will move this change to the end of the series.
Thanks,
Karthik.B.S
>> +
>> plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>>
>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 8cee06314d5d..19aad4199874 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6935,6 +6935,7 @@ enum {
>> #define PLANE_CTL_TILED_X (1 << 10)
>> #define PLANE_CTL_TILED_Y (4 << 10)
>> #define PLANE_CTL_TILED_YF (5 << 10)
>> +#define PLANE_CTL_ASYNC_FLIP (1 << 9)
>> #define PLANE_CTL_FLIP_HORIZONTAL (1 << 8)
>> #define PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE (1 << 4) /* TGL+ */
>> #define PLANE_CTL_ALPHA_MASK (0x3 << 4) /* Pre-GLK */
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-07-28 7:37 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
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 [this message]
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=dbf8d949-4d4b-30eb-90c5-7f476ada2286@intel.com \
--to=karthik.b.s@intel.com \
--cc=daniel.vetter@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=harry.wentland@amd.com \
--cc=intel-gfx@lists.freedesktop.org \
--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).