[Freedreno] [PATCH v2 1/2] drm/msm/dpu: retrieve DSI DSC struct at atomic_check()

[Dmitry Baryshkov]

On 02/06/2023 18:51, Kuogee Hsieh wrote:
> 
> 
>>>       }
>>>   +    index = dpu_enc->disp_info.h_tile_instance[0];
>>> +        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
>>> +        dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);
>>
>> As discussed previously, one should not write to non-state objects 
>> from atomic_check. This chunk does.
> 
> yes, i did think about not to assign dsc here as we had discussed.
> 
> but the get_topology() below did need to know whether dsc is present or 
> not.
> 
> otherwise, i have to create a local variable to pass into get_topology() 
> function.
> 
> The dsc is assigned here but not yet be used.

This is all not relevant. You should not assign dpu_enc->dsc here, full 
stop. I thought I have explained why.

> 
> 
>>
>> Not to mention that this will start exploding once you try adding DP 
>> next to it.
>>
>> Please abstain from posting next revisions until the discussions on 
>> the previous one are more or less finished. For now this is NAK.
>>
>> Not to mention that this patch doesn't pass checkpatch.pl.
>>
>>> +
>>>       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
>>> crtc_state);
>>>         /*
>>> @@ -1034,7 +1038,7 @@ static void 
>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>       struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>>>       int num_lm, num_ctl, num_pp, num_dsc;
>>>       unsigned int dsc_mask = 0;
>>> -    int i;
>>> +    int index, i;
>>>         if (!drm_enc) {
>>>           DPU_ERROR("invalid encoder\n");
>>> @@ -1055,6 +1059,10 @@ static void 
>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>         trace_dpu_enc_mode_set(DRMID(drm_enc));
>>>   +    index = dpu_enc->disp_info.h_tile_instance[0];
>>> +        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
>>> +        dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);
>>
>> Doesn't this seem 100% same as the previous chunk? Doesn't it plead to 
>> be extracted to a helper function?
>>
>>> +
>>>       /* Query resource that have been reserved in atomic check step. */
>>>       num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>>>           drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
>>> @@ -2121,8 +2129,10 @@ void dpu_encoder_helper_phys_cleanup(struct 
>>> dpu_encoder_phys *phys_enc)
>>>                       phys_enc->hw_pp->merge_3d->idx);
>>>       }
>>>   -    if (dpu_enc->dsc)
>>> +    if (dpu_enc->dsc) {
>>>           dpu_encoder_unprep_dsc(dpu_enc);
>>> +        dpu_enc->dsc = NULL;
>>> +    }
>>>         intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>       intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>

-- 
With best wishes
Dmitry