[PATCH v4 08/13] drm/msm/dpu: add support for virtual planes
Abhinav Kumar
quic_abhinavk at quicinc.com
Sat Jun 8 02:45:58 UTC 2024
On 6/7/2024 5:57 PM, Dmitry Baryshkov wrote:
> On Sat, 8 Jun 2024 at 02:55, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 6/7/2024 3:26 PM, Dmitry Baryshkov wrote:
>>> On Sat, 8 Jun 2024 at 00:39, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/7/2024 2:10 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, Jun 07, 2024 at 12:22:16PM -0700, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/7/2024 12:16 AM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Jun 06, 2024 at 03:21:11PM -0700, Abhinav Kumar wrote:
>>>>>>>> On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote:
>>>>>>>>> Only several SSPP blocks support such features as YUV output or scaling,
>>>>>>>>> thus different DRM planes have different features. Properly utilizing
>>>>>>>>> all planes requires the attention of the compositor, who should
>>>>>>>>> prefer simpler planes to YUV-supporting ones. Otherwise it is very easy
>>>>>>>>> to end up in a situation when all featureful planes are already
>>>>>>>>> allocated for simple windows, leaving no spare plane for YUV playback.
>>>>>>>>>
>>>>>>>>> To solve this problem make all planes virtual. Each plane is registered
>>>>>>>>> as if it supports all possible features, but then at the runtime during
>>>>>>>>> the atomic_check phase the driver selects backing SSPP block for each
>>>>>>>>> plane.
>>>>>>>>>
>>>>>>>>> Note, this does not provide support for using two different SSPP blocks
>>>>>>>>> for a single plane or using two rectangles of an SSPP to drive two
>>>>>>>>> planes. Each plane still gets its own SSPP and can utilize either a solo
>>>>>>>>> rectangle or both multirect rectangles depending on the resolution.
>>>>>>>>>
>>>>>>>>> Note #2: By default support for virtual planes is turned off and the
>>>>>>>>> driver still uses old code path with preallocated SSPP block for each
>>>>>>>>> plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1'
>>>>>>>>> kernel parameter.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I like the overall approach in this patch. Some comments below.
>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 50 +++++
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 +
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 230 +++++++++++++++++++---
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 19 ++
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 77 ++++++++
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 28 +++
>>>>>>>>> 7 files changed, 390 insertions(+), 28 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>>> index 88c2e51ab166..794c5643584f 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>>> @@ -1168,6 +1168,49 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
>>>>>>>>> return false;
>>>>>>>>> }
>>>>>>>>> +static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state)
>>>>>>>>> +{
>>>>>>>>> + int total_planes = crtc->dev->mode_config.num_total_plane;
>>>>>>>>> + struct drm_atomic_state *state = crtc_state->state;
>>>>>>>>> + struct dpu_global_state *global_state;
>>>>>>>>> + struct drm_plane_state **states;
>>>>>>>>> + struct drm_plane *plane;
>>>>>>>>> + int ret;
>>>>>>>>> +
>>>>>>>>> + global_state = dpu_kms_get_global_state(crtc_state->state);
>>>>>>>>> + if (IS_ERR(global_state))
>>>>>>>>> + return PTR_ERR(global_state);
>>>>>>>>> +
>>>>>>>>> + dpu_rm_release_all_sspp(global_state, crtc);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Do we need to call dpu_rm_release_all_sspp() even in the
>>>>>>>> _dpu_plane_atomic_disable()?
>>>>>>>
>>>>>>> It allows the driver to optimize the usage of the SSPP rectangles.
>>>>>>>
>>>>>>
>>>>>> No, what I meant was that we should call dpu_rm_release_all_sspp() in
>>>>>> dpu_plane_atomic_update() as well because in the atomic_check() path where
>>>>>> its called today, its being called only for zpos_changed and planes_changed
>>>>>> but during disable we must call this for sure.
>>>>>
>>>>> No. the dpu_rm_release_all_sspp() should only be called during check.
>>>>> When dpu_plane_atomic_update() is called, the state should already be
>>>>> finalised. The atomic_check() callback is called when a plane is going
>>>>> to be disabled.
>>>>>
>>>>
>>>> atomic_check() will be called when plane is disabled but
>>>> dpu_rm_release_all_sspp() may not be called as it is protected by
>>>> zpos_changed and planes_changed. OR you need to add a !visible check
>>>> here to call dpu_rm_release_all_sspp() that time. Thats whay I wrote
>>>> previously.
>>>
>>> Unless I miss something, if a plane gets disabled, then obviously
>>> planes_changed is true.
>>>
>>> [trimmed]
>>>
>>
>> Do you mean DRM fwk sets planes_changed correctly for this case?
>>
>> Currently we have
>>
>> if (!new_state->visible) {
>> _dpu_plane_atomic_disable(plane);
>> } else {
>> dpu_plane_sspp_atomic_update(plane);
>> }
>>
>> So I wanted to ensure that when plane gets disabled, its SSPP is freed
>> too. If this is confirmed, I do not have any concerns.
>
> This is the atomic_update() path, not the atomic_check()
>
Yes, I am aware.
Let me clarify my question here once again.
1) dpu_rm_release_all_sspp() gets called only in atomic_check() when
either planes_changed or zpos_changed is set
2) But even in _dpu_plane_atomic_disable(), we should call
dpu_rm_release_all_sspp() unconditionally. So for this, as you wrote,
the corresponding atomic_check() call of _dpu_plane_atomic_disable() is
supposed to do this. atomic_check() checks planes_changed and
zpos_changed but not !visible before calling dpu_rm_release_all_sspp().
Will planes_changed be set even when !visible?
I am trying to make sure that dpu_rm_release_all_sspp() will get called
for the _dpu_plane_atomic_disable(). Thats all.
>>
>>>>
>>>>>>>>> @@ -1486,7 +1593,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>>>>>>>>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180;
>>>>>>>>> - if (pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION))
>>>>>>>>> + if (inline_rotation)
>>>>>>>>> supported_rotations |= DRM_MODE_ROTATE_MASK;
>>>>>>>>> drm_plane_create_rotation_property(plane,
>>>>>>>>> @@ -1494,10 +1601,81 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>>>>>>>>> drm_plane_enable_fb_damage_clips(plane);
>>>>>>>>> - /* success! finalize initialization */
>>>>>>>>> + DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name,
>>>>>>>>> + pipe, plane->base.id);
>>>>>>>>> + return plane;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +struct drm_plane *dpu_plane_init(struct drm_device *dev,
>>>>>>>>> + uint32_t pipe, enum drm_plane_type type,
>>>>>>>>> + unsigned long possible_crtcs)
>>>>>>>>> +{
>>>>>>>>> + struct drm_plane *plane = NULL;
>>>>>>>>> + struct msm_drm_private *priv = dev->dev_private;
>>>>>>>>> + struct dpu_kms *kms = to_dpu_kms(priv->kms);
>>>>>>>>> + struct dpu_hw_sspp *pipe_hw;
>>>>>>>>> +
>>>>>>>>> + /* initialize underlying h/w driver */
>>>>>>>>> + pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe);
>>>>>>>>> + if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) {
>>>>>>>>> + DPU_ERROR("[%u]SSPP is invalid\n", pipe);
>>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> + plane = dpu_plane_init_common(dev, type, possible_crtcs,
>>>>>>>>> + pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION),
>>>>>>>>> + pipe_hw->cap->sblk->format_list,
>>>>>>>>> + pipe_hw->cap->sblk->num_formats,
>>>>>>>>> + pipe);
>>>>>>>>> + if (IS_ERR(plane))
>>>>>>>>> + return plane;
>>>>>>>>> +
>>>>>>>>> drm_plane_helper_add(plane, &dpu_plane_helper_funcs);
>>>>>>>>> DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name,
>>>>>>>>> pipe, plane->base.id);
>>>>>>>>> +
>>>>>>>>> + return plane;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev,
>>>>>>>>> + enum drm_plane_type type,
>>>>>>>>> + unsigned long possible_crtcs)
>>>>>>>>> +{
>>>>>>>>> + struct drm_plane *plane = NULL;
>>>>>>>>> + struct msm_drm_private *priv = dev->dev_private;
>>>>>>>>> + struct dpu_kms *kms = to_dpu_kms(priv->kms);
>>>>>>>>> + bool has_inline_rotation = false;
>>>>>>>>> + const u32 *format_list = NULL;
>>>>>>>>> + u32 num_formats = 0;
>>>>>>>>> + int i;
>>>>>>>>> +
>>>>>>>>> + /* Determine the largest configuration that we can implement */
>>>>>>>>> + for (i = 0; i < kms->catalog->sspp_count; i++) {
>>>>>>>>> + const struct dpu_sspp_cfg *cfg = &kms->catalog->sspp[i];
>>>>>>>>> +
>>>>>>>>> + if (test_bit(DPU_SSPP_INLINE_ROTATION, &cfg->features))
>>>>>>>>> + has_inline_rotation = true;
>>>>>>>>> +
>>>>>>>>> + if (!format_list ||
>>>>>>>>> + cfg->sblk->csc_blk.len) {
>>>>>>>>
>>>>>>>> But format_list is being assigned to NULL just a few lines above. Why is
>>>>>>>> this check needed?
>>>>>>>
>>>>>>> It was assigned before the loop.
>>>>>>>
>>>>>>
>>>>>> Yes, I got this part but missed on why we needed the loop at all.
>>>>>
>>>>> Which set of formats should the virtual plane use?
>>>>>
>>>>>>>>
>>>>>>>> I dont get why this part can also goto dpu_plane_init_common() as it looks
>>>>>>>> identical to me.
>>>>>>>
>>>>>>> And it is not. For the non-virtual case there is no loop around formats
>>>>>>> list assignment.
>>>>>>>
>>>>>>
>>>>>> Ah okay, I misunderstood the logic. After reading the comment above the loop
>>>>>> I get what you are trying to do here.
>>>>>>
>>>>>> But I dont get why you really need to do that?
>>>>>>
>>>>>> 1) In this patch the relationship between virtual plane and SSPP is still
>>>>>> 1:1 so what is wrong to retain the sspp's actual format for the plane rather
>>>>>> than picking the best format (you are targetting Vig SSPP)
>>>>>
>>>>> No. With this patch there is no 1:1 relationship. The RM will select the
>>>>> SSPP that suits the requirements (YUV, scaling, rotation, etc).
>>>>>
>>>>
>>>> Yes but there is always only one SSPP for one plane is what I meant.
>>>> That does not change till the next patch.
>>>>
>>>> In that sense, I dont see why you need to expose the superset of formats.
>>>
>>> Let me please repeat my question: what set of formats should be used
>>> for plane init?
>>>
>>
>> So, my point here was that in the loop, in this patch, we create one
>> plane for one SSPP, why dont we just use the same SSPP's format for that
>> plane.
>
> Which SSPP? There is no SSPP attached to a virtual plane.
>
dpu_plane_init_virtual() gets called for each SSPP of the catalog in
this patchset just like dpu_plane_init().
The only difference is dpu_plane_init() also passes the pipe but
dpu_plane_init_virtual() does not.
Would it be incorrect to also pass along the sspp's fmt list (from the
catalog->sspp loop) to dpu_plane_init_virtual() instead of the superset?
>>
>> In the next patch, when the same plane can attach to two different
>> SSPPs, we will use the superset of the SSPPs. IOW, do we need the
>> superset in this patch itself?
>>
>>>>
>>>>>> In fact, that will reduce atomic_check() failures with this patch because
>>>>>> compositor will still work the same way as it used to work before by not
>>>>>> trying an unsupported format on a plane.
>>>>>
>>>>> The virtual plane should support any of the formats that the backing
>>>>> hardware can support. If (for example) we only had RGB-only and YUV-only
>>>>> hardware blocks, the driver would have to construct a _superset_ of
>>>>> those formats. Fortunately this is not the case and VIG supports a
>>>>> strict superset of what DMA (or RGB) SSPP supports.
>>>>>
>>>>
>>>> Yes, thats why I said plane_formats_yuv is enough in my next point below
>>>> because Vig is super set of DMA or IOW Vig is the most feature rich plane.
>>>
>>> QCM2290 doesn't have YUV support if I'm not mistaken.
>>>
>>
>> qcm2290_sspp has YUV support but no scaling support as per the catalog.
>> It uses _VIG_SBLK_NOSCALE which still has plane_formats_yuv.
>
> I'll check it on the real hw. I remember that I had questions regarding it.
>
>>
>>>>
>>>>>> If one plane maps to two SSPPs, then yes we can go with the superset of
>>>>>> formats but that comes in a later patch right?
>>>>>>
>>>>>> 2) So even if we want to do it this way from this patch itself, I think all
>>>>>> you are looking for is whether there is a Vig SSPP and if so use
>>>>>> plane_formats_yuv. There is no need for this loop IMO.
>>>>>>
>>>>>> 3) I noticed that virt_format_list is still present in the driver. If you
>>>>>> are planning to not use that perhaps drop it with this series.
>>>>>
>>>>> Ack
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> + format_list = cfg->sblk->format_list;
>>>>>>>>> + num_formats = cfg->sblk->num_formats;
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + plane = dpu_plane_init_common(dev, type, possible_crtcs,
>>>>>>>>> + has_inline_rotation,
>>>>>>>>> + format_list,
>>>>>>>>> + num_formats,
>>>>>>>>> + SSPP_NONE);
>>>>>>>>
>>>>>>>> Ok, here is the part which we were discussing in
>>>>>>>>
>>>>>>>> https://patchwork.freedesktop.org/patch/582820/?series=131109&rev=1#comment_1087370
>>>>>>>>
>>>>>>>> So yes, that part belongs to this patch.
>>>>>>>
>>>>>>> I'll check it while preparing the next iteration.
>>>>>>>
>>>>>>>>
>>>>>>>>> + if (IS_ERR(plane))
>>>>>>>>> + return plane;
>>>>>>>>> +
>>>>>>>>> + drm_plane_helper_add(plane, &dpu_plane_virtual_helper_funcs);
>>>>>>>>> +
>>>>>>>>> + DPU_DEBUG("%s created virtual id:%u\n", plane->name, plane->base.id);
>>>>>>>>> +
>>>>>>>>> return plane;
>>>>>>>>> }
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>>>>>> index a3ae45dc95d0..15f7d60d8b85 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>>>>>> @@ -30,6 +30,7 @@
>>>>>>>>> * @plane_fetch_bw: calculated BW per plane
>>>>>>>>> * @plane_clk: calculated clk per plane
>>>>>>>>> * @needs_dirtyfb: whether attached CRTC needs pixel data explicitly flushed
>>>>>>>>> + * @saved_fmt: format used by the plane's FB, saved for for virtual plane support
>>>>>>>>> */
>>>>>>>>> struct dpu_plane_state {
>>>>>>>>> struct drm_plane_state base;
>>>>>>>>> @@ -46,6 +47,8 @@ struct dpu_plane_state {
>>>>>>>>> u64 plane_clk;
>>>>>>>>> bool needs_dirtyfb;
>>>>>>>>> +
>>>>>>>>> + const struct dpu_format *saved_fmt;
>>>>>>>>> };
>>>>>>>>
>>>>>>>> Why is saved_fmt needed?
>>>>>>>>
>>>>>>>> The use-case which comes to my mind is lets say if we have a RGB format and
>>>>>>>> we need to switch to a YUV format, basically switch from DMA to ViG SSPP,
>>>>>>>> then yes we have to mark planes_changed as we need to switch the underlying
>>>>>>>> SSPP that time, but why cant we simply check that by means of a check to see
>>>>>>>> if the fmt is YUV and whether CSC block is present in the SSPP.
>>>>>>>
>>>>>>> Yes, correct. And vice versa, going from YUV to RGB might free the VIG
>>>>>>> SSPP.
>>>>>>>
>>>>>>>>
>>>>>>>> This will lead to dpu_crtc_reassign_planes() getting called for format
>>>>>>>> changes even when the new format might be available in the same SSPP.
>>>>>>>
>>>>>>> So use 'needs_vig' instead of storing the format? Sounds good to me.
>>>>>>>
>>>>>>
>>>>>> Yes thats the idea. Basically "needs_reassignment". You could even go from
>>>>>> Vig to DMA if the use-case can just use DMA to save up Vig.
>>>>>>
>>>>>> Also, do we really need to cache anything in the plane state to track this?
>>>>>>
>>>>>> If we have a function called dpu_crtc_needs_plane_reassignment() will go
>>>>>> through the current plane state and the current SSPP from the global state
>>>>>> and see if needs reassignment.
>>>>>
>>>>> No, looking at the global state won't be possible here. I'd have to lock
>>>>> the private object before consulting it, which might cause EDEADLOCK
>>>>> later on during resource reallocation. So all necessary information
>>>>> should be stored in the dpu_plane_state.
>>>>>
>>>>
>>>> But you are already calling dpu_kms_get_global_state() in
>>>> dpu_crtc_reassign_planes().
>>>
>>> It happens at a different point. And I'm not sure how modeset locking
>>> will react to an attempt to lock the private object twice.
>>>
>>
>> hmm, I am missing the code flow a bit.
>>
>> Inside drm_atomic_helper_check_planes(), we first have plane's
>> atomic_check followed by crtc's.
>>
>> In plane's atomic_check is where we are setting planes_changed by
>> checking whether we need re-assignment of SSPPs.
>>
>> In CRTC's atomic_check is where we have the logic to check
>> planes_changed and reassign the SSPPs.
>>
>> We already call dpu_kms_get_global_state() in crtc atomic_check which
>> means we acquire the ctx for the private object.
>>
>> Would it be incorrect to acquire it in plane's atomic_check?
>>
>> If so, can we do one of below:
>>
>> 1) call drm_modeset_drop_locks() before plane's atomic_check ends. That
>> way within the drm_atomic_helper_check_planes(), only one ctx is tracked
>> at a time.
>
> No, if I understand it correctly, this would drop all lock, so all the
> objects are unlocked.
>
>>
>> 2) if (1) wont work, would dpu_kms_get_existing_global_state() help? For
>> that one we do not need the locking.
>
> I'll check whether this works as expected.
>
> But really I don't see a problem that you are trying to solve. It is
> too early to run SSPP allocation before dpu_crtc_atomic_check(). We
> need a set of all the plane states that are used by the CRTC. Even
> though it is not needed for this patch (it can work with just single
> plane state), it doesn't make sense to rewire that again, within the
> same patchset.
>
The problem I am trying to solve is to avoid caching any previous state
like saved_fmt OR needs_yuv in the state and keep growing that list when
that information is already available to us in the current global state.
I would like to avoid adding new cached information just to track prev
and current state unless that information is indeed not available.
>>
>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>>> #define to_dpu_plane_state(x) \
>>>>>>>>> @@ -75,6 +78,16 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>>>>>>>>> uint32_t pipe, enum drm_plane_type type,
>>>>>>>>> unsigned long possible_crtcs);
>>>>>>>>> +/**
>>>>>>>>> + * dpu_plane_init_virtual - create new dpu virtualized plane
>>>>>>>>> + * @dev: Pointer to DRM device
>>>>>>>>> + * @type: Plane type - PRIMARY/OVERLAY/CURSOR
>>>>>>>>> + * @possible_crtcs: bitmask of crtc that can be attached to the given pipe
>>>>>>>>> + */
>>>>>>>>> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev,
>>>>>>>>> + enum drm_plane_type type,
>>>>>>>>> + unsigned long possible_crtcs);
>>>>>>>>> +
>>>>>>>>> /**
>>>>>>>>> * dpu_plane_color_fill - enables color fill on plane
>>>>>>>>> * @plane: Pointer to DRM plane object
>>>>>>>>> @@ -91,4 +104,10 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable);
>>>>>>>>> static inline void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) {}
>>>>>>>>> #endif
>>>>>>>>> +int dpu_assign_plane_resources(struct dpu_global_state *global_state,
>>>>>>>>> + struct drm_atomic_state *state,
>>>>>>>>> + struct drm_crtc *crtc,
>>>>>>>>> + struct drm_plane_state **states,
>>>>>>>>> + unsigned int num_planes);
>>>>>>>>> +
>>>>>>>>> #endif /* _DPU_PLANE_H_ */
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>>>>> index 44938ba7a2b7..7264a4d44a14 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>>>>>> @@ -694,6 +694,83 @@ int dpu_rm_reserve(
>>>>>>>>> return ret;
>>>>>>>>> }
>>>>>>>>> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm,
>>>>>>>>> + struct dpu_global_state *global_state,
>>>>>>>>> + struct drm_crtc *crtc,
>>>>>>>>> + struct dpu_rm_sspp_requirements *reqs)
>>>>>>>>> +{
>>>>>>>>> + uint32_t crtc_id = crtc->base.id;
>>>>>>>>> + unsigned int weight, best_weght = UINT_MAX;
>>>>>>>>
>>>>>>>> best_weight?
>>>>>>>
>>>>>>> Yes
>>>>>>>
>>>>>>>>
>>>>>>>>> + struct dpu_hw_sspp *hw_sspp;
>>>>>>>>> + unsigned long mask = 0;
>>>>>>>>> + int i, best_idx = -1;
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * Don't take cursor feature into consideration until there is proper support for SSPP_CURSORn.
>>>>>>>>> + */
>>>>>>>>> + mask |= BIT(DPU_SSPP_CURSOR);
>>>>>>>>> +
>>>>>>>>> + if (reqs->scale)
>>>>>>>>> + mask |= BIT(DPU_SSPP_SCALER_RGB) |
>>>>>>>>> + BIT(DPU_SSPP_SCALER_QSEED2) |
>>>>>>>>> + BIT(DPU_SSPP_SCALER_QSEED3_COMPATIBLE);
>>>>>>>>> +
>>>>>>>>> + if (reqs->yuv)
>>>>>>>>> + mask |= BIT(DPU_SSPP_CSC) |
>>>>>>>>> + BIT(DPU_SSPP_CSC_10BIT);
>>>>>>>>> +
>>>>>>>>> + if (reqs->rot90)
>>>>>>>>> + mask |= BIT(DPU_SSPP_INLINE_ROTATION);
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(rm->hw_sspp); i++) {
>>>>>>>>> + if (!rm->hw_sspp[i])
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + if (global_state->sspp_to_crtc_id[i])
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + hw_sspp = rm->hw_sspp[i];
>>>>>>>>> +
>>>>>>>>> + /* skip incompatible planes */
>>>>>>>>> + if (reqs->scale && !hw_sspp->cap->sblk->scaler_blk.len)
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + if (reqs->yuv && !hw_sspp->cap->sblk->csc_blk.len)
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + if (reqs->rot90 && !(hw_sspp->cap->features & DPU_SSPP_INLINE_ROTATION))
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * For non-yuv, non-scaled planes prefer simple (DMA or RGB)
>>>>>>>>> + * plane, falling back to VIG only if there are no such planes.
>>>>>>>>> + *
>>>>>>>>> + * This way we'd leave VIG sspps to be later used for YUV formats.
>>>>>>>>> + */
>>>>>>>>> + weight = hweight64(hw_sspp->cap->features & ~mask);
>>>>>>>>
>>>>>>>> This approach is assuming that ViG feature masks are more than DMA.
>>>>>>>> Hence the hweight of DMA SSPP's features is less than hweight of ViG SSPPs.
>>>>>>>>
>>>>>>>> Is this really true? Because there are other bits such as DMA_SDM845_MASK
>>>>>>>> which might increase the hweight of DMA SSPPs
>>>>>>>
>>>>>>> Which bits are present in the DMA mask, which are not present in the VIG
>>>>>>> mask? Also for the older platforms there are three kinds of planes: VIG,
>>>>>>> DMA and RGB. The selection algorithm should not require significant
>>>>>>> changes to support that case.
>>>>>>>
>>>>>>
>>>>>> DMA_SDM845_MASK has DPU_SSPP_QOS_8LVL which is not there in VIG_MSM8998_MASK
>>>>>> afaict. But we really cannot be counting the number of feature bits and
>>>>>> going by that.
>>>>>
>>>>> MSM8998 uses DMA_MSM8998_MASK, not DMA_SDM845_MASK.
>>>>>
>>
>> I forgot to update this point, for sm6375_sspp, it uses DMA_SDM845_MASK
>> for DMA and VIG_SDM845_MASK for VIG. So my point is applicable for
>> sm6375 atleast.
>
> DPU_SSPP_QOS_8LVL is enabled in both VIG_SDM845_MASK and DMA_SDM845_MASK
>
>>>>>> Hence, inherently, going by hweight is not right because whenever we add a
>>>>>> catalog change to add a new feature bit to SSPP, we have to come back here
>>>>>> and make sure this logic will not break.
>>>>>>>>
>>>>>>>> I would rather make it simpler.
>>>>>>>>
>>>>>>>> 1) if we need scaling || yuv, then we have to get only a Vig
>>>>>>>> 2) else, first try to get a DMA SSPP
>>>>>>>
>>>>>>> How would you distinguish between VIG and DMA?
>>>>>>>
>>>>>>
>>>>>> the type SSPP_TYPE_VIG OR SSPP_TYPE_DMA. We also have a SSPP_TYPE_RGB so
>>>>>> that should address your concern about the third type of plane (Vig, DMA,
>>>>>> RGB).
>>>>>
>>>>> I don't particularly like the idea of using type. We still need to
>>>>> evaluate plane's features. Consider QCM2290, where VIG planes do not
>>>>> support scaling.
>>>>>
>>>>> I will evaluate if I can rework this part to use type, while still
>>>>> checking for the feature bit. BTW: should we prefer RGB or DMA plane if
>>>>> all other conditions are met?
>>>>>
>>>>
>>>> Ok, qcm2290 really seems like an odd case but point taken.
>>>>
>>>> I am fine if it needs to be a combination of type and feature bit but
>>>> certainly not hweight of feature bit. If you want to use type along with
>>>> presence of scaler blk feature bit thats fine.
>>>>
>>>> I need to check if there is any feature loss in RGB Vs DMA. Let me check
>>>> and get back. This needs some history digging.
>>>
>>> Sure.
>
More information about the dri-devel
mailing list