[PATCH v3 23/27] drm/msm/dpu: rework dpu_plane_atomic_check()
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Feb 7 18:08:10 UTC 2023
On 2/7/2023 9:59 AM, Dmitry Baryshkov wrote:
> On 07/02/2023 19:57, Abhinav Kumar wrote:
>>
>>
>> On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote:
>>> On 07/02/2023 19:49, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>> Split pipe-dependent code from dpu_plane_atomic_check() into the
>>>>> separate function dpu_plane_atomic_check_pipe(). This is one of
>>>>
>>>> Same comment as prev patch, dpu_plane_atomic_check_pipe() --->
>>>> dpu_plane_atomic_check_sspp()
>>>
>>> No, it does what it does: it checks one software pipe configuration.
>>
>> The concept of software pipe is only to encapsulate the hw_sspp along
>> with its params
>>
>> +struct dpu_sw_pipe {
>> + struct dpu_hw_sspp *sspp;
>> + enum dpu_sspp_multirect_index multirect_index;
>> + enum dpu_sspp_multirect_mode multirect_mode;
>> +};
>>
>> So its a very thin differentiation there.
>
> SSPP is a hardware thing, while the sw_pipe is a software instance. It
> can employ an SSPP or a half of SSPP. And if it employs a half of SSPP
> (rec1) than the name dpu_plane_atomic_check_sspp() doesn't make sense:
> we are not checking the SSPP itself (or its configuration). We are
> checking the parameters of SW pipe.
>
Ok, I need to check in the next couple of patches how you are handling
the same dpu_sw_pipe for different rects of the same sspp.
struct dpu_plane_state {
struct drm_plane_state base;
struct msm_gem_address_space *aspace;
- struct dpu_hw_sspp *hw_sspp;
+ struct dpu_sw_pipe pipe;
enum dpu_stage stage;
bool needs_qos_remap;
- uint32_t multirect_index;
So far what I saw was dpu_plane_state ---> dpu_sw_pipe ---> dpu_hw_sppp
In this chain, I couldnt get how the two rects of the DMA SSPP are
differentiated, perhaps there is something in the next few changes.
>>
>>>
>>>>
>>>>> preparational steps to add r_pipe support.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88
>>>>> ++++++++++++++---------
>>>>> 1 file changed, 53 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> index f94e132733f3..e69499490d39 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> @@ -903,6 +903,51 @@ static int
>>>>> dpu_plane_check_inline_rotation(struct dpu_plane *pdpu,
>>>>> return 0;
>>>>> }
>>>>> +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>>>>> + struct dpu_sw_pipe *pipe,
>>>>> + struct dpu_hw_sspp_cfg *pipe_cfg,
>>>>
>>>> pipe_cfg --> sspp_cfg
>>>>
>>>> Also, had a question. For function parameters spreading across
>>>> multiple lines do we want to align to one guideline? Is it going to
>>>> be two tabs more than the prev line or aligning it with the opening
>>>> brace of prev line?
>>>>
>>>> I am seeing a mix in the prev patch of the series and this one.
>>>
>>> I usually tend to keep the existing indent when adding new args and
>>> shifting to open brace when adding new functions. Maybe I failed a
>>> question in this patch, I'll doublecheck it.
>>>
>>>>
>>>>> + const struct dpu_format *fmt)
>>>>> +{
>>>>> + uint32_t min_src_size;
>>>>> +
>>>>> + min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>> +
>>>>> + if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> + (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) ||
>>>>> + !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) {
>>>>> + DPU_DEBUG_PLANE(pdpu,
>>>>> + "plane doesn't have scaler/csc for yuv\n");
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + /* check src bounds */
>>>>> + if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size ||
>>>>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) {
>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>>> + return -E2BIG;
>>>>> + }
>>>>> +
>>>>> + /* valid yuv image */
>>>>> + if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 &
>>>>> 0x1 ||
>>>>> + drm_rect_width(&pipe_cfg->src_rect) & 0x1 ||
>>>>> + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) {
>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT
>>>>> "\n",
>>>>> + DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + /* min dst support */
>>>>> + if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 ||
>>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) {
>>>>> + DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n",
>>>>> + DRM_RECT_ARG(&pipe_cfg->dst_rect));
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>>> struct drm_atomic_state *state)
>>>>> {
>>>>> @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct
>>>>> drm_plane *plane,
>>>>> const struct dpu_format *fmt;
>>>>> struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>> struct drm_rect fb_rect = { 0 };
>>>>> - uint32_t min_src_size, max_linewidth;
>>>>> + uint32_t max_linewidth;
>>>>> unsigned int rotation;
>>>>> uint32_t supported_rotations;
>>>>> const struct dpu_sspp_cfg *pipe_hw_caps =
>>>>> pstate->pipe.sspp->cap;
>>>>> @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct
>>>>> drm_plane *plane,
>>>>> max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>>>>> -
>>>>> - min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>> -
>>>>> - if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> - (!(pipe_hw_caps->features & DPU_SSPP_SCALER) ||
>>>>> - !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) {
>>>>> - DPU_DEBUG_PLANE(pdpu,
>>>>> - "plane doesn't have scaler/csc for yuv\n");
>>>>> - return -EINVAL;
>>>>> -
>>>>> - /* check src bounds */
>>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size ||
>>>>> - drm_rect_height(&pipe_cfg->src_rect) < min_src_size) {
>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>>> - return -E2BIG;
>>>>> -
>>>>> - /* valid yuv image */
>>>>> - } else if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> - (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 &
>>>>> 0x1 ||
>>>>> - drm_rect_width(&pipe_cfg->src_rect) & 0x1 ||
>>>>> - drm_rect_height(&pipe_cfg->src_rect) & 0x1)) {
>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT
>>>>> "\n",
>>>>> - DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>>> - return -EINVAL;
>>>>> -
>>>>> - /* min dst support */
>>>>> - } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 ||
>>>>> drm_rect_height(&pipe_cfg->dst_rect) < 0x1) {
>>>>> - DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n",
>>>>> - DRM_RECT_ARG(&pipe_cfg->dst_rect));
>>>>> - return -EINVAL;
>>>>> -
>>>>> /* check decimated source width */
>>>>> - } else if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>>>> + if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>>>> DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT "
>>>>> line:%u\n",
>>>>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>> return -E2BIG;
>>>>> }
>>>>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>>>>> +
>>>>> + ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe,
>>>>> pipe_cfg, fmt);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0;
>>>>> if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION))
>>>
>
More information about the dri-devel
mailing list