[PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[Daniel Vetter]

Adding stuff to the bottom of struct drm_i915_driver_private is
nowadays considered uncool.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index faface9..2e1e884 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1380,6 +1380,10 @@ typedef struct drm_i915_private {
 	struct drm_crtc *pipe_to_crtc_mapping[3];
 	wait_queue_head_t pending_flip_queue;
 
+#ifdef CONFIG_DEBUG_FS
+	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
+#endif
+
 	int num_shared_dpll;
 	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
 	struct intel_ddi_plls ddi_plls;
@@ -1460,10 +1464,6 @@ typedef struct drm_i915_private {
 	struct i915_dri1_state dri1;
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
-
-#ifdef CONFIG_DEBUG_FS
-	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
-#endif
 } drm_i915_private_t;
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[Damien Lespiau]

On Mon, Oct 21, 2013 at 09:10:20PM +0200, Daniel Vetter wrote:
> Adding stuff to the bottom of struct drm_i915_driver_private is
> nowadays considered uncool.
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ville was suggesting moving the pipe_crc struct into intel_crtc. Might
be even better! But anyway:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index faface9..2e1e884 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1380,6 +1380,10 @@ typedef struct drm_i915_private {
>  	struct drm_crtc *pipe_to_crtc_mapping[3];
>  	wait_queue_head_t pending_flip_queue;
>  
> +#ifdef CONFIG_DEBUG_FS
> +	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
> +#endif
> +
>  	int num_shared_dpll;
>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>  	struct intel_ddi_plls ddi_plls;
> @@ -1460,10 +1464,6 @@ typedef struct drm_i915_private {
>  	struct i915_dri1_state dri1;
>  	/* Old ums support infrastructure, same warning applies. */
>  	struct i915_ums_state ums;
> -
> -#ifdef CONFIG_DEBUG_FS
> -	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
> -#endif
>  } drm_i915_private_t;
>  
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> -- 
> 1.8.4.rc3
> 

Re: [PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[Daniel Vetter]

On Mon, Oct 21, 2013 at 10:47 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Mon, Oct 21, 2013 at 09:10:20PM +0200, Daniel Vetter wrote:
>> Adding stuff to the bottom of struct drm_i915_driver_private is
>> nowadays considered uncool.
>>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Ville was suggesting moving the pipe_crc struct into intel_crtc. Might
> be even better! But anyway:

We enable interrupts before we allocate the intel_crtc structs. So if
we'd store the CRC stuff in there we'd need to be a bit more paranoid
with clearing hw state (since every time we trust the BIOS we end up
burning our hands). So I think this is safer ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[Ville Syrjälä]

On Mon, Oct 21, 2013 at 10:54:16PM +0200, Daniel Vetter wrote:
> On Mon, Oct 21, 2013 at 10:47 PM, Damien Lespiau
> <damien.lespiau@intel.com> wrote:
> > On Mon, Oct 21, 2013 at 09:10:20PM +0200, Daniel Vetter wrote:
> >> Adding stuff to the bottom of struct drm_i915_driver_private is
> >> nowadays considered uncool.
> >>
> >> Cc: Damien Lespiau <damien.lespiau@intel.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Ville was suggesting moving the pipe_crc struct into intel_crtc. Might
> > be even better! But anyway:
> 
> We enable interrupts before we allocate the intel_crtc structs. So if
> we'd store the CRC stuff in there we'd need to be a bit more paranoid
> with clearing hw state (since every time we trust the BIOS we end up
> burning our hands). So I think this is safer ...

FLIP_DONE etc. interrupts already depend on the crtc being there. Not
sure why this is different.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Move the pipe CRC stuff to other pipe data

[Daniel Vetter]

On Tue, Oct 22, 2013 at 10:15 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> FLIP_DONE etc. interrupts already depend on the crtc being there. Not
> sure why this is different.

I'd say we've simply lucked out thus far. E.g. on vlv the
init_clock_gating flushes the primary plane and so casues a flip done
interrupt. But right now we call that clock gating stuff after crtc
init so we're good. So my concern isn't that this can't be done but
that it's fragile. And right now we don't have any infrastructure to
help with the driver boostrapping at all, but we've had tons of cases
where things blew up in funny way. So good ideas welcome.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch