[PATCH 01/17] drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon Jan 29 04:33:26 UTC 2024
On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
>>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
>>>>>
>>>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>>>>>
>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
>>>>>>> ---
>>>>>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 +--
>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 31 ++++++++++---------
>>>>>>> 2 files changed, 18 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> index 993f263433314..37ac385727c3b 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>>>>> * @hw_intf: Hardware interface to the intf registers
>>>>>>> * @hw_wb: Hardware interface to the wb registers
>>>>>>> * @hw_cdm: Hardware interface to the CDM registers
>>>>>>> + * @cdm_cfg: CDM block config needed to store WB/DP block's CDM
>>>>>>> configuration
>>>>>>
>>>>>> Please realign the description.
>>>>> Ack
>>>>>>
>>>>>>> * @dpu_kms: Pointer to the dpu_kms top level
>>>>>>> * @cached_mode: DRM mode cached at mode_set time, acted on in
>>>>>>> enable
>>>>>>> * @vblank_ctl_lock: Vblank ctl mutex lock to protect
>>>>>>> vblank_refcount
>>>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>>>>> struct dpu_hw_intf *hw_intf;
>>>>>>> struct dpu_hw_wb *hw_wb;
>>>>>>> struct dpu_hw_cdm *hw_cdm;
>>>>>>> + struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>
>>>>>> It might be slightly better to move it after all the pointers, so
>>>>>> after the dpu_kms.
>>>>> Ack
>>>>>>
>>>>>>> struct dpu_kms *dpu_kms;
>>>>>>> struct drm_display_mode cached_mode;
>>>>>>> struct mutex vblank_ctl_lock;
>>>>>>> @@ -213,7 +215,6 @@ static inline int
>>>>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>>>>> * @wbirq_refcount: Reference count of writeback interrupt
>>>>>>> * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>>>>> * @wb_cfg: writeback block config to store fb related details
>>>>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
>>>>>>> configuration
>>>>>>> * @wb_conn: backpointer to writeback connector
>>>>>>> * @wb_job: backpointer to current writeback job
>>>>>>> * @dest: dpu buffer layout for current writeback output buffer
>>>>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>>>>>> atomic_t wbirq_refcount;
>>>>>>> int wb_done_timeout_cnt;
>>>>>>> struct dpu_hw_wb_cfg wb_cfg;
>>>>>>> - struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>> struct drm_writeback_connector *wb_conn;
>>>>>>> struct drm_writeback_job *wb_job;
>>>>>>> struct dpu_hw_fmt_layout dest;
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> index 4cd2d9e3131a4..072fc6950e496 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> @@ -269,28 +269,21 @@ static void
>>>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>>>>> * This API does not handle
>>>>>>> DPU_CHROMA_H1V2.
>>>>>>> * @phys_enc:Pointer to physical encoder
>>>>>>> */
>>>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>> dpu_encoder_phys *phys_enc)
>>>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>> dpu_encoder_phys *phys_enc,
>>>>>>> + const struct dpu_format *dpu_fmt,
>>>>>>> + u32 output_type)
>>>>>>> {
>>>>>>> struct dpu_hw_cdm *hw_cdm;
>>>>>>> struct dpu_hw_cdm_cfg *cdm_cfg;
>>>>>>> struct dpu_hw_pingpong *hw_pp;
>>>>>>> - struct dpu_encoder_phys_wb *wb_enc;
>>>>>>> - const struct msm_format *format;
>>>>>>> - const struct dpu_format *dpu_fmt;
>>>>>>> - struct drm_writeback_job *wb_job;
>>>>>>> int ret;
>>>>>>> if (!phys_enc)
>>>>>>> return;
>>>>>>> - wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>>>>>> - cdm_cfg = &wb_enc->cdm_cfg;
>>>>>>> + cdm_cfg = &phys_enc->cdm_cfg;
>>>>>>> hw_pp = phys_enc->hw_pp;
>>>>>>> hw_cdm = phys_enc->hw_cdm;
>>>>>>> - wb_job = wb_enc->wb_job;
>>>>>>> -
>>>>>>> - format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>>>>>> - dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
>>>>>>> wb_job->fb->modifier);
>>>>>>> if (!hw_cdm)
>>>>>>> return;
>>>>>>> @@ -306,10 +299,10 @@ static void
>>>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>>>>> memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>>>>>> - cdm_cfg->output_width = wb_job->fb->width;
>>>>>>> - cdm_cfg->output_height = wb_job->fb->height;
>>>>>>> + cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>>>>>>> + cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>>>>>
>>>>>> This is a semantic change. Instead of passing the FB size, this passes
>>>>>> the mode dimensions. They are not guaranteed to be the same,
>>>>>> especially for the WB case.
>>>>>>
>>>>
>>>> The WB job is storing the output FB of WB. I cannot think of a use-case
>>>> where this cannot match the current mode programmed to the WB encoder.
>>>>
>>>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
>>>> plane can scale the contents but here thats not the case. Here its the
>>>> output FB of WB.
>>>
>>> Is it a part of WB uAPI, to have the FB dimensions equal to mode
>>> dimensions? Or is it just our current limitation? I can easily imagine
>>> WB outputting data to a part of the FB (just like we can clip FB using
>>> plane's clip rectangle).
>>>
>>> This boils down to a question, whether CDM should be setup in terms of
>>> actual output date or the physical memory buffer parameters. I suspect
>>> the former is the case (which makes this change correct). But it
>>> either should be described in the commit message or (even better)
>>> split to a separate commit.
>>>
>>
>> I would say its a combination of both today.
>>
>> The way I would look at it is even if WB crops a certain section of FB,
>> that will not change the FB size. FB size of WB should match the rest of
>> the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
>> decides to write to only a small section of FB (cropping), then we need
>> another WB property like CROP_ROI so that we can program the WB to only
>> write to a small section of the programmed FB. So in some sense, there
>> is no such support in DRM UAPI today. Hence the FB of WB is the full
>> mode of the WB.
>
> I'd say, CROP_ROI can refer to cropping of the image source (esp. in
> the cloned output case). For writing to the part of the FB there can
> be DST_X/_Y/_W/_H properties. But this becomes off-topic.
>
>> CDM is before WB so follows the rest of the pipeline that is whatever
>> the data feeding it was programmed to.
>
> Yes. So the change is correct, but it should be split or documented
> properly. I prefer the first option.
>
Ok just to clarify you prefer below part of the change to be moved to
its own commit right?
- cdm_cfg->output_width = wb_job->fb->width;
- cdm_cfg->output_height = wb_job->fb->height;
+ cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
+ cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
If so, ack.
>
More information about the dri-devel
mailing list