On Fri, Nov 04, 2016 at 03:09:04PM -0200, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 16:28 +0530, Kumar, Mahesh escreveu:
> > This patch implemnets Workariunds related to display arbitrated
> > memory
> > bandwidth. These WA are applicabe for all gen-9 based platforms.
> >
> > Changes since v1:
> > - Rebase on top of Paulo's patch series
> > Changes since v2:
> > - Rebase/rework after addressing Paulo's comments in previous patch
>
> A lot of this code has changed since then, so this will need a
> significant rebase. In the meantime, I added skl_needs_memory_bw_wa()
> and we're now applying the WA by default: we just won't apply the WA
> when we're pretty sure we don't need to. This helps avoiding underruns
> by default.
>
> See more below.
>
>
> >
> > Signed-off-by: "Kumar, Mahesh" <mahesh1.kumar@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 9 +++
> > drivers/gpu/drm/i915/intel_drv.h | 11 +++
> > drivers/gpu/drm/i915/intel_pm.c | 146
> > +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 166 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index adbd9aa..c169360 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1092,6 +1092,13 @@ enum intel_sbi_destination {
> > SBI_MPHY,
> > };
> >
> > +/* SKL+ Watermark arbitrated display bandwidth Workarounds */
> > +enum watermark_memory_wa {
> > + WATERMARK_WA_NONE,
> > + WATERMARK_WA_X_TILED,
> > + WATERMARK_WA_Y_TILED,
> > +};
> > +
> > #define QUIRK_PIPEA_FORCE (1<<0)
> > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
> > #define QUIRK_INVERT_BRIGHTNESS (1<<2)
> > @@ -1644,6 +1651,8 @@ struct skl_ddb_allocation {
> >
> > struct skl_wm_values {
> > unsigned dirty_pipes;
> > + /* any WaterMark memory workaround Required */
>
> We can remove this comment since it doesn't say anything the variable
> name doesn't.
>
> > + enum watermark_memory_wa mem_wa;
>
> Now that we have a proper variable in the state struct, it probably
> makes sense to just kill skl_needs_memory_bw_wa() and read this
> variable when we need to.
>
>
> > struct skl_ddb_allocation ddb;
> > uint32_t wm_linetime[I915_MAX_PIPES];
> > uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index f48e79a..2c1897b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1813,6 +1813,17 @@ intel_atomic_get_crtc_state(struct
> > drm_atomic_state *state,
> > return to_intel_crtc_state(crtc_state);
> > }
> >
> > +static inline struct intel_crtc_state *
> > +intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
> > + struct intel_crtc *crtc)
> > +{
> > + struct drm_crtc_state *crtc_state;
> > +
> > + crtc_state = drm_atomic_get_existing_crtc_state(state,
> > &crtc->base);
> > +
> > + return to_intel_crtc_state(crtc_state);
>
> I really don't like the idea of calling to_intel_crtc_state() on a
> potentially NULL pointer so the caller of this function will also check
> for NULL. Even though it works today, I still think it's unsafe
> practice. Please check crtc_state for NULL directly and then return
> NULL.
I want to make this safe by making it a compile error if offsetof(base) != 0.
https://lists.freedesktop.org/archives/intel-gfx/2016-October/108175.html
But I think we want to go further than that patch by adding a bit more
type safety to things. I did play around with this stuff a bit more,
and I have something sitting on a branch, but I didn't quite figure out
what I want to do about const vs. non const yet.
>
> Also, I think this function should be extracted to its own commit, and
> we'd probably be able to find some callers in the existing i915 code.
I have, on some branch again, _intel_ versions of the for_each_foo_in_state()
macros as well. I think those are going to allow a lot of ugly casting
stuff to disappear. But I think I'll hold off until Maarten's new
iterators go in before I try to send those out.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
