Re: [Intel-gfx] [PATCH] drm/i915/dsb: hide struct intel_dsb better

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 08 Sep 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Sep 08, 2022 at 07:57:02PM +0300, Jani Nikula wrote:
>> struct intel_dsb can be an opaque type, hidden in intel_dsb.c. Make it
>> so. Reduce related includes while at it.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> One thing I was mildly worried about with dsb is the cost
> of creating the batch (updating LUTs involves writing some
> thousands of dwords). So I was pondering whether that should
> be inlined as opposed to being a function call per dword.
> But as it stands it's already a function call, and
> I've not actually measured how fast/slow it really is.
> So can't really argue against this sort of stuff, for the
> moment at least :)

I'm also on a mission to kill useless static inlines. ;)

Anything that requires pulling in additional headers or exposing the
guts of the implementation are suspect and need proper justification.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks,
Jani.

>
>> ---
>>  drivers/gpu/drm/i915/display/intel_color.c   |  1 +
>>  drivers/gpu/drm/i915/display/intel_display.c |  1 +
>>  drivers/gpu/drm/i915/display/intel_dsb.c     | 30 ++++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_dsb.h     | 28 ------------------
>>  drivers/gpu/drm/i915/i915_drv.h              |  1 -
>>  5 files changed, 32 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index ed98c732b24e..6bda4274eae9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -26,6 +26,7 @@
>>  #include "intel_de.h"
>>  #include "intel_display_types.h"
>>  #include "intel_dpll.h"
>> +#include "intel_dsb.h"
>>  #include "vlv_dsi_pll.h"
>>  
>>  struct intel_color_funcs {
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 2b6bb5ee7698..296cbcd1352c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -91,6 +91,7 @@
>>  #include "intel_dmc.h"
>>  #include "intel_dp_link_training.h"
>>  #include "intel_dpt.h"
>> +#include "intel_dsb.h"
>>  #include "intel_fbc.h"
>>  #include "intel_fbdev.h"
>>  #include "intel_fdi.h"
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index c4affcb216fd..fc9c3e41c333 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -9,6 +9,36 @@
>>  #include "i915_drv.h"
>>  #include "intel_de.h"
>>  #include "intel_display_types.h"
>> +#include "intel_dsb.h"
>> +
>> +struct i915_vma;
>> +
>> +enum dsb_id {
>> +	INVALID_DSB = -1,
>> +	DSB1,
>> +	DSB2,
>> +	DSB3,
>> +	MAX_DSB_PER_PIPE
>> +};
>> +
>> +struct intel_dsb {
>> +	enum dsb_id id;
>> +	u32 *cmd_buf;
>> +	struct i915_vma *vma;
>> +
>> +	/*
>> +	 * free_pos will point the first free entry position
>> +	 * and help in calculating tail of command buffer.
>> +	 */
>> +	int free_pos;
>> +
>> +	/*
>> +	 * ins_start_offset will help to store start address of the dsb
>> +	 * instuction and help in identifying the batch of auto-increment
>> +	 * register.
>> +	 */
>> +	u32 ins_start_offset;
>> +};
>>  
>>  #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 6cb9c580cdca..74dd2b3343bb 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -11,34 +11,6 @@
>>  #include "i915_reg_defs.h"
>>  
>>  struct intel_crtc_state;
>> -struct i915_vma;
>> -
>> -enum dsb_id {
>> -	INVALID_DSB = -1,
>> -	DSB1,
>> -	DSB2,
>> -	DSB3,
>> -	MAX_DSB_PER_PIPE
>> -};
>> -
>> -struct intel_dsb {
>> -	enum dsb_id id;
>> -	u32 *cmd_buf;
>> -	struct i915_vma *vma;
>> -
>> -	/*
>> -	 * free_pos will point the first free entry position
>> -	 * and help in calculating tail of command buffer.
>> -	 */
>> -	int free_pos;
>> -
>> -	/*
>> -	 * ins_start_offset will help to store start address of the dsb
>> -	 * instuction and help in identifying the batch of auto-increment
>> -	 * register.
>> -	 */
>> -	u32 ins_start_offset;
>> -};
>>  
>>  void intel_dsb_prepare(struct intel_crtc_state *crtc_state);
>>  void intel_dsb_cleanup(struct intel_crtc_state *crtc_state);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 76aad81c014b..be201ba5e9ab 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -38,7 +38,6 @@
>>  
>>  #include "display/intel_display.h"
>>  #include "display/intel_display_core.h"
>> -#include "display/intel_dsb.h"
>>  
>>  #include "gem/i915_gem_context_types.h"
>>  #include "gem/i915_gem_lmem.h"
>> -- 
>> 2.34.1

-- 
Jani Nikula, Intel Open Source Graphics Center