Re: [PATCH v2 5/6] drm/vs: Add KMS crtc&plane

[Keith Zhao]

sorry 
Dmitry ,accidentally wrote the wrong name
Take no offense

On 2023/10/26 17:42, Keith Zhao wrote:
> hi Ville:
> very glad to receive your feedback
> Some of them are very good ideas.
> Some are not very clear and hope to get your further reply!
> 
> 
> On 2023/10/26 3:49, Ville Syrjälä wrote:
>> On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote:
>>> On 25/10/2023 13:39, Keith Zhao wrote:
>>> > add 2 crtcs and 8 planes in vs-drm
>>> > 
>>> > Signed-off-by: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
>>> > ---
>>> >   drivers/gpu/drm/verisilicon/Makefile   |    8 +-
>>> >   drivers/gpu/drm/verisilicon/vs_crtc.c  |  257 ++++
>>> >   drivers/gpu/drm/verisilicon/vs_crtc.h  |   43 +
>>> >   drivers/gpu/drm/verisilicon/vs_dc.c    | 1002 ++++++++++++
>>> >   drivers/gpu/drm/verisilicon/vs_dc.h    |   80 +
>>> >   drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++
>>> >   drivers/gpu/drm/verisilicon/vs_dc_hw.h |  492 ++++++
>>> >   drivers/gpu/drm/verisilicon/vs_drv.c   |    2 +
>>> >   drivers/gpu/drm/verisilicon/vs_plane.c |  526 +++++++
>>> >   drivers/gpu/drm/verisilicon/vs_plane.h |   58 +
>>> >   drivers/gpu/drm/verisilicon/vs_type.h  |   69 +
>>> >   11 files changed, 4494 insertions(+), 2 deletions(-)
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h
>>> >   create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h
>>> > 
>>> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile
>>> > index 7d3be305b..1d48016ca 100644
>>> > --- a/drivers/gpu/drm/verisilicon/Makefile
>>> > +++ b/drivers/gpu/drm/verisilicon/Makefile
>>> > @@ -1,7 +1,11 @@
>>> >   # SPDX-License-Identifier: GPL-2.0
>>> >   
>>> > -vs_drm-objs := vs_drv.o \
>>> > -		vs_modeset.o
>>> > +vs_drm-objs := vs_dc_hw.o \
>>> > +		vs_dc.o \
>>> > +		vs_crtc.o \
>>> > +		vs_drv.o \
>>> > +		vs_modeset.o \
>>> > +		vs_plane.o
>>> >   
>>> >   obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o
>>> >   
>>> > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
>>> > new file mode 100644
>>> > index 000000000..8a658ea77
>>> > --- /dev/null
>>> > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
>>> > @@ -0,0 +1,257 @@
>>> > +// SPDX-License-Identifier: GPL-2.0
>>> > +/*
>>> > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
>>> > + *
>>> > + */
>>> > +
>>> > +#include <linux/clk.h>
>>> > +#include <linux/debugfs.h>
>>> > +#include <linux/media-bus-format.h>
>>> > +
>>> > +#include <drm/drm_atomic_helper.h>
>>> > +#include <drm/drm_atomic.h>
>>> > +#include <drm/drm_crtc.h>
>>> > +#include <drm/drm_gem_atomic_helper.h>
>>> > +#include <drm/drm_vblank.h>
>>> > +#include <drm/vs_drm.h>
>>> > +
>>> > +#include "vs_crtc.h"
>>> > +#include "vs_dc.h"
>>> > +#include "vs_drv.h"
>>> > +
>>> > +static void vs_crtc_reset(struct drm_crtc *crtc)
>>> > +{
>>> > +	struct vs_crtc_state *state;
>>> > +
>>> > +	if (crtc->state) {
>>> > +		__drm_atomic_helper_crtc_destroy_state(crtc->state);
>>> > +
>>> > +		state = to_vs_crtc_state(crtc->state);
>>> > +		kfree(state);
>>> > +		crtc->state = NULL;
>>> > +	}
>>> 
>>> You can call your crtc_destroy_state function directly here.
> ok got it !
>>> 
>>> > +
>>> > +	state = kzalloc(sizeof(*state), GFP_KERNEL);
>>> > +	if (!state)
>>> > +		return;
>>> > +
>>> > +	__drm_atomic_helper_crtc_reset(crtc, &state->base);
>>> > +}
>>> > +
>>> > +static struct drm_crtc_state *
>>> > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
>>> > +{
>>> > +	struct vs_crtc_state *ori_state;
>>> 
>>> It might be a matter of taste, but it is usually old_state.
>>> 
>>> > +	struct vs_crtc_state *state;
>>> > +
>>> > +	if (!crtc->state)
>>> > +		return NULL;
>>> > +
>>> > +	ori_state = to_vs_crtc_state(crtc->state);
>>> > +	state = kzalloc(sizeof(*state), GFP_KERNEL);
>>> > +	if (!state)
>>> > +		return NULL;
>>> > +
>>> > +	__drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
>>> > +
>>> > +	state->output_fmt = ori_state->output_fmt;
>>> > +	state->encoder_type = ori_state->encoder_type;
>>> > +	state->bpp = ori_state->bpp;
>>> > +	state->underflow = ori_state->underflow;
>>> 
>>> Can you use kmemdup instead?
> ok 
>>> 
>>> > +
>>> > +	return &state->base;
>>> > +}
>>> > +
>>> > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc,
>>> > +					 struct drm_crtc_state *state)
>>> > +{
>>> > +	__drm_atomic_helper_crtc_destroy_state(state);
>>> > +	kfree(to_vs_crtc_state(state));
>>> > +}
>>> > +
>>> > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc)
>>> > +{
>>> > +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>>> > +	struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>>> > +
>>> > +	vs_dc_enable_vblank(dc, true);
>>> > +
>>> > +	return 0;
>>> > +}
>>> > +
>>> > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc)
>>> > +{
>>> > +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>>> > +	struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>>> > +
>>> > +	vs_dc_enable_vblank(dc, false);
>>> > +}
>>> > +
>>> > +static const struct drm_crtc_funcs vs_crtc_funcs = {
>>> > +	.set_config		= drm_atomic_helper_set_config,
>>> > +	.page_flip		= drm_atomic_helper_page_flip,
>>> 
>>> destroy is required, see drm_mode_config_cleanup()
> will add destory 
>>> 
>>> > +	.reset			= vs_crtc_reset,
>>> > +	.atomic_duplicate_state = vs_crtc_atomic_duplicate_state,
>>> > +	.atomic_destroy_state	= vs_crtc_atomic_destroy_state,
>>> 
>>> please consider adding atomic_print_state to output driver-specific bits.
>>> 
> will add atomic_print_state  to print my private definitions
>>> > +	.enable_vblank		= vs_crtc_enable_vblank,
>>> > +	.disable_vblank		= vs_crtc_disable_vblank,
>>> > +};
>>> > +
>>> > +static u8 cal_pixel_bits(u32 bus_format)
>>> 
>>> This looks like a generic helper code, which can go to a common place.
>>> 
>>> > +{
>>> > +	u8 bpp;
>>> > +
>>> > +	switch (bus_format) {
>>> > +	case MEDIA_BUS_FMT_RGB565_1X16:
>>> > +	case MEDIA_BUS_FMT_UYVY8_1X16:
>>> > +		bpp = 16;
>>> > +		break;
>>> > +	case MEDIA_BUS_FMT_RGB666_1X18:
>>> > +	case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
>>> > +		bpp = 18;
>>> > +		break;
>>> > +	case MEDIA_BUS_FMT_UYVY10_1X20:
>>> > +		bpp = 20;
>>> > +		break;
>>> > +	case MEDIA_BUS_FMT_BGR888_1X24:
>>> > +	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
>>> > +	case MEDIA_BUS_FMT_YUV8_1X24:
>>> > +		bpp = 24;
>>> > +		break;
>>> > +	case MEDIA_BUS_FMT_RGB101010_1X30:
>>> > +	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
>>> > +	case MEDIA_BUS_FMT_YUV10_1X30:
>>> > +		bpp = 30;
>>> > +		break;
>>> > +	default:
>>> > +		bpp = 24;
>>> > +		break;
>>> > +	}
>>> > +
>>> > +	return bpp;
>>> > +}
>>> > +
>>> > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
>>> > +				  struct drm_atomic_state *state)
>>> > +{
>>> > +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>>> > +	struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>>> > +	struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state);
>>> > +
>>> > +	vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt);
>>> > +
>>> > +	vs_dc_enable(dc, crtc);
>>> > +	drm_crtc_vblank_on(crtc);
>>> > +}
>>> > +
>>> > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
>>> > +				   struct drm_atomic_state *state)
>>> > +{
>>> > +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>>> > +	struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>>> > +
>>> > +	drm_crtc_vblank_off(crtc);
>>> > +
>>> > +	vs_dc_disable(dc, crtc);
>>> > +
>>> > +	if (crtc->state->event && !crtc->state->active) {
>>> > +		spin_lock_irq(&crtc->dev->event_lock);
>>> > +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> > +		spin_unlock_irq(&crtc->dev->event_lock);
>>> > +
>>> > +		crtc->state->event = NULL;
>>> 
>>> I think even should be cleared within the lock.
>> 
>> event_lock doesn't protect anything in the crtc state.
> 
> how about match like this:
> spin_lock_irq(&crtc->dev->event_lock);
> if (crtc->state->event) {
> 	drm_crtc_send_vblank_event(crtc, crtc->state->event);
> 	crtc->state->event = NULL;
> }
> spin_unlock_irq(&crtc->dev->event_lock);
> 
>> 
>> But the bigger problem in this code is the prevalent crtc->state
>> usage. That should pretty much never be done, especially in anything
>> that can get called from the actual commit phase where you no longer
>> have the locks held. Instead one should almost always use the
>> get_{new,old}_state() stuff, or the old/new/oldnew state iterators.
> 
> the prevalent crtc->state usage :
> crtc->state should not be used directly?
> need replace it by get_{new,old}_state()
> 
> for example:
> drm_crtc_send_vblank_event(crtc, crtc->state->event);
> 
> 
> should do like this :
> struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> ...
> drm_crtc_send_vblank_event(crtc, crtc_state->event);
> ...
> 
> If there is a problem, help further correct
> wish give a example
> Thanks
> 
>> 
>>> 
>>> > +	}
>>> > +}
>>> > +
>>> > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc,
>>> > +				 struct drm_atomic_state *state)
>>> > +{
>>> > +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
>>> > +									  crtc);
>>> > +
>>> > +	struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>>> > +	struct device *dev = vs_crtc->dev;
>>> > +	struct drm_property_blob *blob = crtc->state->gamma_lut;
>> 
>> Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but
>> then proceed to directly access crtc->state anyway.
>> 
> struct drm_property_blob *blob = crtc->state->gamma_lut;
> change to 
> struct drm_property_blob *blob = crtc_state ->gamma_lut;
> 
>>> > +	struct drm_color_lut *lut;
>>> > +	struct vs_dc *dc = dev_get_drvdata(dev);
>>> > +
>>> > +	if (crtc_state->color_mgmt_changed) {
>>> > +		if (blob && blob->length) {
>>> > +			lut = blob->data;
>>> > +			vs_dc_set_gamma(dc, crtc, lut,
>>> > +					blob->length / sizeof(*lut));
>>> > +			vs_dc_enable_gamma(dc, crtc, true);
>>> > +		} else {
>>> > +			vs_dc_enable_gamma(dc, crtc, false);
>>> > +		}
>>> > +	}
>>> > +}
>>