Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On Tue, Feb 20, 2024 at 11:27:18AM -0800, Abhinav Kumar wrote: > > > On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote: > > On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov > > wrote: > >> > >> On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar > >> wrote: > >>> > >>> > >>> > >>> On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote: > >>>> On Thu, 15 Feb 2024 at 21:08, Abhinav Kumar > >>>> wrote: > >>>>> > >>>>> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. > >>>>> Lets move this to drm_dp_helper to achieve this. > >>>>> > >>>>> changes in v2: > >>>>> - rebased on top of drm-tip > >>>>> > >>>>> Acked-by: Dmitry Baryshkov > >>>> > >>>> v1 had an explicit comment before the ack: > >>>> > >>> > >>> Yes, I remember the comment. I did not make any changes to v2 other than > >>> just rebasing it on drm-tip to get the ack from i915 folks. > >>> > >>>>> From my side, with the promise of the size fixup. > >>>> > >>>> However I observe neither a second patch removing the size argument > >>>> nor it being dropped as a part of this patch. > >>>> > >>> > >>> Yes, now that in v2 we got the ack for this patch, I can spin a v3 with > >>> the addition of the next patch to remove the size OR as another series > >>> so as to not block the main series which needs this patch. > >>> > >>> I would prefer the latter. > >> > >> It doesn't work this way. The comment should have been fixed for v2. > > > > This probably deserves some explanation. Currently there is only one > > user of this function. So it is easy to fix it. Once there are several > > users, you have to fix all of them at the same time, patching > > different drm subtrees. That complicates the life of maintainers. > > > > Yes, understood. Its easier to fix it now if its really needed. > > Actually, I think the reason the size was passed was to make sure > a valid struct dp_sdp *sdp was being passed. The size is supposed to be the size of *hardware* buffer where this gets written into. But looks like this wasn't done correctly when the code was copy-pasted from the HDMI inforframe code. -- Ville Syrjälä Intel
Re: [PATCH v3 01/12] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state
On Wed, Feb 14, 2024 at 09:17:06PM +0200, Dmitry Baryshkov wrote: > On Wed, 14 Feb 2024 at 20:47, Ville Syrjälä > wrote: > > > > On Wed, Feb 14, 2024 at 08:37:02PM +0200, Ville Syrjälä wrote: > > > On Thu, Sep 14, 2023 at 08:06:55AM +0300, Dmitry Baryshkov wrote: > > > > The helper drm_atomic_helper_check_plane_state() runs several checks on > > > > plane src and dst rectangles, including the check whether required > > > > scaling fits into the required margins. The msm driver would benefit > > > > from having a function that does all these checks except the scaling > > > > one. Split them into a new helper called > > > > drm_atomic_helper_check_plane_noscale(). > > > > > > What's the point in eliminating a nop scaling check? > > > > Actually, what are you even doing in there? Are you saying that > > the hardware has absolutely no limits on how much it can scale > > in either direction? > > No, I'm just saying that the scaling ability depends on the rotation > and other plane properties. So I had to separate the basic plane > checks and the scaling check. > Basic (noscale) plane check source and destination rectangles, etc. > After that the driver identifies possible hardware pipe usage and > after that it can perform a scaling check. Hmm. We have sport of similar situation in i915 where we pick a scaler much later and so don't know the exact scaling limits at the time when we do this check. But we opted to pass the lower/upper bounds of the scaling limits instead. That will guarantee that at least completely illegal values are rejected as early as possible, and so we don't have to worry about running into them later on. -- Ville Syrjälä Intel
Re: [PATCH v3 01/12] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state
On Wed, Feb 14, 2024 at 08:37:02PM +0200, Ville Syrjälä wrote:
> On Thu, Sep 14, 2023 at 08:06:55AM +0300, Dmitry Baryshkov wrote:
> > The helper drm_atomic_helper_check_plane_state() runs several checks on
> > plane src and dst rectangles, including the check whether required
> > scaling fits into the required margins. The msm driver would benefit
> > from having a function that does all these checks except the scaling
> > one. Split them into a new helper called
> > drm_atomic_helper_check_plane_noscale().
>
> What's the point in eliminating a nop scaling check?
Actually, what are you even doing in there? Are you saying that
the hardware has absolutely no limits on how much it can scale
in either direction?
>
> >
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 110 ++--
> > include/drm/drm_atomic_helper.h | 7 ++
> > 2 files changed, 96 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 292e38eb6218..2d7dd66181c9 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -825,11 +825,9 @@ drm_atomic_helper_check_wb_encoder_state(struct
> > drm_encoder *encoder,
> > EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> >
> > /**
> > - * drm_atomic_helper_check_plane_state() - Check plane state for validity
> > + * drm_atomic_helper_check_plane_noscale() - Check plane state for validity
> > * @plane_state: plane state to check
> > * @crtc_state: CRTC state to check
> > - * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> > - * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> > * @can_position: is it legal to position the plane such that it
> > *doesn't cover the entire CRTC? This will generally
> > *only be false for primary planes.
> > @@ -845,19 +843,16 @@
> > EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
> > * RETURNS:
> > * Zero if update appears valid, error code on failure
> > */
> > -int drm_atomic_helper_check_plane_state(struct drm_plane_state
> > *plane_state,
> > - const struct drm_crtc_state *crtc_state,
> > - int min_scale,
> > - int max_scale,
> > - bool can_position,
> > - bool can_update_disabled)
> > +int drm_atomic_helper_check_plane_noscale(struct drm_plane_state
> > *plane_state,
> > + const struct drm_crtc_state
> > *crtc_state,
> > + bool can_position,
> > + bool can_update_disabled)
> > {
> > struct drm_framebuffer *fb = plane_state->fb;
> > struct drm_rect *src = _state->src;
> > struct drm_rect *dst = _state->dst;
> > unsigned int rotation = plane_state->rotation;
> > struct drm_rect clip = {};
> > - int hscale, vscale;
> >
> > WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state->crtc);
> >
> > @@ -883,17 +878,6 @@ int drm_atomic_helper_check_plane_state(struct
> > drm_plane_state *plane_state,
> >
> > drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
> >
> > - /* Check scaling */
> > - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> > - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> > - if (hscale < 0 || vscale < 0) {
> > - drm_dbg_kms(plane_state->plane->dev,
> > - "Invalid scaling of plane\n");
> > - drm_rect_debug_print("src: ", _state->src, true);
> > - drm_rect_debug_print("dst: ", _state->dst, false);
> > - return -ERANGE;
> > - }
> > -
> > if (crtc_state->enable)
> > drm_mode_get_hv_timing(_state->mode, , );
> >
> > @@ -921,6 +905,90 @@ int drm_atomic_helper_check_plane_state(struct
> > drm_plane_state *plane_state,
> >
> > return 0;
> > }
> > +EXPORT_SYMBOL(drm_atomic_helper_check_plane_noscale);
> > +
> > +/**
> > + * drm_atomic_helper_check_plane_scale() - Check whether plane can be
> > scaled
> > + * @plane_state: plane st
Re: [PATCH v3 01/12] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state
_state *plane_state,
> + int min_scale,
> + int max_scale)
> +{
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct drm_rect src;
> + struct drm_rect dst;
> + int hscale, vscale;
> +
> + if (!plane_state->visible)
> + return 0;
> +
> + src = drm_plane_state_src(plane_state);
> + dst = drm_plane_state_dest(plane_state);
> +
> + drm_rect_rotate(, fb->width << 16, fb->height << 16,
> plane_state->rotation);
> +
> + hscale = drm_rect_calc_hscale(, , min_scale, max_scale);
> + vscale = drm_rect_calc_vscale(, , min_scale, max_scale);
> + if (hscale < 0 || vscale < 0) {
> + drm_dbg_kms(plane_state->plane->dev,
> + "Invalid scaling of plane\n");
> + drm_rect_debug_print("src: ", _state->src, true);
> + drm_rect_debug_print("dst: ", _state->dst, false);
> + return -ERANGE;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_plane_scale);
> +
> +/**
> + * drm_atomic_helper_check_plane_state() - Check plane state for validity
> + * @plane_state: plane state to check
> + * @crtc_state: CRTC state to check
> + * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
> + * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
> + * @can_position: is it legal to position the plane such that it
> + *doesn't cover the entire CRTC? This will generally
> + *only be false for primary planes.
> + * @can_update_disabled: can the plane be updated while the CRTC
> + * is disabled?
> + *
> + * Checks that a desired plane update is valid, and updates various
> + * bits of derived state (clipped coordinates etc.). Drivers that provide
> + * their own plane handling rather than helper-provided implementations may
> + * still wish to call this function to avoid duplication of error checking
> + * code.
> + *
> + * RETURNS:
> + * Zero if update appears valid, error code on failure
> + */
> +int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> + const struct drm_crtc_state *crtc_state,
> + int min_scale,
> + int max_scale,
> + bool can_position,
> + bool can_update_disabled)
> +{
> + int ret;
> +
> + ret = drm_atomic_helper_check_plane_noscale(plane_state, crtc_state,
> can_position, can_update_disabled);
> + if (ret < 0)
> + return ret;
> +
> + return drm_atomic_helper_check_plane_scale(plane_state, min_scale,
> max_scale);
> +}
> EXPORT_SYMBOL(drm_atomic_helper_check_plane_state);
>
> /**
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 536a0b0091c3..32ac55aea94e 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -52,6 +52,13 @@ int drm_atomic_helper_check_modeset(struct drm_device *dev,
> int
> drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>struct drm_connector_state
> *conn_state);
> +int drm_atomic_helper_check_plane_noscale(struct drm_plane_state
> *plane_state,
> + const struct drm_crtc_state
> *crtc_state,
> + bool can_position,
> + bool can_update_disabled);
> +int drm_atomic_helper_check_plane_scale(struct drm_plane_state *plane_state,
> + int min_scale,
> + int max_scale);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> const struct drm_crtc_state *crtc_state,
> int min_scale,
> --
> 2.39.2
--
Ville Syrjälä
Intel
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
gt; - sdp->db[17] = 0x3;
> >> - break;
> >> - case 16:
> >> - sdp->db[17] = 0x4;
> >> - break;
> >> - default:
> >> - MISSING_CASE(vsc->bpc);
> >> - break;
> >> - }
> >> - /* Dynamic Range and Component Bit Depth */
> >> - if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> >> - sdp->db[17] |= 0x80; /* DB17[7] */
> >> -
> >> - /* Content Type */
> >> - sdp->db[18] = vsc->content_type & 0x7;
> >> -
> >> -out:
> >> - return length;
> >> -}
> >> -
> >> static ssize_t
> >> intel_dp_hdr_metadata_infoframe_sdp_pack(struct drm_i915_private *i915,
> >> const struct hdmi_drm_infoframe
> >> *drm_infoframe,
> >> @@ -4269,8 +4202,8 @@ static void intel_write_dp_sdp(struct intel_encoder
> >> *encoder,
> >>
> >> switch (type) {
> >> case DP_SDP_VSC:
> >> - len = intel_dp_vsc_sdp_pack(_state->infoframes.vsc,
> >> ,
> >> - sizeof(sdp));
> >> + len = drm_dp_vsc_sdp_pack(_state->infoframes.vsc,
> >> ,
> >> + sizeof(sdp));
> >> break;
> >> case HDMI_PACKET_TYPE_GAMUT_METADATA:
> >> len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
> >> @@ -4297,7 +4230,7 @@ void intel_write_dp_vsc_sdp(struct intel_encoder
> >> *encoder,
> >> struct dp_sdp sdp = {};
> >> ssize_t len;
> >>
> >> - len = intel_dp_vsc_sdp_pack(vsc, , sizeof(sdp));
> >> + len = drm_dp_vsc_sdp_pack(vsc, , sizeof(sdp));
> >>
> >> if (drm_WARN_ON(_priv->drm, len < 0))
> >> return;
> >> diff --git a/include/drm/display/drm_dp_helper.h
> >> b/include/drm/display/drm_dp_helper.h
> >> index 863b2e7add29..f8db34a2f7a5 100644
> >> --- a/include/drm/display/drm_dp_helper.h
> >> +++ b/include/drm/display/drm_dp_helper.h
> >> @@ -813,4 +813,7 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> >> int bpp_x16, unsigned long flags);
> >> int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
> >>
> >> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> >> + struct dp_sdp *sdp, size_t size);
> >> +
> >> #endif /* _DRM_DP_HELPER_H_ */
> >> --
> >> 2.34.1
> >>
> >
> >
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH v3 2/2] drm/probe_helper: sort out poll_running vs poll_enabled
ng = true;
> }
> EXPORT_SYMBOL(drm_kms_helper_poll_enable);
>
> @@ -592,10 +595,7 @@ int drm_helper_probe_single_connector_modes(struct
> drm_connector *connector,
> }
>
> /* Re-enable polling in case the global poll config changed. */
> - if (drm_kms_helper_poll != dev->mode_config.poll_running)
> - drm_kms_helper_poll_enable(dev);
> -
> - dev->mode_config.poll_running = drm_kms_helper_poll;
> + drm_kms_helper_poll_enable(dev);
>
> if (connector->status == connector_status_disconnected) {
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
> @@ -735,8 +735,11 @@ static void output_poll_execute(struct work_struct *work)
> changed = dev->mode_config.delayed_event;
> dev->mode_config.delayed_event = false;
>
> - if (!drm_kms_helper_poll)
> + if (!drm_kms_helper_poll && dev->mode_config.poll_running) {
> + drm_kms_helper_disable_hpd(dev);
> + dev->mode_config.poll_running = false;
> goto out;
> + }
>
> if (!mutex_trylock(>mode_config.mutex)) {
> repoll = true;
> @@ -833,19 +836,6 @@ bool drm_kms_helper_is_poll_worker(void)
> }
> EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
>
> -static void drm_kms_helper_poll_disable_fini(struct drm_device *dev, bool
> fini)
> -{
> - if (!dev->mode_config.poll_enabled)
> - return;
> -
> - if (fini)
> - dev->mode_config.poll_enabled = false;
> -
> - drm_kms_helper_disable_hpd(dev);
> -
> - cancel_delayed_work_sync(>mode_config.output_poll_work);
> -}
> -
> /**
> * drm_kms_helper_poll_disable - disable output polling
> * @dev: drm_device
> @@ -862,7 +852,12 @@ static void drm_kms_helper_poll_disable_fini(struct
> drm_device *dev, bool fini)
> */
> void drm_kms_helper_poll_disable(struct drm_device *dev)
> {
> - drm_kms_helper_poll_disable_fini(dev, false);
> + if (dev->mode_config.poll_running)
> + drm_kms_helper_disable_hpd(dev);
> +
> + cancel_delayed_work_sync(>mode_config.output_poll_work);
> +
> + dev->mode_config.poll_running = false;
> }
> EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>
> @@ -900,7 +895,12 @@ EXPORT_SYMBOL(drm_kms_helper_poll_init);
> */
> void drm_kms_helper_poll_fini(struct drm_device *dev)
> {
> - drm_kms_helper_poll_disable_fini(dev, true);
> + if (!dev->mode_config.poll_enabled)
> + return;
> +
> + drm_kms_helper_poll_disable(dev);
> +
> + dev->mode_config.poll_enabled = false;
> }
> EXPORT_SYMBOL(drm_kms_helper_poll_fini);
>
> --
> 2.39.0
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v9 1/8] drm/display/dsc: Add flatness and initial scale value calculations
On Wed, May 10, 2023 at 03:54:41PM -0700, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
>
> Signed-off-by: Jessica Zhang
> Signed-off-by: Dmitry Baryshkov
> Reviewed-by: Marijn Suijten
> Signed-off-by: Jessica Zhang
> ---
> include/drm/display/drm_dsc_helper.h | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 0bb0c3afd740..422135a33d65 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -25,5 +25,16 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config
> *vdsc_cfg);
> int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_kind kind);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> +static inline void drm_dsc_set_initial_scale_value(struct drm_dsc_config
> *dsc)
> +{
> + dsc->initial_scale_value = 8 * dsc->rc_model_size /
> + (dsc->rc_model_size - dsc->initial_offset);
> +}
I would suggest using pure functions whenever possible. Makes it much
easier to reason about the code when you know there are no side effects.
> +
> +static inline int drm_dsc_calculate_flatness_det_thresh(struct
> drm_dsc_config *dsc)
'dsc' can be const. The word "calculate" seems pretty much redundant.
> +{
> + return 2 << (dsc->bits_per_component - 8);
> +}
> +
> #endif /* _DRM_DSC_HELPER_H_ */
>
>
> --
> 2.40.1
--
Ville Syrjälä
Intel
Re: [Freedreno] [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
On Wed, Mar 08, 2023 at 03:33:48PM -0800, Rob Clark wrote: > On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä > wrote: > > > > On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: > > > For DRM property blobs created by user mode using > > > drm_property_create_blob(), if the blob value needs to be updated the > > > only way is to destroy the previous blob and create a new one instead. > > > > > > For some of the property blobs, if the size of the blob is more > > > than one page size, kvzalloc() can slow down system as it will first > > > try to allocate physically contiguous memory but upon failure will > > > fall back to non-contiguous (vmalloc) allocation. > > > > > > If the blob property being used is bigger than one page size, in a > > > heavily loaded system, this causes performance issues because > > > some of the blobs are updated on a per-frame basis. > > > > > > To mitigate the performance impact of kvzalloc(), use it only when > > > the size of allocation is less than a page size when creating property > > > blobs > > > > Not sure how badly this will eat into the vmalloc area. > > Normally I wouldn't expect this to be much of a problem, but we don't > appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it > might have been a mistake.. so perhaps we want to either restrict > CREATEBLOBPROP or put an upper threshold limit on total size of all > allocated blob props using vmalloc area? Surprisingly few kms ioctls are master-only it seems. Dunno what the use case for all those being non-master really is. I think blob limits in general were disussed at at various points in the past with no conclusion. I guess it's slightly problematic in that if you limit individual max blob size then they just create more smaller ones. If you limit the total size per fd they just open more fds. If you put a total upper limit then it's just a slightly quicker DoS than without the limit. Shrug. > > BR, > -R > > > Is there no GFP flag to avoid the expensive stuff instead? > > > > > > > > Signed-off-by: Abhinav Kumar > > > --- > > > drivers/gpu/drm/drm_property.c | 6 +- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_property.c > > > b/drivers/gpu/drm/drm_property.c > > > index dfec479830e4..40c2a3142038 100644 > > > --- a/drivers/gpu/drm/drm_property.c > > > +++ b/drivers/gpu/drm/drm_property.c > > > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, > > > size_t length, > > > if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) > > > return ERR_PTR(-EINVAL); > > > > > > - blob = kvzalloc(sizeof(struct drm_property_blob)+length, > > > GFP_KERNEL); > > > + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) > > > + blob = vzalloc(sizeof(struct drm_property_blob)+length); > > > + else > > > + blob = kvzalloc(sizeof(struct drm_property_blob)+length, > > > GFP_KERNEL); > > > + > > > if (!blob) > > > return ERR_PTR(-ENOMEM); > > > > > > -- > > > 2.7.4 > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel
Re: [Freedreno] [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: > For DRM property blobs created by user mode using > drm_property_create_blob(), if the blob value needs to be updated the > only way is to destroy the previous blob and create a new one instead. > > For some of the property blobs, if the size of the blob is more > than one page size, kvzalloc() can slow down system as it will first > try to allocate physically contiguous memory but upon failure will > fall back to non-contiguous (vmalloc) allocation. > > If the blob property being used is bigger than one page size, in a > heavily loaded system, this causes performance issues because > some of the blobs are updated on a per-frame basis. > > To mitigate the performance impact of kvzalloc(), use it only when > the size of allocation is less than a page size when creating property > blobs Not sure how badly this will eat into the vmalloc area. Is there no GFP flag to avoid the expensive stuff instead? > > Signed-off-by: Abhinav Kumar > --- > drivers/gpu/drm/drm_property.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index dfec479830e4..40c2a3142038 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t > length, > if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) > return ERR_PTR(-EINVAL); > > - blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); > + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) > + blob = vzalloc(sizeof(struct drm_property_blob)+length); > + else > + blob = kvzalloc(sizeof(struct drm_property_blob)+length, > GFP_KERNEL); > + > if (!blob) > return ERR_PTR(-ENOMEM); > > -- > 2.7.4 -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v9 11/15] drm/atomic-helper: Set fence deadline for vblank
On Fri, Mar 03, 2023 at 07:45:05AM -0800, Rob Clark wrote:
> On Fri, Mar 3, 2023 at 7:12 AM Ville Syrjälä
> wrote:
> >
> > On Thu, Mar 02, 2023 at 03:53:33PM -0800, Rob Clark wrote:
> > > From: Rob Clark
> > >
> > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate
> > > the next vblank time, and inform the fence(s) of that deadline.
> > >
> > > v2: Comment typo fix (danvet)
> > >
> > > Signed-off-by: Rob Clark
> > > Reviewed-by: Daniel Vetter
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 36 +
> > > 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index d579fd8f7cb8..d8ee98ce2fc5 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1511,6 +1511,40 @@ void
> > > drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > > }
> > > EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
> > >
> > > +/*
> > > + * For atomic updates which touch just a single CRTC, calculate the time
> > > of the
> > > + * next vblank, and inform all the fences of the deadline.
> > > + */
> > > +static void set_fence_deadline(struct drm_device *dev,
> > > +struct drm_atomic_state *state)
> > > +{
> > > + struct drm_crtc *crtc, *wait_crtc = NULL;
> > > + struct drm_crtc_state *new_crtc_state;
> > > + struct drm_plane *plane;
> > > + struct drm_plane_state *new_plane_state;
> > > + ktime_t vbltime;
> > > + int i;
> > > +
> > > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> > > + if (wait_crtc)
> > > + return;
> > > + wait_crtc = crtc;
> > > + }
> > > +
> > > + /* If no CRTCs updated, then nothing to do: */
> > > + if (!wait_crtc)
> > > + return;
> >
> > Is there an actual point in limiting this to single crtc updates?
> > That immediately excludes tiled displays/etc.
> >
> > Handling an arbitrary number of crtcs shouldn't really be a lot
> > more complicated should it?
>
> I guess I could find the soonest upcoming vblank of all the CRTCs and
> use that as the deadline?
Yeah, that seems reasonable. The flips are supposed to happen
atomically (if possible) anyway so collapsing the thing to
a single deadline for all makes sense to me.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v9 15/15] drm/i915: Add deadline based boost support
On Fri, Mar 03, 2023 at 05:00:03PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 03, 2023 at 06:48:43AM -0800, Rob Clark wrote:
> > On Fri, Mar 3, 2023 at 1:58 AM Tvrtko Ursulin
> > wrote:
> > >
> > >
> > > On 03/03/2023 03:21, Rodrigo Vivi wrote:
> > > > On Thu, Mar 02, 2023 at 03:53:37PM -0800, Rob Clark wrote:
> > > >> From: Rob Clark
> > > >>
> > > >
> > > > missing some wording here...
> > > >
> > > >> v2: rebase
> > > >>
> > > >> Signed-off-by: Rob Clark
> > > >> ---
> > > >> drivers/gpu/drm/i915/i915_request.c | 20
> > > >> 1 file changed, 20 insertions(+)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/i915/i915_request.c
> > > >> b/drivers/gpu/drm/i915/i915_request.c
> > > >> index 7503dcb9043b..44491e7e214c 100644
> > > >> --- a/drivers/gpu/drm/i915/i915_request.c
> > > >> +++ b/drivers/gpu/drm/i915/i915_request.c
> > > >> @@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct
> > > >> dma_fence *fence)
> > > >> return i915_request_enable_breadcrumb(to_request(fence));
> > > >> }
> > > >>
> > > >> +static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t
> > > >> deadline)
> > > >> +{
> > > >> +struct i915_request *rq = to_request(fence);
> > > >> +
> > > >> +if (i915_request_completed(rq))
> > > >> +return;
> > > >> +
> > > >> +if (i915_request_started(rq))
> > > >> +return;
> > > >
> > > > why do we skip the boost if already started?
> > > > don't we want to boost the freq anyway?
> > >
> > > I'd wager Rob is just copying the current i915 wait boost logic.
> >
> > Yup, and probably incorrectly.. Matt reported fewer boosts/sec
> > compared to your RFC, this could be the bug
>
> I don't think i915 calls drm_atomic_helper_wait_for_fences()
> so that could explain something.
Oh, I guess this wasn't even supposed to take over the current
display boost stuff since you didn't remove the old one.
The current one just boosts after a missed vblank. The deadline
could use your timer approach I suppose and boost already a bit
earlier in the hopes of not missing the vblank.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v9 11/15] drm/atomic-helper: Set fence deadline for vblank
On Thu, Mar 02, 2023 at 03:53:33PM -0800, Rob Clark wrote:
> From: Rob Clark
>
> For an atomic commit updating a single CRTC (ie. a pageflip) calculate
> the next vblank time, and inform the fence(s) of that deadline.
>
> v2: Comment typo fix (danvet)
>
> Signed-off-by: Rob Clark
> Reviewed-by: Daniel Vetter
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 36 +
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index d579fd8f7cb8..d8ee98ce2fc5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1511,6 +1511,40 @@ void drm_atomic_helper_commit_modeset_enables(struct
> drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>
> +/*
> + * For atomic updates which touch just a single CRTC, calculate the time of
> the
> + * next vblank, and inform all the fences of the deadline.
> + */
> +static void set_fence_deadline(struct drm_device *dev,
> +struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc, *wait_crtc = NULL;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *new_plane_state;
> + ktime_t vbltime;
> + int i;
> +
> + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
> + if (wait_crtc)
> + return;
> + wait_crtc = crtc;
> + }
> +
> + /* If no CRTCs updated, then nothing to do: */
> + if (!wait_crtc)
> + return;
Is there an actual point in limiting this to single crtc updates?
That immediately excludes tiled displays/etc.
Handling an arbitrary number of crtcs shouldn't really be a lot
more complicated should it?
> +
> + if (drm_crtc_next_vblank_start(wait_crtc, ))
> + return;
> +
> + for_each_new_plane_in_state (state, plane, new_plane_state, i) {
> + if (!new_plane_state->fence)
> + continue;
> + dma_fence_set_deadline(new_plane_state->fence, vbltime);
> + }
> +}
> +
> /**
> * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
> * @dev: DRM device
> @@ -1540,6 +1574,8 @@ int drm_atomic_helper_wait_for_fences(struct drm_device
> *dev,
> struct drm_plane_state *new_plane_state;
> int i, ret;
>
> + set_fence_deadline(dev, state);
> +
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> if (!new_plane_state->fence)
> continue;
> --
> 2.39.1
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v9 15/15] drm/i915: Add deadline based boost support
On Fri, Mar 03, 2023 at 06:48:43AM -0800, Rob Clark wrote:
> On Fri, Mar 3, 2023 at 1:58 AM Tvrtko Ursulin
> wrote:
> >
> >
> > On 03/03/2023 03:21, Rodrigo Vivi wrote:
> > > On Thu, Mar 02, 2023 at 03:53:37PM -0800, Rob Clark wrote:
> > >> From: Rob Clark
> > >>
> > >
> > > missing some wording here...
> > >
> > >> v2: rebase
> > >>
> > >> Signed-off-by: Rob Clark
> > >> ---
> > >> drivers/gpu/drm/i915/i915_request.c | 20
> > >> 1 file changed, 20 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_request.c
> > >> b/drivers/gpu/drm/i915/i915_request.c
> > >> index 7503dcb9043b..44491e7e214c 100644
> > >> --- a/drivers/gpu/drm/i915/i915_request.c
> > >> +++ b/drivers/gpu/drm/i915/i915_request.c
> > >> @@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct
> > >> dma_fence *fence)
> > >> return i915_request_enable_breadcrumb(to_request(fence));
> > >> }
> > >>
> > >> +static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t
> > >> deadline)
> > >> +{
> > >> +struct i915_request *rq = to_request(fence);
> > >> +
> > >> +if (i915_request_completed(rq))
> > >> +return;
> > >> +
> > >> +if (i915_request_started(rq))
> > >> + return;
> > >
> > > why do we skip the boost if already started?
> > > don't we want to boost the freq anyway?
> >
> > I'd wager Rob is just copying the current i915 wait boost logic.
>
> Yup, and probably incorrectly.. Matt reported fewer boosts/sec
> compared to your RFC, this could be the bug
I don't think i915 calls drm_atomic_helper_wait_for_fences()
so that could explain something.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Wed, Feb 22, 2023 at 07:44:42AM -0800, Rob Clark wrote: > On Wed, Feb 22, 2023 at 1:57 AM Pekka Paalanen wrote: > > > > On Tue, 21 Feb 2023 09:50:20 -0800 > > Rob Clark wrote: > > > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > wrote: > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > wrote: > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences > > > > > > > > that an > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > --- > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > > > drm_crtc *crtc, > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > +/** > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > > > vblank > > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > > timestamp. > > > > > > > > + * > > > > > > > > + * Calculate the expected time of the next vblank based on > > > > > > > > time of previous > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the > > > > > > > current > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > > > > > I don't know. :-) > > > > > > > > At least for i915 this will give you the maximum frame > > > > duration. > > > > > > I suppose one could argue that maximum frame duration is the actual > > > deadline. Anything less is just moar fps, but not going to involve > > > stalling until vblank N+1, AFAIU > > > > > > > Also this does not calculate the the start of vblank, it > > > > calculates the start of active video. > > > > > > Probably something like end of previous frame's video.. might not be > > > _exactly_ correct (because some buffering involved), but OTOH on the > > > GPU side, I expect the driver to set a timer for a few ms or so before > > > the deadline. So there is some wiggle room. > > > > The vblank timestamp is defined to be the time of the first active > > pixel of the frame in the video signal. At least that's the one that > > UAPI carries (when not tearing?). It is not the start of vblank period. > > > > With VRR, the front porch before the first active pixel can be multiple > > milliseconds. The difference between 144 Hz and 60 Hz is 9.7 ms for > > example. > > What we really want is the deadline for the hw to latch for the next > frame.. which as Ville pointed out is definitely before the end of > vblank. > > Honestly this sort of feature is a lot more critical for the non-VRR > case, and VRR is kind of a minority edge case. So I'd prefer not to > get too hung up on VRR. If there is an easy way for the helpers to > detect VRR, I'd be perfectly fine not setting a deadline hint in that > case, and let someone who actually has a VRR display figure out how to > handle that case. The formula I gave you earlier works for both VRR and non-VRR. -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 02:28:10PM -0800, Rob Clark wrote: > On Tue, Feb 21, 2023 at 1:48 PM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > > > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > > wrote: > > > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > > Rob Clark wrote: > > > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > > wrote: > > > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences > > > > > > > > > that an > > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > @@ -980,6 +980,38 @@ u64 > > > > > > > > > drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > > > > > } > > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the > > > > > > > > > next vblank > > > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > > > timestamp. > > > > > > > > > + * > > > > > > > > > + * Calculate the expected time of the next vblank based on > > > > > > > > > time of previous > > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the > > > > > > > > current > > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a > > > > > > > maximum? > > > > > > > > > > > > I don't know. :-) > > > > > > > > > > At least for i915 this will give you the maximum frame > > > > > duration. > > > > > > > > > > Also this does not calculate the the start of vblank, it > > > > > calculates the start of active video. > > > > > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do > > > > something like: > > > > > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > > > framedur = ns_to_ktime(vblank->framedur_ns); > > > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > > > > > Something like this should work: > > > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > > > deadline = vblanktime + vblank_start > > > > > > That would be the expected time when the next start of vblank > > > happens. > > > > Except that drm_vblank_count_and_time() will give you the last > > sampled timestamp, which may be long ago in the past. Would > > need to add an _accurate version of that if we want to be > > guaranteed a fresh sample. > > IIRC the only time we wouldn't have a fresh sample is if the screen > has been idle for some time? IIRC "some time" == 1 idle frame, for any driver that sets vblank_disable_immediate. > In which case, I think that doesn't > matter. > > BR, > -R > > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > wrote: > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > Rob Clark wrote: > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > wrote: > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > Rob Clark wrote: > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that > > > > > > > an > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > > drm_crtc *crtc, > > > > > > > } > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > +/** > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > > vblank > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > timestamp. > > > > > > > + * > > > > > > > + * Calculate the expected time of the next vblank based on time > > > > > > > of previous > > > > > > > + * vblank and frame duration > > > > > > > > > > > > Hi, > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > > > I don't know. :-) > > > > > > At least for i915 this will give you the maximum frame > > > duration. > > > > > > Also this does not calculate the the start of vblank, it > > > calculates the start of active video. > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do something > > like: > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > framedur = ns_to_ktime(vblank->framedur_ns); > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > Something like this should work: > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > deadline = vblanktime + vblank_start > > That would be the expected time when the next start of vblank > happens. Except that drm_vblank_count_and_time() will give you the last sampled timestamp, which may be long ago in the past. Would need to add an _accurate version of that if we want to be guaranteed a fresh sample. -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > Rob Clark wrote: > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > > atomic update is waiting on. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > --- > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > drm_crtc *crtc, > > > > > > } > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > +/** > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > vblank > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > timestamp. > > > > > > + * > > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > > previous > > > > > > + * vblank and frame duration > > > > > > > > > > Hi, > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > VRR mode, right? > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > I don't know. :-) > > > > At least for i915 this will give you the maximum frame > > duration. > > > > Also this does not calculate the the start of vblank, it > > calculates the start of active video. > > AFAIU, vsync_end/vsync_start are in units of line, so I could do something > like: > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > framedur = ns_to_ktime(vblank->framedur_ns); > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); Something like this should work: vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal deadline = vblanktime + vblank_start That would be the expected time when the next start of vblank happens. -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 03:11:33PM +0200, Pekka Paalanen wrote: > On Tue, 21 Feb 2023 15:01:35 +0200 > Ville Syrjälä wrote: > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > Rob Clark wrote: > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > > atomic update is waiting on. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > --- > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > drm_crtc *crtc, > > > > > > } > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > +/** > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > vblank > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > timestamp. > > > > > > + * > > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > > previous > > > > > > + * vblank and frame duration > > > > > > > > > > Hi, > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > VRR mode, right? > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > I don't know. :-) > > > > At least for i915 this will give you the maximum frame > > duration. > > Really maximum duration? So minimum VRR frequency? Yes. Doing otherwise would complicate the actual timestamp calculation even further. The actual timestamps i915 generates will however match the start of active video, regardless of how long vblank was extended. The only exception might be if you query the timestamp during vblank but VRR exit has not yet been triggered, ie. not commit has been made during the frame. In that case the timestamp will correspond to the max frame duration, which may or may not end up being the case. Depends totally whether a commit will still happen during the vblank to trigger an early vblank exit. > > > Also this does not calculate the the start of vblank, it > > calculates the start of active video. > > Oh indeed, so it's too late. What one would actually need for the > deadline is the driver's deadline to present for the immediately next > start of active video. > > And with VRR that should probably aim for the maximum frame frequency, > not minimum? Yeah, max frame rate seems like the easiest thing to use there. The other option might be some average value based on recent history, but figuring tht out would seem like a lot more work. -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote:
> On Mon, 20 Feb 2023 07:55:41 -0800
> Rob Clark wrote:
>
> > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen wrote:
> > >
> > > On Sat, 18 Feb 2023 13:15:53 -0800
> > > Rob Clark wrote:
> > >
> > > > From: Rob Clark
> > > >
> > > > Will be used in the next commit to set a deadline on fences that an
> > > > atomic update is waiting on.
> > > >
> > > > Signed-off-by: Rob Clark
> > > > ---
> > > > drivers/gpu/drm/drm_vblank.c | 32
> > > > include/drm/drm_vblank.h | 1 +
> > > > 2 files changed, 33 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > index 2ff31717a3de..caf25ebb34c5 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct drm_crtc
> > > > *crtc,
> > > > }
> > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
> > > >
> > > > +/**
> > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank
> > > > + * @crtc: the crtc for which to calculate next vblank time
> > > > + * @vblanktime: pointer to time to receive the next vblank timestamp.
> > > > + *
> > > > + * Calculate the expected time of the next vblank based on time of
> > > > previous
> > > > + * vblank and frame duration
> > >
> > > Hi,
> > >
> > > for VRR this targets the highest frame rate possible for the current
> > > VRR mode, right?
> > >
> >
> > It is based on vblank->framedur_ns which is in turn based on
> > mode->crtc_clock. Presumably for VRR that ends up being a maximum?
>
> I don't know. :-)
At least for i915 this will give you the maximum frame
duration.
Also this does not calculate the the start of vblank, it
calculates the start of active video.
>
> You need a number of clock cycles in addition to the clock frequency,
> and that could still be minimum, maximum, the last realized one, ...
>
> VRR works by adjusting the front porch length IIRC.
>
>
> Thanks,
> pq
>
> > BR,
> > -R
> >
> >
> > >
> > > Thanks,
> > > pq
> > >
> > > > + */
> > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t
> > > > *vblanktime)
> > > > +{
> > > > + unsigned int pipe = drm_crtc_index(crtc);
> > > > + struct drm_vblank_crtc *vblank = >dev->vblank[pipe];
> > > > + u64 count;
> > > > +
> > > > + if (!vblank->framedur_ns)
> > > > + return -EINVAL;
> > > > +
> > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime);
> > > > +
> > > > + /*
> > > > + * If we don't get a valid count, then we probably also don't
> > > > + * have a valid time:
> > > > + */
> > > > + if (!count)
> > > > + return -EINVAL;
> > > > +
> > > > + *vblanktime = ktime_add(*vblanktime,
> > > > ns_to_ktime(vblank->framedur_ns));
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time);
> > > > +
> > > > static void send_vblank_event(struct drm_device *dev,
> > > > struct drm_pending_vblank_event *e,
> > > > u64 seq, ktime_t now)
> > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > > > index 733a3e2d1d10..a63bc2c92f3c 100644
> > > > --- a/include/drm/drm_vblank.h
> > > > +++ b/include/drm/drm_vblank.h
> > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device
> > > > *dev);
> > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
> > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
> > > > ktime_t *vblanktime);
> > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t
> > > > *vblanktime);
> > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
> > > > struct drm_pending_vblank_event *e);
> > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> > >
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote:
> On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang
> wrote:
> >
> >
> >
> > On 11/7/2022 2:09 PM, Rob Clark wrote:
> > > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang
> > > wrote:
> > >>
> > >>
> > >>
> > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> > >>>> properties. When the color fill value is set, and the framebuffer is
> > >>>> set
> > >>>> to NULL, memory fetch will be disabled.
> > >>>
> > >>> Thinking a bit more universally I wonder if there should be
> > >>> some kind of enum property:
> > >>>
> > >>> enum plane_pixel_source {
> > >>>FB,
> > >>>COLOR,
> > >>>LIVE_FOO,
> > >>>LIVE_BAR,
> > >>>...
> > >>> }
> > >>
> > >> Hi Ville,
> > >>
> > >> Makes sense -- this way, we'd also lay some groundwork for cases where
> > >> drivers want to use other non-FB sources.
> > >>
> > >>>
> > >>>> In addition, loosen the NULL FB checks within the atomic commit
> > >>>> callstack
> > >>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
> > >>>> methods where the FB was previously assumed to be non-NULL.
> > >>>>
> > >>>> Finally, have the DPU driver use drm_plane_state.color_fill and
> > >>>> drm_plane_state.color_fill_format instead of
> > >>>> dpu_plane_state.color_fill,
> > >>>> and add extra checks in the DPU atomic commit callstack to account for
> > >>>> a
> > >>>> NULL FB in cases where color_fill is set.
> > >>>>
> > >>>> Some drivers support hardware that have optimizations for solid fill
> > >>>> planes. This series aims to expose these capabilities to userspace as
> > >>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > >>>> hardware composer HAL) that can be set by apps like the Android Gears
> > >>>> app.
> > >>>>
> > >>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to
> > >>>> a
> > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
> > >>>> framebuffer to NULL.
> > >>>
> > >>> Is there some real reason for the format property? Ie. why not
> > >>> just do what was the plan for the crttc background color and
> > >>> specify the color in full 16bpc format and just pick as many
> > >>> msbs from that as the hw can use?
> > >>
> > >> The format property was added because we can't assume that all hardware
> > >> will support/use the same color format for solid fill planes. Even for
> > >> just MSM devices, the hardware supports different variations of RGB
> > >> formats [1].
> > >
> > > Sure, but the driver can convert the format into whatever the hw
> > > wants. A 1x1 color conversion is not going to be problematic ;-)
> >
> > Hi Rob,
> >
> > Hm... that's also a fair point. Just wondering, is there any advantage
> > of having the driver convert the format, other than not having to
> > implement an extra format property?
> >
> > (In case we end up wrapping everything into a prop blob or something)
> >
>
> It keeps the uabi simpler.. for obvious reasons you don't want the
> driver to do cpu color conversion for an arbitrary size plane, which
> is why we go to all the complexity to expose formats and modifiers for
> "real" planes, but we are dealing with a single pixel value here,
> let's not make the uabi more complex than we need to. I'd propose
> making it float32[4] if float weren't a pita for kernel/uabi, but
> u16[4] or u32[4] should be fine, and drivers can translate that easily
> into whatever weird formats their hw wants for solid-fill.
u16[4] fits into a single u64 property value.
That was the plan for the background prop as well:
https://lore.kernel.org/all/20190703125442.gw5...@intel.com/T/
--
Ville Syrjälä
Intel
Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> properties. When the color fill value is set, and the framebuffer is set
> to NULL, memory fetch will be disabled.
Thinking a bit more universally I wonder if there should be
some kind of enum property:
enum plane_pixel_source {
FB,
COLOR,
LIVE_FOO,
LIVE_BAR,
...
}
> In addition, loosen the NULL FB checks within the atomic commit callstack
> to allow a NULL FB when color_fill is nonzero and add FB checks in
> methods where the FB was previously assumed to be non-NULL.
>
> Finally, have the DPU driver use drm_plane_state.color_fill and
> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> and add extra checks in the DPU atomic commit callstack to account for a
> NULL FB in cases where color_fill is set.
>
> Some drivers support hardware that have optimizations for solid fill
> planes. This series aims to expose these capabilities to userspace as
> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> hardware composer HAL) that can be set by apps like the Android Gears
> app.
>
> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> DRM format, setting COLOR_FILL to a color fill value, and setting the
> framebuffer to NULL.
Is there some real reason for the format property? Ie. why not
just do what was the plan for the crttc background color and
specify the color in full 16bpc format and just pick as many
msbs from that as the hw can use?
>
> Jessica Zhang (3):
> drm: Introduce color fill properties for drm plane
> drm: Adjust atomic checks for solid fill color
> drm/msm/dpu: Use color_fill property for DPU planes
>
> drivers/gpu/drm/drm_atomic.c | 68 ---
> drivers/gpu/drm/drm_atomic_helper.c | 34 +++-
> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> drivers/gpu/drm/drm_blend.c | 38 +
> drivers/gpu/drm/drm_plane.c | 8 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++
> include/drm/drm_atomic_helper.h | 5 +-
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 28 ++
> 10 files changed, 188 insertions(+), 76 deletions(-)
>
> --
> 2.38.0
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, May 05, 2022 at 08:53:12AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, May 5, 2022 at 8:29 AM Ville Syrjälä
> wrote:
> >
> > On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > > > wrote:
> > > > > > >
> > > > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > > > powered in order for the DP AUX transfer to work: the DP source
> > > > > > > > and
> > > > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > > > requirement on side-channel operations") added some
> > > > > > > > documentation
> > > > > > > > saying that the DP source is required to power itself up (if
> > > > > > > > needed)
> > > > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > > > about
> > > > > > > > the DP sink.
> > > > > > > >
> > > > > > > > For full fledged DP the sink isn't really a problem. It's
> > > > > > > > expected
> > > > > > > > that if an external DP monitor isn't plugged in that attempting
> > > > > > > > to do
> > > > > > > > AUX transfers won't work. It's also expected that if a DP
> > > > > > > > monitor is
> > > > > > > > plugged in (and thus asserting HPD) that it AUX transfers will
> > > > > > > > work.
> > > > > > > >
> > > > > > > > When we're looking at eDP, however, things are less obvious.
> > > > > > > > Let's add
> > > > > > > > some documentation about expectations. Here's what we'll say:
> > > > > > > >
> > > > > > > > 1. We don't expect the DP AUX transfer function to power on an
> > > > > > > > eDP
> > > > > > > > panel. If an eDP panel is physically connected but powered off
> > > > > > > > then it
> > > > > > > > makes sense for the transfer to fail.
> > > > > > >
> > > > > > > I don't agree with this. I think the panel should just get powred
> > > > > > > up
> > > > > > > for AUX transfers.
> > > > > >
> > > > > > That's definitely a fair thing to think about and I have at times
> > > > > > thought about trying to make it work that way. It always ends up
> > > > > > hitting a roadblock.
> > > >
> > > > How do you even probe the panel initially if you can't power it on
> > > > without doing some kind of full modeset/etc.?
> > >
> > > It's not that we can't power it on without a full modeset. It' that at
> > > panel probe time all the DRM components haven't been hooked together
> > > yet, so the bridge chain isn't available yet. The panel can power
> > > itself on, though. This is why the documentation I added says: "if a
> > > panel driver is initiating a DP AUX transfer it may power itself up
> > > however it wants"
> > >
> > >
> > > > > > The biggest roadblock that I recall is that to make this work then
> > > > > > you'd have to somehow ensure that the bridge chain's pre_enable()
> > > > > > call
> > > > > > was made as part of the AUX transfer, right? Since the transfer
> > > > > > function can be called in any context at all, we have to coordinate
> > > > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > > > panel down then we need to wait for DRM to fully finish powering
> > > > > > down,
> > > > > > then we need to power the panel back up. I don't believe that we can
> > &
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Thu, May 05, 2022 at 08:00:20AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, May 5, 2022 at 7:46 AM Ville Syrjälä
> wrote:
> >
> > On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> > > On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > > > wrote:
> > > > >
> > > > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > > > When doing DP AUX transfers there are two actors that need to be
> > > > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > > > requirement on side-channel operations") added some documentation
> > > > > > saying that the DP source is required to power itself up (if needed)
> > > > > > to do AUX transfers. However, that commit doesn't talk anything
> > > > > > about
> > > > > > the DP sink.
> > > > > >
> > > > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > > > that if an external DP monitor isn't plugged in that attempting to
> > > > > > do
> > > > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > > > >
> > > > > > When we're looking at eDP, however, things are less obvious. Let's
> > > > > > add
> > > > > > some documentation about expectations. Here's what we'll say:
> > > > > >
> > > > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > > > panel. If an eDP panel is physically connected but powered off then
> > > > > > it
> > > > > > makes sense for the transfer to fail.
> > > > >
> > > > > I don't agree with this. I think the panel should just get powred up
> > > > > for AUX transfers.
> > > >
> > > > That's definitely a fair thing to think about and I have at times
> > > > thought about trying to make it work that way. It always ends up
> > > > hitting a roadblock.
> >
> > How do you even probe the panel initially if you can't power it on
> > without doing some kind of full modeset/etc.?
>
> It's not that we can't power it on without a full modeset. It' that at
> panel probe time all the DRM components haven't been hooked together
> yet, so the bridge chain isn't available yet. The panel can power
> itself on, though. This is why the documentation I added says: "if a
> panel driver is initiating a DP AUX transfer it may power itself up
> however it wants"
>
>
> > > > The biggest roadblock that I recall is that to make this work then
> > > > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > > > was made as part of the AUX transfer, right? Since the transfer
> > > > function can be called in any context at all, we have to coordinate
> > > > this with DRM. If, for instance, DRM is mid way through powering the
> > > > panel down then we need to wait for DRM to fully finish powering down,
> > > > then we need to power the panel back up. I don't believe that we can
> > > > just force the panel to stay on if DRM is turning it off because of
> > > > panel power sequencing requirements. At least I know it would have the
> > > > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > > > see the panel power off after it's been disabled.
> > > >
> > > > We also, I believe, need to handle the fact that the bridge chain may
> > > > not have even been created yet. We do AUX transfers to read the EDID
> > > > and also to setup the backlight in the probe function of panel-edp. At
> > > > that point the panel hasn't been linked into the chain. We had _long_
> > > > discussions [1] about moving these out of probe and decided that we
> > > > could move the EDID read to be later but that it was going to really
> > > > ugly to move the AUX backlight later. The backlight would end up
> > > > popping up at some point in time later (the first call to panel
> > > > prepare() or maybe get_modes()) and that seemed weird.
> > > >
&
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Wed, May 04, 2022 at 02:10:08PM -0400, Lyude Paul wrote:
> On Wed, 2022-05-04 at 09:04 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, May 4, 2022 at 5:21 AM Ville Syrjälä
> > wrote:
> > >
> > > On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> > > > When doing DP AUX transfers there are two actors that need to be
> > > > powered in order for the DP AUX transfer to work: the DP source and
> > > > the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> > > > requirement on side-channel operations") added some documentation
> > > > saying that the DP source is required to power itself up (if needed)
> > > > to do AUX transfers. However, that commit doesn't talk anything about
> > > > the DP sink.
> > > >
> > > > For full fledged DP the sink isn't really a problem. It's expected
> > > > that if an external DP monitor isn't plugged in that attempting to do
> > > > AUX transfers won't work. It's also expected that if a DP monitor is
> > > > plugged in (and thus asserting HPD) that it AUX transfers will work.
> > > >
> > > > When we're looking at eDP, however, things are less obvious. Let's add
> > > > some documentation about expectations. Here's what we'll say:
> > > >
> > > > 1. We don't expect the DP AUX transfer function to power on an eDP
> > > > panel. If an eDP panel is physically connected but powered off then it
> > > > makes sense for the transfer to fail.
> > >
> > > I don't agree with this. I think the panel should just get powred up
> > > for AUX transfers.
> >
> > That's definitely a fair thing to think about and I have at times
> > thought about trying to make it work that way. It always ends up
> > hitting a roadblock.
How do you even probe the panel initially if you can't power it on
without doing some kind of full modeset/etc.?
> >
> > The biggest roadblock that I recall is that to make this work then
> > you'd have to somehow ensure that the bridge chain's pre_enable() call
> > was made as part of the AUX transfer, right? Since the transfer
> > function can be called in any context at all, we have to coordinate
> > this with DRM. If, for instance, DRM is mid way through powering the
> > panel down then we need to wait for DRM to fully finish powering down,
> > then we need to power the panel back up. I don't believe that we can
> > just force the panel to stay on if DRM is turning it off because of
> > panel power sequencing requirements. At least I know it would have the
> > potential to break "samsung-atna33xc20.c" which absolutely needs to
> > see the panel power off after it's been disabled.
> >
> > We also, I believe, need to handle the fact that the bridge chain may
> > not have even been created yet. We do AUX transfers to read the EDID
> > and also to setup the backlight in the probe function of panel-edp. At
> > that point the panel hasn't been linked into the chain. We had _long_
> > discussions [1] about moving these out of probe and decided that we
> > could move the EDID read to be later but that it was going to really
> > ugly to move the AUX backlight later. The backlight would end up
> > popping up at some point in time later (the first call to panel
> > prepare() or maybe get_modes()) and that seemed weird.
> >
> > [1]
> > https://lore.kernel.org/lkml/CAD=FV=u5-stdlydkejwlaog-0wgxr49vxtwuyuo7z2puibl...@mail.gmail.com/
> >
> >
> > > Otherwise you can't trust that eg. the /dev/aux
> > > stuff is actually usable.
> >
> > Yeah, it's been on my mind to talk more about /dev/aux. I think
> > /dev/aux has some problems, at least with eDP. Specifically:
> >
> > 1. Even if we somehow figure out how to power the panel on as part of
> > the aux transfer, we actually _still_ not guaranteed to be able to
> > talk to it as far as I understand. My colleague reported to me that on
> > a system he was working with that had PSR (panel self refresh) that
> > when the panel was powered on but in PSR mode that it wouldn't talk
> > over AUX. Assuming that this is correct then I guess we'd also have to
> > do even more coordination with DRM to exit PSR and block future
> > transitions of PSR. (NOTE: it's always possible that my colleague ran
> > into some other bug and that panels are _supposed_ to be able to talk
> > in PSR. If you think this is the case, I can always try to dig more).
>
> TBH - the coordination with drm I don't think would b
Re: [Freedreno] [PATCH] drm: Document that power requirements for DP AUX transfers
On Tue, May 03, 2022 at 04:21:08PM -0700, Douglas Anderson wrote:
> When doing DP AUX transfers there are two actors that need to be
> powered in order for the DP AUX transfer to work: the DP source and
> the DP sync. Commit bacbab58f09d ("drm: Mention the power state
> requirement on side-channel operations") added some documentation
> saying that the DP source is required to power itself up (if needed)
> to do AUX transfers. However, that commit doesn't talk anything about
> the DP sink.
>
> For full fledged DP the sink isn't really a problem. It's expected
> that if an external DP monitor isn't plugged in that attempting to do
> AUX transfers won't work. It's also expected that if a DP monitor is
> plugged in (and thus asserting HPD) that it AUX transfers will work.
>
> When we're looking at eDP, however, things are less obvious. Let's add
> some documentation about expectations. Here's what we'll say:
>
> 1. We don't expect the DP AUX transfer function to power on an eDP
> panel. If an eDP panel is physically connected but powered off then it
> makes sense for the transfer to fail.
I don't agree with this. I think the panel should just get powred up
for AUX transfers. Otherwise you can't trust that eg. the /dev/aux
stuff is actually usable.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH 00/22] drm: Review of mode copies
On Wed, Mar 23, 2022 at 01:39:44PM +0300, Dmitry Baryshkov wrote: > On 22/03/2022 01:37, Ville Syrjälä wrote: > > On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote: > >> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä > >> wrote: > >>> > >>> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > >>>>drm: Add drm_mode_init() > >>>>drm/bridge: Use drm_mode_copy() > >>>>drm/imx: Use drm_mode_duplicate() > >>>>drm/panel: Use drm_mode_duplicate() > >>>>drm/vc4: Use drm_mode_copy() > >>> These have been pushed to drm-misc-next. > >>> > >>>>drm/amdgpu: Remove pointless on stack mode copies > >>>>drm/amdgpu: Use drm_mode_init() for on-stack modes > >>>>drm/amdgpu: Use drm_mode_copy() > >>> amdgpu ones are reviewed, but I'll leave them for the > >>> AMD folks to push to whichever tree they prefer. > >> > >> I pulled patches 2, 4, 5 into my tree. > > > > Thanks. > > > >> For 3, I'm happy to have it > >> land via drm-misc with the rest of the mode_init changes if you'd > >> prefer. > > > > Either way works for me. I don't yet have reviews yet for > > the other drivers, so I'll proably hold off for a bit more > > at least. And the i915 patch I'll be merging via drm-intel. > > > >>>>drm/radeon: Use drm_mode_copy() > >>>>drm/gma500: Use drm_mode_copy() > >>>>drm/tilcdc: Use drm_mode_copy() > >>>>drm/i915: Use drm_mode_copy() > > > > Those are now all in. > > > > Which leaves us with these stragglers: > >>>>drm/hisilicon: Use drm_mode_init() for on-stack modes > > >>>>drm/msm: Nuke weird on stack mode copy > >>>>drm/msm: Use drm_mode_init() for on-stack modes > >>>>drm/msm: Use drm_mode_copy() > > These three patches went beneath my radar for some reason. > > I have just sent a replacement for the first patch ([1]). Other two > patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next > development cycle if that works for you. That'll do fine for me. Thanks. > > [1] https://patchwork.freedesktop.org/series/101682/ > > >>>>drm/mtk: Use drm_mode_init() for on-stack modes > >>>>drm/rockchip: Use drm_mode_copy() > >>>>drm/sti: Use drm_mode_copy() > >>>>drm: Use drm_mode_init() for on-stack modes > >>>>drm: Use drm_mode_copy() > > > > > -- > With best wishes > Dmitry -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH 00/22] drm: Review of mode copies
On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote: > On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä > wrote: > > > > On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > > > drm: Add drm_mode_init() > > > drm/bridge: Use drm_mode_copy() > > > drm/imx: Use drm_mode_duplicate() > > > drm/panel: Use drm_mode_duplicate() > > > drm/vc4: Use drm_mode_copy() > > These have been pushed to drm-misc-next. > > > > > drm/amdgpu: Remove pointless on stack mode copies > > > drm/amdgpu: Use drm_mode_init() for on-stack modes > > > drm/amdgpu: Use drm_mode_copy() > > amdgpu ones are reviewed, but I'll leave them for the > > AMD folks to push to whichever tree they prefer. > > I pulled patches 2, 4, 5 into my tree. Thanks. > For 3, I'm happy to have it > land via drm-misc with the rest of the mode_init changes if you'd > prefer. Either way works for me. I don't yet have reviews yet for the other drivers, so I'll proably hold off for a bit more at least. And the i915 patch I'll be merging via drm-intel. > > > drm/radeon: Use drm_mode_copy() > > > drm/gma500: Use drm_mode_copy() > > > drm/tilcdc: Use drm_mode_copy() > > > drm/i915: Use drm_mode_copy() Those are now all in. Which leaves us with these stragglers: > > > drm/hisilicon: Use drm_mode_init() for on-stack modes > > > drm/msm: Nuke weird on stack mode copy > > > drm/msm: Use drm_mode_init() for on-stack modes > > > drm/msm: Use drm_mode_copy() > > > drm/mtk: Use drm_mode_init() for on-stack modes > > > drm/rockchip: Use drm_mode_copy() > > > drm/sti: Use drm_mode_copy() > > > drm: Use drm_mode_init() for on-stack modes > > > drm: Use drm_mode_copy() -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH 00/22] drm: Review of mode copies
On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > drm: Add drm_mode_init() > drm/bridge: Use drm_mode_copy() > drm/imx: Use drm_mode_duplicate() > drm/panel: Use drm_mode_duplicate() > drm/vc4: Use drm_mode_copy() These have been pushed to drm-misc-next. > drm/amdgpu: Remove pointless on stack mode copies > drm/amdgpu: Use drm_mode_init() for on-stack modes > drm/amdgpu: Use drm_mode_copy() amdgpu ones are reviewed, but I'll leave them for the AMD folks to push to whichever tree they prefer. The rest are still in need of review: > drm/radeon: Use drm_mode_copy() > drm/gma500: Use drm_mode_copy() > drm/hisilicon: Use drm_mode_init() for on-stack modes > drm/msm: Nuke weird on stack mode copy > drm/msm: Use drm_mode_init() for on-stack modes > drm/msm: Use drm_mode_copy() > drm/mtk: Use drm_mode_init() for on-stack modes > drm/rockchip: Use drm_mode_copy() > drm/sti: Use drm_mode_copy() > drm/tilcdc: Use drm_mode_copy() > drm/i915: Use drm_mode_init() for on-stack modes > drm/i915: Use drm_mode_copy() > drm: Use drm_mode_init() for on-stack modes > drm: Use drm_mode_copy() -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops
On Fri, Feb 25, 2022 at 12:26:12PM -0800, Rob Clark wrote:
> From: Rob Clark
>
> Extend the helper macro so we don't have to throw it away if driver adds
> support for optional fops, like show_fdinfo().
>
> Signed-off-by: Rob Clark
> ---
> include/drm/drm_gem.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 35e7f44c2a75..987e78b18244 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -327,7 +327,7 @@ struct drm_gem_object {
> * non-static version of this you're probably doing it wrong and will break
> the
> * THIS_MODULE reference by accident.
> */
> -#define DEFINE_DRM_GEM_FOPS(name) \
> +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> static const struct file_operations name = {\
> .owner = THIS_MODULE,\
> .open = drm_open,\
> @@ -338,6 +338,7 @@ struct drm_gem_object {
> .read = drm_read,\
> .llseek = noop_llseek,\
> .mmap = drm_gem_mmap,\
> + ##__VA_ARGS__\
> }
Would it not be less convoluted to make the macro only provide
the initializers? So you'd get something like:
static const struct file_operations foo = {
DRM_GEM_FOPS,
.my_stuff = whatever,
};
>
> void drm_gem_object_release(struct drm_gem_object *obj);
> --
> 2.35.1
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 22, 2021 at 03:01:52PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote:
> > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote:
> > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote:
> > > > drm_get_edid() internally calls to drm_connector_update_edid_property()
> > > > and then drm_add_display_info(), which parses the EDID.
> > > > This happens in the function intel_hdmi_set_edid() and
> > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
> > > >
> > > > Once EDID is parsed, the monitor HDMI support information is available
> > > > through drm_display_info.is_hdmi. Retriving the same information with
> > > > drm_detect_hdmi_monitor() is less efficient. Change to
> > > > drm_display_info.is_hdmi
> > >
> > > I meant we need to examine all call chains that can lead to
> > > .detect() to make sure all of them do in fact update the
> > > display_info beforehand.
> >
> > Well, I studied it carefully and, yes, all call chains that can lead to
> > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info
> > beforehand. In the case that this doesn't happen, the code is unchanged.
> >
> > Do you want I explain the changes in the code here again ? Or do you want
> > to me change the commit message to be more clear ? In the first case, I can
> > write here a detailed explanation. In the second case I can make a longer
> > commit
> > message.
> >
> > Or both?
>
> I want all those call chains explained in the commit message,
> otherwise I have no easy way to confirm whether the change
> is correct or not.
Hmm. OK, so I had a bit of a dig around and seems that what we do now
.detect()->drm_get_edid()->drm_connector_update_edid_property()->drm_add_display_info()
Now the question is when did that start happening? Looks like it was
commit 4b4df570b41d ("drm: Update edid-derived drm_display_info fields
at edid property set [v2]") that started to call drm_add_display_info()
from drm_connector_update_edid_property(), and then commit 5186421cbfe2
("drm: Introduce epoch counter to drm_connector") started to call
drm_connector_update_edid_property() from drm_get_edid(). Before both
of those commits were in place display_info would still contain
some stale garbage during .detect().
That is the story I think we want in these commit messages since it
a) explains why the old code was directly parsing the edid instead
b) why it's now safe to change this
PS. connector->force handling in drm_get_edid() looks a bit busted
since it doesn't call drm_connector_update_edid_property() at all
in some cases. I think there might be some path that leads there
anywya if/when we change connector->force, but we should fix
drm_get_edid() to do the right thing regarless.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > and then drm_add_display_info(), which parses the EDID. > > > This happens in the function intel_hdmi_set_edid() and > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > through drm_display_info.is_hdmi. Retriving the same information with > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > drm_display_info.is_hdmi > > > > I meant we need to examine all call chains that can lead to > > .detect() to make sure all of them do in fact update the > > display_info beforehand. > > Well, I studied it carefully and, yes, all call chains that can lead to > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > beforehand. In the case that this doesn't happen, the code is unchanged. > > Do you want I explain the changes in the code here again ? Or do you want > to me change the commit message to be more clear ? In the first case, I can > write here a detailed explanation. In the second case I can make a longer > commit > message. > > Or both? I want all those call chains explained in the commit message, otherwise I have no easy way to confirm whether the change is correct or not. -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote:
> drm_get_edid() internally calls to drm_connector_update_edid_property()
> and then drm_add_display_info(), which parses the EDID.
> This happens in the function intel_hdmi_set_edid() and
> intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
>
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi
I meant we need to examine all call chains that can lead to
.detect() to make sure all of them do in fact update the
display_info beforehand.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..008e5b0ba408 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..b4065e4df644 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> connector->display_info.is_hdmi;
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Sat, Oct 16, 2021 at 08:42:26PM +0200, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
We still need proof in the commit message that display_info
is actually populated by the time this gets called.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..008e5b0ba408 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..b4065e4df644 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> connector->display_info.is_hdmi;
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 01/13] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Sat, Oct 16, 2021 at 08:42:14PM +0200, Claudio Suarez wrote:
> According to the documentation, drm_add_edid_modes
> "... Also fills out the _display_info structure and ELD in @connector
> with any information which can be derived from the edid."
>
> drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> value or may be null. When it is not null, connector->display_info and
> connector->eld are updated according to the edid. When edid=NULL, only
> connector->eld is reset. Reset connector->display_info to be consistent
> and accurate.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_edid.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6325877c5fd6..c643db17782c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5356,14 +5356,13 @@ int drm_add_edid_modes(struct drm_connector
> *connector, struct edid *edid)
> int num_modes = 0;
> u32 quirks;
>
> - if (edid == NULL) {
> - clear_eld(connector);
> - return 0;
> - }
> if (!drm_edid_is_valid(edid)) {
OK, so drm_edid_is_valid() will happily accept NULL and considers
it invalid. You may want to mention that explicitly in the commit
message.
> + /* edid == NULL or invalid here */
> clear_eld(connector);
> - drm_warn(connector->dev, "%s: EDID invalid.\n",
> - connector->name);
> + drm_reset_display_info(connector);
> + if (edid)
> + drm_warn(connector->dev, "%s: EDID invalid.\n",
> + connector->name);
Could you respin this to use the standard [CONNECTOR:%d:%s] form
while at it? Or I guess a patch to mass convert the whole drm_edid.c
might be another option.
Patch looks good.
Reviewed-by: Ville Syrjälä
> return 0;
> }
>
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 10:33:29PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
> > On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> > > > According to the documentation, drm_add_edid_modes
> > > > "... Also fills out the _display_info structure and ELD in
> > > > @connector
> > > > with any information which can be derived from the edid."
> > > >
> > > > drm_add_edid_modes accepts a struct edid *edid parameter which may have
> > > > a
> > > > value or may be null. When it is not null, connector->display_info and
> > > > connector->eld are updated according to the edid. When edid=NULL, only
> > > > connector->eld is reset. Reset connector->display_info to be consistent
> > > > and accurate.
> > > >
> > > > Signed-off-by: Claudio Suarez
> > > > ---
> > > > drivers/gpu/drm/drm_edid.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 6325877c5fd6..6cbe09b2357c 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> > > > *connector, struct edid *edid)
> > > >
> > > > if (edid == NULL) {
> > > > clear_eld(connector);
> > > > + drm_reset_display_info(connector);
> > > > return 0;
> > > > }
> > > > if (!drm_edid_is_valid(edid)) {
> > > > clear_eld(connector);
> > > > + drm_reset_display_info(connector);
> > >
> > > Looks easier if you pull both of those out from these branches and
> > > just call them unconditionally at the start.
> >
> > After looking at the full code, I am not sure. This is the code:
> > ==
> > int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> > {
> > int num_modes = 0;
> > u32 quirks;
> >
> > if (edid == NULL) {
> > clear_eld(connector);
> > drm_reset_display_info(connector); <--- added by me
> > return 0;
> > }
> > if (!drm_edid_is_valid(edid)) {
> > clear_eld(connector);
> > drm_reset_display_info(connector); <--- added by me
> > drm_warn(connector->dev, "%s: EDID invalid.\n",
> > connector->name);
> > return 0;
> > }
> >
> > drm_edid_to_eld(connector, edid);
> >
> > quirks = drm_add_display_info(connector, edid);
> > etc...
> > =
> >
> > If we move those out of these branches and edid != NULL, we are executing an
> > unnecessary clear_eld(connector) and an unnecessary
> > drm_reset_display_info(connector)
> > because the fields will be set in the next drm_edid_to_eld(connector, edid)
> > and
> > drm_add_display_info(connector, edid)
> >
> > Do we want this ?
>
> Seems fine by me. And maybe we could nuke the second
> drm_reset_display_info() from deeper inside drm_add_display_info()?
> Not sure if drm_add_display_info() still has to be able to operate
> standalone or not.
>
> Hmm. Another option is to just move all these NULL/invalid edid
> checks into drm_edid_to_eld() and drm_add_display_info().
But maybe that's not so easy. Would still need to bail out
from drm_add_edid_modes() I guess.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
> On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> > > According to the documentation, drm_add_edid_modes
> > > "... Also fills out the _display_info structure and ELD in @connector
> > > with any information which can be derived from the edid."
> > >
> > > drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> > > value or may be null. When it is not null, connector->display_info and
> > > connector->eld are updated according to the edid. When edid=NULL, only
> > > connector->eld is reset. Reset connector->display_info to be consistent
> > > and accurate.
> > >
> > > Signed-off-by: Claudio Suarez
> > > ---
> > > drivers/gpu/drm/drm_edid.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 6325877c5fd6..6cbe09b2357c 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> > > *connector, struct edid *edid)
> > >
> > > if (edid == NULL) {
> > > clear_eld(connector);
> > > + drm_reset_display_info(connector);
> > > return 0;
> > > }
> > > if (!drm_edid_is_valid(edid)) {
> > > clear_eld(connector);
> > > + drm_reset_display_info(connector);
> >
> > Looks easier if you pull both of those out from these branches and
> > just call them unconditionally at the start.
>
> After looking at the full code, I am not sure. This is the code:
> ==
> int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> {
> int num_modes = 0;
> u32 quirks;
>
> if (edid == NULL) {
> clear_eld(connector);
> drm_reset_display_info(connector); <--- added by me
> return 0;
> }
> if (!drm_edid_is_valid(edid)) {
> clear_eld(connector);
> drm_reset_display_info(connector); <--- added by me
> drm_warn(connector->dev, "%s: EDID invalid.\n",
> connector->name);
> return 0;
> }
>
> drm_edid_to_eld(connector, edid);
>
> quirks = drm_add_display_info(connector, edid);
> etc...
> =
>
> If we move those out of these branches and edid != NULL, we are executing an
> unnecessary clear_eld(connector) and an unnecessary
> drm_reset_display_info(connector)
> because the fields will be set in the next drm_edid_to_eld(connector, edid)
> and
> drm_add_display_info(connector, edid)
>
> Do we want this ?
Seems fine by me. And maybe we could nuke the second
drm_reset_display_info() from deeper inside drm_add_display_info()?
Not sure if drm_add_display_info() still has to be able to operate
standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid
checks into drm_edid_to_eld() and drm_add_display_info().
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
> On Fri, 15 Oct 2021, Claudio Suarez wrote:
> > Once EDID is parsed, the monitor HDMI support information is available
> > through drm_display_info.is_hdmi. Retriving the same information with
> > drm_detect_hdmi_monitor() is less efficient. Change to
> > drm_display_info.is_hdmi where possible.
> >
> > This is a TODO task in Documentation/gpu/todo.rst
> >
> > Signed-off-by: Claudio Suarez
> > ---
> > drivers/gpu/drm/i915/display/intel_connector.c | 5 +
> > drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> > 4 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
> > b/drivers/gpu/drm/i915/display/intel_connector.c
> > index 9bed1ccecea0..3346b55df6e1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_connector.c
> > +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> > @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector
> > *connector,
> > return ret;
> > }
> >
> > +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> > +{
> > + return connector->display_info.is_hdmi;
> > +}
> > +
>
> A helper like this belongs in drm, not i915. Seems useful in other
> drivers too.
Not sure it's actually helpful for i915. We end up having to root around
in the display_info in a lot of places anyway. So a helper for single
boolean seems a bit out of place perhaps.
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 15, 2021 at 01:37:13PM +0200, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_connector.c | 5 +
> drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
> b/drivers/gpu/drm/i915/display/intel_connector.c
> index 9bed1ccecea0..3346b55df6e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector,
> return ret;
> }
>
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> +{
> + return connector->display_info.is_hdmi;
> +}
> +
> static const struct drm_prop_enum_list force_audio_names[] = {
> { HDMI_AUDIO_OFF_DVI, "force-dvi" },
> { HDMI_AUDIO_OFF, "off" },
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.h
> b/drivers/gpu/drm/i915/display/intel_connector.h
> index 661a37a3c6d8..ceda6e72ece6 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.h
> +++ b/drivers/gpu/drm/i915/display/intel_connector.h
> @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector
> *connector);
> int intel_connector_update_modes(struct drm_connector *connector,
>struct edid *edid);
> int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter
> *adapter);
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector);
> void intel_attach_force_audio_property(struct drm_connector *connector);
> void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
> void intel_attach_aspect_ratio_property(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..2b1d7c5bebdd 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink =
> intel_connector_is_hdmi_monitor(connector);
Hmm. Have we parse the EDID by this point actually? I don't think that
was the case in the past but maybe it changed at some point.
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..a32279e4fee8 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> intel_connector_is_hdmi_monitor(connector);
FYI there's a third copy of this in intel_dp.c
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> According to the documentation, drm_add_edid_modes
> "... Also fills out the _display_info structure and ELD in @connector
> with any information which can be derived from the edid."
>
> drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> value or may be null. When it is not null, connector->display_info and
> connector->eld are updated according to the edid. When edid=NULL, only
> connector->eld is reset. Reset connector->display_info to be consistent
> and accurate.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_edid.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6325877c5fd6..6cbe09b2357c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> *connector, struct edid *edid)
>
> if (edid == NULL) {
> clear_eld(connector);
> + drm_reset_display_info(connector);
> return 0;
> }
> if (!drm_edid_is_valid(edid)) {
> clear_eld(connector);
> + drm_reset_display_info(connector);
Looks easier if you pull both of those out from these branches and
just call them unconditionally at the start.
> drm_warn(connector->dev, "%s: EDID invalid.\n",
>connector->name);
> return 0;
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > > > Sean, could you revert the whole patch series? I'll have a deeper look into > > the > > patch set and come up with a v3 where all these issues will be addressed. > > > > Hi Sean, > > I now understand the nature of the issue that caused the problem with i915 and > have proceed to remove the global context structure (which revealed a similar > issue in the amdgpu driver). > > I have prepared a V3 version of the patch set where these issues should > hopefully be fixed for both the i915 and amdgpu drivers. > > In order to prevent causing more disruption, could you tell me what the proper > way to proceed would be? In particular: > > 1. Is there any place where I can push my changes so that they are tested > on a i915 machine? (Some type of automated pool) cc:intel-gfx, which it looks like you did, _but_ your patches did did not even apply against drm-tip so our CI rejected it. There was a reply to the patches from CI indicating that. And that is one reason I probably just ignored the whole thing. If it doesn't even apply/build it's not worth my time to read. > > 2. I can test the amdgpu driver on my machine but, what about all the other > architectures? What is the standard procedure? Should I simply publish V3 > and wait for feedback from the different vendors? (I would hate to cause > a > simular situation again) > > 3. Should I post V3 on top of drm-next or drm-misc-next? The normal rule is: always work on drm-tip. That is what gets tested by our CI as well. Yes, it does mean a bit of extra hurdles during development since drm-tip is a rebasing tree, but there are tools like dim retip to help out here. As for where to merge them. I would generally recommed against merging i915 patches through drm-misc unless there is a very compelling reason to do so. i915 is a fast moving target and if there are significant changes coming in via drm-misc they usually will cause conflicts for people during drm-tip rebuild. Also I would expect to see an ack requested from i915 maintainers for merging anything significant via drm-misc, which I don't think happened in this case. -- Ville Syrjälä Intel
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote:
> On 21/10/02 09:13AM, Fernando Ramos wrote:
> > On 21/10/02 05:30AM, Ville Syrjälä wrote:
> > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > > > >
> > > > > > > Thank you for revising, Fernando! I've pushed the set to
> > > > > > > drm-misc-next (along
> > > > > > > with the necessary drm-tip conflict resolutions).
> > > > > >
> > > > > > Ugh. Did anyone actually review the locking changes this does?
> > > > > > I shot the previous i915 stuff down because the commit messages
> > > > > > did not address any of it.
> > > > >
> > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread.
> > > >
> > > > It was much earlir than that.
> > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
>
> Sorry, I'm new to this and it did not occur to me to search for similar
> patches
> in the mailing list archives in case there were additional comments that
> applied
> to my change set.
>
> In case I had done that I would have found that, as you mentioned, you had
> already raised two issues back in June:
>
> On Tue, Jun 29, 2021, Ville Syrjälä wrote:
> >
> > That looks wrong. You're using a private ctx here, but still
> > passing dev->mode_config.acquire_ctx to the lower level stuff.
> >
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> The first one was pointing out the same error I would later repeat in my patch
> series (ups).
>
> After further inspection of the code it looks to me that changing this:
>
> intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>
> ...into this:
>
> intel_modeset_setup_hw_state(dev, );
>
> ...would be enough.
Yes.
>
> Why? The only difference between the old drm_modeset_{lock,unlock}_all()
> functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the
> former use a global context stored in dev->mode_config.acquire_ctx while the
> latter depend on a user provided one (typically in the stack).
>
> In the old (working) code the global context structure is freed in
> drm_modeset_unlock_all() thus we are sure no one is holding a reference to it
> at
> that point. This means that as long as no one accesses the global
> dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN
> and unlock/END, the code should be equivalent before and after my changes.
>
> In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all()
> functions, the acquire_ctx field of the drm_mode_config structure should be
> deleted:
>
> /**
> * @acquire_ctx:
> *
> * Global implicit acquire context used by atomic drivers for legacy
> * IOCTLs. Deprecated, since implicit locking contexts make it
> * impossible to use driver-private drm_modeset_lock. Users of
> * this must hold @mutex.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> If I had done that (ie. removing this field) I would have detected the problem
> when compiling.
>
> There is another place (in the amdgpu driver) where this field is still being
> referenced, but before I investigate that I would like to know if you agree
> that
> this is a good path to follow.
Yeah, removing the mode_config.acquire_ctx is a good idea if it's
no longer needed.
>
> Regarding the second issue you raised...
>
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> ...the only difference regarding mode_config.mutex I see is that in the new
> macros the mutex is locked only under this condition:
>
> if (!drm_drv_uses_atomic_modeset(dev))
>
> ...which seems reasonable, right? Is this what you were referring to or is it
> something else?
In order to eliminate the lock one first has to determine what that lock
might be protecting here, and then prove that such protection is not
actually needed.
--
Ville Syrjälä
Intel
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > > Hi all,
> > > > >
> > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > > was to
> > > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > > what this
> > > > > patch series is about.
> > > > >
> > > > > You will find two types of changes here:
> > > > >
> > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > > boilerplate) with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as
> > > > > it has
> > > > > already been done in previous commits such as b7ea04d2)
> > > > >
> > > > > - Replacing "drm_modeset_lock_all()" with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > > in the remaining places (as it has already been done in previous
> > > > > commits
> > > > > such as 57037094)
> > > > >
> > > > > Most of the changes are straight forward, except for a few cases in
> > > > > the "amd"
> > > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can
> > > > > only be used
> > > > > once inside the same function (the reason being that the macro
> > > > > expansion
> > > > > includes *labels*, and you can not have two labels named the same
> > > > > inside one
> > > > > function)
> > > > >
> > > > > Notice that, even after this patch series, some places remain where
> > > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > > present,
> > > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > > "i915")
> > > > > which cannot be replaced due to the way they are being used.
> > > > >
> > > > > Changes in v2:
> > > > >
> > > > > - Fix commit message typo
> > > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > > - Split drm/i915 patch into two simpler ones
> > > > > - Remove drm_modeset_(un)lock_all()
> > > > > - Fix build problems in non-x86 platforms
> > > > >
> > > > > Fernando Ramos (17):
> > > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup:
> > > > > drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> &
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > Hi all,
> > > >
> > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > was to
> > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > what this
> > > > patch series is about.
> > > >
> > > > You will find two types of changes here:
> > > >
> > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > boilerplate) with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it
> > > > has
> > > > already been done in previous commits such as b7ea04d2)
> > > >
> > > > - Replacing "drm_modeset_lock_all()" with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > in the remaining places (as it has already been done in previous
> > > > commits
> > > > such as 57037094)
> > > >
> > > > Most of the changes are straight forward, except for a few cases in the
> > > > "amd"
> > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only
> > > > be used
> > > > once inside the same function (the reason being that the macro expansion
> > > > includes *labels*, and you can not have two labels named the same
> > > > inside one
> > > > function)
> > > >
> > > > Notice that, even after this patch series, some places remain where
> > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > present,
> > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > "i915")
> > > > which cannot be replaced due to the way they are being used.
> > > >
> > > > Changes in v2:
> > > >
> > > > - Fix commit message typo
> > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > - Split drm/i915 patch into two simpler ones
> > > > - Remove drm_modeset_(un)lock_all()
> > > > - Fix build problems in non-x86 platforms
> > > >
> > > > Fernando Ramos (17):
> > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/vmwgfx: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > > >
> > >
> > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > > (along
> > > with the necessary drm-tip conflict resolutions).
> >
> > Ugh. Did anyone actually review the locking changes this does?
> > I shot the previous i915 stuff down because the commit messages
> > did not address any of it.
>
> I reviewed the set on 9/17, I didn't see your feedback on that thread.
It was much earlir than that.
https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
And I think I might have also shot down a similar thing earlier.
I was actually half considering sending a patch to nuke that
misleading TODO item. I don't think anything which changes
which locks are taken should be considred a starter level task.
And the commit messages here don't seem to address any of it.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > Hi all,
> >
> > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
> > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what
> > this
> > patch series is about.
> >
> > You will find two types of changes here:
> >
> > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate)
> > with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
> > already been done in previous commits such as b7ea04d2)
> >
> > - Replacing "drm_modeset_lock_all()" with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > in the remaining places (as it has already been done in previous commits
> > such as 57037094)
> >
> > Most of the changes are straight forward, except for a few cases in the
> > "amd"
> > and "i915" drivers where some extra dancing was needed to overcome the
> > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be
> > used
> > once inside the same function (the reason being that the macro expansion
> > includes *labels*, and you can not have two labels named the same inside one
> > function)
> >
> > Notice that, even after this patch series, some places remain where
> > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
> > all inside drm core (which makes sense), except for two (in "amd" and
> > "i915")
> > which cannot be replaced due to the way they are being used.
> >
> > Changes in v2:
> >
> > - Fix commit message typo
> > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > - Split drm/i915 patch into two simpler ones
> > - Remove drm_modeset_(un)lock_all()
> > - Fix build problems in non-x86 platforms
> >
> > Fernando Ramos (17):
> > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: remove drm_modeset_(un)lock_all()
> > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> >
>
> Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along
> with the necessary drm-tip conflict resolutions).
Ugh. Did anyone actually review the locking changes this does?
I shot the previous i915 stuff down because the commit messages
did not address any of it.
--
Ville Syrjälä
Intel
Re: [Freedreno] [v8 0/6] drm: Support basic DPCD backlight in panel-simple and add a new panel ATNA33XC20
On Fri, Jul 09, 2021 at 06:54:05AM -0700, Doug Anderson wrote: > Hi, > > On Sat, Jun 26, 2021 at 9:52 AM Rajeev Nandan wrote: > > > > This series adds the support for the eDP panel that needs the backlight > > controlling over the DP AUX channel using DPCD registers of the panel > > as per the VESA's standard. > > > > This series also adds support for the Samsung eDP AMOLED panel that > > needs DP AUX to control the backlight, and introduces new delays in the > > @panel_desc.delay to support this panel. > > > > This patch series depends on the following two series: > > - Doug's series [1], exposed the DP AUX channel to the panel-simple. > > - Lyude's series [2], introduced new drm helper functions for DPCD > > backlight. > > > > This series is the logical successor to the series [3]. > > > > Changes in v1: > > - Created dpcd backlight helper with very basic functionality, added > > backlight registration in the ti-sn65dsi86 bridge driver. > > > > Changes in v2: > > - Created a new DisplayPort aux backlight driver and moved the code from > > drm_dp_aux_backlight.c (v1) to the new driver. > > > > Changes in v3: > > - Fixed module compilation (kernel test bot). > > > > Changes in v4: > > - Added basic DPCD backlight support in panel-simple. > > - Added support for a new Samsung panel ATNA33XC20 that needs DPCD > > backlight controlling and has a requirement of delays between enable > > GPIO and regulator. > > > > Changes in v5: > > Addressed review suggestions from Douglas: > > - Created a new API drm_panel_dp_aux_backlight() in drm_panel.c > > - Moved DP AUX backlight functions from panel-simple.c to drm_panel.c > > - panel-simple probe() calls drm_panel_dp_aux_backlight() to create > > backlight when the backlight phandle is not specified in panel DT > > and DP AUX channel is present. > > - Added check for drm_edp_backlight_supported() before registering. > > - Removed the @uses_dpcd_backlight flag from panel_desc as this > > should be auto-detected. > > - Updated comments/descriptions. > > > > Changes in v6: > > - Rebased > > - Updated wanrning messages, fixed word wrapping in comments. > > - Fixed ordering of memory allocation > > > > Changes in v7: > > - Updated the disable_to_power_off and power_to_enable panel delays > > as discovered at <https://crrev.com/c/2966167> (Douglas) > > > > Changes in v8: > > - Now using backlight_is_blank() to get the backlight blank status (Sam > > Ravnborg) > > - Added a new patch #4 to fix the warnings for eDP panel description (Sam > > Ravnborg) > > > > [1] > > https://lore.kernel.org/dri-devel/20210525000159.3384921-1-diand...@chromium.org/ > > [2] > > https://lore.kernel.org/dri-devel/20210514181504.565252-1-ly...@redhat.com/ > > [3] > > https://lore.kernel.org/dri-devel/1619416756-3533-1-git-send-email-rajee...@codeaurora.org/ > > > > Rajeev Nandan (6): > > drm/panel: add basic DP AUX backlight support > > drm/panel-simple: Support DP AUX backlight > > drm/panel-simple: Support for delays between GPIO & regulator > > drm/panel-simple: Update validation warnings for eDP panel description > > dt-bindings: display: simple: Add Samsung ATNA33XC20 > > drm/panel-simple: Add Samsung ATNA33XC20 > > > > .../bindings/display/panel/panel-simple.yaml | 2 + > > drivers/gpu/drm/drm_panel.c| 108 > > + > > drivers/gpu/drm/panel/panel-simple.c | 73 +- > > include/drm/drm_panel.h| 15 ++- > > 4 files changed, 190 insertions(+), 8 deletions(-) > > Pushed to drm-misc-next. > > 4bfe6c8f7c23 drm/panel-simple: Add Samsung ATNA33XC20 > c20dec193584 dt-bindings: display: simple: Add Samsung ATNA33XC20 > 13aceea56fd5 drm/panel-simple: Update validation warnings for eDP > panel description > 18a1488bf1e1 drm/panel-simple: Support for delays between GPIO & regulator > bfd451403d70 drm/panel-simple: Support DP AUX backlight > 10f7b40e4f30 drm/panel: add basic DP AUX backlight support depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper Looks to be due to drm_edp_backlight_enable(). -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 03/20] drm/dp: Move i2c init to drm_dp_aux_init, add __must_check and fini
On Mon, Apr 19, 2021 at 06:55:05PM -0400, Lyude Paul wrote: > When moving around drm_dp_aux_register() calls, it turned out we > accidentally managed to cause issues with the Tegra driver due to the fact > the Tegra driver would attempt to retrieve a reference to the AUX channel's > i2c adapter - which wouldn't be initialized until drm_dp_aux_register() is > called. > > This doesn't actually make a whole ton of sense, as it's not unexpected for > a driver to need to be able to use an AUX adapter before it's been > registered. Likewise-it's not unexpected for a driver to try using the i2c > adapter for said AUX channel before it's been registered as well. In fact, > the current documentation for drm_dp_aux_init() even seems to imply that > drm_dp_aux_init() is supposed to be handling i2c adapter creation for this > precise reason - not drm_dp_aux_register(). > > Since the i2c adapter doesn't need to be linked to the DRM device in any > way, we can just fix this problem by moving i2c adapter creation out of > drm_dp_aux_register() and into drm_dp_aux_init(). Additionally, since this > means that drm_dp_aux_init() can fail we go ahead and add a __must_check > attribute to it so that drivers don't ignore its return status on failures. > And finally, we add a drm_dp_aux_fini() and hook it up in all DRM drivers > across the kernel to take care of cleaning up the i2c adapter once it's no > longer needed. > > This should also fix the regressions noted in the Tegra driver. The init vs. register split is intentional. Registering the thing and allowing userspace access to it before the rest of the driver is ready isn't particularly great. For a while now we've tried to move towards an architecture where the driver is fully initialzied before anything gets exposed to userspace. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 10/11] drm: Use state helper instead of the plane state pointer
On Thu, Jan 21, 2021 at 05:35:35PM +0100, Maxime Ripard wrote:
> Many drivers reference the plane->state pointer in order to get the
> current plane state in their atomic_update or atomic_disable hooks,
> which would be the new plane state in the global atomic state since
> _swap_state happened when those hooks are run.
>
> Use the drm_atomic_get_new_plane_state helper to get that state to make it
> more obvious.
>
> This was made using the coccinelle script below:
>
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
>
> (
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_disable = func,
> ...,
> };
> |
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_update = func,
> ...,
> };
> )
>
> @ adds_new_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> identifier new_state;
> @@
>
> func(struct drm_plane *plane, struct drm_atomic_state *state)
> {
> ...
> - struct drm_plane_state *new_state = plane->state;
> + struct drm_plane_state *new_state =
> drm_atomic_get_new_plane_state(state, plane);
> ...
> }
>
> @ include depends on adds_new_state @
> @@
>
> #include
>
> @ no_include depends on !include && adds_new_state @
> @@
>
> + #include
> #include
>
> Signed-off-by: Maxime Ripard
Looks great.
Reviewed-by: Ville Syrjälä
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 08/11] drm: Rename plane->state variables in atomic update and disable
On Mon, Jan 25, 2021 at 11:52:18AM +0100, Maxime Ripard wrote:
> Hi Ville,
>
> On Fri, Jan 22, 2021 at 02:15:07PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 21, 2021 at 05:35:33PM +0100, Maxime Ripard wrote:
> > > Some drivers are storing the plane->state pointer in atomic_update and
> > > atomic_disable in a variable simply called state, while the state passed
> > > as an argument is called old_state.
> > >
> > > In order to ease subsequent reworks and to avoid confusing or
> > > inconsistent names, let's rename those variables to new_state.
> > >
> > > This was done using the following coccinelle script, plus some manual
> > > changes for mtk and tegra.
> > >
> > > @ plane_atomic_func @
> > > identifier helpers;
> > > identifier func;
> > > @@
> > >
> > > (
> > > static const struct drm_plane_helper_funcs helpers = {
> > > ...,
> > > .atomic_disable = func,
> > > ...,
> > > };
> > > |
> > > static const struct drm_plane_helper_funcs helpers = {
> > > ...,
> > > .atomic_update = func,
> > > ...,
> > > };
> > > )
> > >
> > > @ moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > symbol old_state;
> > > symbol state;
> > > @@
> > >
> > > func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > > {
> > > ...
> > > - struct drm_plane_state *state = plane->state;
> > > + struct drm_plane_state *new_state = plane->state;
> > > ...
> > > }
> > >
> > > @ depends on moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > identifier old_state;
> > > symbol state;
> > > @@
> > >
> > > func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > > {
> > > <...
> > > - state
> > > + new_state
> > > ...>
> >
> > Was going to say that this migh eat something else, but I guess
> > the dependency prevents that?
>
> Yeah, the dependency takes care of this
>
> > Another way to avoid that I suppose would be to declare 'state'
> > as
> > symbol moves_new_state_old_state.state;
> >
> > That would probably make the intent a bit more obvious, even with
> > the dependency. Or does a dependency somehow automagically imply
> > that?
>
> I'm not sure if it does, but it's a symbol here not an identifier or an
> expression, so here moves_new_state_old_state.state would always resolve
> to state (and only state) anyway
Hm. Right. OK, cocci bits look good to me. Variable naming
bikeshed I'll leave to others :)
Reviewed-by: Ville Syrjälä
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 08/11] drm: Rename plane->state variables in atomic update and disable
On Thu, Jan 21, 2021 at 05:35:33PM +0100, Maxime Ripard wrote:
> Some drivers are storing the plane->state pointer in atomic_update and
> atomic_disable in a variable simply called state, while the state passed
> as an argument is called old_state.
>
> In order to ease subsequent reworks and to avoid confusing or
> inconsistent names, let's rename those variables to new_state.
>
> This was done using the following coccinelle script, plus some manual
> changes for mtk and tegra.
>
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
>
> (
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_disable = func,
> ...,
> };
> |
> static const struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_update = func,
> ...,
> };
> )
>
> @ moves_new_state_old_state @
> identifier plane_atomic_func.func;
> identifier plane;
> symbol old_state;
> symbol state;
> @@
>
> func(struct drm_plane *plane, struct drm_plane_state *old_state)
> {
> ...
> - struct drm_plane_state *state = plane->state;
> + struct drm_plane_state *new_state = plane->state;
> ...
> }
>
> @ depends on moves_new_state_old_state @
> identifier plane_atomic_func.func;
> identifier plane;
> identifier old_state;
> symbol state;
> @@
>
> func(struct drm_plane *plane, struct drm_plane_state *old_state)
> {
> <...
> - state
> + new_state
> ...>
Was going to say that this migh eat something else, but I guess
the dependency prevents that?
Another way to avoid that I suppose would be to declare 'state'
as
symbol moves_new_state_old_state.state;
That would probably make the intent a bit more obvious, even with
the dependency. Or does a dependency somehow automagically imply
that?
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 06/11] drm: Use state helper instead of plane state pointer in atomic_check
On Thu, Jan 21, 2021 at 05:35:31PM +0100, Maxime Ripard wrote:
> Many drivers reference the plane->state pointer in order to get the
> current plane state in their atomic_check hook, which would be the old
> plane state in the global atomic state since _swap_state hasn't happened
> when atomic_check is run.
>
> Use the drm_atomic_get_old_plane_state helper to get that state to make
> it more obvious.
>
> This was made using the coccinelle script below:
>
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
>
> static struct drm_plane_helper_funcs helpers = {
> ...,
> .atomic_check = func,
> ...,
> };
>
> @ replaces_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state, plane_state;
> @@
>
> func(struct drm_plane *plane, struct drm_atomic_state *state) {
> ...
> - struct drm_plane_state *plane_state = plane->state;
> + struct drm_plane_state *plane_state =
> drm_atomic_get_old_plane_state(state, plane);
> ...
> }
>
> @@
> identifier plane_atomic_func.func;
> identifier plane, state, plane_state;
> @@
>
> func(struct drm_plane *plane, struct drm_atomic_state *state) {
> struct drm_plane_state *plane_state =
> drm_atomic_get_old_plane_state(state, plane);
> ...
> - plane->state
> + plane_state
> ...
We don't need the <... ...> style here? It's been a while since
I did any serious cocci so I'm getting a bit rusty on the details...
Otherwise looks great
Reviewed-by: Ville Syrjälä
> }
>
> @ adds_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> @@
>
> func(struct drm_plane *plane, struct drm_atomic_state *state) {
> + struct drm_plane_state *old_plane_state =
> drm_atomic_get_old_plane_state(state, plane);
> ...
> - plane->state
> + old_plane_state
> ...
> }
>
> @ include depends on adds_old_state || replaces_old_state @
> @@
>
> #include
>
> @ no_include depends on !include && (adds_old_state || replaces_old_state) @
> @@
>
> + #include
> #include
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/imx/ipuv3-plane.c | 3 ++-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 4 +++-
> drivers/gpu/drm/tilcdc/tilcdc_plane.c | 3 ++-
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c
> b/drivers/gpu/drm/imx/ipuv3-plane.c
> index b5f6123850bb..6484592e3f86 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -341,7 +341,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> {
> struct drm_plane_state *new_state =
> drm_atomic_get_new_plane_state(state,
>
> plane);
> - struct drm_plane_state *old_state = plane->state;
> + struct drm_plane_state *old_state =
> drm_atomic_get_old_plane_state(state,
> +
> plane);
> struct drm_crtc_state *crtc_state;
> struct device *dev = plane->dev->dev;
> struct drm_framebuffer *fb = new_state->fb;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 4aac6217a5ad..6ce6ce09fecc 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -406,12 +406,14 @@ static int mdp5_plane_atomic_check_with_state(struct
> drm_crtc_state *crtc_state,
> static int mdp5_plane_atomic_check(struct drm_plane *plane,
> struct drm_atomic_state *state)
> {
> + struct drm_plane_state *old_plane_state =
> drm_atomic_get_old_plane_state(state,
> +
> plane);
> struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(state,
>
> plane);
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
>
> - crtc = new_plane_state->crtc ? new_plane_state->crtc :
> plane->state->crtc;
> + crtc = new_plane_state->crtc ? new_plane_state->crtc :
> old_plane_state->crtc;
> if (!crtc)
> return 0;
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index ebdd42dcaf82..c86258132432 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/
Re: [Freedreno] [PATCH 05/12] drm/msm/dpu: Stop copying around mode->private_flags
On Thu, Feb 20, 2020 at 11:24:20AM +, Emil Velikov wrote: > On Wed, 19 Feb 2020 at 20:36, Ville Syrjala > wrote: > > > > From: Ville Syrjälä > > > > The driver never sets mode->private_flags so copying > > it back and forth is entirely pointless. Stop doing it. > > > > Also drop private_flags from the tracepoint. > > > > Cc: Rob Clark > > Cc: Sean Paul > > Cc: linux-arm-...@vger.kernel.org > > Cc: freedreno@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä > > Perhaps the msm team has a WIP which makes use of it ? Maybe if it's one of them five year projects. But anyways, with an atomic driver there are certainly better ways to handle this. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 22/22] drm: Remove legacy version of get_scanout_position()
On Mon, Jan 20, 2020 at 09:23:14AM +0100, Thomas Zimmermann wrote:
> The legacy version of get_scanout_position() was only useful while
> drivers still used drm_driver.get_scanout_position(). With no such
> drivers left, the related typedef and code can be removed
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Ville Syrjälä
> ---
> drivers/gpu/drm/drm_vblank.c| 27 +++
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> include/drm/drm_vblank.h| 10 +-
> 3 files changed, 9 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 34428ce3c676..0bda7d7a0af2 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -576,9 +576,6 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
> * @get_scanout_position:
> * Callback function to retrieve the scanout position. See
> * @struct drm_crtc_helper_funcs.get_scanout_position.
> - * @get_scanout_position_legacy:
> - * Callback function to retrieve the scanout position. See
> - * @struct drm_driver.get_scanout_position.
> *
> * Implements calculation of exact vblank timestamps from given
> drm_display_mode
> * timings and current video scanout position of a CRTC.
> @@ -601,8 +598,7 @@ bool
> drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time,
> bool in_vblank_irq,
> - drm_vblank_get_scanout_position_func get_scanout_position,
> - drm_vblank_get_scanout_position_legacy_func get_scanout_position_legacy)
> + drm_vblank_get_scanout_position_func get_scanout_position)
> {
> struct drm_device *dev = crtc->dev;
> unsigned int pipe = crtc->index;
> @@ -620,7 +616,7 @@ drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> }
>
> /* Scanout position query not supported? Should not happen. */
> - if (!get_scanout_position && !get_scanout_position_legacy) {
> + if (!get_scanout_position) {
> DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
> return false;
> }
> @@ -651,19 +647,10 @@ drm_crtc_vblank_helper_get_vblank_timestamp_internal(
>* Get vertical and horizontal scanout position vpos, hpos,
>* and bounding timestamps stime, etime, pre/post query.
>*/
> - if (get_scanout_position) {
> - vbl_status = get_scanout_position(crtc,
> - in_vblank_irq,
> - , ,
> - , ,
> - mode);
> - } else {
> - vbl_status = get_scanout_position_legacy(dev, pipe,
> - in_vblank_irq,
> - , ,
> - , ,
> - mode);
> - }
> + vbl_status = get_scanout_position(crtc, in_vblank_irq,
> + , ,
> + , ,
> + mode);
>
> /* Return as no-op if scanout query unsupported or failed. */
> if (!vbl_status) {
> @@ -755,7 +742,7 @@ bool drm_crtc_vblank_helper_get_vblank_timestamp(struct
> drm_crtc *crtc,
> {
> return drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> crtc, max_error, vblank_time, in_vblank_irq,
> - crtc->helper_private->get_scanout_position, NULL);
> + crtc->helper_private->get_scanout_position);
> }
> EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp);
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 29bf847999f5..3245f7c5c84f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -886,7 +886,7 @@ bool intel_crtc_get_vblank_timestamp(struct drm_crtc
> *crtc, int *max_error,
> {
> return drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> crtc, max_error, vblank_time, in_vblank_irq,
> - i915_get_crtc_scanoutpos, NULL);
> + i915_get_crtc_scanoutpos);
> }
>
> int intel_get_crtc_scanline(struct intel_crtc *crtc)
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index a6ca8be93dd8..0f474e855e7f 100644
&
Re: [Freedreno] [Intel-gfx] [PATCH v2 03/21] drm: Add get_vblank_timestamp() to struct drm_crtc_funcs
On Thu, Jan 16, 2020 at 09:44:55AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 15.01.20 um 15:49 schrieb Ville Syrjälä:
> > On Wed, Jan 15, 2020 at 01:16:34PM +0100, Thomas Zimmermann wrote:
> >> @@ -2020,3 +2042,193 @@ int drm_crtc_queue_sequence_ioctl(struct
> >> drm_device *dev, void *data,
> >>kfree(e);
> >>return ret;
> >> }
> >> +
> >> +/*
> >> + * Helpers for struct drm_crtc_funcs
> >> + */
> >> +
> >> +/**
> >> + * drm_crtc_vblank_helper_get_vblank_timestamp_internal - precise vblank
> >> + *timestamp helper
> >> + * @dev: DRM device
> >> + * @pipe: index of CRTC whose vblank timestamp to retrieve
> >> + * @max_error: Desired maximum allowable error in timestamps (nanosecs)
> >> + * On return contains true maximum error of timestamp
> >> + * @vblank_time: Pointer to time which should receive the timestamp
> >> + * @in_vblank_irq:
> >> + * True when called from drm_crtc_handle_vblank(). Some drivers
> >> + * need to apply some workarounds for gpu-specific vblank irq quirks
> >> + * if flag is set.
> >> + * @get_scanout_position:
> >> + * Callback function to retrieve the scanout position. See
> >> + * @struct drm_crtc_helper_funcs.get_scanout_position.
> >> + *
> >> + * Implements calculation of exact vblank timestamps from given
> >> drm_display_mode
> >> + * timings and current video scanout position of a CRTC.
> >> + *
> >> + * The current implementation only handles standard video modes. For
> >> double scan
> >> + * and interlaced modes the driver is supposed to adjust the hardware mode
> >> + * (taken from _crtc_state.adjusted mode for atomic modeset drivers)
> >> to
> >> + * match the scanout position reported.
> >> + *
> >> + * Note that atomic drivers must call drm_calc_timestamping_constants()
> >> before
> >> + * enabling a CRTC. The atomic helpers already take care of that in
> >> + * drm_atomic_helper_update_legacy_modeset_state().
> >> + *
> >> + * Returns:
> >> + *
> >> + * Returns true on success, and false on failure, i.e. when no accurate
> >> + * timestamp could be acquired.
> >> + */
> >> +bool
> >> +drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> >> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time,
> >> + bool in_vblank_irq,
> >> + bool (*get_scanout_position)(struct drm_crtc *crtc,
> >> + bool in_vblank_irq, int *vpos, int
> >> *hpos,
> >> + ktime_t *stime, ktime_t *etime,
> >> + const struct drm_display_mode *mode))
> >> +{
> >> + struct drm_device *dev = crtc->dev;
> >> + unsigned int pipe = crtc->index;
> >> + struct drm_vblank_crtc *vblank = >vblank[pipe];
> >> + struct timespec64 ts_etime, ts_vblank_time;
> >> + ktime_t stime, etime;
> >> + bool vbl_status;
> >> + const struct drm_display_mode *mode;
> >> + int vpos, hpos, i;
> >> + int delta_ns, duration_ns;
> >> +
> >> + if (pipe >= dev->num_crtcs) {
> >> + DRM_ERROR("Invalid crtc %u\n", pipe);
> >> + return false;
> >> + }
> >> +
> >> + /* Scanout position query not supported? Should not happen. */
> >> + if (!get_scanout_position) {
> >> + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
> >> + return false;
> >> + }
> >> +
> >> + if (drm_drv_uses_atomic_modeset(dev))
> >> + mode = >hwmode;
> >> + else
> >> + mode = >hwmode;
> >> +
> >> + /* If mode timing undefined, just return as no-op:
> >> + * Happens during initial modesetting of a crtc.
> >> + */
> >> + if (mode->crtc_clock == 0) {
> >> + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe);
> >> + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev));
> >> + return false;
> >> + }
> >> +
> >> + /* Get current scanout position with system timestamp.
> >> + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times
> >> + * if single query takes longer than max_error nanoseconds.
> >> + *
> &g
Re: [Freedreno] [PATCH v2 07/21] drm/i915: Convert to CRTC VBLANK callbacks
et) % vtotal;
> }
>
> -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index,
> - bool in_vblank_irq, int *vpos, int *hpos,
> - ktime_t *stime, ktime_t *etime,
> - const struct drm_display_mode *mode)
> +static bool i915_get_crtc_scanoutpos(struct drm_crtc *dcrtc,
'struct drm_crtc *_crtc'
is the style we're going with these days.
> + bool in_vblank_irq,
> + int *vpos, int *hpos,
> + ktime_t *stime, ktime_t *etime,
> + const struct drm_display_mode *mode)
> {
> + struct drm_device *dev = dcrtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev,
> index));
> + struct intel_crtc *crtc = to_intel_crtc(dcrtc);
> enum pipe pipe = crtc->pipe;
> int position;
> int vbl_start, vbl_end, hsync_start, htotal, vtotal;
> @@ -879,6 +881,14 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev,
> unsigned int index,
> return true;
> }
>
> +bool i915_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
> + ktime_t *vblank_time, bool in_vblank_irq)
'intel_crtc_get_vblank_timestamp' pls.
Otherwise lgtm
Reviewed-by: Ville Syrjälä
> +{
> + return drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> + crtc, max_error, vblank_time, in_vblank_irq,
> + i915_get_crtc_scanoutpos);
> +}
> +
> int intel_get_crtc_scanline(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h
> index 812c47a9c2d6..53ec921c1c67 100644
> --- a/drivers/gpu/drm/i915/i915_irq.h
> +++ b/drivers/gpu/drm/i915/i915_irq.h
> @@ -101,10 +101,8 @@ void gen8_irq_power_well_post_enable(struct
> drm_i915_private *dev_priv,
> void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
>u8 pipe_mask);
>
> -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int pipe,
> - bool in_vblank_irq, int *vpos, int *hpos,
> - ktime_t *stime, ktime_t *etime,
> - const struct drm_display_mode *mode);
> +bool i915_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int *max_error,
> + ktime_t *vblank_time, bool in_vblank_irq);
>
> u32 i915_get_vblank_counter(struct drm_crtc *crtc);
> u32 g4x_get_vblank_counter(struct drm_crtc *crtc);
> --
> 2.24.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH v2 03/21] drm: Add get_vblank_timestamp() to struct drm_crtc_funcs
* measured. Note that this is a helper callback which is only used
> - * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the
> - * @drm_driver.get_vblank_timestamp callback.
> + * if a driver uses drm_crtc_vblank_helper_get_vblank_timestamp()
> + * for the @drm_crtc_funcs.get_vblank_timestamp callback.
>*
>* Parameters:
>*
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index c16c44052b3d..248fbd5de177 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -174,13 +174,13 @@ struct drm_vblank_crtc {
> unsigned int pipe;
> /**
>* @framedur_ns: Frame/Field duration in ns, used by
> - * drm_calc_vbltimestamp_from_scanoutpos() and computed by
> + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by
>* drm_calc_timestamping_constants().
>*/
> int framedur_ns;
> /**
>* @linedur_ns: Line duration in ns, used by
> - * drm_calc_vbltimestamp_from_scanoutpos() and computed by
> + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by
>* drm_calc_timestamping_constants().
>*/
> int linedur_ns;
> @@ -190,8 +190,8 @@ struct drm_vblank_crtc {
>*
>* Cache of the current hardware display mode. Only valid when @enabled
>* is set. This is used by helpers like
> - * drm_calc_vbltimestamp_from_scanoutpos(). We can't just access the
> - * hardware mode by e.g. looking at _crtc_state.adjusted_mode,
> + * drm_crtc_vblank_helper_get_vblank_timestamp(). We can't just access
> + * the hardware mode by e.g. looking at _crtc_state.adjusted_mode,
>* because that one is really hard to get from interrupt context.
>*/
> struct drm_display_mode hwmode;
> @@ -238,4 +238,22 @@ void drm_calc_timestamping_constants(struct drm_crtc
> *crtc,
> wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
> void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> u32 max_vblank_count);
> +
> +/*
> + * Helpers for struct drm_crtc_funcs
> + */
> +
> +bool
> +drm_crtc_vblank_helper_get_vblank_timestamp_internal(
> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time,
> + bool in_vblank_irq,
> + bool (*get_scanout_position)(struct drm_crtc *crtc,
> + bool in_vblank_irq, int *vpos, int
> *hpos,
> + ktime_t *stime, ktime_t *etime,
> + const struct drm_display_mode *mode));
Ugly alignment. Could maybe add a typedef for the function pointer if it
otherwise gets super horrible with proper alignment.
> +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc,
> + int *max_error,
> + ktime_t *vblank_time,
> + bool in_vblank_irq);
> +
> #endif
> --
> 2.24.1
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH v2 02/21] drm: Evaluate struct drm_device.vblank_disable_immediate on each use
On Wed, Jan 15, 2020 at 01:16:33PM +0100, Thomas Zimmermann wrote:
> VBLANK interrupts can be disabled immediately or with a delay, where the
> latter is the default. The former option can be selected by setting
> get_vblank_timestamp, and enabling vblank_disable_immediate in struct
> drm_device.
>
> The setup is only evaluated once when DRM initializes VBLANKs. Evaluating
> the settings on each use of vblank_disable_immediate will allow for easy
> integration of CRTC VBLANK functions.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_vblank.c | 31 ++-
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 3f1dd54cc8bb..abb085c67d82 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -481,19 +481,6 @@ int drm_vblank_init(struct drm_device *dev, unsigned int
> num_crtcs)
>
> DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
>
> - /* Driver specific high-precision vblank timestamping supported? */
> - if (dev->driver->get_vblank_timestamp)
> - DRM_INFO("Driver supports precise vblank timestamp query.\n");
> - else
> - DRM_INFO("No driver support for vblank timestamp query.\n");
> -
> - /* Must have precise timestamping for reliable vblank instant disable */
> - if (dev->vblank_disable_immediate &&
> !dev->driver->get_vblank_timestamp) {
> - dev->vblank_disable_immediate = false;
> - DRM_INFO("Setting vblank_disable_immediate to false because "
> - "get_vblank_timestamp == NULL\n");
> - }
Which drivers are so broken they set vblank_disable_immediate to true
without having the vfunc specified? IMO this code should just go away
(or converted to a WARN).
> -
> return 0;
>
> err:
> @@ -1070,6 +1057,15 @@ int drm_crtc_vblank_get(struct drm_crtc *crtc)
> }
> EXPORT_SYMBOL(drm_crtc_vblank_get);
>
> +static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int
> pipe)
> +{
> + if (!dev->vblank_disable_immediate)
> + return false;
> + if (!dev->driver->get_vblank_timestamp)
> + return false;
> + return true;
> +}
> +
> static void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_vblank_crtc *vblank = >vblank[pipe];
> @@ -1086,7 +1082,7 @@ static void drm_vblank_put(struct drm_device *dev,
> unsigned int pipe)
> return;
> else if (drm_vblank_offdelay < 0)
> vblank_disable_fn(>disable_timer);
> - else if (!dev->vblank_disable_immediate)
> + else if (__vblank_disable_immediate(dev, pipe))
> mod_timer(>disable_timer,
> jiffies + ((drm_vblank_offdelay * HZ)/1000));
> }
> @@ -1663,7 +1659,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void
> *data,
> /* If the counter is currently enabled and accurate, short-circuit
>* queries to return the cached timestamp of the last vblank.
>*/
> - if (dev->vblank_disable_immediate &&
> + if (__vblank_disable_immediate(dev, pipe) &&
> drm_wait_vblank_is_query(vblwait) &&
> READ_ONCE(vblank->enabled)) {
> drm_wait_vblank_reply(dev, pipe, >reply);
> @@ -1820,7 +1816,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned
> int pipe)
>* been signaled. The disable has to be last (after
>* drm_handle_vblank_events) so that the timestamp is always accurate.
>*/
> - disable_irq = (dev->vblank_disable_immediate &&
> + disable_irq = (__vblank_disable_immediate(dev, pipe) &&
> drm_vblank_offdelay > 0 &&
> !atomic_read(>refcount));
>
> @@ -1893,7 +1889,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev,
> void *data,
> pipe = drm_crtc_index(crtc);
>
> vblank = >vblank[pipe];
> - vblank_enabled = dev->vblank_disable_immediate &&
> READ_ONCE(vblank->enabled);
> + vblank_enabled = __vblank_disable_immediate(dev, pipe) &&
> + READ_ONCE(vblank->enabled);
>
> if (!vblank_enabled) {
> ret = drm_crtc_vblank_get(crtc);
> --
> 2.24.1
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
On Fri, Jan 10, 2020 at 03:56:06PM +0200, Jani Nikula wrote: > On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > > Hi > > > > Am 10.01.20 um 12:59 schrieb Jani Nikula: > >> On Fri, 10 Jan 2020, Thomas Zimmermann wrote: > >>> The callback struct drm_driver.get_scanout_position() is deprecated in > >>> favor of struct drm_crtc_helper_funcs.get_scanout_position(). > >>> > >>> i915 doesn't use CRTC helpers. The patch duplicates the caller > >>> drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback > >>> function is not needed. > >>> > >>> Signed-off-by: Thomas Zimmermann > >>> --- > >>> drivers/gpu/drm/i915/i915_drv.c | 3 +- > >>> drivers/gpu/drm/i915/i915_irq.c | 117 ++-- > >>> drivers/gpu/drm/i915/i915_irq.h | 9 +-- > >>> 3 files changed, 119 insertions(+), 10 deletions(-) > >> > >> Not really enthusiastic about the diffstat in a "cleanup" series. > > > > Well, the cleanup is about the content of drm_driver :) > > > >> > >> I wonder if you could add a generic helper version of > >> drm_calc_vbltimestamp_from_scanoutpos where you pass the > >> get_scanout_position function as a parameter. Both > >> drm_calc_vbltimestamp_from_scanoutpos and the new > >> i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin > >> wrappers passing in the relevant get_scanout_position function. > > > > Of course. Will be in v2 of the series. > > Please give Ville (Cc'd) a moment before sending v2 in case he wants to > chime in on this. Passing the function pointer was one option I considered for this a while back. Can't remeber what other solutions I condsidered. But I guess I didn't like any of them enough to make an actual patch. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm/msm/dpu: add rotation property
On Wed, Aug 21, 2019 at 06:57:24PM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 45bfac9e3af7..c5653771e8fa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1040,8 +1040,21 @@ static void dpu_plane_sspp_atomic_update(struct
> drm_plane *plane)
> pstate->multirect_mode);
>
> if (pdpu->pipe_hw->ops.setup_format) {
> + unsigned int rotation;
> +
> src_flags = 0x0;
>
> + rotation = drm_rotation_simplify(state->rotation,
> + DRM_MODE_ROTATE_0 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y);
> +
> + if (rotation & DRM_MODE_REFLECT_X)
> + src_flags |= DPU_SSPP_FLIP_UD;
> +
> + if (rotation & DRM_MODE_REFLECT_Y)
> + src_flags |= DPU_SSPP_FLIP_LR;
> +
UD vs. LR (assuming those mean what I think they mean) seem the wrong
way around here.
>
> /* update format */
> pdpu->pipe_hw->ops.setup_format(pdpu->pipe_hw, fmt, src_flags,
> pstate->multirect_index);
> @@ -1522,6 +1535,13 @@ struct drm_plane *dpu_plane_init(struct drm_device
> *dev,
> if (ret)
> DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>
> + drm_plane_create_rotation_property(plane,
> + DRM_MODE_ROTATE_0,
> + DRM_MODE_ROTATE_0 |
> + DRM_MODE_ROTATE_180 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y);
> +
> drm_plane_enable_fb_damage_clips(plane);
>
> /* success! finalize initialization */
> --
> 2.21.0
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [RFC] Expanding drm_mode_modeinfo flags
> involve the drm objects which are seamless modeset aware (you could imagine > a bridge chain where the bridges downstream of the first bridge don't care). > > > > > There's then still the question of how to pick video vs command mode, but > > imo better to start with implementing the transition behaviour correctly > > first. > > Connector property? Possibly a terrible idea, but I wonder if we could [re]use > the vrr properties for command mode. The docs state that the driver has the > option of putting upper and lower bounds on the refresh rate. Not really sure why this needs new props and whatnot. This is kinda what the i915 "fastset" stuff already does: 1. userspace asks for something to be changed via atomic 2. driver calculates whether a modeset is actually required 3. atomic validates need_modeset() vs. DRM_MODE_ATOMIC_ALLOW_MODESET 4. if (need_modeset) heavyweight_commit() else lightweight_commit() Ie. why should userspace really care about anything except the "flickers are OK" vs. "flickers not wanted" thing? Also what's the benefit of using video mode if your panel supportes command mode? Can you turn off the memory in the panel and actually save power that way? And if there is a benefit can't the driver just automagically switch between the two based on how often things are getting updated? That would match how eDP PSR already works. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4 01/23] drm: Include ddc adapter pointer in struct drm_connector
On Thu, Jul 11, 2019 at 01:26:28PM +0200, Andrzej Pietrasiewicz wrote:
> Add generic code which creates symbolic links in sysfs, pointing to ddc
> interface used by a particular video output. For example:
>
> ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> -> ../../../../soc/1388.i2c/i2c-2
>
> This makes it easy for user to associate a display with its ddc adapter
> and use e.g. ddcutil to control the chosen monitor.
>
> This patch adds an i2c_adapter pointer to struct drm_connector. Particular
> drivers can then use it instead of using their own private instance. If a
> connector contains a ddc, then create a symbolic link in sysfs.
>
> Signed-off-by: Andrzej Pietrasiewicz
> Acked-by: Daniel Vetter
> Reviewed-by: Andrzej Hajda
> ---
> drivers/gpu/drm/drm_sysfs.c | 7 +++
> include/drm/drm_connector.h | 11 +++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ad10810bc972..26d359b39785 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -294,6 +294,9 @@ int drm_sysfs_connector_add(struct drm_connector
> *connector)
> /* Let userspace know we have a new connector */
> drm_sysfs_hotplug_event(dev);
>
> + if (connector->ddc)
> + return sysfs_create_link(>kdev->kobj,
> + >ddc->dev.kobj, "ddc");
> return 0;
> }
>
> @@ -301,6 +304,10 @@ void drm_sysfs_connector_remove(struct drm_connector
> *connector)
> {
> if (!connector->kdev)
> return;
> +
> + if (connector->ddc)
> + sysfs_remove_link(>kdev->kobj, "ddc");
> +
> DRM_DEBUG("removing \"%s\" from sysfs\n",
> connector->name);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ca745d9feaf5..1ad3d1d54ba7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -23,6 +23,7 @@
> #ifndef __DRM_CONNECTOR_H__
> #define __DRM_CONNECTOR_H__
>
> +#include
struct i2c_adapter;
would suffice.
> #include
> #include
> #include
> @@ -1308,6 +1309,16 @@ struct drm_connector {
>* [0]: progressive, [1]: interlaced
>*/
> int audio_latency[2];
> +
> + /**
> + * @ddc: associated ddc adapter.
> + * A connector usually has its associated ddc adapter. If a driver uses
> + * this field, then an appropriate symbolic link is created in connector
> + * sysfs directory to make it easy for the user to tell which i2c
> + * adapter is for a particular display.
> + */
> + struct i2c_adapter *ddc;
> +
> /**
>* @null_edid_counter: track sinks that give us all zeros for the EDID.
>* Needed to workaround some HW bugs where we get all 0s
> --
> 2.17.1
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4 23/23] drm/i915: Provide ddc symlink in hdmi connector sysfs directory
On Thu, Jul 11, 2019 at 01:26:50PM +0200, Andrzej Pietrasiewicz wrote: > Use the ddc pointer provided by the generic connector. We already have a symlink via intel_hdmi_create_i2c_symlink(). I guess we should remove that in favor of the generic one. Oleg? > > Signed-off-by: Andrzej Pietrasiewicz > --- > drivers/gpu/drm/i915/display/intel_hdmi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 0ebec69bbbfc..678fa4d1bd4e 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -3094,6 +3094,9 @@ void intel_hdmi_init_connector(struct > intel_digital_port *intel_dig_port, >intel_dig_port->max_lanes, port_name(port))) > return; > > + intel_hdmi->ddc_bus = intel_hdmi_ddc_pin(dev_priv, port); > + connector->ddc = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > + > drm_connector_init(dev, connector, _hdmi_connector_funcs, > DRM_MODE_CONNECTOR_HDMIA); > drm_connector_helper_add(connector, _hdmi_connector_helper_funcs); > @@ -3105,8 +3108,6 @@ void intel_hdmi_init_connector(struct > intel_digital_port *intel_dig_port, > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > connector->ycbcr_420_allowed = true; > > - intel_hdmi->ddc_bus = intel_hdmi_ddc_pin(dev_priv, port); > - > if (WARN_ON(port == PORT_A)) > return; > intel_encoder->hpd_pin = intel_hpd_pin_default(dev_priv, port); > -- > 2.17.1 -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/2] drm: Pretty print mode flags
On Thu, Jun 20, 2019 at 10:25:42PM +0200, Sam Ravnborg wrote:
> Hi Ville.
>
> On Thu, Jun 20, 2019 at 09:50:48PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Decode the mode flags when printing the modeline so that I
> > no longer have to decode the hex number myself.
> You are extending the current way to print mode flags,
> but I would anyway like to point out a different approach.
>
> When I need to print a fourcc code it is as simple as:
>
> {
> struct drm_format_name_buf fbuf;
>
> printk("My format: %s\n", drm_get_format_name(format, );
> }
>
> This way to handle this feels more straightforward
> than the current approach used for modes.
>
> Maybe bikeshedding, as your mileage may vary.
You're suggesting to print the whole mode to a temp buffer, and then
print that with just %s? I thought about that, but it would waste
even more stack so didn't think it was all that great.
>
> A middle ground could be to introduce a struct for the buf so we know
> the callers do it right.
I'm not a fan of that struct approach because it forces you to use
a dedicated buffer for the thing. With just a raw char[] you can
print to anything, so it could be used more like strcat() as well.
Also I think it makes it more clear how much stack space you're
blowing away. And it looks a bit like snprintf() so it's more
clear what's happening on. Although in this case that last point
is kinda lost due to the macro thing.
>
> Most of the code would be the same, but all call sites would need to be
> updated.
> What do you think?
>
> Sam
>
>
> >
> > To do this neatly I made the caller provide a temporary
> > on stack buffer where we can produce the results. I choce 64
> > bytes as a reasonable size for this. The worst case I think
> > is > 100 bytes but that kind of mode would be nonsense anyway
> > so I figured correct decoding isn't as important in such
> > cases.
> >
> > Cc: Russell King
> > Cc: Neil Armstrong
> > Cc: Rob Clark
> > Cc: Tomi Valkeinen
> > Cc: Thierry Reding
> > Cc: Sam Ravnborg
> > Cc: Benjamin Gaignard
> > Cc: Vincent Abriou
> > Cc: linux-amlo...@lists.infradead.org
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > Cc: Ilia Mirkin
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/armada/armada_crtc.c | 3 +-
> > drivers/gpu/drm/drm_atomic.c | 3 +-
> > drivers/gpu/drm/drm_modes.c | 116 +-
> > drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
> > drivers/gpu/drm/meson/meson_dw_hdmi.c | 3 +-
> > drivers/gpu/drm/meson/meson_venc.c| 4 +-
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 3 +-
> > .../gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 4 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 3 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 +-
> > drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +-
> > drivers/gpu/drm/msm/edp/edp_bridge.c | 3 +-
> > drivers/gpu/drm/omapdrm/omap_connector.c | 5 +-
> > drivers/gpu/drm/omapdrm/omap_crtc.c | 3 +-
> > drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 3 +-
> > drivers/gpu/drm/sti/sti_crtc.c| 3 +-
> > include/drm/drm_modes.h | 14 ++-
> > 20 files changed, 165 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> > b/drivers/gpu/drm/armada/armada_crtc.c
> > index ba4a3fab7745..ce9335682bd2 100644
> > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > @@ -262,6 +262,7 @@ static void armada_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> > unsigned long flags;
> > unsigned i;
> > bool interlaced = !!(adj->flags & DRM_MODE_FLAG_INTERLACE);
> > + char buf[DRM_MODE_FLAGS_BUF_LEN];
> >
> > i = 0;
> > rm = adj->crtc_hsync_start - adj->crtc_hdisplay;
> > @@ -270,7 +271,7 @@ static void armada_drm_crtc_mode_set_nofb(struct
> > drm_crtc *crtc)
> > tm = adj->crtc_vtotal - adj->crtc_vsync_end;
> >
> > DRM_DEBUG_KMS("[CRTC:%d:%s] mode " DRM_MODE_FMT "\n",
> > - crtc->base.id, crtc->name, DRM_MODE_ARG(adj));
> > +
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Dec 05, 2018 at 08:40:34AM +0100, Andrzej Hajda wrote:
> On 04.12.2018 20:02, Ville Syrjälä wrote:
> > On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
> >> On 03.12.2018 22:48, Ville Syrjälä wrote:
> >>> On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> >>>> Quite late, hopefully not too late.
> >>>>
> >>>>
> >>>> On 21.11.2018 12:51, Ville Syrjälä wrote:
> >>>>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>>>>>> return;
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> index a6e8f4591e63..0cc293a6ac24 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct
> >>>>>>> sii8620 *ctx,
> >>>>>>> int ret;
> >>>>>>>
> >>>>>>> ret = drm_hdmi_avi_infoframe_from_display_mode(,
> >>>>>>> -mode,
> >>>>>>> -true);
> >>>>>>> +NULL, mode);
> >>>>>>> if (ctx->use_packed_pixel)
> >>>>>>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> index 64c3cf027518..88b720b63126 100644
> >>>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi
> >>>>>>> *hdmi, struct drm_display_mode *mode)
> >>>>>>> u8 val;
> >>>>>>>
> >>>>>>> /* Initialise info frame from DRM mode */
> >>>>>>> - drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> >>>>>>> + drm_hdmi_avi_infoframe_from_display_mode(,
> >>>>>>> + >connector,
> >>>>>>> mode);
> >>>>>>>
> >>>>>>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>>>>> frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>>>> index b506e3622b08..501ac05ba7da 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct
> >>>>>>> drm_connector *connector,
> >>>>>>> }
> >>>>>>> EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>>>>>
> >>>>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >>>>>> You're usually known for adding const all around, why not const pointer
> >>>>>> here and in all the other drm_* functions that call this?
> >>>>> My current approach is to constify states/fbs/etc. but not so much
> >>>>> crtcs/connectors/etc. Too much const can sometimes get in the way
> >>>>> of things requiring that you remove the const later. But I guess
> >>>>> in this case the const shouldn't really get in the way of anything
> >>>>> because these are pretty much supposed to be pure functions.
> >>>>>
> >>>>>>> +{
> >>>>>>> + /*
> >>>>>>> + * FIXME: sil-sii8620 doesn't have a connector around when
> >>>>>>> + * we need one, so we have to be prepared for a NULL connector.
> >>>>>>> + */
> >>>>>>> + if (!connector)
> >>>>>>> + return false;
> >>>>>> This actually changes the i
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > On 03.12.2018 22:38, Ville Syrjälä wrote: > > On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > >> On 21.11.2018 19:19, Laurent Pinchart wrote: > >>> Hi Ville, > >>> > >>> Thank you for the patch. > >>> > >>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >>>> From: Ville Syrjälä > >>>> > >>>> Make life easier for drivers by simply passing the connector > >>>> to drm_hdmi_avi_infoframe_from_display_mode() and > >>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >>>> need to worry about is_hdmi2_sink mess. > >>> While this is good for display controller drivers, the change isn't great > >>> for > >>> bridge drivers. Down the road we're looking at moving connector support > >>> out of > >>> the bridge drivers. Adding an additional dependency to connectors in the > >>> bridges will make that more difficult. Ideally bridges should retrieve > >>> the > >>> information from their sink, regardless of whether it is a connector or > >>> another bridge. > >> > >> I agree with it, and case of sii8620 shows that there are cases where > >> bridge has no direct access to the connector. > > It's just a matter of plumbing it through. > > > What do you mean exactly? void bridge_foo(... + ,struct drm_connector *connector); > > > > > >> On the other side, since you are passing connector to > >> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > >> parameter and rename the function to > >> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > >> mode set on the connector differs? > > Connectors don't have a mode. > > > As they are passing video stream they should have it, even if not > directly, for example: > > connector->state->crtc->mode That's not really how atomic works. One shouldn't go digging through the obj->state pointers when we're not holding the relevant locks anymore. The atomic way would be to pass either both crtc state and connector state, or drm_atomic_state + crtc/connector. > > In moment of creating infoframe it should be set properly. > > > Regards > > Andrzej > > > > > >> > >> Regards > >> > >> Andrzej > >> > >> > >>> Please see below for an additional comment. > >>> > >>>> Cc: Alex Deucher > >>>> Cc: "Christian König" > >>>> Cc: "David (ChunMing) Zhou" > >>>> Cc: Archit Taneja > >>>> Cc: Andrzej Hajda > >>>> Cc: Laurent Pinchart > >>>> Cc: Inki Dae > >>>> Cc: Joonyoung Shim > >>>> Cc: Seung-Woo Kim > >>>> Cc: Kyungmin Park > >>>> Cc: Russell King > >>>> Cc: CK Hu > >>>> Cc: Philipp Zabel > >>>> Cc: Rob Clark > >>>> Cc: Ben Skeggs > >>>> Cc: Tomi Valkeinen > >>>> Cc: Sandy Huang > >>>> Cc: "Heiko Stübner" > >>>> Cc: Benjamin Gaignard > >>>> Cc: Vincent Abriou > >>>> Cc: Thierry Reding > >>>> Cc: Eric Anholt > >>>> Cc: Shawn Guo > >>>> Cc: Ilia Mirkin > >>>> Cc: amd-...@lists.freedesktop.org > >>>> Cc: linux-arm-...@vger.kernel.org > >>>> Cc: freedreno@lists.freedesktop.org > >>>> Cc: nouv...@lists.freedesktop.org > >>>> Cc: linux-te...@vger.kernel.org > >>>> Signed-off-by: Ville Syrjälä > >>>> --- > >>>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > >>>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > >>>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > >>>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >>>> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > >>>> drivers/gpu/drm/bridge/sii902x.c | 3 ++- > >>>> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > >>>> drivers/gpu/drm/drm_edid.c| 33 ++- > >>>> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > >>>> driver
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Dec 04, 2018 at 08:03:53AM +0100, Andrzej Hajda wrote:
> On 03.12.2018 22:48, Ville Syrjälä wrote:
> > On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> >> Quite late, hopefully not too late.
> >>
> >>
> >> On 21.11.2018 12:51, Ville Syrjälä wrote:
> >>> On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>>>> return;
> >>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> index a6e8f4591e63..0cc293a6ac24 100644
> >>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>>>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620
> >>>>> *ctx,
> >>>>> int ret;
> >>>>>
> >>>>> ret = drm_hdmi_avi_infoframe_from_display_mode(,
> >>>>> - mode,
> >>>>> - true);
> >>>>> + NULL, mode);
> >>>>> if (ctx->use_packed_pixel)
> >>>>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> index 64c3cf027518..88b720b63126 100644
> >>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>>>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
> >>>>> struct drm_display_mode *mode)
> >>>>> u8 val;
> >>>>>
> >>>>> /* Initialise info frame from DRM mode */
> >>>>> - drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> >>>>> + drm_hdmi_avi_infoframe_from_display_mode(,
> >>>>> +>connector,
> >>>>> mode);
> >>>>>
> >>>>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>>>> frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>> index b506e3622b08..501ac05ba7da 100644
> >>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct
> >>>>> drm_connector *connector,
> >>>>> }
> >>>>> EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>>>
> >>>>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >>>> You're usually known for adding const all around, why not const pointer
> >>>> here and in all the other drm_* functions that call this?
> >>> My current approach is to constify states/fbs/etc. but not so much
> >>> crtcs/connectors/etc. Too much const can sometimes get in the way
> >>> of things requiring that you remove the const later. But I guess
> >>> in this case the const shouldn't really get in the way of anything
> >>> because these are pretty much supposed to be pure functions.
> >>>
> >>>>> +{
> >>>>> + /*
> >>>>> +* FIXME: sil-sii8620 doesn't have a connector around when
> >>>>> +* we need one, so we have to be prepared for a NULL connector.
> >>>>> +*/
> >>>>> + if (!connector)
> >>>>> + return false;
> >>>> This actually changes the is_hdmi2_sink value for sil-sii8620.
> >>> Hmm. No idea why they would have set that to true when everyone else is
> >>> passing false.
> >>
> >> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> >> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
> >>
> >> Unfortunately I have no access to MHL specs, but my experiments and
> >> vendor drivers strongly suggests it is done this way.
> >>
> >> This is important in case of 4K modes which are handled differently by
> &g
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 09:46:16AM +0100, Andrzej Hajda wrote:
> Quite late, hopefully not too late.
>
>
> On 21.11.2018 12:51, Ville Syrjälä wrote:
> > On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> >>
> >>> return;
> >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> index a6e8f4591e63..0cc293a6ac24 100644
> >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> >>> @@ -1104,8 +1104,7 @@ static void sii8620_set_infoframes(struct sii8620
> >>> *ctx,
> >>> int ret;
> >>>
> >>> ret = drm_hdmi_avi_infoframe_from_display_mode(,
> >>> -mode,
> >>> -true);
> >>> +NULL, mode);
> >>> if (ctx->use_packed_pixel)
> >>> frm.avi.colorspace = HDMI_COLORSPACE_YUV422;
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> index 64c3cf027518..88b720b63126 100644
> >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >>> @@ -1344,7 +1344,8 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
> >>> struct drm_display_mode *mode)
> >>> u8 val;
> >>>
> >>> /* Initialise info frame from DRM mode */
> >>> - drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> >>> + drm_hdmi_avi_infoframe_from_display_mode(,
> >>> + >connector, mode);
> >>>
> >>> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >>> frame.colorspace = HDMI_COLORSPACE_YUV444;
> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>> index b506e3622b08..501ac05ba7da 100644
> >>> --- a/drivers/gpu/drm/drm_edid.c
> >>> +++ b/drivers/gpu/drm/drm_edid.c
> >>> @@ -4830,19 +4830,32 @@ void drm_set_preferred_mode(struct drm_connector
> >>> *connector,
> >>> }
> >>> EXPORT_SYMBOL(drm_set_preferred_mode);
> >>>
> >>> +static bool is_hdmi2_sink(struct drm_connector *connector)
> >> You're usually known for adding const all around, why not const pointer
> >> here and in all the other drm_* functions that call this?
> > My current approach is to constify states/fbs/etc. but not so much
> > crtcs/connectors/etc. Too much const can sometimes get in the way
> > of things requiring that you remove the const later. But I guess
> > in this case the const shouldn't really get in the way of anything
> > because these are pretty much supposed to be pure functions.
> >
> >>> +{
> >>> + /*
> >>> + * FIXME: sil-sii8620 doesn't have a connector around when
> >>> + * we need one, so we have to be prepared for a NULL connector.
> >>> + */
> >>> + if (!connector)
> >>> + return false;
> >> This actually changes the is_hdmi2_sink value for sil-sii8620.
> > Hmm. No idea why they would have set that to true when everyone else is
> > passing false.
>
>
> Because false does not work :) More precisely MHLv3 (used in Sii8620)
> uses CTA-861-F standard for infoframes, which is specific to HDMI2.0.
>
> Unfortunately I have no access to MHL specs, but my experiments and
> vendor drivers strongly suggests it is done this way.
>
> This is important in case of 4K modes which are handled differently by
> HDMI 1.4 and HDMI2.0.
HDMI 2.0 handles 4k just like 1.4 handled it when you use one of
the 4k modes defined in 1.4. Only if you use features beyond 1.4 do we
switch over to the HDMI 2.0 specific signalling.
>
> The pipeline looks like (in parenthesis HDMI version on the stream):
>
> exynos_hdmi --(1.4)--> SII8620 --(2.0)--> MHL_dongle --(1.4)--> TV
>
>
> > I guess I can change this to true to not change it. IIRC
> > that was the only driver that didn't have a connector around.
> >
> > That said, I was actually thinking of removing this hdmi2 vs. not
> > stuff from drm_hdmi_avi_infoframe_from_display_mode(). We added it
> > "just in case", but we already have a similar issue with earlier
> > cea-861 revisions and haven't seen any bugs where an older monitor
> > would get confused by a VIC not yet defined when the monitor was
> > produced.
> >
> Are you sure this is a good idea? As I said earlier 4K modes are present
> in HDMI 1.4 and 2.0 but they are handled differently, so this is not
> only matter of unknown VIC in AVIF.
>
>
> Regards
>
> Andrzej
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > On 21.11.2018 19:19, Laurent Pinchart wrote: > > Hi Ville, > > > > Thank you for the patch. > > > > On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >> From: Ville Syrjälä > >> > >> Make life easier for drivers by simply passing the connector > >> to drm_hdmi_avi_infoframe_from_display_mode() and > >> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >> need to worry about is_hdmi2_sink mess. > > While this is good for display controller drivers, the change isn't great > > for > > bridge drivers. Down the road we're looking at moving connector support out > > of > > the bridge drivers. Adding an additional dependency to connectors in the > > bridges will make that more difficult. Ideally bridges should retrieve the > > information from their sink, regardless of whether it is a connector or > > another bridge. > > > I agree with it, and case of sii8620 shows that there are cases where > bridge has no direct access to the connector. It's just a matter of plumbing it through. > > On the other side, since you are passing connector to > drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > parameter and rename the function to > drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > mode set on the connector differs? Connectors don't have a mode. > > > Regards > > Andrzej > > > > > > Please see below for an additional comment. > > > >> Cc: Alex Deucher > >> Cc: "Christian König" > >> Cc: "David (ChunMing) Zhou" > >> Cc: Archit Taneja > >> Cc: Andrzej Hajda > >> Cc: Laurent Pinchart > >> Cc: Inki Dae > >> Cc: Joonyoung Shim > >> Cc: Seung-Woo Kim > >> Cc: Kyungmin Park > >> Cc: Russell King > >> Cc: CK Hu > >> Cc: Philipp Zabel > >> Cc: Rob Clark > >> Cc: Ben Skeggs > >> Cc: Tomi Valkeinen > >> Cc: Sandy Huang > >> Cc: "Heiko Stübner" > >> Cc: Benjamin Gaignard > >> Cc: Vincent Abriou > >> Cc: Thierry Reding > >> Cc: Eric Anholt > >> Cc: Shawn Guo > >> Cc: Ilia Mirkin > >> Cc: amd-...@lists.freedesktop.org > >> Cc: linux-arm-...@vger.kernel.org > >> Cc: freedreno@lists.freedesktop.org > >> Cc: nouv...@lists.freedesktop.org > >> Cc: linux-te...@vger.kernel.org > >> Signed-off-by: Ville Syrjälä > >> --- > >> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > >> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > >> drivers/gpu/drm/bridge/sii902x.c | 3 ++- > >> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > >> drivers/gpu/drm/drm_edid.c| 33 ++- > >> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > >> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > >> drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > >> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > >> drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > >> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > >> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > >> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > >> drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > >> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > >> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > >> drivers/gpu/drm/tegra/hdmi.c | 3 ++- > >> drivers/gpu/drm/tegra/sor.c | 3 ++- > >> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +--- > >> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > >> include/drm/drm_edid.h| 8 +++--- > >> 27 files changed, 94 insertions(+), 66 deletions(-) > > For dw-hdmi and omapdrm, > > > > Reviewed-by: Laurent Pinchart > > -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Wed, Nov 21, 2018 at 01:40:43PM +0200, Jani Nikula wrote:
> On Tue, 20 Nov 2018, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Make life easier for drivers by simply passing the connector
> > to drm_hdmi_avi_infoframe_from_display_mode() and
> > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> > need to worry about is_hdmi2_sink mess.
>
> Overall looks about right and nice,
>
> Reviewed-by: Jani Nikula
>
> But please do take that with a grain of salt for everything outside of
> i915 and drm core.
>
> Please also find a few comments inline below.
>
> > Cc: Alex Deucher
> > Cc: "Christian König"
> > Cc: "David (ChunMing) Zhou"
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Inki Dae
> > Cc: Joonyoung Shim
> > Cc: Seung-Woo Kim
> > Cc: Kyungmin Park
> > Cc: Russell King
> > Cc: CK Hu
> > Cc: Philipp Zabel
> > Cc: Rob Clark
> > Cc: Ben Skeggs
> > Cc: Tomi Valkeinen
> > Cc: Sandy Huang
> > Cc: "Heiko Stübner"
> > Cc: Benjamin Gaignard
> > Cc: Vincent Abriou
> > Cc: Thierry Reding
> > Cc: Eric Anholt
> > Cc: Shawn Guo
> > Cc: Ilia Mirkin
> > Cc: amd-...@lists.freedesktop.org
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
> > Cc: nouv...@lists.freedesktop.org
> > Cc: linux-te...@vger.kernel.org
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++-
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++--
> > drivers/gpu/drm/bridge/sii902x.c | 3 ++-
> > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> > drivers/gpu/drm/drm_edid.c| 33 ++-
> > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++-
> > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
> > drivers/gpu/drm/i915/intel_hdmi.c | 14 +-
> > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++-
> > drivers/gpu/drm/i915/intel_sdvo.c | 10 ---
> > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++-
> > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++--
> > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++--
> > drivers/gpu/drm/radeon/radeon_audio.c | 2 +-
> > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-
> > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++-
> > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++-
> > drivers/gpu/drm/tegra/hdmi.c | 3 ++-
> > drivers/gpu/drm/tegra/sor.c | 3 ++-
> > drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +---
> > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-
> > include/drm/drm_edid.h| 8 +++---
> > 27 files changed, 94 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > index 4cfecdce29a3..1f0426d2fc2a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> > @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder
> > *encoder,
> > dce_v10_0_audio_write_sad_regs(encoder);
> > dce_v10_0_audio_write_latency_fields(encoder, mode);
> >
> > - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> > + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> > if (err < 0) {
> > DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> > return;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > index 7c868916d90f..2280b971d758 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> > @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder
> > *encoder,
> > dce_v11_0_audio_write_sad_regs(encoder);
> > dce_v11_0_audio_write_latency_fields(encoder, mode);
> >
> > - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> > + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> > if (err < 0) {
> >
Re: [Freedreno] [PATCH 6/8] drm/msm: dpu: Separate crtc assignment from vblank enable
On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote: > On 2018-11-14 07:16, Sean Paul wrote: > > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote: > >> On 2018-11-13 12:52, Sean Paul wrote: > >> > From: Sean Paul > >> > > >> > Instead of assigning/clearing the crtc on vblank enable/disable, we > > can > >> > just assign and clear the crtc on modeset. That allows us to just > > toggle > >> > the encoder's vblank interrupts on vblank_enable. > >> > > >> > So why is this important? Previously the driver was using the legacy > >> > pointers to assign/clear the crtc. Legacy pointers are cleared _after_ > >> Which pointers are you referring here as legacy pointers? CRTC? > > > > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on > > enc->crtc == crtc > > > >> > disabling the hardware, so the legacy pointer was valid during > >> > vblank_disable, but that's not something we should rely on. > >> > > >> > Instead of relying on the core ordering the legacy pointer assignments > >> > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the > >> > only place that mapping can change, so we're covered there. > >> > > >> > We're also taking advantage of drm_crtc_vblank_on/off. By using this, > > we > >> > ensure that vblank_enable/disable can never be called while the crtc > > is > >> > off (which means the assigned crtc will always be valid). As such, we > >> > >> What about the drm_vblank_enable/disable triggered by drm_vblank_get > > when > >> crtc > >> is disabled? What is the expected behavior there? the vblank_requested > >> variable removed by the next patch was introduced to cache the > >> request. > > > > That case is covered by the modeset locks held when calling disable(). > > > I am sure it will take care of drm_crtc_vblank_on/off triggered within > crtc_disable. > My question was what was the expected behaviour when > DRM_IOCTL_WAIT_VBLANK is > called when crtc is disabled? the ioctl will try to call drm_vblank_get > and I > don't see the patch checking for crtc being enabled in the path. drm_vblank_off() // .enable_vblank/.disable_vblank will never be called here drm_vblank_on() So if you use drm_vblank_on/off judiciously you will never have to deal with this particular problem. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Intel-gfx] [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere
struct drm_crtc_state *crtc_state = > + kzalloc(sizeof(*crtc->state), GFP_KERNEL); > + > if (crtc->state) > __drm_atomic_helper_crtc_destroy_state(crtc->state); > > kfree(crtc->state); > - crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL); > - > - if (crtc->state) > - crtc->state->crtc = crtc; > + __drm_atomic_helper_crtc_reset(crtc, crtc_state); > } > EXPORT_SYMBOL(drm_atomic_helper_crtc_reset); > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index f383417571ec..907ffeb64781 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15457,7 +15457,7 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > > __drm_atomic_helper_crtc_destroy_state(_state->base); > memset(crtc_state, 0, sizeof(*crtc_state)); > - crtc_state->base.crtc = >base; > + __drm_atomic_helper_crtc_reset(>base, _state->base); intel_crtc_init() could use the same treatment. And intel_create_plane_state() could use the plane reset. In fact it looks like we aren't intializing plane constant alpha at all. So it'll start out as zero which probably isn't what most people would expect. I also wonder if this patch shouldn't be split up more. Just in case there's some unforseen regression somewhere I'd hate to see the entire thing get reverted. -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 11/18] drm/msm: Use drm_atomic_helper_shutdown
On Wed, Oct 03, 2018 at 11:16:44AM +0200, Daniel Vetter wrote:
> drm_plane_helper_disable is a non-atomic drivers only function, and
> will blow up (since no one passes the locking context it needs).
>
> Atomic drivers which want to quiescent their hw on unload should
> use drm_atomic_helper_shutdown() instead.
lgtm
Reviewed-by: Ville Syrjälä
>
> Signed-off-by: Daniel Vetter
> Cc: Rob Clark
> Cc: Rajesh Yadav
> Cc: Chandan Uddaraju
> Cc: Archit Taneja
> Cc: Jeykumar Sankaran
> Cc: Sean Paul
> Cc: Maarten Lankhorst
> Cc: Sinclair Yeh
> Cc: "Ville Syrjälä"
> Cc: Russell King
> Cc: Gustavo Padovan
> Cc: Arnd Bergmann
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 --
> drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 1 -
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 1 -
> drivers/gpu/drm/msm/msm_drv.c | 1 +
> 4 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 015341e2dd4c..ec959f847d5f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1482,8 +1482,6 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>
> mutex_destroy(>lock);
>
> - drm_plane_helper_disable(plane, NULL);
> -
> /* this will destroy the states as well */
> drm_plane_cleanup(plane);
>
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> index 79ff653d8081..7a499731ce93 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
> @@ -68,7 +68,6 @@ static void mdp4_plane_destroy(struct drm_plane *plane)
> {
> struct mdp4_plane *mdp4_plane = to_mdp4_plane(plane);
>
> - drm_plane_helper_disable(plane, NULL);
> drm_plane_cleanup(plane);
>
> kfree(mdp4_plane);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 7d306c5acd09..d5e4f0de321a 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -46,7 +46,6 @@ static void mdp5_plane_destroy(struct drm_plane *plane)
> {
> struct mdp5_plane *mdp5_plane = to_mdp5_plane(plane);
>
> - drm_plane_helper_disable(plane, NULL);
> drm_plane_cleanup(plane);
>
> kfree(mdp5_plane);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c1abad8a8612..69dbdba183fe 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -312,6 +312,7 @@ static int msm_drm_uninit(struct device *dev)
> if (fbdev && priv->fbdev)
> msm_fbdev_free(ddev);
> #endif
> + drm_atomic_helper_shutdown(ddev);
> drm_mode_config_cleanup(ddev);
>
> pm_runtime_get_sync(dev);
> --
> 2.19.0.rc2
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm: dpu: Allow planes to extend past active display
On Tue, Aug 28, 2018 at 04:50:37PM -0400, Sean Paul wrote:
> From: Sean Paul
>
> The atomic_check is a bit too aggressive with respect to planes which
> leave the active area. This caused a bunch of log spew when the cursor
> got to the edge of the screen and stopped it from going all the way.
>
> This patch removes the conservative bounds checks from atomic and clips
> the dst rect such that we properly display planes which go off the
> screen.
>
> Changes in v2:
> - Apply the clip to src as well (taking into account scaling)
>
> Cc: Sravanthi Kollukuduru
> Cc: Jeykumar Sankaran
> Signed-off-by: Sean Paul
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 14 +-
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 07c2d15b45f2..f0a5e776ba32 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1551,8 +1551,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> cnt++;
>
> dst = drm_plane_state_dest(pstate);
> - if (!drm_rect_intersect(, ) ||
> - !drm_rect_equals(, )) {
> + if (!drm_rect_intersect(, )) {
> DPU_ERROR("invalid vertical/horizontal destination\n");
> DPU_ERROR("display: " DRM_RECT_FMT " plane: "
> DRM_RECT_FMT "\n", DRM_RECT_ARG(_rect),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index efdf9b200dd9..adfd16625188 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1254,7 +1254,8 @@ static int dpu_plane_sspp_atomic_update(struct
> drm_plane *plane,
> const struct dpu_format *fmt;
> struct drm_crtc *crtc;
> struct drm_framebuffer *fb;
> - struct drm_rect src, dst;
> + struct drm_rect clip = { 0 }, src, dst;
> + int hscale, vscale;
>
> if (!plane) {
> DPU_ERROR("invalid plane\n");
> @@ -1300,6 +1301,17 @@ static int dpu_plane_sspp_atomic_update(struct
> drm_plane *plane,
>
> dst = drm_plane_state_dest(state);
>
> + hscale = drm_rect_calc_hscale(, ,
> + pdpu->pipe_sblk->maxupscale,
> + pdpu->pipe_sblk->maxdwnscale);
> + vscale = drm_rect_calc_vscale(, ,
> + pdpu->pipe_sblk->maxupscale,
> + pdpu->pipe_sblk->maxdwnscale);
> +
> + clip.x2 = crtc->state->adjusted_mode.hdisplay;
> + clip.y2 = crtc->state->adjusted_mode.vdisplay;
> + drm_rect_clip_scaled(, , , hscale, vscale);
Can't use drm_atomic_helper_check_plane_state() ?
> +
> DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FMT "->crtc%u " DRM_RECT_FMT
> ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_ARG(),
> crtc->base.id, DRM_RECT_ARG(),
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess
On Thu, Jun 28, 2018 at 04:13:06PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä > > Changes from the previous version mainly involve Danoie's suggestion Can't type today either: "Daniel's" > of hiding the drm_encoder_find() in the iterator macro. I also polished > the msm and tilcdc cases a bit more with another small helper. > > Cc: Alex Deucher > Cc: amd-...@lists.freedesktop.org > Cc: Ben Skeggs > Cc: "Christian König" > Cc: Daniel Vetter > Cc: "David (ChunMing) Zhou" > Cc: Dhinakaran Pandiyan > Cc: freedreno@lists.freedesktop.org > Cc: Harry Wentland > Cc: Jyri Sarha > Cc: linux-arm-...@vger.kernel.org > Cc: nouv...@lists.freedesktop.org > Cc: Rob Clark > Cc: Tomi Valkeinen > > Ville Syrjälä (9): > drm/fb-helper: Eliminate the .best_encoder() usage > drm/i915: Nuke intel_mst_best_encoder() > drm: Add drm_connector_for_each_possible_encoder() > drm/amdgpu: Use drm_connector_for_each_possible_encoder() > drm/nouveau: Use drm_connector_for_each_possible_encoder() > drm/radeon: Use drm_connector_for_each_possible_encoder() > drm: Add drm_connector_has_possible_encoder() > drm/msm: Use drm_connector_has_possible_encoder() > drm/tilcdc: Use drm_connector_has_possible_encoder() > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 81 ++- > drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 15 ++--- > drivers/gpu/drm/drm_connector.c| 44 + > drivers/gpu/drm/drm_fb_helper.c| 34 +- > drivers/gpu/drm/drm_probe_helper.c | 10 +-- > drivers/gpu/drm/i915/intel_dp_mst.c| 10 --- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 8 +-- > drivers/gpu/drm/nouveau/nouveau_connector.c| 21 +- > drivers/gpu/drm/radeon/radeon_connectors.c | 90 > -- > drivers/gpu/drm/tilcdc/tilcdc_external.c | 9 ++- > include/drm/drm_connector.h| 16 + > 11 files changed, 128 insertions(+), 210 deletions(-) > > -- > 2.16.4 -- Ville Syrjälä Intel ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote: > tinydrm is also using plane->fb: > > $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ > drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) > drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) > drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = > mipi->tinydrm.pipe.plane.fb; Oh dear, and naturally it's the most annoying one of the bunch. So we want to take the plane lock in the fb.dirty() hook to look at the fb, but mipi-dbi.c calls that directly from the bowels of its .atomic_enable() hook. So that would deadlock pretty neatly if the commit happens while already holding the lock. So we'd either need to plumb an acquire context into fb.dirty(), or maybe have tinydrm provide a lower level lockless dirty() hook that gets called by both (there are just too many layers in this particular cake to immediately see where such a hook would be best placed). Or maybe there's some other solution I didn't think of. Looking at this I'm also left wondering how this is supposed handle fb.dirty() getting called concurrently with a modeset. The dirty_mutex only seems to offer any protection for fb.dirty() against another fb.dirty()... -- Ville Syrjälä Intel OTC ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 00/15] drm: More plane clipping polish
On Thu, Nov 23, 2017 at 09:04:47PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > This series first unifies all users of drm_atomic_helper_check_plane_state() > to populate the clip rectangle with drm_mode_get_hv_timing(), and once > everything is unified the clip rectangle handling is sucked into > drm_atomic_helper_check_plane_state() away from driver code. > > Entire series available here: > git://github.com/vsyrjala/linux.git atomic_plane_helper_clip > > Cc: Archit Taneja <arch...@codeaurora.org> > Cc: Ben Skeggs <bske...@redhat.com> > Cc: Brian Starkey <brian.star...@arm.com> > Cc: CK Hu <ck...@mediatek.com> > Cc: Daniel Vetter <dan...@ffwll.ch> > Cc: freedreno@lists.freedesktop.org > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: linux-amlo...@lists.infradead.org > Cc: linux-arm-...@vger.kernel.org > Cc: linux-te...@vger.kernel.org > Cc: Liviu Dudau <liviu.du...@arm.com> > Cc: Mali DP Maintainers <mal...@foss.arm.com> > Cc: Mark Yao <mark@rock-chips.com> > Cc: Neil Armstrong <narmstr...@baylibre.com> > Cc: Noralf Trønnes <nor...@tronnes.org> > Cc: nouv...@lists.freedesktop.org > Cc: Philipp Zabel <p.za...@pengutronix.de> > Cc: Rob Clark <robdcl...@gmail.com> > Cc: Shawn Guo <shawn...@kernel.org> > Cc: Sinclair Yeh <s...@vmware.com> > Cc: Thierry Reding <thierry.red...@gmail.com> > Cc: Thomas Hellstrom <thellst...@vmware.com> > Cc: VMware Graphics <linux-graphics-maintai...@vmware.com> > > Ville Syrjälä (15): > drm/i915: Reject odd pipe source width with double wide/dual link > drm/i915: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/arm/hdlcd: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/arm/mali-dp: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/simple_kms_helper: Use drm_mode_get_hv_timing() to populate plane > clip rectangle > drm/imx: Use drm_mode_get_hv_timing() to populate plane clip rectangle > drm/mediatek: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/meson: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/msm/mdp5: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/nouveau/kms/nv50: Use drm_mode_get_hv_timing() to populate plane > clip rectangle > drm/rockchip: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/tegra/dc: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/vmwgfx: Use drm_mode_get_hv_timing() to populate plane clip > rectangle > drm/zte: Use drm_mode_get_hv_timing() to populate plane clip rectangle Everything up to here pushed to drm-misc-next. Thanks for the reviews. There have been a few new users of the clip helper so I'll have to take care of those and respin the final patch. Also armada looks broken to me since it has started to use the atomic version of the helper without actually being an atomic driver. So I'll have to figure out what's going on there as well. > drm: Don't pass clip to drm_atomic_helper_check_plane_state() > > drivers/gpu/drm/arm/hdlcd_crtc.c| 6 +- > drivers/gpu/drm/arm/malidp_planes.c | 5 + > drivers/gpu/drm/armada/armada_overlay.c | 2 +- > drivers/gpu/drm/drm_atomic_helper.c | 12 +++- > drivers/gpu/drm/drm_plane_helper.c | 11 +++ > drivers/gpu/drm/drm_simple_kms_helper.c | 5 - > drivers/gpu/drm/i915/intel_atomic_plane.c | 8 > drivers/gpu/drm/i915/intel_display.c| 12 +++- > drivers/gpu/drm/i915/intel_drv.h| 1 - > drivers/gpu/drm/i915/intel_sprite.c | 8 ++-- > drivers/gpu/drm/imx/ipuv3-plane.c | 7 +-- > drivers/gpu/drm/mediatek/mtk_drm_plane.c| 6 +- > drivers/gpu/drm/meson/meson_plane.c | 6 +- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 14 ++ > drivers/gpu/drm/nouveau/nv50_display.c | 8 > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +--- > drivers/gpu/drm/tegra/dc.c | 8 +--- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 +--- > drivers/gpu/drm/zte/zx_plane.c | 15 +-- > include/drm/drm_atomic_helper.h | 1 - > include/drm/drm_plane_helper.h | 1 - > 21 files changed, 35 insertions(+), 117 deletions(-) > > -- > 2.13.6 -- Ville Syrjälä Intel OTC ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v4 2/2] drm: Add DRM_MODE_ROTATE_ and DRM_MODE_REFLECT_ to UAPI
drm_object_attach_property(obj, priv->zorder_prop, 0); > @@ -273,7 +273,7 @@ static void omap_plane_reset(struct drm_plane *plane) >*/ > omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY > ? 0 : omap_plane->id; > - omap_state->base.rotation = DRM_ROTATE_0; > + omap_state->base.rotation = DRM_MODE_ROTATE_0; > > plane->state = _state->base; > plane->state->plane = plane; > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > i
Re: [Freedreno] [PATCH v3] drm: Add DRM_MODE_ROTATE_ and DRM_MODE_REFLECT_ to UAPI
On Wed, May 17, 2017 at 09:39:11PM -0400, Robert Foss wrote:
> +/*
> + * DRM_MODE_REFLECT_
> + *
> + * Signals that the contents of a drm plane has been reflected in
> + * the axis.
Still vague.
Also you didn't respond to my comment about the use of past tense.
> + *
> + * This define is provided as a convenience, looking up the property id
> + * using the name->prop id lookup is the preferred method.
> + */
> +#define DRM_MODE_REFLECT_X (1<<4)
> +#define DRM_MODE_REFLECT_Y (1<<5)
> +
> +/*
> + * DRM_MODE_REFLECT_MASK
> + *
> + * Bitmask used to look for drm plane reflections.
> + */
> +#define DRM_MODE_REFLECT_MASK (\
> + DRM_MODE_REFLECT_X | \
> + DRM_MODE_REFLECT_Y)
> +
> +
> struct drm_mode_modeinfo {
> __u32 clock;
> __u16 hdisplay;
> --
> 2.11.0.453.g787f75f05
--
Ville Syrjälä
Intel OTC
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 12/16] drm/vblank: Switch to bool in_vblank_irq in get_vblank_timestamp
On Wed, Mar 22, 2017 at 09:36:13AM +0100, Daniel Vetter wrote:
> It's overkill to have a flag parameter which is essentially used just
> as a boolean. This takes care of core + adjusting drivers.
>
> Adjusting the scanout position callback is a bit harder, since radeon
> also supplies it's own driver-private flags in there.
This part worried me, but indeed radeon only passes the custom flag to
the scanout position hook. Patch lgtm
Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Cc: Mario Kleiner <mario.klei...@tuebingen.mpg.de>
> Cc: Eric Anholt <e...@anholt.net>
> Cc: Rob Clark <robdcl...@gmail.com>
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Cc: Christian König <christian.koe...@amd.com>
> Cc: Ben Skeggs <bske...@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 ++---
> drivers/gpu/drm/drm_irq.c | 41
> +--
> drivers/gpu/drm/i915/i915_irq.c | 4 +--
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 4 +--
> drivers/gpu/drm/nouveau/nouveau_display.c | 5 ++--
> drivers/gpu/drm/nouveau/nouveau_display.h | 2 +-
> drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
> drivers/gpu/drm/radeon/radeon_kms.c | 4 +--
> drivers/gpu/drm/vc4/vc4_crtc.c| 4 +--
> drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> include/drm/drm_drv.h | 11 -
> include/drm/drm_irq.h | 2 +-
> 13 files changed, 46 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index acd8631d8024..edb3bb83e1a9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1771,7 +1771,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev,
> unsigned int pipe);
> bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int
> pipe,
>int *max_error,
>struct timeval *vblank_time,
> - unsigned flags);
> + bool in_vblank_irq);
> long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
>unsigned long arg);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index ac42f707c046..ad295e822d45 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -834,7 +834,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev,
> unsigned int pipe)
> * @crtc: crtc to get the timestamp for
> * @max_error: max error
> * @vblank_time: time value
> - * @flags: flags passed to the driver
> + * @in_vblank_irq: called from drm_handle_vblank()
> *
> * Gets the timestamp on the requested crtc based on the
> * scanout position. (all asics).
> @@ -843,7 +843,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev,
> unsigned int pipe)
> bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int
> pipe,
>int *max_error,
>struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct drm_crtc *crtc;
> struct amdgpu_device *adev = dev->dev_private;
> @@ -864,7 +864,7 @@ bool amdgpu_get_vblank_timestamp_kms(struct drm_device
> *dev, unsigned int pipe,
>
> /* Helper routine in DRM core does all the work: */
> return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
>>hwmode);
> }
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 2121ea29e1b2..059c3346db68 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -54,7 +54,7 @@
>
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> - struct timeval *tvblank, unsigned flags);
> + struct timeval *tvblank, bool in_vblank_irq);
>
> static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
>
> @@ -138,7 +138,7 @@ static void drm_reset_vblank_t
Re: [Freedreno] [PATCH] drm/msm/mdp5: handle non-fullscreen base plane case
On Thu, Oct 13, 2016 at 12:48:46PM -0400, Rob Clark wrote:
> If the bottom-most layer is not fullscreen, we need to use the BASE
> mixer stage for solid fill (ie. MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT). The
> blend_setup() code pretty much handled this already, we just had to
> figure this out in _atomic_check() and assign the stages appropriately.
>
> Signed-off-by: Rob Clark <robdcl...@gmail.com>
> ---
> TODO mdp4 might need similar treatment?
>
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 44
>
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> index fa2be7c..e42f62d 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> @@ -223,11 +223,6 @@ static void blend_setup(struct drm_crtc *crtc)
> plane_cnt++;
> }
>
> - /*
> - * If there is no base layer, enable border color.
> - * Although it's not possbile in current blend logic,
> - * put it here as a reminder.
> - */
> if (!pstates[STAGE_BASE] && plane_cnt) {
> ctl_blend_flags |= MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT;
> DBG("Border Color is enabled");
> @@ -365,6 +360,15 @@ static int pstate_cmp(const void *a, const void *b)
> return pa->state->zpos - pb->state->zpos;
> }
>
> +/* is there a helper for this? */
> +static bool is_fullscreen(struct drm_crtc_state *cstate,
> + struct drm_plane_state *pstate)
> +{
> + return (pstate->crtc_x == 0) && (pstate->crtc_y == 0) &&
> + (pstate->crtc_w == cstate->mode.hdisplay) &&
> + (pstate->crtc_h == cstate->mode.vdisplay);
> +}
And what if the plane is larger than the screen size while covering it
fully?
> +
> static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_crtc_state *state)
> {
> @@ -375,21 +379,11 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> struct plane_state pstates[STAGE_MAX + 1];
> const struct mdp5_cfg_hw *hw_cfg;
> const struct drm_plane_state *pstate;
> - int cnt = 0, i;
> + int cnt = 0, base = 0, i;
>
> DBG("%s: check", mdp5_crtc->name);
>
> - /* verify that there are not too many planes attached to crtc
> - * and that we don't have conflicting mixer stages:
> - */
> - hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
> drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
> - if (cnt >= (hw_cfg->lm.nb_stages)) {
> - dev_err(dev->dev, "too many planes!\n");
> - return -EINVAL;
> - }
> -
> -
> pstates[cnt].plane = plane;
> pstates[cnt].state = to_mdp5_plane_state(pstate);
>
> @@ -399,8 +393,24 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
> /* assign a stage based on sorted zpos property */
> sort(pstates, cnt, sizeof(pstates[0]), pstate_cmp, NULL);
>
> + /* if the bottom-most layer is not fullscreen, we need to use
> + * it for solid-color:
> + */
> + if (!is_fullscreen(state, [0].state->base))
> + base++;
> +
> + /* verify that there are not too many planes attached to crtc
> + * and that we don't have conflicting mixer stages:
> + */
> + hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
> +
> + if ((cnt + base) >= hw_cfg->lm.nb_stages) {
> + dev_err(dev->dev, "too many planes!\n");
> + return -EINVAL;
> + }
> +
> for (i = 0; i < cnt; i++) {
> - pstates[i].state->stage = STAGE_BASE + i;
> + pstates[i].state->stage = STAGE_BASE + i + base;
> DBG("%s: assign pipe %s on stage=%d", mdp5_crtc->name,
> pipe2name(mdp5_plane_pipe(pstates[i].plane)),
> pstates[i].state->stage);
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
