Re: [PATCH v4] drm/tidss: dispc: Rewrite naive plane positioning code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 14/02/2020 13:20, Tomi Valkeinen wrote:
> On 13/02/2020 21:37, Jyri Sarha wrote:
>> The old implementation of placing planes on the CRTC while configuring
>> the planes was naive and relied on the order in which the planes were
>> configured, enabled, and disabled. The situation where a plane's zpos
>> was changed on the fly was completely broken. The usual symptoms of
>> this problem was scrambled display and a flood of sync lost errors,
>> when a plane was active in two layers at the same time, or a missing
>> plane, in case when a layer was accidentally disabled.
>>
>> The rewrite takes a more straight forward approach when HW is
>> concerned. The plane positioning registers are in the CRTC (or
>> actually OVR) register space and it is more natural to configure them
>> in a one go when configuring the CRTC. To do this we need make sure we
>> have all the planes on the updated CRTCs in the new atomic state. The
>> untouched planes on CRTCs that need plane position update are added to
>> the atomic state in tidss_atomic_check().
> 
> The subject needs updating. This is a fix for a bug, and subject needs
> to reflect that.
> 

Ok

>> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
>> ---
>>   drivers/gpu/drm/tidss/tidss_crtc.c  | 55 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tidss/tidss_crtc.h  |  2 ++
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++++++++++------------------
>>   drivers/gpu/drm/tidss/tidss_dispc.h |  5 +++
>>   drivers/gpu/drm/tidss/tidss_kms.c   | 49 ++++++++++++++++++++++++-
>>   5 files changed, 130 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
>> b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 032c31ee2820..631ec61b086a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -17,6 +17,7 @@
>>   #include "tidss_dispc.h"
>>   #include "tidss_drv.h"
>>   #include "tidss_irq.h"
>> +#include "tidss_plane.h"
>>     /* Page flip and frame done IRQs */
>>   @@ -111,6 +112,54 @@ static int tidss_crtc_atomic_check(struct
>> drm_crtc *crtc,
>>       return dispc_vp_bus_check(dispc, hw_videoport, state);
>>   }
>>   +/*
>> + * This needs all affected planes to be present in the atomic
>> + * state. The untouched planes are added to the state in
>> + * tidss_atomic_check().
>> + */
>> +static void tidss_crtc_position_planes(struct tidss_device *tidss,
>> +                       struct drm_crtc *crtc,
>> +                       struct drm_crtc_state *old_state,
>> +                       bool newmodeset)
>> +{
>> +    struct drm_atomic_state *ostate = old_state->state;
>> +    struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>> +    struct drm_crtc_state *cstate = crtc->state;
>> +    int zpos;
>> +
>> +    if (!newmodeset && !cstate->zpos_changed &&
>> +        !to_tidss_crtc_state(cstate)->plane_pos_changed)
>> +        return;
>> +
>> +    for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) {
>> +        struct drm_plane_state *pstate;
>> +        struct drm_plane *plane;
>> +        bool zpos_taken = false;
>> +        int i;
>> +
>> +        for_each_new_plane_in_state(ostate, plane, pstate, i) {
>> +            if (pstate->crtc != crtc || !pstate->visible)
>> +                continue;
>> +
>> +            if (pstate->normalized_zpos == zpos) {
>> +                zpos_taken = true;
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (zpos_taken) {
>> +            struct tidss_plane *tplane = to_tidss_plane(plane);
>> +
>> +            dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id,
>> +                        tcrtc->hw_videoport,
>> +                        pstate->crtc_x, pstate->crtc_y,
>> +                        zpos);
>> +        }
>> +        dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos,
>> +                       zpos_taken);
>> +    }
>> +}
> 
> Nitpicking, but... I think the "zpos" above is really "layer". Even the
> params, to which you pass "zpos", in the ovr functions are named "layer".
> 

Well, it is both. But I'll change that.

> "zpos_taken" sounds like it's reserved and not available for us, or
> something like that. Maybe "layer_active" conveys better that we're just
> collecting which layers are active and which are not.
> 

Ok, that sounds better.

