[RFC PATCH v2 05/13] drm/msm/dpu: get rid of struct dpu_rm_requirements
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu May 18 23:19:24 UTC 2023
On 5/17/2023 4:53 PM, Abhinav Kumar wrote:
>
>
> On 5/14/2023 10:06 AM, Dmitry Baryshkov wrote:
>> On Sat, 13 May 2023 at 01:39, Abhinav Kumar
>> <quic_abhinavk at quicinc.com> wrote:
>>> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
>>>> The struct dpu_rm_requirements was used to wrap display topology and
>>>> hw resources, which meant INTF indices. As of commit ef58e0ad3436
>>>> ("drm/msm/dpu: get INTF blocks directly rather than through RM") the hw
>>>> resources struct was removed, leaving struct dpu_rm_requirements
>>>> containing a single field (topology). Remove the useless wrapper.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> ---
>>>
>>> Irrespective of where we plan to have the topology, this change doesn't
>>> seem incorrect as such.
>>>
>>> The only thing I can think of is when we need more information to be
>>> passed to the RM to allocate the blocks in addition to the topology this
>>> struct could have been expanded.
>>>
>>> So one example I can think of is lets say I want to add CDM block
>>> support. Then that information is outside of topology today because I
>>> will use CDM if my output format is yuv. It has nothing to do with
>>> topology but that block still needs to come from RM.
>>
>> I'd say, it is a part of the topology. CDM blocks are a part of the
>> pipeline, aren't they?
>>
>> If you prefer, we can rename msm_display_topology to
>> dpu_rm_requirements itself.
>>
>
> I am fine with renaming msm_display_topology to dpu_rm_requirements.
>
> Because making CDM part of topology wont be right.
>
Just to add, perhaps rename it to dpu_rm_display_requirements
>>> I know that usually I have lost on these type of discussions saying that
>>> if the code is not there yet, it should be dropped but I do have a plan
>>> to add that support soon probably by the next cycle. That time we will
>>> need some sort of wrapper to hold the topology and "extra" information
>>> to allocate the blocks.
>>>
>>> One alternative ofcourse is to expand dpu_rm_reserve() to accept
>>> something like "needs_cdm" but this is not scalable.
>>>
>>> Thoughts?
>>>
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69
>>>> +++++++--------------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +-
>>>> 3 files changed, 23 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 4ee708264f3b..a2cb23dea0b8 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -638,7 +638,7 @@ static int dpu_encoder_virt_atomic_check(
>>>>
>>>> if (!crtc_state->active_changed || crtc_state->enable)
>>>> ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>>>> - drm_enc, crtc_state, topology);
>>>> + drm_enc, crtc_state, &topology);
>>>> }
>>>>
>>>> trace_dpu_enc_atomic_check_flags(DRMID(drm_enc),
>>>> adj_mode->flags);
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> index f4dda88a73f7..952e139c0234 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -24,15 +24,6 @@ static inline bool reserved_by_other(uint32_t
>>>> *res_map, int idx,
>>>> return res_map[idx] && res_map[idx] != enc_id;
>>>> }
>>>>
>>>> -/**
>>>> - * struct dpu_rm_requirements - Reservation requirements parameter
>>>> bundle
>>>> - * @topology: selected topology for the display
>>>> - * @hw_res: Hardware resources required as reported by the
>>>> encoders
>>>> - */
>>>> -struct dpu_rm_requirements {
>>>> - struct msm_display_topology topology;
>>>> -};
>>>> -
>>>> int dpu_rm_destroy(struct dpu_rm *rm)
>>>> {
>>>> int i;
>>>> @@ -329,14 +320,13 @@ static bool _dpu_rm_check_lm_peer(struct
>>>> dpu_rm *rm, int primary_idx,
>>>> * mixer in rm->pingpong_blks[].
>>>> * @dspp_idx: output parameter, index of dspp block attached to
>>>> the layer
>>>> * mixer in rm->dspp_blks[].
>>>> - * @reqs: input parameter, rm requirements for HW blocks needed in the
>>>> - * datapath.
>>>> + * @topology: selected topology for the display
>>>> * Return: true if lm matches all requirements, false otherwise
>>>> */
>>>> static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm
>>>> *rm,
>>>> struct dpu_global_state *global_state,
>>>> uint32_t enc_id, int lm_idx, int *pp_idx, int *dspp_idx,
>>>> - struct dpu_rm_requirements *reqs)
>>>> + struct msm_display_topology *topology)
>>>> {
>>>> const struct dpu_lm_cfg *lm_cfg;
>>>> int idx;
>>>> @@ -361,7 +351,7 @@ static bool
>>>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>>>> }
>>>> *pp_idx = idx;
>>>>
>>>> - if (!reqs->topology.num_dspp)
>>>> + if (!topology->num_dspp)
>>>> return true;
>>>>
>>>> idx = lm_cfg->dspp - DSPP_0;
>>>> @@ -383,7 +373,7 @@ static bool
>>>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>>>> static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>> struct dpu_global_state *global_state,
>>>> uint32_t enc_id,
>>>> - struct dpu_rm_requirements *reqs)
>>>> + struct msm_display_topology *topology)
>>>>
>>>> {
>>>> int lm_idx[MAX_BLOCKS];
>>>> @@ -391,14 +381,14 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>> int dspp_idx[MAX_BLOCKS] = {0};
>>>> int i, j, lm_count = 0;
>>>>
>>>> - if (!reqs->topology.num_lm) {
>>>> - DPU_ERROR("invalid number of lm: %d\n",
>>>> reqs->topology.num_lm);
>>>> + if (!topology->num_lm) {
>>>> + DPU_ERROR("invalid number of lm: %d\n",
>>>> topology->num_lm);
>>>> return -EINVAL;
>>>> }
>>>>
>>>> /* Find a primary mixer */
>>>> for (i = 0; i < ARRAY_SIZE(rm->mixer_blks) &&
>>>> - lm_count < reqs->topology.num_lm; i++) {
>>>> + lm_count < topology->num_lm; i++) {
>>>> if (!rm->mixer_blks[i])
>>>> continue;
>>>>
>>>> @@ -407,7 +397,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>>
>>>> if (!_dpu_rm_check_lm_and_get_connected_blks(rm,
>>>> global_state,
>>>> enc_id, i, &pp_idx[lm_count],
>>>> - &dspp_idx[lm_count], reqs)) {
>>>> + &dspp_idx[lm_count], topology)) {
>>>> continue;
>>>> }
>>>>
>>>> @@ -415,7 +405,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>>
>>>> /* Valid primary mixer found, find matching peers */
>>>> for (j = i + 1; j < ARRAY_SIZE(rm->mixer_blks) &&
>>>> - lm_count < reqs->topology.num_lm; j++) {
>>>> + lm_count < topology->num_lm; j++) {
>>>> if (!rm->mixer_blks[j])
>>>> continue;
>>>>
>>>> @@ -428,7 +418,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>> if (!_dpu_rm_check_lm_and_get_connected_blks(rm,
>>>> global_state, enc_id, j,
>>>> &pp_idx[lm_count],
>>>> &dspp_idx[lm_count],
>>>> - reqs)) {
>>>> + topology)) {
>>>> continue;
>>>> }
>>>>
>>>> @@ -437,7 +427,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>> }
>>>> }
>>>>
>>>> - if (lm_count != reqs->topology.num_lm) {
>>>> + if (lm_count != topology->num_lm) {
>>>> DPU_DEBUG("unable to find appropriate mixers\n");
>>>> return -ENAVAIL;
>>>> }
>>>> @@ -446,7 +436,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
>>>> global_state->mixer_to_enc_id[lm_idx[i]] = enc_id;
>>>> global_state->pingpong_to_enc_id[pp_idx[i]] = enc_id;
>>>> global_state->dspp_to_enc_id[dspp_idx[i]] =
>>>> - reqs->topology.num_dspp ? enc_id : 0;
>>>> + topology->num_dspp ? enc_id : 0;
>>>>
>>>> trace_dpu_rm_reserve_lms(lm_idx[i] + LM_0, enc_id,
>>>> pp_idx[i] + PINGPONG_0);
>>>> @@ -539,44 +529,30 @@ static int _dpu_rm_make_reservation(
>>>> struct dpu_rm *rm,
>>>> struct dpu_global_state *global_state,
>>>> struct drm_encoder *enc,
>>>> - struct dpu_rm_requirements *reqs)
>>>> + struct msm_display_topology *topology)
>>>> {
>>>> int ret;
>>>>
>>>> - ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id, reqs);
>>>> + ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id,
>>>> topology);
>>>> if (ret) {
>>>> DPU_ERROR("unable to find appropriate mixers\n");
>>>> return ret;
>>>> }
>>>>
>>>> ret = _dpu_rm_reserve_ctls(rm, global_state, enc->base.id,
>>>> - &reqs->topology);
>>>> + topology);
>>>> if (ret) {
>>>> DPU_ERROR("unable to find appropriate CTL\n");
>>>> return ret;
>>>> }
>>>>
>>>> - ret = _dpu_rm_reserve_dsc(rm, global_state, enc,
>>>> &reqs->topology);
>>>> + ret = _dpu_rm_reserve_dsc(rm, global_state, enc, topology);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> return ret;
>>>> }
>>>>
>>>> -static int _dpu_rm_populate_requirements(
>>>> - struct drm_encoder *enc,
>>>> - struct dpu_rm_requirements *reqs,
>>>> - struct msm_display_topology req_topology)
>>>> -{
>>>> - reqs->topology = req_topology;
>>>> -
>>>> - DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
>>>> - reqs->topology.num_lm, reqs->topology.num_dsc,
>>>> - reqs->topology.num_intf);
>>>> -
>>>> - return 0;
>>>> -}
>>>> -
>>>> static void _dpu_rm_clear_mapping(uint32_t *res_mapping, int cnt,
>>>> uint32_t enc_id)
>>>> {
>>>> @@ -608,9 +584,8 @@ int dpu_rm_reserve(
>>>> struct dpu_global_state *global_state,
>>>> struct drm_encoder *enc,
>>>> struct drm_crtc_state *crtc_state,
>>>> - struct msm_display_topology topology)
>>>> + struct msm_display_topology *topology)
>>>> {
>>>> - struct dpu_rm_requirements reqs;
>>>> int ret;
>>>>
>>>> /* Check if this is just a page-flip */
>>>> @@ -625,13 +600,11 @@ int dpu_rm_reserve(
>>>> DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
>>>> enc->base.id, crtc_state->crtc->base.id);
>>>>
>>>> - ret = _dpu_rm_populate_requirements(enc, &reqs, topology);
>>>> - if (ret) {
>>>> - DPU_ERROR("failed to populate hw requirements\n");
>>>> - return ret;
>>>> - }
>>>> + DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
>>>> + topology->num_lm, topology->num_dsc,
>>>> + topology->num_intf);
>>>>
>>>> - ret = _dpu_rm_make_reservation(rm, global_state, enc, &reqs);
>>>> + ret = _dpu_rm_make_reservation(rm, global_state, enc, topology);
>>>> if (ret)
>>>> DPU_ERROR("failed to reserve hw resources: %d\n", ret);
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> index d62c2edb2460..f05697462856 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> @@ -71,7 +71,7 @@ int dpu_rm_reserve(struct dpu_rm *rm,
>>>> struct dpu_global_state *global_state,
>>>> struct drm_encoder *drm_enc,
>>>> struct drm_crtc_state *crtc_state,
>>>> - struct msm_display_topology topology);
>>>> + struct msm_display_topology *topology);
>>>>
>>>> /**
>>>> * dpu_rm_reserve - Given the encoder for the display chain,
>>>> release any
>>
>>
>>
More information about the dri-devel
mailing list