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

[Kuogee Hsieh]

>>       }
>>   +    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.


>
> 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);
>