| Message ID | 20220908165702.973854-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsb: hide struct intel_dsb better | expand |
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 :) Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > 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
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
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"
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> --- 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(-)