>> +
>>   static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>>                       struct drm_crtc_state *old_crtc_state)
>>   {
>> @@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct
>> drm_crtc *crtc,
>>       /* Write vp properties to HW if needed. */
>>       dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state,
>> false);
>>   +    /* Update plane positions if needed. */
>> +    tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false);
>> +
>>       WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>>         spin_lock_irqsave(&ddev->event_lock, flags);
>> @@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct
>> drm_crtc *crtc,
>>           return;
>>         dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state,
>> true);
>> +    tidss_crtc_position_planes(tidss, crtc, old_state, true);
>>         /* Turn vertical blanking interrupt reporting on. */
>>       drm_crtc_vblank_on(crtc);
>> @@ -318,6 +371,8 @@ static struct drm_crtc_state
>> *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
>>         __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
>>   +    state->plane_pos_changed = false;
>> +
>>       state->bus_format = current_state->bus_format;
>>       state->bus_flags = current_state->bus_flags;
>>   diff --git a/drivers/gpu/drm/tidss/tidss_crtc.h
>> b/drivers/gpu/drm/tidss/tidss_crtc.h
>> index df9d90b1ad2d..09e773666228 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.h
>> @@ -32,6 +32,8 @@ struct tidss_crtc_state {
>>       /* Must be first. */
>>       struct drm_crtc_state base;
>>   +    bool plane_pos_changed;
>> +
>>       u32 bus_format;
>>       u32 bus_flags;
>>   };
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index eeb160dc047b..e79dad246b1e 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -281,11 +281,6 @@ struct dss_vp_data {
>>       u32 *gamma_table;
>>   };
>>   -struct dss_plane_data {
>> -    u32 zorder;
>> -    u32 hw_videoport;
>> -};
>> -
>>   struct dispc_device {
>>       struct tidss_device *tidss;
>>       struct device *dev;
>> @@ -307,8 +302,6 @@ struct dispc_device {
>>         struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>>   -    struct dss_plane_data plane_data[TIDSS_MAX_PLANES];
>> -
>>       u32 *fourccs;
>>       u32 num_fourccs;
>>   @@ -1247,7 +1240,7 @@ int dispc_vp_set_clk_rate(struct dispc_device
>> *dispc, u32 hw_videoport,
>>   /* OVR */
>>   static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc,
>>                       u32 hw_plane, u32 hw_videoport,
>> -                    u32 x, u32 y, u32 zpos)
>> +                    u32 x, u32 y, u32 layer)
>>   {
>>       /* On k2g there is only one plane and no need for ovr */
>>       dispc_vid_write(dispc, hw_plane, DISPC_VID_K2G_POSITION,
>> @@ -1256,44 +1249,43 @@ static void dispc_k2g_ovr_set_plane(struct
>> dispc_device *dispc,
>>     static void dispc_am65x_ovr_set_plane(struct dispc_device *dispc,
>>                         u32 hw_plane, u32 hw_videoport,
>> -                      u32 x, u32 y, u32 zpos)
>> +                      u32 x, u32 y, u32 layer)
>>   {
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>>               hw_plane, 4, 1);
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>>               x, 17, 6);
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>>               y, 30, 19);
>>   }
>>     static void dispc_j721e_ovr_set_plane(struct dispc_device *dispc,
>>                         u32 hw_plane, u32 hw_videoport,
>> -                      u32 x, u32 y, u32 zpos)
>> +                      u32 x, u32 y, u32 layer)
>>   {
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>>               hw_plane, 4, 1);
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer),
>>               x, 13, 0);
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos),
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer),
>>               y, 29, 16);
>>   }
>>   -static void dispc_ovr_set_plane(struct dispc_device *dispc,
>> -                u32 hw_plane, u32 hw_videoport,
>> -                u32 x, u32 y, u32 zpos)
>> +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
>> +             u32 hw_videoport, u32 x, u32 y, u32 layer)
>>   {
>>       switch (dispc->feat->subrev) {
>>       case DISPC_K2G:
>>           dispc_k2g_ovr_set_plane(dispc, hw_plane, hw_videoport,
>> -                    x, y, zpos);
>> +                    x, y, layer);
>>           break;
>>       case DISPC_AM65X:
>>           dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
>> -                      x, y, zpos);
>> +                      x, y, layer);
>>           break;
>>       case DISPC_J721E:
>>           dispc_j721e_ovr_set_plane(dispc, hw_plane, hw_videoport,
>> -                      x, y, zpos);
>> +                      x, y, layer);
>>           break;
>>       default:
>>           WARN_ON(1);
>> @@ -1301,10 +1293,13 @@ static void dispc_ovr_set_plane(struct
>> dispc_device *dispc,
>>       }
>>   }
>>   -static void dispc_ovr_enable_plane(struct dispc_device *dispc,
>> -                   u32 hw_videoport, u32 zpos, bool enable)
>> +void dispc_ovr_enable_layer(struct dispc_device *dispc,
>> +                u32 hw_videoport, u32 layer, bool enable)
>>   {
>> -    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
>> +    if (dispc->feat->subrev == DISPC_K2G)
>> +        return;
>> +
>> +    OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>>               !!enable, 0, 0);
>>   }
>>   @@ -2070,21 +2065,11 @@ int dispc_plane_setup(struct dispc_device
>> *dispc, u32 hw_plane,
>>           VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0,
>>                   28, 28);
>>   -    dispc_ovr_set_plane(dispc, hw_plane, hw_videoport,
>> -                state->crtc_x, state->crtc_y,
>> -                state->normalized_zpos);
>> -
>> -    dispc->plane_data[hw_plane].zorder = state->normalized_zpos;
>> -    dispc->plane_data[hw_plane].hw_videoport = hw_videoport;
>> -
>>       return 0;
>>   }
>>     int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane,
>> bool enable)
>>   {
>> -    dispc_ovr_enable_plane(dispc,
>> dispc->plane_data[hw_plane].hw_videoport,
>> -                   dispc->plane_data[hw_plane].zorder, enable);
>> -
>>       VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable,
>> 0, 0);
>>         return 0;
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h
>> b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index e65e6a2bb821..a4a68249e44b 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -94,6 +94,11 @@ extern const struct dispc_features dispc_j721e_feats;
>>   void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask);
>>   dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc);
>>   +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
>> +             u32 hw_videoport, u32 x, u32 y, u32 layer);
>> +void dispc_ovr_enable_layer(struct dispc_device *dispc,
>> +                u32 hw_videoport, u32 layer, bool enable);
>> +
>>   void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>>                 const struct drm_crtc_state *state);
>>   void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c
>> b/drivers/gpu/drm/tidss/tidss_kms.c
>> index 5311e0f1c551..24b3f02b90c6 100644
>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>> @@ -47,9 +47,56 @@ static const struct drm_mode_config_helper_funcs
>> mode_config_helper_funcs = {
>>       .atomic_commit_tail = tidss_atomic_commit_tail,
>>   };
>>   +static int tidss_atomic_check(struct drm_device *ddev,
>> +                  struct drm_atomic_state *state)
>> +{
>> +    struct drm_plane_state *opstate;
>> +    struct drm_plane_state *npstate;
>> +    struct drm_plane *plane;
>> +    struct drm_crtc_state *cstate;
>> +    struct drm_crtc *crtc;
>> +    int ret, i;
>> +
>> +    ret = drm_atomic_helper_check(ddev, state);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /*
>> +     * If a plane on a CRTC changes add all active planes on that
> 
> Only if the plane's position (x/y/z) changes.
> 

Ok.

>> +     * CRTC to the atomic state. This is needed for updating the
>> +     * plane positions in tidss_crtc_position_planes() which is
>> +     * called from crtc_atomic_enable() and crtc_atomic_flush().
>> +     * The update is needed for x,y-position changes too, so
>> +     * zpos_changed condition is not enough and we need add the
>> +     * our own plane_pos_changed flag.
> 
> Strictly speaking, we don't need to add all active planes if only
> plane's x/y pos changes. In that case it's enough to just update the OVR
> layer's x & y.
> 

The we are back to plane-specific updates. The whole idea of this fix is
to keep things simple and just update all ovr layers if anything there
changes.

> And when a plane is enabled or disabled, we need to update OVR. Does the
> code handled that? Keeping plane's x/y/z pos the same, but enabling and
> disabling it. I think both zpos_changed and pos_changed could be false
> in that case.
> 

zpos_changed flag handles that. If any plane on the crtc is
disabled/enabled/moved then the flag is set (in
drm_atomic_normalize_zpos()).

>> +     */
>> +    for_each_oldnew_plane_in_state(state, plane, opstate, npstate, i) {
>> +        if (npstate->crtc && npstate->visible &&
>> +            (!opstate->crtc ||
>> +             opstate->crtc_x != npstate->crtc_x ||
>> +             opstate->crtc_y != npstate->crtc_y)) {
> 
> The above becomes a bit easier to read if you first:
> 
> if (!npstate->crtc || !npstate->visible)
>     continue;
> 

Sure.

>> +            cstate = drm_atomic_get_crtc_state(state,
>> +                               npstate->crtc);
>> +            if (IS_ERR(cstate))
>> +                return PTR_ERR(cstate);
>> +            to_tidss_crtc_state(cstate)->plane_pos_changed = true;
>> +        }
>> +    }
> 
> Add blank line here.
> 

The two loops are part of the same procedure, but never mind I'll add a
newline.

>> +    for_each_new_crtc_in_state(state, crtc, cstate, i) {
>> +        if (to_tidss_crtc_state(cstate)->plane_pos_changed ||
>> +            cstate->zpos_changed) {
>> +            ret = drm_atomic_add_affected_planes(state, crtc);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +    }
> 
>  Tomi
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux