[Intel-gfx] [PATCH v8 5/8] drm/i915: Add dedicated plane hook for async flip case

[Karthik B S]

On 9/16/2020 6:30 PM, Karthik B S wrote:
> 
> 
> On 9/15/2020 8:11 PM, Ville Syrjälä wrote:
>> On Mon, Sep 14, 2020 at 11:26:30AM +0530, Karthik B S wrote:
>>> This hook is added to avoid writing other plane registers in case of
>>> async flips, so that we do not write the double buffered registers
>>> during async surface address update.
>>>
>>> v7: -Plane ctl needs bits from skl_plane_ctl_crtc as well. (Ville)
>>>      -Add a vfunc for skl_program_async_surface_address
>>>       and call it from intel_update_plane. (Ville)
>>>
>>> v8: -Rebased.
>>>
>>> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
>>> Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
>>> ---
>>>   .../gpu/drm/i915/display/intel_atomic_plane.c |  7 ++++++
>>>   .../drm/i915/display/intel_display_types.h    |  3 +++
>>>   drivers/gpu/drm/i915/display/intel_sprite.c   | 24 +++++++++++++++++++
>>>   3 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
>>> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> index 79032701873a..fdc633020255 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> @@ -408,6 +408,13 @@ void intel_update_plane(struct intel_plane *plane,
>>>       struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>>       trace_intel_update_plane(&plane->base, crtc);
>>> +
>>> +    if (crtc_state->uapi.async_flip) {
>>
>> Hmm. Now I'm starting to wonder how this is actually going to interact
>> with legacy cursor updates. The crtc_state we use there I think comes
>> from the previous update and so will have this flag set it if was an
>> async flip. Which means the cursor ioctl will oops.
>>
>> We may want the igt to check this particular combination of ioctls
>> actually.
>>
> 
> I tried this out locally by using the DRM_IOCTL_MODE_CURSOR ioctl after 
> an async flip. And looks like its working fine. During the cursor commit 
> it actually takes the 'else' path.
> 
> I'll send out the new version of the IGT shortly with this subtest 
> added. Please let me know if I'm missing something there.
> 

I've pushed the IGT with the cursor subtest added.
https://patchwork.freedesktop.org/series/79701/

Thanks,
Karthik.B.S
> Thanks,
> Karthik.B.S
>>> +        plane->program_async_surface_address(plane,
>>> +                             crtc_state, plane_state);
>>> +        return;
>>> +    }
>>> +
>>>       plane->update_plane(plane, crtc_state, plane_state);
>>>   }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h 
>>> b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> index b2d0edacc58c..d2ae781e4d81 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>>> @@ -1190,6 +1190,9 @@ struct intel_plane {
>>>                  struct intel_plane_state *plane_state);
>>>       int (*min_cdclk)(const struct intel_crtc_state *crtc_state,
>>>                const struct intel_plane_state *plane_state);
>>> +    void (*program_async_surface_address)(struct intel_plane *plane,
>>> +                          const struct intel_crtc_state *crtc_state,
>>> +                          const struct intel_plane_state *plane_state);
>>>   };
>>>   struct intel_watermark_params {
>>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
>>> b/drivers/gpu/drm/i915/display/intel_sprite.c
>>> index f0c89418d2e1..69407dfcebf6 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>>> @@ -609,6 +609,29 @@ 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 intel_plane *plane,
>>> +                  const struct intel_crtc_state *crtc_state,
>>> +                  const struct intel_plane_state *plane_state)
>>> +{
>>> +    struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> +    unsigned long irqflags;
>>> +    enum plane_id plane_id = plane->id;
>>> +    enum pipe pipe = plane->pipe;
>>> +    u32 surf_addr = plane_state->color_plane[0].offset;
>>> +    u32 plane_ctl = plane_state->ctl;
>>> +
>>> +    plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>>> +
>>> +    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,
>>> @@ -3096,6 +3119,7 @@ skl_universal_plane_create(struct 
>>> drm_i915_private *dev_priv,
>>>       plane->get_hw_state = skl_plane_get_hw_state;
>>>       plane->check_plane = skl_plane_check;
>>>       plane->min_cdclk = skl_plane_min_cdclk;
>>> +    plane->program_async_surface_address = 
>>> skl_program_async_surface_address;
>>>       if (INTEL_GEN(dev_priv) >= 11)
>>>           formats = icl_get_plane_formats(dev_priv, pipe,
>>> -- 
>>> 2.22.0
>>