| Message ID | 20240517145356.26103-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: DSC stuff | expand |
On Fri, 17 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Extract a helper to check whether the source+sink combo > supports DSC. That basic check is needed both during mode > validation and compute config. We'll also need to add extra > checks to both places, so having a single place for it is nicer. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 1e88449fe5f2..7bf283b4df7f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, > connector->force_bigjoiner_enable; > } > > +static bool intel_dp_has_dsc(struct intel_connector *connector) Why not const? > +{ > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + > + if (!HAS_DSC(i915)) > + return false; > + > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) > + return false; > + > + return true; > +} > + > static enum drm_mode_status > intel_dp_mode_valid(struct drm_connector *_connector, > struct drm_display_mode *mode) > @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, > mode_rate = intel_dp_link_required(target_clock, > intel_dp_mode_min_output_bpp(connector, mode)); > > - if (HAS_DSC(dev_priv) && > - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) { > + if (intel_dp_has_dsc(connector)) { > enum intel_output_format sink_format, output_format; > int pipe_bpp;
On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: > On Fri, 17 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Extract a helper to check whether the source+sink combo > > supports DSC. That basic check is needed both during mode > > validation and compute config. We'll also need to add extra > > checks to both places, so having a single place for it is nicer. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 1e88449fe5f2..7bf283b4df7f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, > > connector->force_bigjoiner_enable; > > } > > > > +static bool intel_dp_has_dsc(struct intel_connector *connector) > > Why not const? We've generally not consted these things. And then whenver add one const somewhere it usually ends up getting in the way later, not because we need mutability but simply because we want to call something that doesn't have the const. I suppose if we do want to start consting things more we should just do some kind of bigger pass over the whole codebase so that that there's less chance of pain later. We're also not using container_of_const() for these right now, so the const can vanish semi-accidentally when casting things. I suppose this thing might be low level enough that the const could be kept. I'll have another think about it. > > > +{ > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + > > + if (!HAS_DSC(i915)) > > + return false; > > + > > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) > > + return false; > > + > > + return true; > > +} > > + > > static enum drm_mode_status > > intel_dp_mode_valid(struct drm_connector *_connector, > > struct drm_display_mode *mode) > > @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, > > mode_rate = intel_dp_link_required(target_clock, > > intel_dp_mode_min_output_bpp(connector, mode)); > > > > - if (HAS_DSC(dev_priv) && > > - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) { > > + if (intel_dp_has_dsc(connector)) { > > enum intel_output_format sink_format, output_format; > > int pipe_bpp; > > -- > Jani Nikula, Intel
On Mon, 20 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: >> On Fri, 17 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Extract a helper to check whether the source+sink combo >> > supports DSC. That basic check is needed both during mode >> > validation and compute config. We'll also need to add extra >> > checks to both places, so having a single place for it is nicer. >> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++++-- >> > 1 file changed, 14 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> > index 1e88449fe5f2..7bf283b4df7f 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, >> > connector->force_bigjoiner_enable; >> > } >> > >> > +static bool intel_dp_has_dsc(struct intel_connector *connector) >> >> Why not const? > > We've generally not consted these things. And then whenver add > one const somewhere it usually ends up getting in the way later, > not because we need mutability but simply because we want to > call something that doesn't have the const. > > I suppose if we do want to start consting things more we should > just do some kind of bigger pass over the whole codebase so that > that there's less chance of pain later. > > We're also not using container_of_const() for these right now, > so the const can vanish semi-accidentally when casting things. > > I suppose this thing might be low level enough that the const > could be kept. I'll have another think about it. It's just that this series drops a bunch of const because of this, which feels like the opposite of what you usually do. :) BR, Jani. > >> >> > +{ >> > + struct drm_i915_private *i915 = to_i915(connector->base.dev); >> > + >> > + if (!HAS_DSC(i915)) >> > + return false; >> > + >> > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) >> > + return false; >> > + >> > + return true; >> > +} >> > + >> > static enum drm_mode_status >> > intel_dp_mode_valid(struct drm_connector *_connector, >> > struct drm_display_mode *mode) >> > @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, >> > mode_rate = intel_dp_link_required(target_clock, >> > intel_dp_mode_min_output_bpp(connector, mode)); >> > >> > - if (HAS_DSC(dev_priv) && >> > - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) { >> > + if (intel_dp_has_dsc(connector)) { >> > enum intel_output_format sink_format, output_format; >> > int pipe_bpp; >> >> -- >> Jani Nikula, Intel
On Tue, May 21, 2024 at 12:51:03PM +0300, Jani Nikula wrote: > On Mon, 20 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: > >> On Fri, 17 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > >> > Extract a helper to check whether the source+sink combo > >> > supports DSC. That basic check is needed both during mode > >> > validation and compute config. We'll also need to add extra > >> > checks to both places, so having a single place for it is nicer. > >> > > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++++-- > >> > 1 file changed, 14 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index 1e88449fe5f2..7bf283b4df7f 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, > >> > connector->force_bigjoiner_enable; > >> > } > >> > > >> > +static bool intel_dp_has_dsc(struct intel_connector *connector) > >> > >> Why not const? > > > > We've generally not consted these things. And then whenver add > > one const somewhere it usually ends up getting in the way later, > > not because we need mutability but simply because we want to > > call something that doesn't have the const. > > > > I suppose if we do want to start consting things more we should > > just do some kind of bigger pass over the whole codebase so that > > that there's less chance of pain later. > > > > We're also not using container_of_const() for these right now, > > so the const can vanish semi-accidentally when casting things. > > > > I suppose this thing might be low level enough that the const > > could be kept. I'll have another think about it. > > It's just that this series drops a bunch of const because of this, which > feels like the opposite of what you usually do. :) I suppose. My current rule of thumb is: - atomic object states and fbs should be const if possible - everything else is not I wouldn't mind making more things const, but I suspect there are several sizeable rabbit holes that need to be dug out beforehand.
On Tue, 21 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, May 21, 2024 at 12:51:03PM +0300, Jani Nikula wrote: >> On Mon, 20 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: >> >> On Fri, 17 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> > >> >> > Extract a helper to check whether the source+sink combo >> >> > supports DSC. That basic check is needed both during mode >> >> > validation and compute config. We'll also need to add extra >> >> > checks to both places, so having a single place for it is nicer. >> >> > >> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++++-- >> >> > 1 file changed, 14 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> >> > index 1e88449fe5f2..7bf283b4df7f 100644 >> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> >> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, >> >> > connector->force_bigjoiner_enable; >> >> > } >> >> > >> >> > +static bool intel_dp_has_dsc(struct intel_connector *connector) >> >> >> >> Why not const? >> > >> > We've generally not consted these things. And then whenver add >> > one const somewhere it usually ends up getting in the way later, >> > not because we need mutability but simply because we want to >> > call something that doesn't have the const. >> > >> > I suppose if we do want to start consting things more we should >> > just do some kind of bigger pass over the whole codebase so that >> > that there's less chance of pain later. >> > >> > We're also not using container_of_const() for these right now, >> > so the const can vanish semi-accidentally when casting things. >> > >> > I suppose this thing might be low level enough that the const >> > could be kept. I'll have another think about it. >> >> It's just that this series drops a bunch of const because of this, which >> feels like the opposite of what you usually do. :) > > I suppose. > > My current rule of thumb is: > - atomic object states and fbs should be const if possible > - everything else is not > > I wouldn't mind making more things const, but I suspect > there are several sizeable rabbit holes that need to be > dug out beforehand. Fair enough. Like I said, the series is R-b. J.
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 1e88449fe5f2..7bf283b4df7f 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp, connector->force_bigjoiner_enable; } +static bool intel_dp_has_dsc(struct intel_connector *connector) +{ + struct drm_i915_private *i915 = to_i915(connector->base.dev); + + if (!HAS_DSC(i915)) + return false; + + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) + return false; + + return true; +} + static enum drm_mode_status intel_dp_mode_valid(struct drm_connector *_connector, struct drm_display_mode *mode) @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, mode_rate = intel_dp_link_required(target_clock, intel_dp_mode_min_output_bpp(connector, mode)); - if (HAS_DSC(dev_priv) && - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) { + if (intel_dp_has_dsc(connector)) { enum intel_output_format sink_format, output_format; int pipe_bpp;