diff mbox series

[v6,12/28] drm/i915/dp: Add DSC params and DSC config to intel_crtc_state

Message ID 20181024222840.25683-13-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series Display Stream Compression enabling on eDP/DP | expand

Commit Message

Navare, Manasi Oct. 24, 2018, 10:28 p.m. UTC
Basic DSC parameters and DSC configuration data needs to be computed
for each of the requested mode during atomic check. This is
required since for certain modes, valid DSC parameters and config
data might not be computed in which case compression cannot be
enabled for that mode.
For that reason we need to add these params and config structure
to the intel_crtc_state so that if valid this state information
can directly be used while enabling DSC in atomic commit.

v2:
* Rebase on drm-tip (Manasi)

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 1 +
 drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
 2 files changed, 10 insertions(+)

Comments

Navare, Manasi Oct. 30, 2018, 11:53 p.m. UTC | #1
On Wed, Oct 24, 2018 at 03:28:24PM -0700, Manasi Navare wrote:
> Basic DSC parameters and DSC configuration data needs to be computed
> for each of the requested mode during atomic check. This is
> required since for certain modes, valid DSC parameters and config
> data might not be computed in which case compression cannot be
> enabled for that mode.
> For that reason we need to add these params and config structure
> to the intel_crtc_state so that if valid this state information
> can directly be used while enabling DSC in atomic commit.
> 
> v2:
> * Rebase on drm-tip (Manasi)
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 1 +
>  drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2d7761b8ac07..45fd7894722b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -53,6 +53,7 @@
>  #include <drm/drm_auth.h>
>  #include <drm/drm_cache.h>
>  #include <drm/drm_util.h>
> +#include <drm/drm_dsc.h>
>  
>  #include "i915_params.h"
>  #include "i915_reg.h"
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 62c051098859..27d47950f438 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -931,6 +931,15 @@ struct intel_crtc_state {
>  
>  	/* Output down scaling is done in LSPCON device */
>  	bool lspcon_downsampling;
> +
> +	/* Display Stream compression state */
> +	struct {
> +		bool compression_enable;
> +		bool dsc_split;
> +		u16 compressed_bpp;
> +		u8 slice_count;
> +	} dsc_params;
> +	struct drm_dsc_config dp_dsc_cfg;

Ville, Jani should this be defined as a pointer to struct drm_dsc_config?
struct drm_dsc_config *dp_dsc_cfg
since we populate this in intel_dp_compute_config and then on only read during
commit.

Manasi

>  };
>  
>  struct intel_crtc {
> -- 
> 2.18.0
>
Ville Syrjälä Oct. 31, 2018, 1:10 p.m. UTC | #2
On Tue, Oct 30, 2018 at 04:53:49PM -0700, Manasi Navare wrote:
> On Wed, Oct 24, 2018 at 03:28:24PM -0700, Manasi Navare wrote:
> > Basic DSC parameters and DSC configuration data needs to be computed
> > for each of the requested mode during atomic check. This is
> > required since for certain modes, valid DSC parameters and config
> > data might not be computed in which case compression cannot be
> > enabled for that mode.
> > For that reason we need to add these params and config structure
> > to the intel_crtc_state so that if valid this state information
> > can directly be used while enabling DSC in atomic commit.
> > 
> > v2:
> > * Rebase on drm-tip (Manasi)
> > 
> > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 1 +
> >  drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2d7761b8ac07..45fd7894722b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -53,6 +53,7 @@
> >  #include <drm/drm_auth.h>
> >  #include <drm/drm_cache.h>
> >  #include <drm/drm_util.h>
> > +#include <drm/drm_dsc.h>
> >  
> >  #include "i915_params.h"
> >  #include "i915_reg.h"
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 62c051098859..27d47950f438 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -931,6 +931,15 @@ struct intel_crtc_state {
> >  
> >  	/* Output down scaling is done in LSPCON device */
> >  	bool lspcon_downsampling;
> > +
> > +	/* Display Stream compression state */
> > +	struct {
> > +		bool compression_enable;
> > +		bool dsc_split;
> > +		u16 compressed_bpp;
> > +		u8 slice_count;
> > +	} dsc_params;
> > +	struct drm_dsc_config dp_dsc_cfg;
> 
> Ville, Jani should this be defined as a pointer to struct drm_dsc_config?
> struct drm_dsc_config *dp_dsc_cfg
> since we populate this in intel_dp_compute_config and then on only read during
> commit.

Pointer to where exactly? You need the memory for it somewhere.

> 
> Manasi
> 
> >  };
> >  
> >  struct intel_crtc {
> > -- 
> > 2.18.0
> >
Navare, Manasi Oct. 31, 2018, 4:05 p.m. UTC | #3
On Wed, Oct 31, 2018 at 03:10:15PM +0200, Ville Syrjälä wrote:
> On Tue, Oct 30, 2018 at 04:53:49PM -0700, Manasi Navare wrote:
> > On Wed, Oct 24, 2018 at 03:28:24PM -0700, Manasi Navare wrote:
> > > Basic DSC parameters and DSC configuration data needs to be computed
> > > for each of the requested mode during atomic check. This is
> > > required since for certain modes, valid DSC parameters and config
> > > data might not be computed in which case compression cannot be
> > > enabled for that mode.
> > > For that reason we need to add these params and config structure
> > > to the intel_crtc_state so that if valid this state information
> > > can directly be used while enabling DSC in atomic commit.
> > > 
> > > v2:
> > > * Rebase on drm-tip (Manasi)
> > > 
> > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  | 1 +
> > >  drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 2d7761b8ac07..45fd7894722b 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -53,6 +53,7 @@
> > >  #include <drm/drm_auth.h>
> > >  #include <drm/drm_cache.h>
> > >  #include <drm/drm_util.h>
> > > +#include <drm/drm_dsc.h>
> > >  
> > >  #include "i915_params.h"
> > >  #include "i915_reg.h"
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 62c051098859..27d47950f438 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -931,6 +931,15 @@ struct intel_crtc_state {
> > >  
> > >  	/* Output down scaling is done in LSPCON device */
> > >  	bool lspcon_downsampling;
> > > +
> > > +	/* Display Stream compression state */
> > > +	struct {
> > > +		bool compression_enable;
> > > +		bool dsc_split;
> > > +		u16 compressed_bpp;
> > > +		u8 slice_count;
> > > +	} dsc_params;
> > > +	struct drm_dsc_config dp_dsc_cfg;
> > 
> > Ville, Jani should this be defined as a pointer to struct drm_dsc_config?
> > struct drm_dsc_config *dp_dsc_cfg
> > since we populate this in intel_dp_compute_config and then on only read during
> > commit.
> 
> Pointer to where exactly? You need the memory for it somewhere.
>

struct drm_dsc_config  is defined in drm_dsc.h and gets populated later in the dp_compute_config.
So I guess wwe can leave it as strcut drm_dsc_config dp_dsc_cfg; in crtc_state?
Pass a pointer to that while populating in the compute-config() but then later in the commit
when we have to read from this to write the sdp infoframe, we just pass the struct directly, no 
need to pass a pointer since its gonna be read and also crtc_state will be a const

Is that correct?

Manasi
 
> > 
> > Manasi
> > 
> > >  };
> > >  
> > >  struct intel_crtc {
> > > -- 
> > > 2.18.0
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Oct. 31, 2018, 4:13 p.m. UTC | #4
On Wed, Oct 31, 2018 at 09:05:05AM -0700, Manasi Navare wrote:
> On Wed, Oct 31, 2018 at 03:10:15PM +0200, Ville Syrjälä wrote:
> > On Tue, Oct 30, 2018 at 04:53:49PM -0700, Manasi Navare wrote:
> > > On Wed, Oct 24, 2018 at 03:28:24PM -0700, Manasi Navare wrote:
> > > > Basic DSC parameters and DSC configuration data needs to be computed
> > > > for each of the requested mode during atomic check. This is
> > > > required since for certain modes, valid DSC parameters and config
> > > > data might not be computed in which case compression cannot be
> > > > enabled for that mode.
> > > > For that reason we need to add these params and config structure
> > > > to the intel_crtc_state so that if valid this state information
> > > > can directly be used while enabling DSC in atomic commit.
> > > > 
> > > > v2:
> > > > * Rebase on drm-tip (Manasi)
> > > > 
> > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h  | 1 +
> > > >  drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
> > > >  2 files changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 2d7761b8ac07..45fd7894722b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -53,6 +53,7 @@
> > > >  #include <drm/drm_auth.h>
> > > >  #include <drm/drm_cache.h>
> > > >  #include <drm/drm_util.h>
> > > > +#include <drm/drm_dsc.h>
> > > >  
> > > >  #include "i915_params.h"
> > > >  #include "i915_reg.h"
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 62c051098859..27d47950f438 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -931,6 +931,15 @@ struct intel_crtc_state {
> > > >  
> > > >  	/* Output down scaling is done in LSPCON device */
> > > >  	bool lspcon_downsampling;
> > > > +
> > > > +	/* Display Stream compression state */
> > > > +	struct {
> > > > +		bool compression_enable;
> > > > +		bool dsc_split;
> > > > +		u16 compressed_bpp;
> > > > +		u8 slice_count;
> > > > +	} dsc_params;
> > > > +	struct drm_dsc_config dp_dsc_cfg;
> > > 
> > > Ville, Jani should this be defined as a pointer to struct drm_dsc_config?
> > > struct drm_dsc_config *dp_dsc_cfg
> > > since we populate this in intel_dp_compute_config and then on only read during
> > > commit.
> > 
> > Pointer to where exactly? You need the memory for it somewhere.
> >
> 
> struct drm_dsc_config  is defined in drm_dsc.h and gets populated later in the dp_compute_config.
> So I guess wwe can leave it as strcut drm_dsc_config dp_dsc_cfg; in crtc_state?
> Pass a pointer to that while populating in the compute-config() but then later in the commit
> when we have to read from this to write the sdp infoframe, we just pass the struct directly, no 
> need to pass a pointer since its gonna be read and also crtc_state will be a const
> 
> Is that correct?

You don't want to be passing large structures by value, if that's what
you're suggesting.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2d7761b8ac07..45fd7894722b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -53,6 +53,7 @@ 
 #include <drm/drm_auth.h>
 #include <drm/drm_cache.h>
 #include <drm/drm_util.h>
+#include <drm/drm_dsc.h>
 
 #include "i915_params.h"
 #include "i915_reg.h"
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 62c051098859..27d47950f438 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -931,6 +931,15 @@  struct intel_crtc_state {
 
 	/* Output down scaling is done in LSPCON device */
 	bool lspcon_downsampling;
+
+	/* Display Stream compression state */
+	struct {
+		bool compression_enable;
+		bool dsc_split;
+		u16 compressed_bpp;
+		u8 slice_count;
+	} dsc_params;
+	struct drm_dsc_config dp_dsc_cfg;
 };
 
 struct intel_crtc {