Re: [PATCH 00/12] Consolidation of DSS Control in Separate Files

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 8/26/2024 6:04 PM, Jani Nikula wrote:
> On Mon, 26 Aug 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Currently, DSS control is configured from various files; this change aims
>> to consolidate all DSS-related functionalities, such as display stream
>> splitting, joining, MSO configuration, and joining configuration,
>> into one place.
>>
>> A new file, intel_dss_regs.h, will store register information, while the
>> helpers to configure DSS and related stuff will be moved to intel_dss.c
>> with its corresponding header file intel_dss.h.
>> Along with this, the helpers related to retrieve information about the
>> pipe joiners are also moved from intel_display.c to intel_dss.
>>
>> Additionally, wherever possible, the drm_i915_private structure is
>> replaced with the new intel_display structure as part of ongoing efforts
>> to phase out the old structure.
> A bunch of nitpicks here and there, but overall I like the
> direction. Thanks for doing this.

Thanks Jani for the review comments and suggestions.

I agree to the changes and corrections suggested and will address them 
in next revision.


>
> I see that the dsi and mso stuff are just thrown in there instead of
> really "integrated" into anything, but I think that's fine as the first
> step.

I agree. I was thinking if we should have a separate struct intel_dss 
with members for different features, which we configure in 
compute_config phase and later write dss_ctl register at one place?

Same for reading and get_config. If it makes sense, perhaps can take it 
as a separate series.

>
> I think maybe we do want to have a separate file for joiner stuff like
> you suggested offline, maybe intel_joiner.[ch]? There's a whole lot of
> joiner stuff still left in intel_display.[ch] after this.
>
> Sooo... anything to do with DSS_CTL regs would go to intel_dss.[ch],
> anything to do with compression would go to intel_vdsc.[ch], and
> anything to do with joining would go to intel_joiner.[ch]?

Yes right. The new joiner file can house all the helpers for retrieving 
primary/secondary joiner_pipes along with helpers to modify src_size, 
timings etc.


Thanks again for the comments!

Regards,

Ankit

>
> Ville?
>
>
> BR,
> Jani.
>
>
>> Ankit Nautiyal (12):
>>    drm/i915/display: Move all DSS control registers to a new file
>>    drm/i915/ddi: Move all mso related helpers to a new file
>>    drm/i915/dss: Move to struct intel_display
>>    drm/i915/icl_dsi: Move helpers to configure dsi dual link to intel_dss
>>    drm/i915/vdsc: Rename helper to check if the pipe supports dsc
>>    drm/i915/vdsc: Move all dss stuff in dss files
>>    drm/i915/display: Move dss stuff in intel_dss files
>>    drm/i915/display: Move helper to get joined pipe mask to intel_dss
>>    drm/i915/display: Move helpers for primary joiner to intel_dss
>>    drm/i915/display: Move helper to check for secondary joiner pipe
>>    drm/i915/display: Move helper to get all secondary pipes
>>    drm/i915/display: Move intel_joiner_num_pipes to intel dss
>>
>>   drivers/gpu/drm/i915/Makefile                 |   1 +
>>   drivers/gpu/drm/i915/display/icl_dsi.c        |  55 +---
>>   .../gpu/drm/i915/display/intel_atomic_plane.c |   3 +-
>>   .../drm/i915/display/intel_crtc_state_dump.c  |   5 +-
>>   drivers/gpu/drm/i915/display/intel_ddi.c      |  94 +-----
>>   drivers/gpu/drm/i915/display/intel_display.c  | 158 +++------
>>   drivers/gpu/drm/i915/display/intel_display.h  |   4 -
>>   .../drm/i915/display/intel_display_debugfs.c  |   3 +-
>>   drivers/gpu/drm/i915/display/intel_dp_mst.c   |   7 +-
>>   drivers/gpu/drm/i915/display/intel_drrs.c     |   5 +-
>>   drivers/gpu/drm/i915/display/intel_dss.c      | 305 ++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dss.h      |  46 +++
>>   drivers/gpu/drm/i915/display/intel_dss_regs.h |  49 +++
>>   .../drm/i915/display/intel_modeset_setup.c    |  15 +-
>>   drivers/gpu/drm/i915/display/intel_vdsc.c     |  74 +----
>>   drivers/gpu/drm/i915/display/intel_vdsc.h     |   2 +-
>>   .../gpu/drm/i915/display/intel_vdsc_regs.h    |  38 ---
>>   drivers/gpu/drm/xe/Makefile                   |   1 +
>>   18 files changed, 497 insertions(+), 368 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_dss.c
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_dss.h
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_dss_regs.h