| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e08be3c8-e625-14c3-1c2c-5b01a6cc2fd0@linaro.org>
Date: Wed, 16 Nov 2022 18:06:51 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Kalyan Thota <kalyant@....qualcomm.com>,
"Kalyan Thota (QUIC)" <quic_kalyant@...cinc.com>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"freedreno@...ts.freedesktop.org" <freedreno@...ts.freedesktop.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robdclark@...omium.org" <robdclark@...omium.org>,
"dianders@...omium.org" <dianders@...omium.org>,
"swboyd@...omium.org" <swboyd@...omium.org>,
"Vinod Polimera (QUIC)" <quic_vpolimer@...cinc.com>,
"Abhinav Kumar (QUIC)" <quic_abhinavk@...cinc.com>
Subject: Re: [v1] drm/msm/disp/dpu1: add color management support for the crtc
On 16/11/2022 17:29, Kalyan Thota wrote:
>
>
>> -----Original Message-----
>> From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>> Sent: Saturday, November 12, 2022 4:13 AM
>> To: Kalyan Thota (QUIC) <quic_kalyant@...cinc.com>; dri-
>> devel@...ts.freedesktop.org; linux-arm-msm@...r.kernel.org;
>> freedreno@...ts.freedesktop.org; devicetree@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org; robdclark@...omium.org;
>> dianders@...omium.org; swboyd@...omium.org; Vinod Polimera (QUIC)
>> <quic_vpolimer@...cinc.com>; Abhinav Kumar (QUIC)
>> <quic_abhinavk@...cinc.com>
>> Subject: Re: [v1] drm/msm/disp/dpu1: add color management support for the
>> crtc
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 11/11/2022 16:57, Kalyan Thota wrote:
>>> Add color management support for the crtc provided there are enough
>>> dspps that can be allocated from the catalogue.
>>>
>>> Changes in v1:
>>> - cache color enabled state in the dpu crtc obj (Dmitry)
>>> - simplify dspp allocation while creating crtc (Dmitry)
>>> - register for color when crtc is created (Dmitry)
>>>
>>> Signed-off-by: Kalyan Thota <quic_kalyant@...cinc.com>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 5 +--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 ++--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53
>> ++++++++++++++++++++++++++++-
>>> 4 files changed, 64 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 4170fbe..ca4c3b3 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -1571,7 +1571,7 @@ static const struct drm_crtc_helper_funcs
>>> dpu_crtc_helper_funcs = {
>>>
>>> /* initialize crtc */
>>> struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
>> *plane,
>>> - struct drm_plane *cursor)
>>> + struct drm_plane *cursor, unsigned long
>>> + features)
>>> {
>>> struct drm_crtc *crtc = NULL;
>>> struct dpu_crtc *dpu_crtc = NULL; @@ -1583,6 +1583,7 @@ struct
>>> drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
>>> *plane,
>>>
>>> crtc = &dpu_crtc->base;
>>> crtc->dev = dev;
>>> + dpu_crtc->color_enabled = features & BIT(DPU_DSPP_PCC);
>>>
>>> spin_lock_init(&dpu_crtc->spin_lock);
>>> atomic_set(&dpu_crtc->frame_pending, 0); @@ -1604,7 +1605,7 @@
>>> struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct
>>> drm_plane *plane,
>>>
>>> drm_crtc_helper_add(crtc, &dpu_crtc_helper_funcs);
>>>
>>> - drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> + drm_crtc_enable_color_mgmt(crtc, 0, dpu_crtc->color_enabled, 0);
>>>
>>> /* save user friendly CRTC name for later */
>>> snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>>> crtc->base.id); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 539b68b..342f9ae 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -136,6 +136,7 @@ struct dpu_crtc_frame_event {
>>> * @enabled : whether the DPU CRTC is currently enabled. updated in the
>>> * commit-thread, not state-swap time which is earlier, so
>>> * safe to make decisions on during VBLANK on/off work
>>> + * @color_enabled : whether crtc supports color management
>>> * @feature_list : list of color processing features supported on a crtc
>>> * @active_list : list of color processing features are active
>>> * @dirty_list : list of color processing features are dirty
>>> @@ -164,7 +165,7 @@ struct dpu_crtc {
>>> u64 play_count;
>>> ktime_t vblank_cb_time;
>>> bool enabled;
>>> -
>>> + bool color_enabled;
>>> struct list_head feature_list;
>>> struct list_head active_list;
>>> struct list_head dirty_list;
>>> @@ -269,10 +270,11 @@ void dpu_crtc_complete_commit(struct drm_crtc
>> *crtc);
>>> * @dev: dpu device
>>> * @plane: base plane
>>> * @cursor: cursor plane
>>> + * @features: color features
>>> * @Return: new crtc object or error
>>> */
>>> struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
>> *plane,
>>> - struct drm_plane *cursor);
>>> + struct drm_plane *cursor, unsigned long
>>> + features);
>>>
>>> /**
>>> * dpu_crtc_register_custom_event - api for enabling/disabling crtc
>>> event diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index c9058aa..d72edb8 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -579,6 +579,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
>> *drm_enc)
>>> static struct msm_display_topology dpu_encoder_get_topology(
>>> struct dpu_encoder_virt *dpu_enc,
>>> struct dpu_kms *dpu_kms,
>>> + struct dpu_crtc *dpu_crtc,
>>> struct drm_display_mode *mode)
>>> {
>>> struct msm_display_topology topology = {0}; @@ -607,7 +608,7 @@
>>> static struct msm_display_topology dpu_encoder_get_topology(
>>> else
>>> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
>>> ? 2 : 1;
>>>
>>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>> + if (dpu_crtc->color_enabled) {
>>> if (dpu_kms->catalog->dspp &&
>>> (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>> topology.num_dspp = topology.num_lm; @@ -642,6
>>> +643,7 @@ static int dpu_encoder_virt_atomic_check(
>>> struct drm_display_mode *adj_mode;
>>> struct msm_display_topology topology;
>>> struct dpu_global_state *global_state;
>>> + struct dpu_crtc *dpu_crtc;
>>> int i = 0;
>>> int ret = 0;
>>>
>>> @@ -652,6 +654,7 @@ static int dpu_encoder_virt_atomic_check(
>>> }
>>>
>>> dpu_enc = to_dpu_encoder_virt(drm_enc);
>>> + dpu_crtc = to_dpu_crtc(crtc_state->crtc);
>>> DPU_DEBUG_ENC(dpu_enc, "\n");
>>>
>>> priv = drm_enc->dev->dev_private; @@ -677,7 +680,7 @@ static int
>>> dpu_encoder_virt_atomic_check(
>>> }
>>> }
>>>
>>> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>>> + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, dpu_crtc,
>>> + adj_mode);
>>>
>>> /* Reserve dynamic resources now. */
>>> if (!ret) {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 0709da2..ce6f5e6 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -734,7 +734,54 @@ static int _dpu_kms_setup_displays(struct drm_device
>> *dev,
>>> return rc;
>>> }
>>>
>>> +/**
>>> + * _dpu_kms_populate_dspp_features - Evaluate how many dspps pairs can be
>> facilitated
>>> + to enable color features for encoder and assign
>>> + color features prefering primary
>>
>> preferring
>>
>>> + * @dpu_kms: Pointer to dpu kms structure
>>> + * @features: Pointer to feature array
>>> + *
>>> + * Baring single entry, if atleast 2 dspps are available in the
>>> + catalogue,
>>
>> at least
>>
>>> + * then color can be enabled for that crtc */ static void
>>> +_dpu_kms_populate_dspp_features(struct dpu_kms *dpu_kms,
>>> + unsigned long *features)
>>> +{
>>> + struct drm_encoder *encoder;
>>> + u32 external_enc_mask = 0;
>>> + u32 num_dspps = dpu_kms->catalog->dspp_count;
>>> +
>>> + if (!num_dspps)
>>> + return;
>>> + else if (num_dspps > 1)
>>> + num_dspps /= 2;
>>> +
>>> + /* Ensure all primary encoders get first allocation of color */
>>> + drm_for_each_encoder(encoder, dpu_kms->dev) {
>>> + if(dpu_encoder_is_primary(encoder)) {
>>> + if (num_dspps) {
>>> + features[encoder->index] = dpu_kms->catalog->dspp-
>>> features;
>>> + num_dspps--;
>>> + }
>>> + } else if(dpu_encoder_is_external(encoder)) {
>>> + external_enc_mask |= drm_encoder_mask(encoder);
>>> + }
>>> + }
>>> +
>>> + if(!num_dspps)
>>> + return;
>>> +
>>> + /* Allocate color for external encoders after primary */
>>
>> Please. No "primary" encoders. You already have sense of internal vs external,
>> plugable, or which ever way you would like to call them. You don't need to add
>> another safety check, do you?
>>
>>> + drm_for_each_encoder_mask(encoder, dpu_kms->dev,
>> external_enc_mask) {
>>> + if (num_dspps) {
>>> + features[encoder->index] = dpu_kms->catalog->dspp->features;
>>> + num_dspps--;
>>> + }
>>> + }
>>> +}
>>> +
>>> #define MAX_PLANES 20
>>> +#define MAX_ENCODERS 10
>>> static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>>> {
>>> struct drm_device *dev;
>>> @@ -749,6 +796,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>>> *dpu_kms)
>>>
>>> int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret;
>>> int max_crtc_count;
>>> + unsigned long features[MAX_ENCODERS] = {0};
>>> +
>>> dev = dpu_kms->dev;
>>> priv = dev->dev_private;
>>> catalog = dpu_kms->catalog;
>>> @@ -798,12 +847,14 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>> *dpu_kms)
>>> }
>>>
>>> max_crtc_count = min(max_crtc_count, primary_planes_idx);
>>> + _dpu_kms_populate_dspp_features(dpu_kms, features);
>>>
>>> /* Create one CRTC per encoder */
>>> i = 0;
>>> drm_for_each_encoder(encoder, dev) {
>>> if (i < max_crtc_count) {
>>> - crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
>>> + crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i],
>>> + features[encoder->index]);
>>
>> A simple counter should be enough. You know the number of 'rich' CRTCs, which
>> can get DSPP (or a pair of DSPPs). Then you can create a feature-rich CRTC
>> provided the current encoder should use one and if the number of created 'rich'
>> CRTCs is not greater than the 'possibly rich'
>> number.
> Since encoder list is iterative, and pluggable/builtin encoders can be registered in any order. we need to traverse the list
> to pick up preferred encoders so as to attach color features. I couldn't get a logic on how to accomplish that using a counter. can you explain a bit more on the idea that you have.
drm_for_each_encoder(encoder, dev) {
bool use_ctm = false;
if (dpu_encoder_is_builtin(encoder, state))
use_ctm = true;
crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i], use_ctm);
}
>
>>
>>> if (IS_ERR(crtc)) {
>>> ret = PTR_ERR(crtc);
>>> return ret;
>>
>> --
>> With best wishes
>> Dmitry
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists