On Thu, 08 Sep 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
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
